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

Status: RESOLVED FIXED

(Core :: DOM: Workers, enhancement, P2)

Description

[Andrea Marchesini [:baku]]

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.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: global.patch

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

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?

Comment 6

[Andrea Marchesini [:baku]]

Attachment: Bug 1455020 - Extend CheckInnerWindowCorrectness() to check the state of the worker, r=smaug

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a114232e886
Extend CheckInnerWindowCorrectness() to check the state of the worker, r=smaug

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7a114232e886

Comment 9

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16486 for changes under testing/web-platform/tests

Comment 10

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged