fix importScripts() restrictions to reflect current spec

RESOLVED FIXED in Firefox 62

Status

()

defect
P2
normal
RESOLVED FIXED
11 months ago
3 months ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks 1 bug)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 disabled, firefox60 wontfix, firefox61 wontfix, firefox62 fixed)

Details

Attachments

(8 attachments, 2 obsolete attachments)

3.33 KB, patch
asuth
: review+
Details | Diff | Splinter Review
3.51 KB, patch
asuth
: review+
Details | Diff | Splinter Review
1.10 KB, patch
asuth
: review+
Details | Diff | Splinter Review
3.80 KB, patch
asuth
: review+
Details | Diff | Splinter Review
4.02 KB, patch
asuth
: review+
Details | Diff | Splinter Review
1.62 KB, patch
asuth
: review+
Details | Diff | Splinter Review
2.82 KB, patch
asuth
: review+
Details | Diff | Splinter Review
4.32 KB, patch
asuth
: review+
Details | Diff | Splinter Review
(Assignee)

Description

11 months ago
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
(Assignee)

Comment 1

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

Updated

11 months ago
Priority: -- → P2
(Assignee)

Comment 3

11 months ago
After further review, the only issue with the test is that it does not expect the right kind of exception.
(Assignee)

Comment 6

11 months ago
From the orange in the try build it appears we have a number of mochitests which depend on this non-standard behavior.
(Assignee)

Updated

11 months ago
Depends on: 1466359
(Assignee)

Comment 14

11 months ago
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.
Attachment #8983067 - Flags: review?(bugmail)
(Assignee)

Comment 15

11 months ago
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.
Attachment #8983173 - Flags: review?(bugmail)
(Assignee)

Comment 16

11 months ago
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.
Attachment #8983068 - Flags: review?(bugmail)
(Assignee)

Comment 17

11 months ago
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.
Attachment #8983069 - Flags: review?(bugmail)
(Assignee)

Comment 18

11 months ago
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.
Attachment #8983070 - Flags: review?(bugmail)
(Assignee)

Comment 19

11 months ago
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.
Attachment #8983071 - Flags: review?(bugmail)
(Assignee)

Comment 20

11 months ago
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.
Attachment #8983137 - Flags: review?(bugmail)
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.
Attachment #8983067 - Flags: review?(bugmail) → review+
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.
Attachment #8983068 - Flags: review?(bugmail) → review+
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.
Attachment #8983069 - Flags: review?(bugmail) → review+
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.)
Attachment #8983070 - Flags: review?(bugmail) → review+
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.)
Attachment #8983071 - Flags: review?(bugmail) → review+
Attachment #8983137 - Flags: review?(bugmail) → review+
Attachment #8983173 - Flags: review?(bugmail) → review+
(Assignee)

Comment 26

11 months ago
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.
Attachment #8983444 - Flags: review?(bugmail)
Attachment #8983444 - Flags: review?(bugmail) → review+

Comment 27

11 months ago
Pushed by bkelly@mozilla.com:
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
(Assignee)

Updated

11 months ago
Duplicate of this bug: 1201887
(Assignee)

Updated

11 months ago
No longer depends on: 1466359
Duplicate of this bug: 1428452
You need to log in before you can comment on or make changes to this bug.