Closed Bug 1436812 Opened 6 years ago Closed 6 years ago

Split ServiceWorkerContainer into an inner and outer object

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(9 files, 38 obsolete files)

4.85 KB, patch
baku
: review+
Details | Diff | Splinter Review
1.75 KB, patch
baku
: review+
Details | Diff | Splinter Review
3.82 KB, patch
baku
: review+
Details | Diff | Splinter Review
5.55 KB, patch
baku
: review+
Details | Diff | Splinter Review
10.11 KB, patch
baku
: review+
Details | Diff | Splinter Review
4.43 KB, patch
baku
: review+
Details | Diff | Splinter Review
15.29 KB, patch
baku
: review+
Details | Diff | Splinter Review
5.62 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
7.50 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
Previous bugs have split ServiceWorker and ServiceWorkerRegistration into inner/outer objects.  The next step is to split ServiceWorkerContainer into inner/outer objects as well.
Priority: -- → P2
I think the trickiest part of this bug will be the ready promise.  The spec has some weirdness:

https://github.com/w3c/ServiceWorker/issues/1278

And our implementation mixes ServiceWorkerContainer/ServiceWorkerManager/nsPIDOMWindowInner together.  I'll need to disentangle all that to make a clean interface that can be remoted.
Attached patch ready_wip.patch (obsolete) — Splinter Review
Depends on: 1438211
Since I keep getting interrupted by stuff at home I'm going to split this bug up a bit so I can land it in smaller chunks.  These 4 patches look good, so I'll move them to a new bug and ask for review there.
Depends on: 1448012
Comment on attachment 8960239 [details] [diff] [review]
P1 Add ServiceWorkerContainer::Create() factory method. r=asuth

This patch is just a small refactor to change ServiceWorkerContainer to use a factory method for creation.  We will use this in the future to select the kind of inner object (same-process vs remote) based on a pref.
Attachment #8960239 - Flags: review?(bugmail)
Comment on attachment 8960241 [details] [diff] [review]
P2 Eagerly create the ServiceWorkerContainer.controller. r=asuth

This patch makes the container eagerly allocate the controller ServiceWorker binding object.  This is necessary for a few reasons:

1. We need it available synchonously for the .controller getter.
2. Once we are remote we cannot just ask the SWM for it on demand.
3. We want to make sure the binding object gets updates if the state of the ServiceWorker changes, etc.  The easiest way to do this is to create the binding object and let it request these updates from the parent process.
Attachment #8960241 - Flags: review?(bugmail)
Whoops... I am flagging for review on the wrong bug.  Sorry.
Attachment #8960239 - Attachment is obsolete: true
Attachment #8960239 - Flags: review?(bugmail)
Attachment #8960241 - Attachment is obsolete: true
Attachment #8960241 - Flags: review?(bugmail)
Attachment #8961156 - Attachment is obsolete: true
Attachment #8961157 - Attachment is obsolete: true
This currently causes a crash in detached-context.https.html.  I'll have to investigate.
I still need to do the register() implementation, but I can start requesting/landing the first patches here.  Going to flag for review since try is green.
Comment on attachment 8967497 [details] [diff] [review]
P1 Add a ServiceWorkerContainer::Inner scaffold. r=baku

Andrea, this patch just stubs out an "inner" class for ServiceWorkerContainer.

The overall goal of this bug is to migrate all the ServiceWorkerContainer methods over to an implementation that calls through the inner class.  Then we can swap out the inner implementation in a later bug to remote to the parent process.

Note, since we want to be able to remote to another process we need to make sure the inner class neither takes arguments or returns values directly tied to the binding layer.  So part of this bug will be switching SWM over to use MozPromise internally instead of Promise for these operations, etc.
Attachment #8967497 - Flags: review?(amarchesini)
Comment on attachment 8968249 [details] [diff] [review]
P2 Implement ServiceWorkerContainer::GetReady() via the Inner class. r=baku

This implements the GetReady() method using the inner class.  This is pretty simple since I already refactored SWM to use a MozPromise for the ready stuff.
Attachment #8968249 - Flags: review?(amarchesini)
Comment on attachment 8968250 [details] [diff] [review]
P3 Implement ServiceWorkerContainer::GetRegistration() via the inner clas. r=baku

