Title: [225975] trunk
Revision
225975
Author
cdu...@apple.com
Date
2017-12-15 11:36:51 -0800 (Fri, 15 Dec 2017)

Log Message

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.

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:

Modified Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (225974 => 225975)


--- trunk/LayoutTests/ChangeLog	2017-12-15 15:39:27 UTC (rev 225974)
+++ trunk/LayoutTests/ChangeLog	2017-12-15 19:36:51 UTC (rev 225975)
@@ -1,3 +1,20 @@
+2017-12-15  Chris Dumez  <cdu...@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  <you...@apple.com>
 
         Implement <iframe allow="camera; microphone">

Modified: trunk/LayoutTests/http/tests/workers/service/basic-register-expected.txt (225974 => 225975)


--- trunk/LayoutTests/http/tests/workers/service/basic-register-expected.txt	2017-12-15 15:39:27 UTC (rev 225974)
+++ trunk/LayoutTests/http/tests/workers/service/basic-register-expected.txt	2017-12-15 19:36:51 UTC (rev 225975)
@@ -5,5 +5,5 @@
 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
 

Modified: trunk/LayoutTests/http/tests/workers/service/basic-unregister-then-register-again-no-reuse.html (225974 => 225975)


--- trunk/LayoutTests/http/tests/workers/service/basic-unregister-then-register-again-no-reuse.html	2017-12-15 15:39:27 UTC (rev 225974)
+++ trunk/LayoutTests/http/tests/workers/service/basic-unregister-then-register-again-no-reuse.html	2017-12-15 19:36:51 UTC (rev 225975)
@@ -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 @@
 
          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

Modified: trunk/LayoutTests/http/tests/workers/service/registration-clear-redundant-worker.html (225974 => 225975)


--- trunk/LayoutTests/http/tests/workers/service/registration-clear-redundant-worker.html	2017-12-15 15:39:27 UTC (rev 225974)
+++ trunk/LayoutTests/http/tests/workers/service/registration-clear-redundant-worker.html	2017-12-15 19:36:51 UTC (rev 225975)
@@ -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) {

Modified: trunk/LayoutTests/http/tests/workers/service/resources/basic-register.js (225974 => 225975)


--- trunk/LayoutTests/http/tests/workers/service/resources/basic-register.js	2017-12-15 15:39:27 UTC (rev 225974)
+++ trunk/LayoutTests/http/tests/workers/service/resources/basic-register.js	2017-12-15 19:36:51 UTC (rev 225975)
@@ -45,12 +45,16 @@
             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();
 }
 

Modified: trunk/LayoutTests/http/tests/workers/service/service-worker-gc-event.html (225974 => 225975)


--- trunk/LayoutTests/http/tests/workers/service/service-worker-gc-event.html	2017-12-15 15:39:27 UTC (rev 225974)
+++ trunk/LayoutTests/http/tests/workers/service/service-worker-gc-event.html	2017-12-15 19:36:51 UTC (rev 225975)
@@ -13,7 +13,7 @@
     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();

Modified: trunk/LayoutTests/http/tests/workers/service/service-worker-registration-gc-event.html (225974 => 225975)


--- trunk/LayoutTests/http/tests/workers/service/service-worker-registration-gc-event.html	2017-12-15 15:39:27 UTC (rev 225974)
+++ trunk/LayoutTests/http/tests/workers/service/service-worker-registration-gc-event.html	2017-12-15 19:36:51 UTC (rev 225975)
@@ -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");

Modified: trunk/LayoutTests/imported/w3c/ChangeLog (225974 => 225975)


--- trunk/LayoutTests/imported/w3c/ChangeLog	2017-12-15 15:39:27 UTC (rev 225974)
+++ trunk/LayoutTests/imported/w3c/ChangeLog	2017-12-15 19:36:51 UTC (rev 225975)
@@ -1,3 +1,18 @@
+2017-12-15  Chris Dumez  <cdu...@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  <you...@apple.com>
 
         Implement <iframe allow="camera; microphone">

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https-expected.txt (225974 => 225975)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https-expected.txt	2017-12-15 15:39:27 UTC (rev 225974)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https-expected.txt	2017-12-15 19:36:51 UTC (rev 225975)
@@ -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 
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/registration-iframe.https-expected.txt (225974 => 225975)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/registration-iframe.https-expected.txt	2017-12-15 15:39:27 UTC (rev 225974)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/registration-iframe.https-expected.txt	2017-12-15 19:36:51 UTC (rev 225975)
@@ -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
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/registration-script.https-expected.txt (225974 => 225975)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/registration-script.https-expected.txt	2017-12-15 15:39:27 UTC (rev 225974)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/registration-script.https-expected.txt	2017-12-15 19:36:51 UTC (rev 225975)
@@ -5,7 +5,7 @@
 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 
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/registration-security-error.https-expected.txt (225974 => 225975)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/registration-security-error.https-expected.txt	2017-12-15 15:39:27 UTC (rev 225974)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/registration-security-error.https-expected.txt	2017-12-15 19:36:51 UTC (rev 225975)
@@ -3,7 +3,7 @@
 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

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/unregister-then-register-new-script.https-expected.txt (225974 => 225975)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/unregister-then-register-new-script.https-expected.txt	2017-12-15 15:39:27 UTC (rev 225974)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/unregister-then-register-new-script.https-expected.txt	2017-12-15 19:36:51 UTC (rev 225975)
@@ -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 
 

Modified: trunk/Source/WebCore/ChangeLog (225974 => 225975)


--- trunk/Source/WebCore/ChangeLog	2017-12-15 15:39:27 UTC (rev 225974)
+++ trunk/Source/WebCore/ChangeLog	2017-12-15 19:36:51 UTC (rev 225975)
@@ -1,3 +1,21 @@
+2017-12-15  Chris Dumez  <cdu...@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  <you...@apple.com>
 
         WebRTC Stats should not be console logged from a background thread

Modified: trunk/Source/WebCore/workers/WorkerScriptLoader.cpp (225974 => 225975)


--- trunk/Source/WebCore/workers/WorkerScriptLoader.cpp	2017-12-15 15:39:27 UTC (rev 225974)
+++ trunk/Source/WebCore/workers/WorkerScriptLoader.cpp	2017-12-15 19:36:51 UTC (rev 225975)
@@ -181,6 +181,8 @@
 void WorkerScriptLoader::notifyError()
 {
     m_failed = true;
+    if (m_error.isNull())
+        m_error = ResourceError { errorDomainWebKitInternal, 0, url(), "Failed to load script", ResourceError::Type::General };
     notifyFinished();
 }
 

Modified: trunk/Source/WebCore/workers/service/ServiceWorkerJob.cpp (225974 => 225975)


--- trunk/Source/WebCore/workers/service/ServiceWorkerJob.cpp	2017-12-15 15:39:27 UTC (rev 225974)
+++ trunk/Source/WebCore/workers/service/ServiceWorkerJob.cpp	2017-12-15 19:36:51 UTC (rev 225975)
@@ -120,8 +120,11 @@
     
     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;
 }
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to