Bug 1271692 - Do not fail event processing if an exception is thrown in ServiceWorker. r=bkelly
authorTom Tung <ttung@mozilla.com>
Thu, 10 Nov 2016 17:53:24 +0800
changeset 325024 362a85a199376e27508d4f819bb3411b90c8f106
parent 325023 33de2e2e2cf3876d74c33e4ca335090ee5224c2c
child 325025 0c212b76bca625813bae097ff2b8765b07261895
push id24
push usermaklebus@msu.edu
push dateTue, 20 Dec 2016 03:11:33 +0000
reviewersbkelly
bugs1271692
milestone53.0a1
Bug 1271692 - Do not fail event processing if an exception is thrown in ServiceWorker. r=bkelly
dom/workers/ServiceWorkerPrivate.cpp
dom/workers/test/serviceworkers/install_event_error_worker.js
testing/web-platform/meta/service-workers/service-worker/ServiceWorkerGlobalScope/registration-attribute.https.html.ini
testing/web-platform/tests/service-workers/service-worker/oninstall-script-error.https.html
testing/web-platform/tests/service-workers/service-worker/resources/fetch-cors-xhr-iframe.html
testing/web-platform/tests/service-workers/service-worker/resources/fetch-event-network-error-controllee-iframe.html
testing/web-platform/tests/service-workers/service-worker/resources/fetch-event-network-error-worker.js
testing/web-platform/tests/service-workers/service-worker/resources/oninstall-waituntil-throw-error-worker.js
--- a/dom/workers/ServiceWorkerPrivate.cpp
+++ b/dom/workers/ServiceWorkerPrivate.cpp
@@ -400,49 +400,57 @@ public:
     AssertIsOnMainThread();
     MOZ_ASSERT(aWorkerPrivate);
     MOZ_ASSERT(aKeepAliveToken);
 
     mKeepAliveToken =
       new nsMainThreadPtrHolder<KeepAliveToken>(aKeepAliveToken);
   }
 
-  bool
+  nsresult
   DispatchExtendableEventOnWorkerScope(JSContext* aCx,
                                        WorkerGlobalScope* aWorkerScope,
                                        ExtendableEvent* aEvent,
                                        ExtendableEventCallback* aCallback)
   {
     MOZ_ASSERT(aWorkerScope);
     MOZ_ASSERT(aEvent);
     nsCOMPtr<nsIGlobalObject> sgo = aWorkerScope;
     WidgetEvent* internalEvent = aEvent->WidgetEventPtr();
 
     RefPtr<KeepAliveHandler> keepAliveHandler =
       new KeepAliveHandler(mKeepAliveToken, aCallback);
     if (NS_WARN_IF(!keepAliveHandler->UseWorkerHolder())) {
-      return false;
+      return NS_ERROR_FAILURE;
     }
 
     // This must always be set *before* dispatching the event, otherwise
     // waitUntil calls will fail.
     aEvent->SetKeepAliveHandler(keepAliveHandler);
 
     ErrorResult result;
     result = aWorkerScope->DispatchDOMEvent(nullptr, aEvent, nullptr, nullptr);
-    if (NS_WARN_IF(result.Failed()) || internalEvent->mFlags.mExceptionWasRaised) {
+    if (NS_WARN_IF(result.Failed())) {
       result.SuppressException();
-      return false;
+      return NS_ERROR_FAILURE;
     }
 
     // [[ If e’s extend lifetime promises is empty, unset e’s extensions allowed
     //    flag and abort these steps. ]]
     keepAliveHandler->MaybeDone();
 
-    return true;
+    // We don't block the event when getting an exception but still report the
+    // error message.
+    // Report exception message. Note: This will not stop the event.
+    if (internalEvent->mFlags.mExceptionWasRaised) {
+      result.SuppressException();
+      return NS_ERROR_XPC_JS_THREW_EXCEPTION;
+    }
+
+    return NS_OK;
   }
 };
 
 class SendMesssageEventRunnable final : public ExtendableEventWorkerRunnable
                                       , public StructuredCloneHolder
 {
   UniquePtr<ServiceWorkerClientInfo> mEventSource;
 
@@ -492,18 +500,20 @@ public:
                                           init, rv);
     if (NS_WARN_IF(rv.Failed())) {
       rv.SuppressException();
       return false;
     }
 
     extendableEvent->SetTrusted(true);
 
