1591101 - Non-idle, non-controlling, and unregistered Service Workers may not shutdown properly

Status: RESOLVED FIXED

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

Description

[Perry Jiang [:perry] [no longer employee, use ni?]]

Service Worker registrations that are unregistered and have no remaining controlled clients are both removed from the ServiceWorkerManager's mRegistrationInfos map and the mControlledClients map. If such a registration's active worker isn't idle, we'll wait for it to become idle and then terminate it.

This is a problem if there's an ordering like:

1. An unregistered registration stops controlling clients, but its active worker isn't idle yet.
2. ServiceWorkerManager begins shutting down Service Workers. It'll miss the registration in 1) because that registration has been removed from ServiceWorkerManager's knowledge.
3. ServiceWorkerShutdownBlocker won't have an accurate count of the pending workers that are shutting down.
4. An assertion possibly fires when the registration from 1) tries to terminate too late.

Comment 1

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1591101 - bookkeep orphaned registrations r?asuth

Part of ServiceWorkerManager cleanup involves blocking shutdown until all
Service Workers are shutdown. The ServiceWorkerManager's data won't contain
non-idle Service Workers that are both unregistered and not controlling clients,
however, so it won't be able to shutdown these Service Workers. Fortunately,
such workers will always be scheduled to shutdown once they become idle, so
we can just make them shutdown blockers as soon as they transition into that
state.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c15753220e0
bookkeep orphaned registrations r=asuth

Comment 4

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/6c15753220e0

Comment 5

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c15753220e0

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

I just hit a crash at this assertion when starting a local-built browser (from a m-c pull today, 11/13) in a debug build. I had just created a new profile and told it to start.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Attachment: backtrace

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

This is repeatable with -profilemanager (you don't have to create a new profile)

Comment 9

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

profile manager sadness is bug 1591353, I've cc'ed you.

Comment 10

[Catalin Sasca, Desktop QA [:csasca]]

Hi Perry, is qa needed here? If so, could you provide us with some steps? Thanks!

Comment 11

[Perry Jiang [:perry] [no longer employee, use ni?]]

Hi Catalin, I don't think QA is needed for this one. Thanks!