Extend CheckInnerWindowCorrectness() to check the state of the worker.

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
3 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

58 Branch
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(2 attachments)

In this bug I want to rename CheckInnerWindowCorrectness() to CheckCurrentGlobalCorrectness() and add the check to the state of workers in order to avoid the dispatching of events when the worker is in a terminating/canceling state.
Posted patch global.patchSplinter Review
Attachment #8968961 - Flags: review?(bugs)
Comment on attachment 8968961 [details] [diff] [review]
global.patch

Review of attachment 8968961 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/DOMEventTargetHelper.cpp
@@ +385,5 @@
> +  }
> +
> +  // Web Worker.
> +  WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
> +  if (workerPrivate && !workerPrivate->IsActive()) {

We now call DisconnectFromOwner() for all DETH objects when a worker private enters Terminating.  So I thought you can just do:

  if (!mParentObject) {
    return NS_ERROR_FAILURE;
  }

The check for IsCurrentInnerWindow() is probably still needed to handle bfcache case.
Component: DOM → DOM: Workers
Priority: -- → P2
Comment on attachment 8968961 [details] [diff] [review]
global.patch

>+DOMEventTargetHelper::CheckCurrentGlobalCorrectness() const
>+{
>+  NS_ENSURE_STATE(!mHasOrHasHadOwnerWindow || mOwnerWindow);
>+
>+  // Main-thread.
>+  if (mOwnerWindow && !mOwnerWindow->IsCurrentInnerWindow()) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  if (NS_IsMainThread()) {
>+    return NS_OK;
>+  }
>+
>+  // Web Worker.
>+  WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
>+  if (workerPrivate && !workerPrivate->IsActive()) {
As bkelly says, just checking mParentObject here should be enough, and will hopefully cover worklets.
(or should at least in the future)

so
if (!mParentObject) {
  return NS_ERROR_FAILURE;
}

>+++ b/dom/messagechannel/MessagePort.cpp
>   bool
>+  IsActive()
>+  {
>+    AssertIsOnWorkerThread();
>+
>+    MutexAutoLock lock(mMutex);
>+    return mStatus < Terminating;
>+  }
...then this wouldn't be needed
Attachment #8968961 - Flags: review?(bugs) → review+
Checking mParentObject would also help us catch bugs where we don't bind DETH objects to global right now.  We haven't really enforced that in the past; especially for workers.

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:baku, could you have a look please?

Flags: needinfo?(amarchesini)
Depends on: 1539407
Depends on: 1539524
Depends on: 1539528
Depends on: 1539532
Depends on: 1539552
Depends on: 1540159
Flags: needinfo?(amarchesini)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a114232e886
Extend CheckInnerWindowCorrectness() to check the state of the worker, r=smaug
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16486 for changes under testing/web-platform/tests
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.