1434701 - make ServiceWorkerRegistration based on ServiceWorkerRegistrationDescriptor

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

This bug is to use ServiceWorkerRegistrationDescriptor in ServiceWorkerRegistration.  Also, it will try to make ServiceWorkerRegistration not touch the ServiceWorkerManager.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make ServiceWorkerRegistration::CreateForMainThread() take a ServiceWorkerRegistrationDescriptor. r=catalinb

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Don't expose internal IPC headers in ServiceWorkerRegistrationDescriptor.h and other minor fixes. r=catalinb

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Pass ServiceWorkerRegistrationDescriptor to ServiceWorkerRegistration::CreateForWorker(). r=catalinb

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Pass ServiceWorkerRegistrationDescriptor to ServiceWorkerRegistration::CreateForWorker(). r=catalinb

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Move ServiceWorkerRegistrationListener into its own header. r=catalinb

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Work-in-progress.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Move main thread and worker implementation code into ServiceWorkerRegistrationImpl.cpp. r=catalinb

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Make ServiceWorkerRegistrationListener updates take a ServiceWorkerRegistrationDescriptor. r=catalinb

This patch still needs some more cleanup.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Make ServiceWorkerRegistrationListener updates take a ServiceWorkerRegistrationDescriptor. r=catalinb

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Make ServiceWorkerRegistrationListener updates take a ServiceWorkerRegistrationDescriptor. r=catalinb

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Store the registration descriptor in ServiceWorkerRegistration. r=catalinb

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Store the registration descriptor in ServiceWorkerRegistration. r=catalinb

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Make ServiceWorkerRegistrationListener updates take a ServiceWorkerRegistrationDescriptor. r=catalinb

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Store the registration descriptor in ServiceWorkerRegistration. r=catalinb

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Make ServiceWorkerRegistration own the ServiceWorker references itself and handle the descriptor update. r=catalinb

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P9 move GetScope() and UpdateViaCache() into ServiceWorkerRegistration. r=catalinb

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Make ServiceWorkerRegistration own the ServiceWorker references itself and handle the descriptor update. r=catalinb

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: hmm

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: hmm

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Store the registration descriptor in ServiceWorkerRegistration. r=catalinb

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Make ServiceWorkerRegistration own the ServiceWorker references itself and handle the descriptor update. r=catalinb

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P9 move UpdateViaCache() into ServiceWorkerRegistration. r=catalinb

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P10 Fix ServiceWorker*Descriptor assinment to not crash when assigning a descriptor to itself. r=catalinb

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P11 Make ServiceWorkerDescriptor use an Inner class with main and worker thread implementations. r=catalinb

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P9 move UpdateViaCache() into ServiceWorkerRegistration. r=catalinb

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f85f49de96fd41bc5de41abacc8e4d80fcce0225

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

I may add one more patch, but the current patches are pretty stable.  I'm going to start flagging them for review.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8947212 [details] [diff] [review]
P1 Make ServiceWorkerRegistration::CreateForMainThread() take a ServiceWorkerRegistrationDescriptor. r=catalinb

Catalin, this is the first of many patches to incrementally make ServiceWorkerDescriptor a concrete class defined by a thread-safe ServiceWorkerRegistrationDescriptor.  There are number of further refactorings to do in the future, but I tried to limit changes to only what is necessary here.

This patch starts by passing the ServiceWorkerRegistrationDescriptor into the main thread creation path.

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8947247 [details] [diff] [review]
P2 Don't expose internal IPC headers in ServiceWorkerRegistrationDescriptor.h and other minor fixes. r=catalinb

This patch moves the IPCServiceWorkerRegistrationDescriptor.h include into the ServiceWorkerRegistrationDescriptor.cpp file.  We don't want to export this into all directories that might end up including ServiceWorkerRegistrationDescriptor.h since it makes them need some extra bits in their moz.build.

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8947258 [details] [diff] [review]
P3 Pass ServiceWorkerRegistrationDescriptor to ServiceWorkerRegistration::CreateForWorker(). r=catalinb

This path adds the ServiceWorkerRegistrationDescriptor the worker creation path.  This requires setting the descriptor on the WorkerPrivate so we have it available when it comes time to create the ServiceWorkerGlobalScope.

Note, this patch also makes us eagerly allocate the ServiceWorkerRegistration binding object that will be used for `self.registration`.  This is necessary for a couple reasons:

