1432640 - navigator.serviceWorker.controller returns null with parent-side intercept enabled

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

While checking bug 1431847 I noticed that sites often can't see navigator.serviceWorker.controller even though their network events are being intercepted.  This is due to a service worker ID mismatch.  The service worker ID is a one-up counter in each process.  So even if the service worker is identical the ID can get out of sync.  This is especially true since installation still happens in the child.

Perhaps we should just get rid of the ID check for now.  I don't think we had it in the past.  I introduced it in bug 1293277.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I was thinking about this some more.  I don't think there is a quick and easy solution.  Not comparing the ID values is not really an option.  We either:

1. Always need to mint service worker identifiers in a single process by running SWM only in the parent process.
2. Or switch to UUIDs and persist the identifiers in the ServiceWorkerRegistrar.

I think (2) is too much work for something we will throw away immediately.  We should just wait for (1) here.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Actually, I take it back.  We can look up the registration using ServiceWorkerDescriptor and return the .active worker.  This is what we did prior to bug 1293277.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Remove ServiceWorkerManager::GetDocumentController() and make ServiceWorkerContainer::GetController() look up the reg instead. r=asuth

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Re: 2, when i reviewed the most recent patch stack I was assuming that there was something in a stack somewhere about using the cache "name" UUID we already store in the registrar.  We already use that as the id for these purposes, albeit hackily:

https://searchfox.org/mozilla-central/rev/e3cba77cee3ff1be38313abe9c804d13c51bd95b/dom/workers/ServiceWorkerManager.cpp#1828

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Hmm, I guess that would make using UUID for the identifier easier.  It would still require work to transition everything over.  Right now we don't have the "cacheName" in ServiceWorkerDescriptor.  So I can't rely on it for the check unless we change the ID everywhere.

Since we ran for years without checking the ID and just using the latest active here, I think we can do that again for a while.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4110f35875f1688ce8f558763595c91c9f5dbce4

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8945187 [details] [diff] [review]
Remove ServiceWorkerManager::GetDocumentController() and make ServiceWorkerContainer::GetController() look up the reg instead. r=asuth

This patch does a couple things:

1. It removes the GetDocumentController() method.  This is part of our overall goal of splitting ServiceWorkerManager from child-side objects.
2. It just gets the registration and returns the currently active worker for the controller.

Step 2 is effectively what we used to do:

https://searchfox.org/mozilla-central/rev/890e126aaac0c0ac0a6fcff8960f9e3cf197a2fc/dom/workers/ServiceWorkerManager.cpp#2846-2854

It also corresponds to how the spec used to be written, although it doesn't use that language any more.  Effectively the current active worker should always be correct here due to how workers are placed in the waiting state unless they call skipWaiting().

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8945187 [details] [diff] [review]
Remove ServiceWorkerManager::GetDocumentController() and make ServiceWorkerContainer::GetController() look up the reg instead. r=asuth

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

Makes sense, definitely an improvement.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbac8c59d58c
Remove ServiceWorkerManager::GetDocumentController() and make ServiceWorkerContainer::GetController() look up the reg instead. r=asuth

Comment 10

[Natalia Csoregi [:nataliaCs]]

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