This migrates getRegistration() over to an inner using a MozPromise.
Attachment #8968250 - Flags: review?(amarchesini)
Comment on attachment 8968251 [details] [diff] [review]
P4 Factor out a method for checking global validity in ServiceWorkerContainer to reduce code duplication. r=baku

I realized that a lot of this code will have to verify that storage access is allowed for the window.  I decided to move this into a helper method that could be re-used in ServiceWorkerContainer.
Attachment #8968251 - Flags: review?(amarchesini)
Comment on attachment 8968252 [details] [diff] [review]
P5 Implement ServiceWorkerContainer::GetRegistrations() via the inner class. r=baku

This implements getRegistrations() using the inner class.
Attachment #8968252 - Flags: review?(amarchesini)
Attached patch register() wip (obsolete) — Splinter Review
This is my work-in-progress on moving register().  Its taking a bit longer because I have to disentangle some tight coupling between the container and SWM.
Comment on attachment 8967497 [details] [diff] [review]
P1 Add a ServiceWorkerContainer::Inner scaffold. r=baku

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

::: dom/serviceworkers/ServiceWorkerContainer.cpp
@@ +62,5 @@
>  ServiceWorkerContainer::Create(nsIGlobalObject* aGlobal)
>  {
> +  RefPtr<Inner> inner = new ServiceWorkerContainerImpl();
> +  RefPtr<ServiceWorkerContainer> ref =
> +    new ServiceWorkerContainer(aGlobal, inner);

inner.forget()

@@ +67,5 @@
>    return ref.forget();
>  }
>  
> +ServiceWorkerContainer::ServiceWorkerContainer(nsIGlobalObject* aGlobal,
> +                                               ServiceWorkerContainer::Inner* aInner)

already_AddRefed<ServiceWorkerContainer::Inner> aInner

@@ +72,2 @@
>    : DOMEventTargetHelper(aGlobal)
> +  , mInner(aInner)

Move()

::: dom/serviceworkers/ServiceWorkerContainerImpl.cpp
@@ +8,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +ServiceWorkerContainerImpl::~ServiceWorkerContainerImpl()

= default; but I guess it will change in the next patches.
Attachment #8967497 - Flags: review?(amarchesini) → review+
Attachment #8968249 - Flags: review?(amarchesini) → review+
Attachment #8968250 - Flags: review?(amarchesini) → review+
Attachment #8968251 - Flags: review?(amarchesini) → review+
Attachment #8968252 - Flags: review?(amarchesini) → review+
Depends on: 1454646
Attachment #8967497 - Attachment is obsolete: true
Attachment #8968249 - Attachment is obsolete: true
Attachment #8968250 - Attachment is obsolete: true
Attachment #8968251 - Attachment is obsolete: true
Attachment #8968252 - Attachment is obsolete: true
Attached patch register() wip (obsolete) — Splinter Review
This passes mochitests, but still fails some WPT tests:

/service-workers/service-worker/registration-security-error.https.html
  FAIL Registering script outside domain - assert_throws: Registration script outside domain should fail with SecurityError. function "function() { throw e }" threw object "[Exception... "<no message>"  nsresult: "0x805e0006 (<unknown>)"  location: "JS frame :: https://web-platform.test:8443/service-workers/service-worker/registration-security-error.https.html :: <TOP_LEVEL> :: line 8"  data: no]" that is not a DOMException SecurityError: property "code" is equal to undefined, expected 18
registration_tests_security_error/<@https://web-platform.test:8443/service-workers/service-worker/resources/registration-tests-security-error.js:35:14
Test.prototype.step@https://web-platform.test:8443/resources/testharness.js:1501:20
promise_test/tests.promise_tests<@https://web-platform.test:8443/resources/testharness.js:543:27
promise callback*promise_test@https://web-platform.test:8443/resources/testharness.js:539:31
registration_tests_security_error@https://web-platform.test:8443/service-workers/service-worker/resources/registration-tests-security-error.js:32:3
@https://web-platform.test:8443/service-workers/service-worker/registration-security-error.https.html:8:1
  FAIL Script URL is same-origin filesystem: URL - assert_throws: Registering a script which has same-origin filesystem: URL should fail with SecurityError. function "function() { throw e }" threw object "[Exception... "<no message>"  nsresult: "0x805e0006 (<unknown>)"  location: "JS frame :: https://web-platform.test:8443/service-workers/service-worker/registration-security-error.https.html :: <TOP_LEVEL> :: line 8"  data: no]" that is not a DOMException SecurityError: property "code" is equal to undefined, expected 18
