1257977 - queue tasks to update attributes on ServiceWorker and ServiceWorkerRegistration

Status: ASSIGNED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently the spec requires us to wait for statechange events to fire before activating directly after installation.  We probably need to wait for any statechange events any time .installing, .waiting, or .active are modified.  I have an issue for this:

  https://github.com/slightlyoff/ServiceWorker/issues/848

Currently we handle this by starting activate asynchronously.  Since we only fire statechange events on the main thread this ensures the event dispatch runnable has been flushed.  Once we implement bug 1113522, however, this will not be adequate.  We will need a more explicit async wait step.

Filing this to tackle after bug 1256428.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

After thinking about this more I think we just need to implement something like this instead:

  https://github.com/slightlyoff/ServiceWorker/issues/860

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0 Make ServiceWorkerRegistrationInfo::mScope const. r=jdm

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make ServiceWorkerRegistrationInfo worker members private. r=jdm

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Move ServiceWorker update logic into central place in ServiceWorkerRegistrationInfo methods. r=jdm

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Track registration DOM instances in ServiceWorkerRegistrationInfo. r=jdm

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Make ServiceWorkerRegistrationMainThread register as a DOM instance. r=jdm

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Make worker registration DOM instances register with the registration info. r=jdm

These patches are all refactoring towards allowing us to update registrations cleanly when the state of the registration changes.  More refactoring to come.  I'm trying to keep the patches small, so there will be lots of them.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Unfortunately I won't have time to complete this before moving to another effort.  I moved the first 3 patches to bug 1263307 to land separately.  The remaining patches are a work-in-progress towards associating DOM ServiceWorkerRegistration objects directly with their backing ServiceWorkerRegistrationInfo objects.  Once that is done we could more easily fire update to keep attributes in sync.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Note this should also fix this issue if its still a problem in our implementation:

  https://github.com/slightlyoff/ServiceWorker/issues/851

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Also we should include this spec issue when this bug is fixed:

  https://github.com/slightlyoff/ServiceWorker/issues/848

Comment 11

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

With my fixes for bug 1113522 we are seeing registration-attribute.https.html change from failing because we don't expose ServiceWorker in workers to failing because the test expects to see updatefound after evaluate but instead it is seeing an "install" event followed by the "updatefound" event.

While there's some degree of bit-rot intersecting this bug, I think this is probably the appropriate bug to address this problem given the related github issue and discussion.

Related bugs of note:

• bug 1510809 changed updatefound resolution time

Comment 12

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

Analysis of the test failure / ordering going on:

• In install in the spec the notable tasks / state updates are:
  • Step 4 invokes Update Registration State is invoked with (registration, "installing", a new worker that was created by update which invoked install). This dispatches a task to all registration objects to set installing. The worker rep comes from get the service worker object which uses the per-global service worker object map which follows a get-or-create idiom.
    • We do not update the ServiceWorker's state if its identity was already known! Only if we are freshly creating the binding for the global in the map. In this "installing" case, we generally expect the ServiceWorker to be freshly created because we have no "evaluating" slot on the registration, but it's conceivable it might be able to use the Clients API to postMessage a client and have its ServiceWorker binding created and exposed in some pathological situations we don't really try to enable.
    • Because this is a task, but because there is no event fired in this step, this change is notionally observable distinct from the next step, but I don't believe there's any requirement that the steps must be something that can be observed as distinct. Like it's not clear there's any way the spec requires to be able to run a task after this task and before the task that will come from the next step. This is notable because we explicitly folded the two steps together, with our registration being responsible for updating the registration and the ServiceWorker state. (That is, mozilla_dom::PServiceWorkerRegistration::UpdateState does both, and PServiceWorker is just a handle for calling PostMessage.
  • Step 5 invokes Update Worker State with (the new worker, "installing"). This dispatches a task to all globals to update the ServiceWorker iff it exists in the map and fires a "statechange" event. As noted above, our implementation folds these together. We also have some support infrastructure with callbacks for the ready promise.
  • Step 7 resolves the job promise which will expose the new worker in the "installing" slot (and the registration and ServiceWorker are coherent since this happens after the 2 steps above))
  • Step 9 iterates over the globals and queues tasks to enumerate their registration objects and fire "updatefound" on them.
  • Step 11 queues firing the "install" event at the ServiceWorker.
• Our mozilla::dom::ServiceWorkerUpdateJob::Install implementation:
  • Step 4 + 5: Triggers the movement from evaluating to installing on the registration (because we do have a concept of evaluating). This ends up calling mozilla::dom::ServiceWorkerRegistrationProxy::UpdateState on the main thread which bounces a runnable to call mozilla::dom::ServiceWorkerRegistrationProxy::UpdateStateOnBGThread on PBackground to the relevant actor (one proxy and one runnable per one actor per global).
  • Step 7: Invoke the result callbacks which should resolve the job promise.
  • Step 9: Initiate the firing of updatefound.
    • We do a problematic thing in mozilla::dom::ServiceWorkerRegistration::MaybeDispatchUpdateFoundRunnable where we explicitly delay the notification by a turn of the event loop.
      • There's a somewhat interesting history here. :bkelly's bug 1462772 gave us the ServiceWorkerRegistration-driven information propagation and then bug 1471929 gave us updatefound logic handling which intentionally diverged from the spec because of the inherent async update problem where parent-process APIs like getRegistrations() provide non-self-updating snapshots that require a roundtrip to get updates for, with us inferring when we should generate an updatefound event. (Noting that a number of IPC improvements since then make it much more feasible to eliminate the need for the roundtrip.) Then :mrbkap's bug 1510809 implementation brought back the explicit updatefound notification but the cleanup in https://phabricator.services.mozilla.com/D13368 left an event loop delay that made sense when we were triggering the "updatefound" notification off of the step 7 promise being resolved because we needed "updatefound" to be its own task that would come after the promise was resolved. It is likely the delay ceased to make sense at that time, although there were child intercept complications that made this all much harder to reason about (and not of our modern searchfox niceties).
      • I believe we still do probably need the updatefound inference logic for now.
  • Step 11: We fire the lifecycle event.
    • Note that until the approved patches on bug 1672493 land, this does follow a different propagation path than the previous steps since there is a bounce to the content process main thread, but it will have been much less race-prone since we moved to a single IPC channel per-process-pair and the "install" event is the one that's subject to additional delays, and it's already the last thing in that sequence and establishes a synchronizing data dependency on the job queue state machine advancing, so the subsequent state transitions can't happen until it is observed on the worker thread.

Comment 13

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

Comment on attachment 8737370 [details] [diff] [review]
P3 Track registration DOM instances in ServiceWorkerRegistrationInfo. r=jdm

I believe these 3 patches were all mooted by the time of bug 1459209 if not before then.

Comment 14

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

Attachment: Bug 1257977 - Fix SWR updatefound event timing. r=#dom-workers!

This corrects a longstanding race in our updatefound logic and makes
testing/web-platform/tests/service-workers/service-worker/ServiceWorkerGlobalScope/registration-attribute.https.html
consistently pass. (Other patches in this stack made the test no
longer permafail and removed the meta .ini, but surfaced the race.)

https://bugzilla.mozilla.org/show_bug.cgi?id=1257977#c12 provides
detailed context, but the basic idea is that bug 1510809 cleaned up
our updatefound logic but left a runnable delay introduced in
bug 1471929 that made sense for the fix there, but stopped making sense
with bug 1510809.

This fix repurposes the FireUpdateFound() method declaration that had
no actual implementing method to call into the private
MaybeDispatchUpdateFound which is part of a sufficiently confusing
internal state machine that it makes sense to leave it private and use
a more sane public name.