Make ServiceWorker keep its document and window alive

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Without this, ServiceWorker::PostMessage won't work when the window has
been cleaned up, since at that time the event targets are disconnected
from their owners and GetParentObject() will return null.
Attachment #8628570 - Flags: review?(amarchesini)
Assignee: nobody → ehsan
Comment on attachment 8628570 [details] [diff] [review]
Make ServiceWorker keep its document and window alive

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

::: dom/workers/ServiceWorker.cpp
@@ +45,5 @@
>                               SharedWorker* aSharedWorker)
>    : DOMEventTargetHelper(aWindow),
>      mInfo(aInfo),
> +    mSharedWorker(aSharedWorker),
> +    mDocument(aWindow ? aWindow->GetExtantDoc() : nullptr),

I prefer to have this 2 assignment into the CTOR code.
Something like:

if (aWindow) {
  mDocument = aWindow->GetExtantDoc();
  mWindow = aWindow->GetOuterWindow();
}
Attachment #8628570 - Flags: review?(amarchesini) → review+
https://hg.mozilla.org/mozilla-central/rev/daffc9a65ab0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
I think the problem is that the WorkerPrivate associated with the underlying shared worker is destroyed when the document is detached. Keeping the document and the window alive doesn't prevent this.

Are there any tests that are fixed by this change?
Flags: needinfo?(ehsan)
(In reply to Cătălin Badea (:catalinb) from comment #5)
> I think the problem is that the WorkerPrivate associated with the underlying
> shared worker is destroyed when the document is detached. Keeping the
> document and the window alive doesn't prevent this.
> 
> Are there any tests that are fixed by this change?

No, this is part of bug 1179401.  The rest of that bug talks about restarting the worker if the previous one has died.  Doing that would fix the test in that bug.
Flags: needinfo?(ehsan)
Bug 1188545 - Disentangle service workers from shared workers and refactor event dispatching code into a separate class.  r?nsm,bkelly,khuey
Attachment #8646019 - Flags: review?(nsm.nikhil)
Attachment #8646019 - Flags: review?(khuey)
Attachment #8646019 - Flags: review?(bkelly)
Bug 1188545: Cosmetic changes regarding workerPrivate properties shared between shared workers and service workers. r?nsm
Attachment #8646020 - Flags: review?(nsm.nikhil)
Bug 1188545: Terminate service workers that have been idle for some time. r?nsm
Attachment #8646021 - Flags: review?(nsm.nikhil)
(In reply to Cătălin Badea (:catalinb) from comment #7)
> Created attachment 8646019 [details]
> MozReview Request: Bug 1188545 - Disentangle service workers from shared
> workers and refactor event dispatching code into a separate class. 
> r?nsm,bkelly,khuey
> 
> Bug 1188545 - Disentangle service workers from shared workers and refactor
> event dispatching code into a separate class.  r?nsm,bkelly,khuey

bkelly, please have a look at how the worker load group is handled in SpawnServiceWorkerIfNeeded.
khuey, for the changes in RuntimeService and WorkerPrivate.
nsm, please have another look at the changes in ServiceWorkerPrivate, the part about AllowedWindowInteraction and notification click events.
ah, crap - reviewboard used the first commit to attach the reviews. sorry about that.
Attachment #8646019 - Attachment is obsolete: true
Attachment #8646019 - Flags: review?(nsm.nikhil)
Attachment #8646019 - Flags: review?(khuey)
Attachment #8646019 - Flags: review?(bkelly)
Attachment #8646020 - Attachment is obsolete: true
Attachment #8646020 - Flags: review?(nsm.nikhil)
Attachment #8646021 - Attachment is obsolete: true
Attachment #8646021 - Flags: review?(nsm.nikhil)
You need to log in before you can comment on or make changes to this bug.