registration_tests_security_error/<@https://web-platform.test:8443/service-workers/service-worker/resources/registration-tests-security-error.js:72:14
Test.prototype.step@https://web-platform.test:8443/resources/testharness.js:1501:20
promise_test/tests.promise_tests<@https://web-platform.test:8443/resources/testharness.js:543:27
promise callback*promise_test@https://web-platform.test:8443/resources/testharness.js:539:31
registration_tests_security_error@https://web-platform.test:8443/service-workers/service-worker/resources/registration-tests-security-error.js:69:3
@https://web-platform.test:8443/service-workers/service-worker/registration-security-error.https.html:8:1
/service-workers/service-worker/rejections.https.html
  FAIL Rejections are DOMExceptions - assert_true: expected true got false
@https://web-platform.test:8443/service-workers/service-worker/rejections.https.html:14:17
Test.prototype.step@https://web-platform.test:8443/resources/testharness.js:1501:20
Test.prototype.step_func/<@https://web-platform.test:8443/resources/testharness.js:1525:20
promise callback*@https://web-platform.test:8443/service-workers/service-worker/rejections.https.html:11:9
Test.prototype.step@https://web-platform.test:8443/resources/testharness.js:1501:20
@https://web-platform.test:8443/service-workers/service-worker/rejections.https.html:9:5
@https://web-platform.test:8443/service-workers/service-worker/rejections.https.html:7:2
Attachment #8968327 - Attachment is obsolete: true
Also, there is a good chance I will need bug 1357463 to do this right.
Depends on: 1357463
Attached patch register() wip (obsolete) — Splinter Review
This passes tests locally but still needs some cleanup and patch splitting.
Attachment #8968666 - Attachment is obsolete: true
Actually, I won't need bug 1357463 yet.  We might need it when we add the IPC layer to the service worker binding code.
No longer depends on: 1357463
Attached patch register() wip (obsolete) — Splinter Review
Attachment #8968956 - Attachment is obsolete: true
Attached patch register() wip (obsolete) — Splinter Review
Attachment #8969036 - Attachment is obsolete: true
See Also: → 1455077
Attached patch work-in-progress (obsolete) — Splinter Review
Attachment #8969092 - Attachment is obsolete: true
Attached patch work-in-progress (obsolete) — Splinter Review
Attachment #8969383 - Attachment is obsolete: true
Attached patch work-in-progress (obsolete) — Splinter Review
Attachment #8969412 - Attachment is obsolete: true
It seems I broke something while patch splitting.  I'll have to investigate the orange.
Comment on attachment 8968939 [details] [diff] [review]
P1 Make ServiceWorker MozPromise types exclusive so they can reject with ErrorResult values. r=baku

Andrea, the ServiceWorkerContainer::Register() method is a bit of a mess, so I'm going to do the split in a series of patches.

The first step is to make the MozPromise types for service worker registrations reject with ErrorResult.  We need this in order to preserve TypeError messages that are produces in the ServiceWorkerManager job code.

Its a bit tricky using ErrorResult with MozPromise because:

1. ErrorResult does not have a copy constructor.
2. ErrorResult will assert in its destructor if its failed, but not consumed.

I've discovered I can mostly make this work by using the MozPromise exclusive flag and by always allowing the MozPromise to resolve/reject fully.  We can't use its Disconnect() feature.
Attachment #8968939 - Flags: review?(amarchesini)
Comment on attachment 8969076 [details] [diff] [review]
P2 Don't try to use the document's nsILoadGroup when registering a new service worker script. r=baku

This removes some code that attached the document's nsILoadGroup to the initial service worker fetch and execution.  I believe this was originally needed to deal with firefox os security checking, but we don't need it any more.  I also think its wrong if this load group cancels the register operation when the window is closed.  It should really complete.
Attachment #8969076 - Flags: review?(amarchesini)
Comment on attachment 8969082 [details] [diff] [review]
P3 Refactor ServiceWorkerContainer::GetGlobalIfValid() to accomodate Register() usage. r=baku

