Closed Bug 1434701 Opened 2 years ago Closed 2 years ago

make ServiceWorkerRegistration based on ServiceWorkerRegistrationDescriptor

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(11 files, 16 obsolete files)

8.78 KB, patch
catalinb
: review+
Details | Diff | Splinter Review
6.21 KB, patch
asuth
: review+
Details | Diff | Splinter Review
11.44 KB, patch
asuth
: review+
Details | Diff | Splinter Review
5.23 KB, patch
asuth
: review+
Details | Diff | Splinter Review
77.68 KB, patch
asuth
: review+
Details | Diff | Splinter Review
12.64 KB, patch
asuth
: review+
Details | Diff | Splinter Review
11.36 KB, patch
asuth
: review+
Details | Diff | Splinter Review
1.90 KB, patch
asuth
: review+
Details | Diff | Splinter Review
30.69 KB, patch
asuth
: review+
Details | Diff | Splinter Review
6.32 KB, patch
asuth
: review+
Details | Diff | Splinter Review
38.01 KB, patch
asuth
: review+
Details | Diff | Splinter Review
This bug is to use ServiceWorkerRegistrationDescriptor in ServiceWorkerRegistration.  Also, it will try to make ServiceWorkerRegistration not touch the ServiceWorkerManager.
Priority: -- → P2
Attached patch wip (obsolete) — Splinter Review
Work-in-progress.
Attached patch hmm (obsolete) — Splinter Review
Attachment #8947653 - Attachment is obsolete: true
Attached patch hmm (obsolete) — Splinter Review
Attachment #8948821 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
I may add one more patch, but the current patches are pretty stable.  I'm going to start flagging them for review.
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.
Attachment #8947212 - Flags: review?(catalin.badea392)
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.
Attachment #8947247 - Flags: review?(catalin.badea392)
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.
Attachment #8947258 - Flags: review?(catalin.badea392)
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.
Attachment #8947563 - Flags: review?(catalin.badea392)
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.
Attachment #8947910 - Flags: review?(catalin.badea392)
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.
Attachment #8948753 - Flags: review?(catalin.badea392)
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.
Attachment #8949192 - Flags: review?(catalin.badea392)
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.
Attachment #8949193 - Flags: review?(catalin.badea392)
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.
Attachment #8949197 - Flags: review?(catalin.badea392)
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.
Attachment #8949200 - Flags: review?(catalin.badea392)
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.
Attachment #8949198 - Flags: review?(catalin.badea392)
Depends on: 1436763
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.
Attachment #8948753 - Attachment is obsolete: true
Attachment #8948753 - Flags: review?(catalin.badea392)
Attachment #8949430 - Flags: review?(catalin.badea392)
Comment on attachment 8949201 [details] [diff] [review]
wip

I'm not going to do this one here.  Lets just get these patches landed first.
Attachment #8949201 - Attachment is obsolete: true
Blocks: 1436812
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.
Except I can't do that change until Andrew opens his review queue again.  Andrew, can you steal these reviews when you get back?
Flags: needinfo?(bugmail)
Attachment #8947212 - Flags: review?(catalin.badea392) → review+
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.
Attachment #8947247 - Flags: review?(catalin.badea392) → review+
Attachment #8947258 - Flags: review?(catalin.badea392) → review+
Attachment #8947563 - Flags: review?(catalin.badea392) → review+
Attachment #8947910 - Flags: review?(catalin.badea392) → review+
Attachment #8949430 - Flags: review?(catalin.badea392) → review+
Attachment #8949192 - Flags: review?(catalin.badea392) → review+
Attachment #8949193 - Flags: review?(catalin.badea392) → review+
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"
Attachment #8949200 - Flags: review?(catalin.badea392) → review+
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/
Attachment #8949197 - Flags: review?(catalin.badea392) → review+
Attachment #8949198 - Flags: review?(catalin.badea392) → review+
Pushed by bkelly@mozilla.com:
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
Sorry, I forgot to fix the nits in my rush to fix all the commit messages for the different r=.
Flags: needinfo?(bugmail)
Depends on: 1439212
No longer depends on: 1439212
You need to log in before you can comment on or make changes to this bug.