-    return DispatchExtendableEventOnWorkerScope(aCx, aWorkerPrivate->GlobalScope(),
-                                                extendableEvent, nullptr);
+    return NS_SUCCEEDED(DispatchExtendableEventOnWorkerScope(aCx,
+                                                             aWorkerPrivate->GlobalScope(),
+                                                             extendableEvent,
+                                                             nullptr));
   }
 };
 
 } // anonymous namespace
 
 nsresult
 ServiceWorkerPrivate::SendMessageEvent(JSContext* aCx,
                                        JS::Handle<JS::Value> aMessage,
@@ -754,18 +764,22 @@ LifecycleEventWorkerRunnable::DispatchLi
   // is still executing. This can happen with infinite loops, for example.
   RefPtr<LifeCycleEventWatcher> watcher =
     new LifeCycleEventWatcher(aWorkerPrivate, mCallback);
 
   if (!watcher->Init()) {
     return true;
   }
 
-  if (!DispatchExtendableEventOnWorkerScope(aCx, aWorkerPrivate->GlobalScope(),
-                                       event, watcher)) {
+  nsresult rv = DispatchExtendableEventOnWorkerScope(aCx,
+                                                     aWorkerPrivate->GlobalScope(),
+                                                     event,
+                                                     watcher);
+  // Do not fail event processing when an exception is thrown.
+  if (NS_FAILED(rv) && rv != NS_ERROR_XPC_JS_THREW_EXCEPTION) {
     watcher->ReportResult(false);
   }
 
   return true;
 }
 
 } // anonymous namespace
 
@@ -818,17 +832,16 @@ public:
       Report(nsIPushErrorReporter::DELIVERY_UNHANDLED_REJECTION);
     }
   }
 
   void Report(uint16_t aReason = nsIPushErrorReporter::DELIVERY_INTERNAL_ERROR)
   {
     WorkerPrivate* workerPrivate = mWorkerPrivate;
     mWorkerPrivate->AssertIsOnWorkerThread();
-    mWorkerPrivate = nullptr;
 
     if (NS_WARN_IF(aReason > nsIPushErrorReporter::DELIVERY_INTERNAL_ERROR) ||
         mMessageId.IsEmpty()) {
       return;
     }
     nsCOMPtr<nsIRunnable> runnable =
       NewRunnableMethod<uint16_t>(this,
         &PushErrorReporter::ReportOnMainThread, aReason);
@@ -896,18 +909,22 @@ public:
       PushEvent::Constructor(globalObj, NS_LITERAL_STRING("push"), pei, result);
     if (NS_WARN_IF(result.Failed())) {
       result.SuppressException();
       errorReporter->Report();
       return false;
     }
     event->SetTrusted(true);
 
-    if (!DispatchExtendableEventOnWorkerScope(aCx, aWorkerPrivate->GlobalScope(),
-                                              event, errorReporter)) {
+    nsresult rv = DispatchExtendableEventOnWorkerScope(aCx,
+                                                       aWorkerPrivate->GlobalScope(),
+                                                       event,
+                                                       errorReporter);
+    if (NS_FAILED(rv)) {
+      // We don't cancel WorkerPrivate when catching an excetpion.
       errorReporter->Report(nsIPushErrorReporter::DELIVERY_UNCAUGHT_EXCEPTION);
     }
 
     return true;
   }
 };
 
 class SendPushSubscriptionChangeEventRunnable final : public ExtendableEventWorkerRunnable
@@ -1225,18 +1242,22 @@ public:
     if (NS_WARN_IF(result.Failed())) {
       return false;
     }
 
     event->SetTrusted(true);
     aWorkerPrivate->GlobalScope()->AllowWindowInteraction();
     RefPtr<AllowWindowInteractionHandler> allowWindowInteraction =
       new AllowWindowInteractionHandler(aWorkerPrivate);