While I previously factored out the storage permission check in GetGlobalIfValid() I forgot that the exact console report message on failure needs to be different depending on where it is used.  There are tests that verify this is correct (thankfully).

To support the different message lets provide a lambda hook to perform the error report instead of hard coding it.

This also changes the chrome script assertion in GetGlobalIfValid() to a runtime check to match what Register() needs.
Attachment #8969082 - Flags: review?(amarchesini)
Comment on attachment 8969382 [details] [diff] [review]
P4 Add the ServiceWorkerContainer::GetBaseURIFromGlobal() method. r=baku

While splitting this stuff apart I have been trying to remove as many window-specific pieces of code as I can.  In the near future we will need to support worker globals as well.

Getting the base URL is one thing we'll have to correct as part of that effort.  We have bug 1432481 filed to make this easier.

For now, this patch moves the base URL stuff off into a separate static function to make it easier to see what else needs the window/doc references.
Attachment #8969382 - Flags: review?(amarchesini)
Comment on attachment 8969411 [details] [diff] [review]
P5 Move the IsFromAuthenticatedOrigin() check from the ServiceWorkerManager to the ServiceWorkerContainer. r=baku

One of the recurring issues in this bug are security checks performed in ServiceWorkerManager that require the nsIDocument or window pointer.  We can't do that in the future when SWM is in the parent process.  Whenever possible I am converting it to a non-window-specific check, but some cannot.

The IsFromAuthenticatedOrigin() code is one such check.  It wants to walk the iframe parent tree.  So this patch moves the check from SWM to ServiceWorkerContainer.

This whole bit of code is kind of bad, though.  We should really move this into a [Func] to better reflect how [SecureContext] is supposed to work in webidl.  The interface should not even be exposed if its not secure.  See bug 1455078.
Attachment #8969411 - Flags: review?(amarchesini)
Comment on attachment 8969415 [details] [diff] [review]
P6 Move the NS_CheckContentLoadPolicy() from ServiceWorkerManager to ServiceWorkerContainer. r=baku

This patch moves another security check, NS_CheckContentLoadPolicy(), from SWM to the binding object.  We must do this in the binding layer so that the check can use the nsIDocument's exact principal to get the CSP value.

Once bug 965637 is fixed this can be changed to use ClientInfo for CSP and performed in the validation function I add in a later patch here.
Attachment #8969415 - Flags: review?(amarchesini)
Comment on attachment 8969638 [details] [diff] [review]
P7 Move many checks into ServiceWorkerScopeAndScriptAreValid() utility method. r=baku

This patch takes the remaining validity checks from both the container and SWM and wraps them into a utility method.  It:

1. Checks for http/https schemes only.
2. Checks for illegal escaped characters.
3. Verifies any URL ref has already been stripped.
4. Verifies the client's principal can load the URL.  (Effectively same-origin check.)

For now this patch calls this only in ServiceWorkerContainer.  The last patch in this bug will also make SWM call this method.  When we are in e10s mode we want the SWM to double-check the validity to avoid a content process lying to us.
Attachment #8969638 - Flags: review?(amarchesini)
Comment on attachment 8969677 [details] [diff] [review]
P8 Move storage permission check and other window specific code to ServiceWorkerContainer. r=baku

Andrea, this removes the storage permission check from SWM and makes ServiceWorkerContainer::Register() do it via GetGlobalIfValid().
Attachment #8969677 - Flags: review?(amarchesini)
Comment on attachment 8969678 [details] [diff] [review]
P9 Implement the ServiceWorkerContainer::Register() method using the inner class. r=baku

Finally, this implements the remainder of the changes.  It:

1. Makes ServiceWorkerContainer::Register() call through the inner class to SWM.
2. Removes the xpcom register() method.
3. Calls the validation function in the SWM.
4. Makes the job callback trigger the MozPromise, etc.
Attachment #8969678 - Flags: review?(amarchesini)
Attachment #8968939 - Flags: review?(amarchesini) → review+
Attachment #8969076 - Flags: review?(amarchesini) → review+
Attachment #8969382 - Flags: review?(amarchesini) → review+
Comment on attachment 8969411 [details] [diff] [review]
P5 Move the IsFromAuthenticatedOrigin() check from the ServiceWorkerManager to the ServiceWorkerContainer. r=baku

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

