Closed Bug 1432640 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Priority: -- → P2
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.
Assignee: bkelly → nobody
Status: ASSIGNED → NEW
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.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
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
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 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().
Attachment #8945187 - Flags: review?(bugmail)
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.
Attachment #8945187 - Flags: review?(bugmail) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbac8c59d58c
Remove ServiceWorkerManager::GetDocumentController() and make ServiceWorkerContainer::GetController() look up the reg instead. r=asuth
https://hg.mozilla.org/mozilla-central/rev/bbac8c59d58c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.