Closed Bug 1469036 Opened 2 years ago Closed 1 year ago

SpecialPowers.isServiceWorkerRegistered() must check SWM in the parent process if e10s pref is flipped

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: bkelly, Assigned: mrbkap)

References

Details

(Whiteboard: SW-MUST)

Attachments

(1 file)

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.
Priority: -- → P2
Whiteboard: SW-MUST
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 mrbkap@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b876444d90a6
Stop using the ServiceWorkerManager in the child. r=asuth
https://hg.mozilla.org/mozilla-central/rev/b876444d90a6
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
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).
Flags: needinfo?(mrbkap)
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b79f38fa7ee
Stop using the ServiceWorkerManager in the child. r=asuth
https://hg.mozilla.org/mozilla-central/rev/5b79f38fa7ee
Status: REOPENED → RESOLVED
Closed: 1 year ago1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.