::: dom/serviceworkers/ServiceWorkerContainer.cpp
@@ +276,5 @@
> +    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> +    return nullptr;
> +  }
> +
> +  // Next, implement a lame version of [SecureContext[ with an

[SecureContext]
Attachment #8969411 - Flags: review?(amarchesini) → review+
Attachment #8969415 - Flags: review?(amarchesini) → review+
Attachment #8969638 - Flags: review?(amarchesini) → review+
Attachment #8969678 - Flags: review?(amarchesini) → review+
Comment on attachment 8969082 [details] [diff] [review]
P3 Refactor ServiceWorkerContainer::GetGlobalIfValid() to accomodate Register() usage. r=baku

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

I still like GetGlobalIfValid() written in this way. For sure, would be better to rename it.
Plus, all the uses here and in part 8 are related to nsContentUtils::ReportToConsole(). Wondering if you can just pass an optional "params" argument instead having a callback.
Up to you if you want this method with or without callback, but rename it and write a comment related to when the callback is executed.

::: dom/serviceworkers/ServiceWorkerContainer.cpp
@@ +229,5 @@
>  
>  already_AddRefed<Promise>
>  ServiceWorkerContainer::GetRegistrations(ErrorResult& aRv)
>  {
> +  nsIGlobalObject* global = GetGlobalIfValid(aRv, [](nsIDocument* aDoc) {

Maybe rename the method to something else? I don't have a better name to suggest, but it's important to have a reason for this callback.

@@ +233,5 @@
> +  nsIGlobalObject* global = GetGlobalIfValid(aRv, [](nsIDocument* aDoc) {
> +    nsContentUtils::ReportToConsole(nsIScriptError::errorFlag,
> +                                    NS_LITERAL_CSTRING("Service Workers"), aDoc,
> +                                    nsContentUtils::eDOM_PROPERTIES,
> +                                    "ServiceWorkerGetRegistrationStorageError");

Why ServiceWorkerGetRegistrationStorageError?
Attachment #8969082 - Flags: review?(amarchesini) → review+
Comment on attachment 8969677 [details] [diff] [review]
P8 Move storage permission check and other window specific code to ServiceWorkerContainer. r=baku

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

You are probably going to rename GetGlobalIfValid() in the previous patches.
Attachment #8969677 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini [:baku] from comment #75)
> @@ +233,5 @@
> > +  nsIGlobalObject* global = GetGlobalIfValid(aRv, [](nsIDocument* aDoc) {
> > +    nsContentUtils::ReportToConsole(nsIScriptError::errorFlag,
> > +                                    NS_LITERAL_CSTRING("Service Workers"), aDoc,
> > +                                    nsContentUtils::eDOM_PROPERTIES,
> > +                                    "ServiceWorkerGetRegistrationStorageError");
> 
> Why ServiceWorkerGetRegistrationStorageError?

We have always used this same error string for both getRegistration() and getRegistrations().
Added a comment, but did not rename per our IRC discussion.
Attachment #8969082 - Attachment is obsolete: true
Attachment #8970160 - Flags: review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/915fd6a149d2
P1 Make ServiceWorker MozPromise types exclusive so they can reject with ErrorResult values. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/25a7296ac1fc
P2 Don't try to use the document's nsILoadGroup when registering a new service worker script. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/84c69758ca15
P3 Refactor ServiceWorkerContainer::GetGlobalIfValid() to accomodate Register() usage. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f32e0d8a75f
P4 Add the ServiceWorkerContainer::GetBaseURIFromGlobal() method. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/06082fbb04ce
P5 Move the IsFromAuthenticatedOrigin() check from the ServiceWorkerManager to the ServiceWorkerContainer. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f27625fd511
P6 Move the NS_CheckContentLoadPolicy() from ServiceWorkerManager to ServiceWorkerContainer. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/16ddf190ca51
P7 Move many checks into ServiceWorkerScopeAndScriptAreValid() utility method. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfadc112f9f7
P8 Move storage permission check and other window specific code to ServiceWorkerContainer. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f62b226f25
P9 Implement the ServiceWorkerContainer::Register() method using the inner class. r=baku
You need to log in before you can comment on or make changes to this bug.