-    if (!DispatchExtendableEventOnWorkerScope(aCx, aWorkerPrivate->GlobalScope(),
-                                              event, allowWindowInteraction)) {
+    nsresult rv = DispatchExtendableEventOnWorkerScope(aCx,
+                                                       aWorkerPrivate->GlobalScope(),
+                                                       event,
+                                                       allowWindowInteraction);
+    // Don't reject when catching an exception
+    if (NS_FAILED(rv) && rv != NS_ERROR_XPC_JS_THREW_EXCEPTION) {
       allowWindowInteraction->FinishedWithResult(Rejected);
     }
     aWorkerPrivate->GlobalScope()->ConsumeWindowInteraction();
 
     return true;
   }
 };
 
@@ -1576,35 +1597,29 @@ private:
     if (NS_WARN_IF(result.Failed())) {
       result.SuppressException();
       return false;
     }
 
     event->PostInit(mInterceptedChannel, mRegistration, mScriptSpec);
     event->SetTrusted(true);
 
-    bool rv2 =
+    nsresult rv2 =
       DispatchExtendableEventOnWorkerScope(aCx, aWorkerPrivate->GlobalScope(),
                                            event, nullptr);
-    if (NS_WARN_IF(!rv2) || !event->WaitToRespond()) {
+    if (NS_WARN_IF(NS_FAILED(rv2)) || !event->WaitToRespond()) {
       nsCOMPtr<nsIRunnable> runnable;
       MOZ_ASSERT(!aWorkerPrivate->UsesSystemPrincipal(),
                  "We don't support system-principal serviceworkers");
       if (event->DefaultPrevented(CallerType::NonSystem)) {
-        event->ReportCanceled();
-      } else if (event->WidgetEventPtr()->mFlags.mExceptionWasRaised) {
-        // Exception logged via the WorkerPrivate ErrorReporter
-      } else {
-        runnable = new ResumeRequest(mInterceptedChannel);
-      }
-
-      if (!runnable) {
         runnable = new CancelChannelRunnable(mInterceptedChannel,
                                              mRegistration,
                                              NS_ERROR_INTERCEPTION_FAILED);
+      } else {
+        runnable = new ResumeRequest(mInterceptedChannel);
       }
 
       MOZ_ALWAYS_SUCCEEDS(mWorkerPrivate->DispatchToMainThread(runnable.forget()));
     }
 
     return true;
   }
 
--- a/dom/workers/test/serviceworkers/install_event_error_worker.js
+++ b/dom/workers/test/serviceworkers/install_event_error_worker.js
@@ -1,4 +1,7 @@
 // Worker that errors on receiving an install event.
 oninstall = function(e) {
-  undefined.doSomething;
+  e.waitUntil( new Promise(function(resolve, reject) {
+    undefined.doSomething;
+    resolve();
+  }));
 };
--- a/testing/web-platform/meta/service-workers/service-worker/ServiceWorkerGlobalScope/registration-attribute.https.html.ini
+++ b/testing/web-platform/meta/service-workers/service-worker/ServiceWorkerGlobalScope/registration-attribute.https.html.ini
@@ -1,6 +1,5 @@
 [registration-attribute.https.html]
   type: testharness
-  expected: TIMEOUT
   [Verify registration attribute on ServiceWorkerGlobalScope]
-    expected: TIMEOUT
+    expected: FAIL
 
