Closed Bug 1469036 Opened 2 years ago Closed 1 year ago
Powers .is Service Worker Registered() must check SWM in the parent process if e10s pref is flipped
47 bytes, text/x-phabricator-request
|Details | Review|
We have some mochitest sanity checking code that uses SpecialPowers.isServiceWorkerRegistered() to determine if any service workers have been left lying around. Unfortunately this uses the immediate process which is incompatible with the new SW e10s mode. When the pref is flipped it will have to go to the parent process instead. One complication is that this API is currently defined as synchronous. I'm not really going to tackle this one for now. It will have to be fixed before the pref is enabled, though.
This is currently a boolean check that runs after every test. The "obvious" fix would be to make it asynchronous and wait for the response from the parent but I'm worried that might have a noticeable impact given how many tests we run.
Since there's two sides to SpecialPowers (like the Schwartz?), we could have the parent part send "there's now >= 1 SWs" and "there's now 0 SWs again" and store them in the child in a boolean, which is what we check. The theory is that all good tests should have asynchronously unregistered their SWs and properly waited for that before terminating, so a synchronous check becomes acceptable. Note that the current check is actually weaker than one might desire. At least until resurrection in particular is eliminated (https://github.com/w3c/ServiceWorker/issues/1204), registrations can be resurrected and cause a headache. And our GetAllRegistrations check at https://searchfox.org/mozilla-central/source/dom/serviceworkers/ServiceWorkerManager.cpp#2810 pretends like pending uninstalls don't exist for check purposes. So we do need to decide when we're actually going to send the "0" message.
(In reply to Andrew Sutherland [:asuth] from comment #2) > for check purposes. So we do need to decide when we're actually going to > send the "0" message. I punted on this question for now. It looks like resurrection is going away soon, anyway, so it doesn't seem worth it to spend time worrying about it.
Assignee: nobody → mrbkap
With parent intercept enabled, the source of truth for ServiceWorker registrations lives in the parent process. In order to avoid sync messages or having to wait after every test, we count the number of active registrations in the parent and inform the children when there is or isn't a registration. In non-parent-intercept mode, the parent finds out about new service worker registrations too late, so we have to keep the old code hanging around for the time being.
(In reply to Blake Kaplan (:mrbkap) from comment #4) > (In reply to Andrew Sutherland [:asuth] from comment #2) > > for check purposes. So we do need to decide when we're actually going to > > send the "0" message. > > I punted on this question for now. It looks like resurrection is going away > soon, anyway, so it doesn't seem worth it to spend time worrying about it. The way you implemented it (for "parent_intercept"=true), we send the "0" when the registration is actually removed and resurrection is no longer possible. This is very good from the perspective of making sure our tests aren't flaky because it's also checking that the SW is idle! The bad news is it may mean that more tests turn orange until we fix them because they fail to ensure that their controlled clients are dead and that the SW is idle before concluding. I made this cool diagram https://clicky.visophyte.org/files/screenshots/20181119-112731.png to help express the cascade of events that can result in the listener notification you're adding being called and not just because I can. (Okay, 50/50.)
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/b876444d90a6 Stop using the ServiceWorkerManager in the child. r=asuth
Backed out changeset b876444d90a6 (Bug 1469036) for causing mochitest failures on test_console_serviceworker_cached.html and test_devtools_track_serviceworker_time.html a=backout Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=2ac85aee845090097edf39e8d468f2551fbc06bf&selectedJob=212806419&searchStr=Linux%2Cx64%2Cdebug%2Cserviceworker Failure log e.g.: https://treeherder.mozilla.org/logviewer.html#?job_id=212806419&repo=mozilla-central&lineNumber=185154 https://treeherder.mozilla.org/logviewer.html#?job_id=212806421&repo=mozilla-central https://treeherder.mozilla.org/logviewer.html#?job_id=212806569&repo=mozilla-central Backout push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=535a9477461a0ed42b2a8d5cc26fef87095443ea
Sorry about that! I guess the sw-e10s stuff doesn't run by default on try? The problem is exactly what Andrew was worried about in comment 6: ServiceWorkerRegistration.unregister's promise resolves before we call the ServiceWorkerManager observer and the resulting notifications race to tell the child about the updated state. We definitely want the test to be predicated on the stronger guarantee of where we notify the SWM observers, but can't change the promise's resolution and this all races because we need to bounce off of the PBackground thread in the parent process. I've pushed a patch to try that adds a sync message that will only be called if we get to the end of a test and still think that we have a registered service worker, in which case we'll double-check to see if we lost a race. I think it's an OK tradeoff on test performance and semantics (this should be rare and limited to tests that register service workers).
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/5b79f38fa7ee Stop using the ServiceWorkerManager in the child. r=asuth
You need to log in before you can comment on or make changes to this bug.