Bugzilla

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #38)
> This may be a case where the runnable should be going out of its way to drop references in its run method (on the worker thread) instead of assuming that it will be destroyed on the thread it ran on?

I phrased this badly.  A better take is that we have a 2-part systemic bug in workers [where step 1 is we wrap the runnable and retain a reference to the wrapper](https://searchfox.org/mozilla-central/rev/78af6dbe0591376d9d5d678adaf21aa69dcc3506/dom/workers/WorkerEventTarget.cpp#119-122) on the dispatching thread.  In general, the right way for runnable dispatch to work is that we transfer ownership into the dispatch queue in the success case by moving the existing strong refcount.
```cpp
RefPtr<WorkerRunnable> r =
    mWorkerPrivate->MaybeWrapAsWorkerRunnable(runnable.forget());
if (r->Dispatch()) {
  return NS_OK;
```

The 2nd part of the bug is that [ExternalRunnable does not drop its mWrappedRunnable in Run and Cancel](https://searchfox.org/mozilla-central/rev/78af6dbe0591376d9d5d678adaf21aa69dcc3506/dom/workers/WorkerPrivate.cpp#208-227), but it totally could and that can be our fix here, perhaps.
```cpp
virtual bool WorkerRun(JSContext* aCx,
                       WorkerPrivate* aWorkerPrivate) override {
  nsresult rv = mWrappedRunnable->Run();
  if (NS_FAILED(rv)) {
    if (!JS_IsExceptionPending(aCx)) {
      Throw(aCx, rv);
    }
    return false;
  }
  return true;
}

nsresult Cancel() override {
  nsCOMPtr<nsIDiscardableRunnable> doomed =
      do_QueryInterface(mWrappedRunnable);
  if (doomed) {
    doomed->OnDiscard();
  }
  return NS_OK;
}
```

The caveat is that [if the runnable is already a WorkerRunnable we just give it back](https://searchfox.org/mozilla-central/rev/78af6dbe0591376d9d5d678adaf21aa69dcc3506/dom/workers/WorkerPrivate.cpp#1738-1742) and those WorkerRunnables are still on the hook to make sure they drop everything in their WorkerRun/Cancel methods.

Our short/medium-term plans are to eliminate the wrapping, but we can't do that on this bug.

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #38)
> This may be a case where the runnable should be going out of its way to drop references in its run method (on the worker thread) instead of assuming that it will be destroyed on the thread it ran on?

I phrased this badly.  A better take is that we have a 2-part systemic bug in workers [where step 1 is we wrap the runnable and retain a reference to the wrapper](https://searchfox.org/mozilla-central/rev/78af6dbe0591376d9d5d678adaf21aa69dcc3506/dom/workers/WorkerEventTarget.cpp#119-122) on the dispatching thread.  In general, the right way for runnable dispatch to work is that we transfer ownership into the dispatch queue in the success case by moving the existing strong refcount (and this is not that).
```cpp
RefPtr<WorkerRunnable> r =
    mWorkerPrivate->MaybeWrapAsWorkerRunnable(runnable.forget());
if (r->Dispatch()) {
  return NS_OK;
```

The 2nd part of the bug is that [ExternalRunnable does not drop its mWrappedRunnable in Run and Cancel](https://searchfox.org/mozilla-central/rev/78af6dbe0591376d9d5d678adaf21aa69dcc3506/dom/workers/WorkerPrivate.cpp#208-227), but it totally could and that can be our fix here, perhaps.
```cpp
virtual bool WorkerRun(JSContext* aCx,
                       WorkerPrivate* aWorkerPrivate) override {
  nsresult rv = mWrappedRunnable->Run();
  if (NS_FAILED(rv)) {
    if (!JS_IsExceptionPending(aCx)) {
      Throw(aCx, rv);
    }
    return false;
  }
  return true;
}

nsresult Cancel() override {
  nsCOMPtr<nsIDiscardableRunnable> doomed =
      do_QueryInterface(mWrappedRunnable);
  if (doomed) {
    doomed->OnDiscard();
  }
  return NS_OK;
}
```

The caveat is that [if the runnable is already a WorkerRunnable we just give it back](https://searchfox.org/mozilla-central/rev/78af6dbe0591376d9d5d678adaf21aa69dcc3506/dom/workers/WorkerPrivate.cpp#1738-1742) and those WorkerRunnables are still on the hook to make sure they drop everything in their WorkerRun/Cancel methods.

Our short/medium-term plans are to eliminate the wrapping, but we can't do that on this bug.

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #38)
> This may be a case where the runnable should be going out of its way to drop references in its run method (on the worker thread) instead of assuming that it will be destroyed on the thread it ran on?

I phrased this badly.  A better take is that we have a 2-part systemic bug in workers [where step 1 is we wrap the runnable and retain a reference to the wrapper](https://searchfox.org/mozilla-central/rev/78af6dbe0591376d9d5d678adaf21aa69dcc3506/dom/workers/WorkerEventTarget.cpp#119-122) on the dispatching thread.  In general, the right way for runnable dispatch to work is that we transfer ownership into the dispatch queue in the success case by moving the existing strong refcount (and this is not that).
```cpp
RefPtr<WorkerRunnable> r =
    mWorkerPrivate->MaybeWrapAsWorkerRunnable(runnable.forget());
if (r->Dispatch()) {
  return NS_OK;
```

The 2nd part of the bug is that [ExternalRunnable does not drop its mWrappedRunnable in Run and Cancel](https://searchfox.org/mozilla-central/rev/78af6dbe0591376d9d5d678adaf21aa69dcc3506/dom/workers/WorkerPrivate.cpp#208-227), but it totally could and that can be our fix here, perhaps.
```cpp
virtual bool WorkerRun(JSContext* aCx,
                       WorkerPrivate* aWorkerPrivate) override {
  nsresult rv = mWrappedRunnable->Run();
  if (NS_FAILED(rv)) {
    if (!JS_IsExceptionPending(aCx)) {
      Throw(aCx, rv);
    }
    return false;
  }
  return true;
}

nsresult Cancel() override {
  nsCOMPtr<nsIDiscardableRunnable> doomed =
      do_QueryInterface(mWrappedRunnable);
  if (doomed) {
    doomed->OnDiscard();
  }
  return NS_OK;
}
```

The caveat is that [if the runnable is already a WorkerRunnable we just give it back](https://searchfox.org/mozilla-central/rev/78af6dbe0591376d9d5d678adaf21aa69dcc3506/dom/workers/WorkerPrivate.cpp#1738-1742) and those WorkerRunnables are still on the hook to make sure they drop everything in their WorkerRun/Cancel methods.

Our short/medium-term plans are to eliminate the wrapping which can avoid all the WorkerRunnable awkwardness and we can just be dispatching to a normal-ish event queue, but we can't do that on this bug.