1. The very first line of the service worker script may access `self.registration` synchronously.
2. We need to receive updates for the registration's various worker properties.  If we create the binding object immediately we get those updates as part of its normal update process.  If we try to lazy allocate as we did before, then we need to build a separate path to update the descriptor on the WorkerPrivate.  Its less code just to allocate the binding object immediately.

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8947563 [details] [diff] [review]
P4 Move ServiceWorkerRegistrationListener into its own header. r=catalinb

To make it easier to reason about ServiceWorkerRegistration as a concrete class I want to move the the various implementation detail bits out into separate code modules.  The first step here is to move ServiceWorkerRegistrationListener to a separate header.

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8947910 [details] [diff] [review]
P5 Move main thread and worker implementation code into ServiceWorkerRegistrationImpl.cpp. r=catalinb

This patch then moves all of the main thread and worker thread specific implementation details into a separate Imp .h/.cpp pair.  None of the contents of the implementation code changes yet, though.

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8948753 [details] [diff] [review]
P6 Make ServiceWorkerRegistrationListener updates take a ServiceWorkerRegistrationDescriptor. r=catalinb

This patch refactors the way we set and update the worker properties on the registration.

In the past we would lazy allocate the ServiceWorker binding objects the first time we accessed the property and then cache that object on the registration.  If the state changed, then we simply invalidated the cached objects.  This approach assumed synchronous access to SWM to get the ServiceWorker binding objects, but that does not hold for worker thread or a remote IPC implementation.

This patch refactors the worker properties in these ways:

1. We allocate the ServiceWorker binding objects eagerly based on the descriptor.  This lets us to get worker .state updates automatically as we did with the registration on the WorkerPrivate, etc.
2. We manage changes to the registration by simply updating the descriptor and resetting the workers.
3. This allows us to get rid of the old mechanism for getting the workers from SWM and the ServiceWorkerCommon enumeration type, etc.
4. We refactor ServiceWorkerRegistrationInfo to trigger the registration descriptor update in a consistent pattern.  We always update the worker members, call UpdateRegistrationState(), and then call any necessary UpdateWorkerState() methods.  This matches the order of updates in the spec.

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8949192 [details] [diff] [review]
P7 Store the registration descriptor in ServiceWorkerRegistration. r=catalinb

This stores the full descriptor in the ServiceWorkerRegistration binding object.

Ideally this would remove the need for an mScope member, but I found doing that had a large ripple effect.  Unfortunately a lot of the registration code uses a wide nsString scope, but the descriptor uses the more efficient nsCString scope.

To minimize patch size I just store the wide nsString scope in the Impl classes for now.  In the future we should refactor all this code to use nsCString for scope, but lets leave that to a separate bug.

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8949193 [details] [diff] [review]
P8 Make ServiceWorkerRegistration own the ServiceWorker references itself and handle the descriptor update. r=catalinb

This patch moves the worker members from the Impl classes to ServiceWorkerRegistration itself.

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8949197 [details] [diff] [review]
P10 Fix ServiceWorker*Descriptor assinment to not crash when assigning a descriptor to itself. r=catalinb

While working on this bug I ran into a case where I called UpdateState() with a descriptor that was actually the same descriptor owned by the registration.  So it was doing `foo = foo`.

It turns out the descriptor classes have a bug when you try to assign an object to itself.  They call UniquePtr::reset() and then try to copy the underlying type.  Of course, when both the left and right side are the same object this resets the source of the copy as well.

The solution here is to short-circuit the self-assignment since it should be a no-op.

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8949200 [details] [diff] [review]
P9 move UpdateViaCache() into ServiceWorkerRegistration. r=catalinb

This patch moves the GetUpdateViaCache() method into ServiceWorkerRegistration and makes it use the descriptor for its value.  It also makes ServiceWorkerRegistrationInfo::SetUpdateViaCache() trigger a registration update to keep things in sync.

Note, we had a mochitest that expected the InvalidStateError to be thrown on a detached object.  We now correctly continue to return the enum, so this patch also corrects the test.

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8949198 [details] [diff] [review]
P11 Make ServiceWorkerDescriptor use an Inner class with main and worker thread implementations. r=catalinb

This patch finally removes the class inheritance from ServiceWorkerRegistration.  Instead, the Impl classes implement an abstract Inner interface.  The ServiceWorkerRegistration delegates to the mInner that is provided to it.  The ServiceWorkerRegistration also sets itself on the Impl classes so they can call back to the outer binding object.

