Closed Bug 1455020 Opened 7 years ago Closed 6 years ago

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

Categories

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

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(2 files)

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.
Attached 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: 6 years 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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: