1465670 - fix importScripts() restrictions to reflect current spec

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

In bug 1448141 I made importScripts() throw an InvalidStateError if its called in a service worker outside the initial parsed state.  Per the current spec it should also be allowed in the installing state.  Also, if this fails it should throw a NetworkError.

See:

https://github.com/w3c/ServiceWorker/issues/1319#issuecomment-393386106

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

If possible we should avoid shipping this incorrect implementation when 61 merges to release.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

It seems there is a WPT test for this, but it looks a bit broken to me:

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/resources/import-scripts-updated-flag-worker.js

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

After further review, the only issue with the test is that it does not expect the right kind of exception.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make importScripts() called in a service worker script after install throw NetworkError. r=asuth

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Fix import-scripts-updated-flag.https.html WPT to expect NetworkError and to clear previous test state. r=asuth

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

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

From the orange in the try build it appears we have a number of mochitests which depend on this non-standard behavior.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make importScripts() called in a service worker script after install throw NetworkError. r=asuth

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Fix dom/cache service worker test harness to call importScripts() during install. r=asuth

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Fix test_importscript.html to expect spec behavior. r=asuth

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Fix test_serviceworker_interfaces.html to expect spec importScript behavior. r=asuth

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Fix fetch test framework to expect spec-compliant importScripts() behavior. r=asuth

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

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Make the fetch mochitest reroute test framework use a unique scope. r=asuth

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

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Fix import-scripts-updated-flag.https.html WPT to expect NetworkError and to clear previous test state. r=asuth

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8983067 [details] [diff] [review]
P1 Make importScripts() called in a service worker script after install throw NetworkError. r=asuth

This patch fixes a couple things with our importScripts() implementation:

1. We now consult our offline Cache API for late importScripts().  Previously we would always go to the network after the initial script load.
2. We allow offlining new importScripts() in either the Parsed or Installing states.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8983173 [details] [diff] [review]
P2 Fix import-scripts-updated-flag.https.html WPT to expect NetworkError and to clear previous test state. r=asuth

This patch brings in the upstream WPT changes for:

https://github.com/web-platform-tests/wpt/pull/11315

And updates our test expectations.  If I land this here explicitly, then I will ask James to stop bug 1466359.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8983068 [details] [diff] [review]
P3 Fix dom/cache service worker test harness to call importScripts() during install. r=asuth

This makes the dom/cache test framework execute its test importScripts() in the installing state.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8983069 [details] [diff] [review]
P4 Fix test_importscript.html to expect spec behavior. r=asuth

This updates our test_importscript.html mochitest to expect the new behavior.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8983070 [details] [diff] [review]
P5 Fix test_serviceworker_interfaces.html to expect spec importScript behavior. r=asuth

Fix test_serviceworker_interfaces.html to not use a late importScripts() and instead perform do everything during the installing state.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8983071 [details] [diff] [review]
P6 Fix fetch test framework to expect spec-compliant importScripts() behavior. r=asuth

This fixes the fetch test framework to operate under the installing state.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8983137 [details] [diff] [review]
P7 Make the fetch mochitest reroute test framework use a unique scope. r=asuth

This fixes the fetch "reroute" test framework to use a more targeted scope.  It previously was using the top level "/" scope.  The other test changes in this directory seemed to cause the reroute tests to start perma-hanging.  I believe these tests have always be a bit unstable due to the scope catch extra mochitest frames or windows and keep the service worker alive until unexpected time.  This seems to fix the issue.

Comment 21

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

Comment on attachment 8983067 [details] [diff] [review]
P1 Make importScripts() called in a service worker script after install throw NetworkError. r=asuth

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

I think the comment lines at https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/dom/workers/WorkerPrivate.h#1052-1053 want to be removed now?:
  // importScripts() in function calls or event handlers are always fetched
  // from the network.

That said, maybe s/LoadScriptAsPartOfLoadingServiceWorkerScript/IsLoadingTopLevelServiceWorkerScript/g since the remaining use of the function after this patch is not a script loading context per-se:
- noop-ing SWR::update() from the toplevel: https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/dom/serviceworkers/ServiceWorkerRegistrationImpl.cpp#731

::: dom/workers/ScriptLoader.cpp
@@ +916,5 @@
>      if (IsMainWorkerScript() && mWorkerPrivate->IsServiceWorker()) {
>        mWorkerPrivate->SetLoadingWorkerScript(true);
>      }
>  
> +    if (!mWorkerPrivate->IsServiceWorker() || IsDebuggerScript()) {

Restating:
- If the condition returns true, we go into the load-from-network case, LoadScript.  (And because we didn't get there via the CacheCreator call path that sets ScriptLoadInfo::ToBeCached, no effort will be made to save off whatever is fetched from the network.)
- The removal of the OR'ed LoadScriptAsPartOfLoadingServiceWorkerScript() means that we will now always try and check the cache if we are a ServiceWorker.  (Note that the previous behavior had issues.)
- Debugger scripts for service workers now need their own case because they always want to go to the network and they were depending on the !LoadScriptAsPartOfLoadingServiceWorkerScript() check.  (IsMainWorkerScript() would never return true for them, so LoadScriptAsPartOfLoadingServiceWorkerScript would never return true.)

@@ +1796,5 @@
> +    // then this is an error.  This can happen for internal reasons, like
> +    // storage was probably wiped without removing the service worker
> +    // registration.  It can also happen for exposed reasons like the
> +    // service worker script calling importScripts() after install.
> +    if (NS_WARN_IF(mIsWorkerScript || (mState != ServiceWorkerState::Parsed &&

Restating:
- We now always check the cache for ServiceWorker importScripts calls; this was not the case previously.  This code is part of that path, but there are other cases too.
- This is the control flow path for importScripts() failing to find a given script in the cache.  So we're only imposing a constraint on importScripts() going to the network, not when you can use it.  You can call importScripts() anytime you want as long as the script is already in the cache, which is intentional and covered by tests in part 4.
- The local change here just makes us be okay with going to network (and caching the result as a byproduct) when we're in the installing state in addition to being in the parsed (/parsing) state.

Comment 22

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

Comment on attachment 8983068 [details] [diff] [review]
P3 Fix dom/cache service worker test harness to call importScripts() during install. r=asuth

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

Restating:
- Driver switches from ready() promise which fires once active to the the register() promise that fires once installing.
- The triggering message to the SW only gets sent after that promise resolves, so completeInstall in the SW will definitely already have been assigned because the install event will have fired.
- message_receiver.html probably won't be controlled anymore because the iframe is now creating during installation, so the clients match call has been updated.
- We now remove the iframe which is good from a hygiene perspective in case the iframe's document did end up controlled.

Comment 23

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

Comment on attachment 8983069 [details] [diff] [review]
P4 Fix test_importscript.html to expect spec behavior. r=asuth

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

Restating:
- The postMessage comes once the SW has "activated".
- This test change notably confirms that importScripts() is okay even after parsing and installation if the result is in the cache, but that it's not okay if the result was not in the cache.
- importscript.sjs returns a script payload of `callByScript();` which increments counter(), so we're looking for the script to be evaluated an extra time now in our new success case.

Comment 24

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

Comment on attachment 8983070 [details] [diff] [review]
P5 Fix test_serviceworker_interfaces.html to expect spec importScript behavior. r=asuth

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

(Basically the same changes as part 3.  The client-finding logic becomes more like the test logic in part 3 already had.)

Comment 25

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

Comment on attachment 8983071 [details] [diff] [review]
P6 Fix fetch test framework to expect spec-compliant importScripts() behavior. r=asuth

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

(Again, similar to part 3, but this was using register()-then-waitForState() rather than using the ready promise.)

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Convert WorkerPrivate::LoadScriptAsPartOfLoadingServiceWorkerScript() to simply IsLoadWorkerScript(). r=asuth

Andrew, this addresses your review feedback for P1.  I decided to make the API simply track whether main script is being loaded for all worker types.  It seemed unnecessary to make this service worker specific.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef1dd76af6f9
P1 Make importScripts() called in a service worker script after install throw NetworkError. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/9eb69a99a087
P2 Fix import-scripts-updated-flag.https.html WPT to expect NetworkError and to clear previous test state. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c55a618ddef
P3 Fix dom/cache service worker test harness to call importScripts() during install. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9792e276611
P4 Fix test_importscript.html to expect spec behavior. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/14c1d777809b
P5 Fix test_serviceworker_interfaces.html to expect spec importScript behavior. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/8551bb8503d6
P6 Fix fetch test framework to expect spec-compliant importScripts() behavior. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/eab80a391434
P7 Make the fetch mochitest reroute test framework use a unique scope. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8103e36d8cf
P8 Convert WorkerPrivate::LoadScriptAsPartOfLoadingServiceWorkerScript() to simply IsLoadWorkerScript(). r=asuth

Comment 29

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/ef1dd76af6f9
https://hg.mozilla.org/mozilla-central/rev/9eb69a99a087
https://hg.mozilla.org/mozilla-central/rev/8c55a618ddef
https://hg.mozilla.org/mozilla-central/rev/d9792e276611
https://hg.mozilla.org/mozilla-central/rev/14c1d777809b
https://hg.mozilla.org/mozilla-central/rev/8551bb8503d6
https://hg.mozilla.org/mozilla-central/rev/eab80a391434
https://hg.mozilla.org/mozilla-central/rev/c8103e36d8cf