This is what this bug has largely been driving at.  In a follow-on bug I will implement a third Inner implementation that goes to a PBackground actor instead of the current MT/worker impls.

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Make ServiceWorkerRegistrationListener updates take a ServiceWorkerRegistrationDescriptor. r=catalinb

I updated this patch to fix the detached-context.https.html WPT expectations now that I have this bug depending on bug 1436763.

The reason this test was previously failing was that our lazy initialization of the service worker properties was failing when the global was detached.  This test expected the properties to still exist.  We then needed bug 1436763 to fix the promise rejection expectation to avoid timing out.

See comment 34 for the description of this patch.

Comment 41

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8949201 [details] [diff] [review]
wip

I'm not going to do this one here.  Lets just get these patches landed first.

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

I didn't realize Catalin was on PTO at the end of this week as well.  Since Andrew is back next week and has reviewed a lot of the work up to this point I think I'm just going to switch the reviews back.

Comment 43

[Ben Kelly [:bkelly, not reviewing]]

Except I can't do that change until Andrew opens his review queue again.  Andrew, can you steal these reviews when you get back?

Comment 44

[Cătălin Badea (:catalinb)]

ah, didn't read the last comment and started going through the patches. If Andrew is available, he's probably more suited to review this.

Comment 45

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

Comment on attachment 8949200 [details] [diff] [review]
P9 move UpdateViaCache() into ServiceWorkerRegistration. r=catalinb

Review of attachment 8949200 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/serviceworkers/test/test_bug1408734.html
@@ +45,5 @@
>  
>    // call unregister()
>    await registration.unregister();
>  
>    // access registration.updateViaCache to trigger the bug

nit: remove "In the future we will fix"

Comment 46

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

Comment on attachment 8949197 [details] [diff] [review]
P10 Fix ServiceWorker*Descriptor assinment to not crash when assigning a descriptor to itself. r=catalinb

Review of attachment 8949197 [details] [diff] [review]:
-----------------------------------------------------------------

commit message nit: s/assinment/assignment/

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84f2405b197a
P1 Make ServiceWorkerRegistration::CreateForMainThread() take a ServiceWorkerRegistrationDescriptor. r=catalinb r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef90ec65a2e3
P2 Don't expose internal IPC headers in ServiceWorkerRegistrationDescriptor.h and other minor fixes. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/14e52519867e
P3 Pass ServiceWorkerRegistrationDescriptor to ServiceWorkerRegistration::CreateForWorker(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2f19419e6cc
P4 Move ServiceWorkerRegistrationListener into its own header. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/1538099138c1
P5 Move main thread and worker implementation code into ServiceWorkerRegistrationImpl.cpp. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/1746f05ce7e6
P6 Make ServiceWorkerRegistrationListener updates take a ServiceWorkerRegistrationDescriptor. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ffbafa80255
P7 Store the registration descriptor in ServiceWorkerRegistration. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbeeb35b0c8e
P8 Make ServiceWorkerRegistration own the ServiceWorker references itself and handle the descriptor update. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c150999cc4b
P9 move UpdateViaCache() into ServiceWorkerRegistration. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/142e6dc5ea6d
P10 Fix ServiceWorker*Descriptor assinment to not crash when assigning a descriptor to itself. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/405e4d2846f3
P11 Make ServiceWorkerDescriptor use an Inner class with main and worker thread implementations. r=asuth

Comment 48

[Ben Kelly [:bkelly, not reviewing]]

Sorry, I forgot to fix the nits in my rush to fix all the commit messages for the different r=.

Comment 49

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/84f2405b197a
https://hg.mozilla.org/mozilla-central/rev/ef90ec65a2e3
https://hg.mozilla.org/mozilla-central/rev/14e52519867e
https://hg.mozilla.org/mozilla-central/rev/d2f19419e6cc
https://hg.mozilla.org/mozilla-central/rev/1538099138c1
https://hg.mozilla.org/mozilla-central/rev/1746f05ce7e6
https://hg.mozilla.org/mozilla-central/rev/2ffbafa80255
https://hg.mozilla.org/mozilla-central/rev/cbeeb35b0c8e
https://hg.mozilla.org/mozilla-central/rev/9c150999cc4b
https://hg.mozilla.org/mozilla-central/rev/142e6dc5ea6d
https://hg.mozilla.org/mozilla-central/rev/405e4d2846f3