gloo-timers 0.2.3 can panic

[ctm]

Describe the Bug

Code using gloo-timers 0.2.3 can panic when the timer is dropped.

Steps to Reproduce

1. Go to https://github.com/ctm/gloo-timers-regression
2. clone the repository
3. yarn install && yarn dev:start
4. open http://localhost:8080
5. look at JavaScript console

If applicable, add a link to a test case (as a zip file or link to a repository
we can clone).

Expected Behavior

There should be no panic.

Actual Behavior

There is a panic.

Additional Context

Add any other context about the problem here.

[ranile]

I can't reproduce this behavior locally with trunk.

# Cargo.toml 
[package]
name = "gloo-timers-regression"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
gloo-timers = "0.2.3"
gloo-console = "*"

// src/main.rs 
#[macro_use]
extern crate gloo_console;

use gloo_timers::callback::Timeout;

const DELAY_MILLIS: u32 = 30;

fn main() {
    log!("hello");
    let tt = Timeout::new(DELAY_MILLIS, move || { log!("inside") });
    drop(tt);
}

I used trunk serve with a simple index.html file, loaded it in browser and found that "hello" is logged but not "inside", showcasing that drop worked.

[ranile]

Since it works under trunk, I think it's safe to say that it's not a problem with gloo-timers

[ctm]

I guess it depends on the definition of "a problem with gloo-timers". The gloo-timers crate used to work under webpack and now it doesn't.

I've created a globals branch of my demo that illustrates that calling the "global" clearTimer function fails under webpack, but it can successfully be called as a method on the return value of js_sys::global().

FWIW, my app's use of gloo-timers is relatively small, so if there's no desire to go back to code that works with webpack, I can leave gloo-timers pegged to 0.2.2 for now and then just rewrite the subset of functionality that I use. I can't trivially switch from webpack to trunk though. Nor do I have enough knowledge of the JavaScript ecosystem to look much deeper into this issue.

[ranile]

I wonder if this is an issue with wasm-pack. Webpack calls wasm-pack to bundle the Rust app

[ctm]

It could be. However, the presence of js_sys::global() and the detailed comments within its source make me wonder if it's really "supposed" to be safe to call the "global" clearTimer (or setTimer for that matter) from any environment, or whether it's luck / an implementation artifact that it's working under trunk (and apparently node).

[ranile]

The CI passes. I have a feeling it might be a wasm-pack bug. I will confirm later

[ivnsch]

Can confirm - getting the same error with 0.2.3 and wasm-pack.

[ctm]

@ivanschuetz I forked and quickly hacked in a proof-of-concept fix in my js_sys-experiment branch. If that branch works for you, I'll be happy to turn it into a PR, perhaps behind a feature flag. Currently I've just pinned 0.2.2, so cargo outdated chirps at me and that's getting old.

[PhilippGackstatter]

Perhaps one of you can try gloo-timers as a git dependency with commit c798e33fec12837bf60a722c9b8e882d1ad19c26, which is the first commit from PR #185. This was before thread_locals were removed, and perhaps they are causing the issue. I don't really think that's the issue, but it might be worth a shot.

[ctm]

Thanks for looking into it. I tried using:

gloo-timers = { version = "0.2.2", git = "https://github.com/rustwasm/gloo", rev = "c798e33fec12837bf60a722c9b8e882d1ad19c26" }

in my Cargo.toml, but cargo (and my own repository forked from rustwasm) claims that's not a commit. I see where that commit is referenced in the pull-request, but I think it may have been squashed when it went into master.

I tried looking in PhilippGackstatter for the `branch that it came from, but without success.

Based on my previous experience, that commit would not work, because it contains:

[wasm_bindgen(js_name = "clearTimeout")]
    fn clear_timeout_with_handle(handle: i32);

which seems to cause a panic when used with wasm-pack, which is why I have

    #[wasm_bindgen(method, js_name = "clearTimeout")]
    fn clear_timeout(this: &Global, handle: i32);

in my fork that doesn't panic when using wasm-pack.

[ctm]

I should have said "probably would not work", because dealing with globals is always tricky and there may be something in your commit that sets something up so that the JavaScript clearTimeout can work as a non-method. It certainly appears to work as a non-method "everywhere else".

[bnheise]

I'm getting this error with wasm-pack with version 0.2.4 of gloo-timers.

[ctm]

@bnheise I wish I had more time to look into this further, but I haven't been able to make the time, although in part that's because it appeared like it was biting so few people that I could just live with my own branch:

gloo-timers = { version = "0.2.3", git = "https://github.com/ctm/gloo", branch = "js_sys-experiment" }

The biggest thing that's holding me back is that I simply don't have enough domain knowledge to be able to do anything beyond what I've already done without doing a lot of research.

Perhaps @hamza1311 can confirm (or refute) that this is a wasm-pack bug.

[ranile]

The issue seems to be the build with --target bundler (which is what webpack uses and wasm-bindgen passes by default) passed to wasm-bindgen CLI. I don't have a project setup where I can reproduce this right now so i wasn't able to look into why that is happening.

In case anyone knows why this might be, I would like to hear that. I'm not sure about the history of --target bundler and why it's present. All major browsers support es6 modules so we should be using that instead. That's also what trunk uses. I don't have access to wasm-pack repo or the webpack plugin npm package so I can't update those. Maybe @fitzgen can give me rights?

@ctm, can the issue be solved --target web (or --target no-modules) is used with wasm-pack? If you can try that, that would be helpful.

[bnheise]

Thanks for looking into this guys. It might take me a few days, but I can test out --target web and --target no-modules in the project where I ran into this issue. Just to make sure, the target flag here is for wasm-pack, right?

[ctm]

@hamza1311 Thanks! I don't know how to use those flags off the top of my head, but I'll poke around and either give you an answer or ask you some questions soon (probably later today).

[ctm]

@hamza1311, thanks for the quick response. Initially I tried blindly adding --target=web and --target=no-modules to the relevant extraArgs portion of my project's webpack.config.js file. Both failed, although with different errors in the JavaScript console.

--target=web:

Uncaught (in promise) TypeError: Cannot read properties of undefined (reading '__wbindgen_add_to_stack_pointer')
    at Module.run_app (index.js:352:14)
    at eval (bootstrap.js:105:12)

--target= no-modules:

Uncaught (in promise) TypeError: module.run_app is not a function
    at eval (bootstrap.js:105:12)

I then read a bit about what the different targets mean and found:

The --target web mode is not able to use NPM dependencies.

which precludes me from using it in my software, because my software does indeed have NPM dependencies. It looks to me like no-modules is going to have that same restriction.