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)
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.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8968961 -
Flags: review?(bugs)
Comment 2•7 years ago
|
||
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.
Updated•7 years ago
|
Component: DOM → DOM: Workers
Updated•7 years ago
|
Priority: -- → P2
Comment 3•7 years ago
|
||
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+
Comment 4•7 years ago
|
||
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•6 years ago
|
||
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)
Assignee | ||
Comment 6•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
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
Comment 8•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox68:
--- → fixed
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.
Description
•