1179567 - Make ServiceWorker keep its document and window alive

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[(no longer active)]

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.

Comment 1

[(no longer active)]

Attachment: Make ServiceWorker keep its document and window alive

Comment 2

[Andrea Marchesini [:baku]]

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();
}

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/daffc9a65ab0

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/daffc9a65ab0

Comment 5

[Cătălin Badea (:catalinb)]

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?

Comment 6

[(no longer active)]

(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.

Comment 7

[Cătălin Badea (:catalinb)]

Attachment: 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

Comment 8

[Cătălin Badea (:catalinb)]

Attachment: MozReview Request: Bug 1188545: Cosmetic changes regarding workerPrivate properties shared between shared workers and service workers. r?nsm

Bug 1188545: Cosmetic changes regarding workerPrivate properties shared between shared workers and service workers. r?nsm

Comment 9

[Cătălin Badea (:catalinb)]

Attachment: MozReview Request: Bug 1188545: Terminate service workers that have been idle for some time. r?nsm

Bug 1188545: Terminate service workers that have been idle for some time. r?nsm

Comment 10

[Cătălin Badea (:catalinb)]

(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.

Comment 11

[Cătălin Badea (:catalinb)]

ah, crap - reviewboard used the first commit to attach the reviews. sorry about that.