--- a/testing/web-platform/tests/service-workers/service-worker/oninstall-script-error.https.html
+++ b/testing/web-platform/tests/service-workers/service-worker/oninstall-script-error.https.html
@@ -28,40 +28,45 @@ function make_test(name, script, expect_
           })
     }, name);
 }
 
 [
   {
     name: 'install handler throws an error',
     script: 'resources/oninstall-throw-error-worker.js',
-    expect_install: false
+    expect_install: true
   },
   {
     name: 'install handler throws an error, error handler does not cancel',
     script: 'resources/oninstall-throw-error-with-empty-onerror-worker.js',
-    expect_install: false
+    expect_install: true
   },
   {
     name: 'install handler dispatches an event that throws an error',
     script: 'resources/oninstall-throw-error-from-nested-event-worker.js',
     expect_install: true
   },
+  {
+    name: 'install handler throws an error in the waitUntil',
+    script: 'resources/oninstall-waituntil-throw-error-worker.js',
+    expect_install: false
+  },
 
   // The following two cases test what happens when the ServiceWorkerGlobalScope
   // 'error' event handler cancels the resulting error event.  Since the
   // original 'install' event handler through, the installation should still
   // be stopped in this case.  See:
   // https://github.com/slightlyoff/ServiceWorker/issues/778
   {
     name: 'install handler throws an error that is cancelled',
     script: 'resources/oninstall-throw-error-then-cancel-worker.js',
-    expect_install: false
+    expect_install: true
   },
   {
     name: 'install handler throws an error and prevents default',
     script: 'resources/oninstall-throw-error-then-prevent-default-worker.js',
-    expect_install: false
+    expect_install: true
   }
 ].forEach(function(test_case) {
     make_test(test_case.name, test_case.script, test_case.expect_install);
   });
 </script>
--- a/testing/web-platform/tests/service-workers/service-worker/resources/fetch-cors-xhr-iframe.html
+++ b/testing/web-platform/tests/service-workers/service-worker/resources/fetch-cors-xhr-iframe.html
@@ -61,18 +61,18 @@ window.addEventListener('message', funct
     // expected output text.
     var TEST_CASES = [
       // Reject tests
       [url + '?reject', false, FAIL],
       [url + '?reject', true, FAIL],
       [remote_url + '?reject', false, FAIL],
       [remote_url + '?reject', true, FAIL],
       // Event handler exception tests
-      [url + '?throw', false, FAIL],
-      [url + '?throw', true, FAIL],
+      [url + '?throw', false, SUCCESS],
+      [url + '?throw', true, SUCCESS],
       [remote_url + '?throw', false, FAIL],
       [remote_url + '?throw', true, FAIL],
       // Reject(resolve-null) tests
       [url + '?resolve-null', false, FAIL],
       [url + '?resolve-null', true, FAIL],
       [remote_url + '?resolve-null', false, FAIL],
       [remote_url + '?resolve-null', true, FAIL],
       // Fallback tests
--- a/testing/web-platform/tests/service-workers/service-worker/resources/fetch-event-network-error-controllee-iframe.html
+++ b/testing/web-platform/tests/service-workers/service-worker/resources/fetch-event-network-error-controllee-iframe.html
@@ -35,17 +35,18 @@ function make_test(testcase) {
 function run_tests() {
   var tests = [
     { name: 'prevent-default-and-respond-with', expect_load: true },
     { name: 'prevent-default', expect_load: false },
     { name: 'reject', expect_load: false },
     { name: 'unused-body', expect_load: true },
     { name: 'used-body', expect_load: false },
     { name: 'unused-fetched-body', expect_load: true },
-    { name: 'used-fetched-body', expect_load: false }
+    { name: 'used-fetched-body', expect_load: false },
+    { name: 'throw-exception', expect_load: true },
   ].map(make_test);
 
   Promise.all(tests)
     .then(function() {
         window.parent.notify_test_done('PASS');
       })
     .catch(function(error) {
         window.parent.notify_test_done('FAIL: ' + error.message);
--- a/testing/web-platform/tests/service-workers/service-worker/resources/fetch-event-network-error-worker.js
+++ b/testing/web-platform/tests/service-workers/service-worker/resources/fetch-event-network-error-worker.js
@@ -27,16 +27,19 @@ self.addEventListener('fetch', function(
         }));
       break;
     case '?used-fetched-body':
       event.respondWith(fetch('other.html').then(function(res){
           res.text();
           return res;
         }));
       break;
+    case '?throw-exception':
+      throw('boom');
+      break;
     }
   });
 
 self.addEventListener('fetch', function(event) {});
 
 self.addEventListener('fetch', function(event) {
     var testcase = new URL(event.request.url).search;
     if (testcase == '?prevent-default-and-respond-with')
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/service-workers/service-worker/resources/oninstall-waituntil-throw-error-worker.js
@@ -0,0 +1,5 @@
+self.addEventListener('install', function(event) {
+  event.waitUntil(new Promise(function(aRequest, aResponse) {
+      throw new Error();
+    }));
+});