Service Worker Registration promise is sometimes not rejected when the script load...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Dec 2017 19:36:51 +0000 (19:36 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Dec 2017 19:36:51 +0000 (19:36 +0000)
https://bugs.webkit.org/show_bug.cgi?id=180849

Reviewed by Brady Eidson.

LayoutTests/imported/w3c:

Rebaseline tests that are now passing.

* web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https-expected.txt:
* web-platform-tests/service-workers/service-worker/registration-iframe.https-expected.txt:
* web-platform-tests/service-workers/service-worker/registration-script.https-expected.txt:
* web-platform-tests/service-workers/service-worker/registration-security-error.https-expected.txt:
* web-platform-tests/service-workers/service-worker/unregister-then-register-new-script.https-expected.txt:

Source/WebCore:

Service Worker Registration promise is sometimes not rejected when the script load fails.
This was caused by the ServiceWorkerJob sometimes passing a null ResourceError to the
StorageProcess, even though the load failed.

No new tests, rebaselined exisiting tests.

* workers/WorkerScriptLoader.cpp:
(WebCore::WorkerScriptLoader::notifyError):
* workers/service/ServiceWorkerJob.cpp:
(WebCore::ServiceWorkerJob::notifyFinished):

LayoutTests:

Fix WebKit-specific tests that had invalid URLs for workers. We failed to notice this
before because we were wrongly resolving the registration promise.

* http/tests/workers/service/basic-register-expected.txt:
* http/tests/workers/service/basic-unregister-then-register-again-no-reuse.html:
* http/tests/workers/service/registration-clear-redundant-worker.html:
* http/tests/workers/service/resources/basic-register.js:
* http/tests/workers/service/service-worker-gc-event.html:
* http/tests/workers/service/service-worker-registration-gc-event.html:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@225975 268f45cc-cd09-0410-ab3c-d52691b4dbfc

16 files changed:
LayoutTests/ChangeLog
LayoutTests/http/tests/workers/service/basic-register-expected.txt
LayoutTests/http/tests/workers/service/basic-unregister-then-register-again-no-reuse.html
LayoutTests/http/tests/workers/service/registration-clear-redundant-worker.html
LayoutTests/http/tests/workers/service/resources/basic-register.js
LayoutTests/http/tests/workers/service/service-worker-gc-event.html
LayoutTests/http/tests/workers/service/service-worker-registration-gc-event.html
LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https-expected.txt
LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/registration-iframe.https-expected.txt
LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/registration-script.https-expected.txt
LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/registration-security-error.https-expected.txt
LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/unregister-then-register-new-script.https-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/workers/WorkerScriptLoader.cpp
Source/WebCore/workers/service/ServiceWorkerJob.cpp

index bcdb087..702ec4f 100644 (file)
@@ -1,3 +1,20 @@
+2017-12-15  Chris Dumez  <cdumez@apple.com>
+
+        Service Worker Registration promise is sometimes not rejected when the script load fails
+        https://bugs.webkit.org/show_bug.cgi?id=180849
+
+        Reviewed by Brady Eidson.
+
+        Fix WebKit-specific tests that had invalid URLs for workers. We failed to notice this
+        before because we were wrongly resolving the registration promise.
+
+        * http/tests/workers/service/basic-register-expected.txt:
+        * http/tests/workers/service/basic-unregister-then-register-again-no-reuse.html:
+        * http/tests/workers/service/registration-clear-redundant-worker.html:
+        * http/tests/workers/service/resources/basic-register.js:
+        * http/tests/workers/service/service-worker-gc-event.html:
+        * http/tests/workers/service/service-worker-registration-gc-event.html:
+
 2017-12-14  Youenn Fablet  <youenn@apple.com>
 
         Implement <iframe allow="camera; microphone">
index 0d54b5d..bd8806b 100644 (file)
@@ -5,5 +5,5 @@ PASS: registration object's scope is valid
 PASS: registration object's updateViaCache is valid
 PASS: A service worker is now registered for this origin
 PASS: No service worker is registered for this origin in private session
-Registered!
+PASS: registering a script which does not exist rejected the promise
 
index 6ef8e51..d78782c 100644 (file)
@@ -8,7 +8,7 @@
 async function test()
 {
     try {
-         let registration1 = await navigator.serviceWorker.register("resources/empty.js", { });
+         let registration1 = await navigator.serviceWorker.register("resources/empty-worker.js", { });
          await waitForState(registration1.installing, "activated");
          if (registration1.installing)
              log("FAIL: registration1 should not have an installing worker");
@@ -27,7 +27,7 @@ async function test()
 
          await registration1.unregister();
 
-         let registration2 = await navigator.serviceWorker.register("resources/empty.js", { });
+         let registration2 = await navigator.serviceWorker.register("resources/empty-worker.js", { });
          if (registration2.installing)
              log("PASS: registration2 should have an installing worker");
          else
index e47532a..b27249b 100644 (file)
@@ -4,7 +4,7 @@
 </head>
 <body>
 <script>
-navigator.serviceWorker.register("resources/resources/empty-worker.js", { }).then(function(registration) {
+navigator.serviceWorker.register("resources/empty-worker.js", { }).then(function(registration) {
     waitForState(registration.installing, "activated").then(function() {
         let worker = registration.active;
         registration.unregister().then(function (success) {
index b5e246c..b947ab9 100644 (file)
@@ -45,12 +45,16 @@ async function test()
             log("FAIL: A service worker is registered for this origin in private session");
 
         testRunner.setPrivateBrowsingEnabled(false);
-
-        r = await navigator.serviceWorker.register("resources/empty-worker-doesnt-exist.js", { })
-        log("Registered!");
     } catch (e) {
         log("Exception registering: " + e);
     }
+    try {
+        r = await navigator.serviceWorker.register("resources/empty-worker-doesnt-exist.js", { })
+        log("FAIL: registering a script should have rejected the promise");
+    } catch(e) {
+        log("PASS: registering a script which does not exist rejected the promise");
+    }
+
     done();
 }
 
index cf932b4..e425d42 100644 (file)
@@ -13,7 +13,7 @@ function getNewestWorker(registration)
     return registration.active;
 }
 
-navigator.serviceWorker.register("resources/empty.js", { }).then(function(_registration) {
+navigator.serviceWorker.register("resources/empty-worker.js", { }).then(function(_registration) {
     registration = _registration;
     registration.installing.addEventListener("statechange", function() {
         gc();
index 445c60e..5898137 100644 (file)
@@ -7,7 +7,7 @@
 <script>
 let updatefoundCount = 0;
 
-navigator.serviceWorker.register("resources/empty.js", { }).then(function(registration) {
+navigator.serviceWorker.register("resources/empty-worker.js", { }).then(function(registration) {
     registration.addEventListener("updatefound", function() {
         gc();
         log("updatefound event fired");
index 9639121..3ece6c7 100644 (file)
@@ -1,3 +1,18 @@
+2017-12-15  Chris Dumez  <cdumez@apple.com>
+
+        Service Worker Registration promise is sometimes not rejected when the script load fails
+        https://bugs.webkit.org/show_bug.cgi?id=180849
+
+        Reviewed by Brady Eidson.
+
+        Rebaseline tests that are now passing.
+
+        * web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https-expected.txt:
+        * web-platform-tests/service-workers/service-worker/registration-iframe.https-expected.txt:
+        * web-platform-tests/service-workers/service-worker/registration-script.https-expected.txt:
+        * web-platform-tests/service-workers/service-worker/registration-security-error.https-expected.txt:
+        * web-platform-tests/service-workers/service-worker/unregister-then-register-new-script.https-expected.txt:
+
 2017-12-14  Youenn Fablet  <youenn@apple.com>
 
         Implement <iframe allow="camera; microphone">
index 948692f..ae65f49 100644 (file)
@@ -1,7 +1,7 @@
 
 PASS Register different scripts concurrently 
 PASS Register then register new script URL 
-FAIL Register then register new script URL that 404s assert_unreached: unexpected rejection: assert_unreached: register should reject Reached unreachable code Reached unreachable code
+PASS Register then register new script URL that 404s 
 FAIL Register then register new script that does not install assert_unreached: unexpected rejection: assert_equals: on redundant, installing should be null expected null but got object "[object ServiceWorker]" Reached unreachable code
 PASS Register same-scope new script url effect on controller 
 
index b96101f..3b38fdb 100644 (file)
@@ -1,6 +1,6 @@
 
 
 PASS register method should use the "relevant global object" to parse its scriptURL and scope - normal case 
-FAIL register method should use the "relevant global object" to parse its scriptURL and scope - error case assert_unreached: unexpected rejection: assert_unreached: register() should reject Reached unreachable code Reached unreachable code
+PASS register method should use the "relevant global object" to parse its scriptURL and scope - error case 
 FAIL A scope url should start with the given script url assert_unreached: unexpected rejection: assert_unreached: register() should reject Reached unreachable code Reached unreachable code
 
index f25cce0..0422e5c 100644 (file)
@@ -5,7 +5,7 @@ PASS Registering script including parse error
 PASS Registering script including undefined error 
 PASS Registering script including uncaught exception 
 PASS Registering script importing malformed script 
-FAIL Registering non-existent script assert_unreached: Should have rejected: Registration of non-existent script should fail. Reached unreachable code
+PASS Registering non-existent script 
 PASS Registering script importing non-existent script 
 PASS Registering script including caught exception 
 
index 353085d..4839d4f 100644 (file)
@@ -3,7 +3,7 @@ FAIL Registering same scope as the script directory without the last slash asser
 FAIL Registration scope outside the script directory assert_unreached: Should have rejected: Registration scope outside the script directory should fail with SecurityError. Reached unreachable code
 PASS Registering scope outside domain 
 PASS Registering script outside domain 
-FAIL Registering redirected script assert_unreached: Should have rejected: Registration of redirected script should fail. Reached unreachable code
+FAIL Registering redirected script assert_throws: Registration of redirected script should fail. function "function () { throw e }" threw object "TypeError: Script URL https://localhost:9443/service-workers/service-worker/resources/redirect.py?Redirect=%2Fresources%2Fregistration-worker.js fetch resulted in error: Failed to load script" that is not a DOMException SecurityError: property "code" is equal to undefined, expected 18
 FAIL Scope including parent-reference and not under the script directory assert_unreached: Should have rejected: Scope not under the script directory should be rejected. Reached unreachable code
 FAIL Script URL including consecutive slashes assert_unreached: Should have rejected: Consecutive slashes in the script url should not be unified. Reached unreachable code
 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 "TypeError: serviceWorker.register() must be called with a script URL whose protocol is either HTTP or HTTPS" that is not a DOMException SecurityError: property "code" is equal to undefined, expected 18
index 17f6a31..8e42bee 100644 (file)
@@ -1,5 +1,5 @@
 
 PASS Registering a new script URL while an unregistered registration is in use 
-FAIL Registering a new script URL that 404s does not resurrect an unregistered registration assert_unreached: unexpected rejection: assert_unreached: register should reject the promise Reached unreachable code Reached unreachable code
+PASS Registering a new script URL that 404s does not resurrect an unregistered registration 
 PASS Registering a new script URL that fails to install does not resurrect an unregistered registration 
 
index b6e9392..8d604bf 100644 (file)
@@ -1,3 +1,21 @@
+2017-12-15  Chris Dumez  <cdumez@apple.com>
+
+        Service Worker Registration promise is sometimes not rejected when the script load fails
+        https://bugs.webkit.org/show_bug.cgi?id=180849
+
+        Reviewed by Brady Eidson.
+
+        Service Worker Registration promise is sometimes not rejected when the script load fails.
+        This was caused by the ServiceWorkerJob sometimes passing a null ResourceError to the
+        StorageProcess, even though the load failed.
+
+        No new tests, rebaselined exisiting tests.
+
+        * workers/WorkerScriptLoader.cpp:
+        (WebCore::WorkerScriptLoader::notifyError):
+        * workers/service/ServiceWorkerJob.cpp:
+        (WebCore::ServiceWorkerJob::notifyFinished):
+
 2017-12-15  Youenn Fablet  <youenn@apple.com>
 
         WebRTC Stats should not be console logged from a background thread
index 4da13fb..af3e592 100644 (file)
@@ -181,6 +181,8 @@ void WorkerScriptLoader::didFail(const ResourceError& error)
 void WorkerScriptLoader::notifyError()
 {
     m_failed = true;
+    if (m_error.isNull())
+        m_error = ResourceError { errorDomainWebKitInternal, 0, url(), "Failed to load script", ResourceError::Type::General };
     notifyFinished();
 }
 
index d28494f..a3d86f2 100644 (file)
@@ -120,8 +120,11 @@ void ServiceWorkerJob::notifyFinished()
     
     if (!m_scriptLoader->failed())
         m_client->jobFinishedLoadingScript(*this, m_scriptLoader->script());
-    else
-        m_client->jobFailedLoadingScript(*this, m_scriptLoader->error(), std::nullopt);
+    else {
+        auto& error =  m_scriptLoader->error();
+        ASSERT(!error.isNull());
+        m_client->jobFailedLoadingScript(*this, error, std::nullopt);
+    }
 
     m_scriptLoader = nullptr;
 }