Bug 1408734 - Return InvalidStateError when accessing ServiceWorkerRegistration::updateViaCache after unregister. r=bkelly, a=ritu
authorEden Chuang <echuang@mozilla.com>
Thu, 26 Oct 2017 22:55:19 +0800
changeset 435305 39275addee5933725bdaf7f5de328e47df39790a
parent 435304 24b1d36cd71070d9a8b7b6b719139351d62f5167
child 435306 84af2700ac30dfc0e80759c17f66b46b9733021b
push id1567
push userjlorenzo@mozilla.com
push dateThu, 02 Nov 2017 12:36:05 +0000
treeherdermozilla-release@e512c14a0406 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbkelly, ritu
bugs1408734
milestone57.0
Bug 1408734 - Return InvalidStateError when accessing ServiceWorkerRegistration::updateViaCache after unregister. r=bkelly, a=ritu This patch implements followings 1. Adding extended attribute [Throws] on ServiceWorkerRegistration ::updateViaCacheattribute. 2. Instead of calling MOZ_ASSERT, returning InvalidStateError when fail to get the registration in ServiceWorkerRegistration::GetUpdateViaCache(). 3. Adding a new mochitest test_bug1408734.html to reproduce the bug introduced by accessing ServiceWorkerRegistration::updateViaCache after unregister() finishes.
dom/webidl/ServiceWorkerRegistration.webidl
dom/workers/ServiceWorkerRegistration.cpp
dom/workers/ServiceWorkerRegistration.h
dom/workers/test/serviceworkers/mochitest.ini
dom/workers/test/serviceworkers/test_bug1408734.html
--- a/dom/webidl/ServiceWorkerRegistration.webidl
+++ b/dom/webidl/ServiceWorkerRegistration.webidl
@@ -12,16 +12,17 @@
 [Func="mozilla::dom::ServiceWorkerRegistration::Visible",
  Exposed=(Window,Worker)]
 interface ServiceWorkerRegistration : EventTarget {
   [Unforgeable] readonly attribute ServiceWorker? installing;
   [Unforgeable] readonly attribute ServiceWorker? waiting;
   [Unforgeable] readonly attribute ServiceWorker? active;
 
   readonly attribute USVString scope;
+  [Throws]
   readonly attribute ServiceWorkerUpdateViaCache updateViaCache;
 
   [Throws, NewObject]
   Promise<void> update();
 
   [Throws, NewObject]
   Promise<boolean> unregister();
 
--- a/dom/workers/ServiceWorkerRegistration.cpp
+++ b/dom/workers/ServiceWorkerRegistration.cpp
@@ -135,31 +135,42 @@ public:
 
   void
   GetScope(nsAString& aScope) const override
   {
     aScope = mScope;
   }
 
   ServiceWorkerUpdateViaCache
-  UpdateViaCache() const override
+  GetUpdateViaCache(ErrorResult& aRv) const override
   {
     RefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
     MOZ_ASSERT(swm);
 
     nsCOMPtr<nsPIDOMWindowInner> window = GetOwner();
     MOZ_ASSERT(window);
 
     nsCOMPtr<nsIDocument> doc = window->GetExtantDoc();
     MOZ_ASSERT(doc);
 
     nsCOMPtr<nsIServiceWorkerRegistrationInfo> registration;
     nsresult rv = swm->GetRegistrationByPrincipal(doc->NodePrincipal(), mScope,
                                                   getter_AddRefs(registration));
-    MOZ_ASSERT(NS_SUCCEEDED(rv) && registration);
+
+    /*
+     *  xxx: We should probably store the `updateViaCache` value on the
+     *  ServiceWorkerRegistration object and update it when necessary.
+     *  We don't have a good way to reach all ServiceWorkerRegistration objects
+     *  from the ServiceWorkerRegistratinInfo right now, though.
+     *  This is a short term fix to avoid crashing.
+     */
+    if (NS_FAILED(rv) || !registration) {
+      aRv = NS_ERROR_DOM_INVALID_STATE_ERR;
+      return ServiceWorkerUpdateViaCache::None;
+    }
 
     uint16_t updateViaCache;
     rv = registration->GetUpdateViaCache(&updateViaCache);
     MOZ_ASSERT(NS_SUCCEEDED(rv));
 
     // Silence possible compiler warnings.
     Unused << rv;
 
@@ -939,17 +950,17 @@ public:
 
   void
   GetScope(nsAString& aScope) const override
   {
     aScope = mScope;
   }
 
   ServiceWorkerUpdateViaCache
-  UpdateViaCache() const override
+  GetUpdateViaCache(ErrorResult& aRv) const override
   {
     // FIXME(hopang): Will be implemented after Bug 1113522.
     return ServiceWorkerUpdateViaCache::Imports;
   }
 
   bool
   Notify(Status aStatus) override;
 
--- a/dom/workers/ServiceWorkerRegistration.h
+++ b/dom/workers/ServiceWorkerRegistration.h
@@ -87,17 +87,17 @@ public:
 
   virtual already_AddRefed<workers::ServiceWorker>
   GetActive() = 0;
 
   virtual void
   GetScope(nsAString& aScope) const = 0;
 
   virtual ServiceWorkerUpdateViaCache
-  UpdateViaCache() const = 0;
+  GetUpdateViaCache(ErrorResult& aRv) const = 0;
 
   virtual already_AddRefed<Promise>
   Update(ErrorResult& aRv) = 0;
 
   virtual already_AddRefed<Promise>
   Unregister(ErrorResult& aRv) = 0;
 
   virtual already_AddRefed<PushManager>
--- a/dom/workers/test/serviceworkers/mochitest.ini
+++ b/dom/workers/test/serviceworkers/mochitest.ini
@@ -220,16 +220,17 @@ support-files =
   service_worker.js
   service_worker_client.html
   utils.js
   bug1290951_worker_main.sjs
   bug1290951_worker_imported.sjs
 
 [test_bug1151916.html]
 [test_bug1240436.html]
+[test_bug1408734.html]
 [test_claim.html]
 [test_claim_oninstall.html]
 [test_controller.html]
 [test_cookie_fetch.html]
 [test_cross_origin_url_after_redirect.html]
 skip-if = debug # Bug 1262224
 [test_csp_upgrade-insecure_intercept.html]
 [test_empty_serviceworker.html]
new file mode 100644
--- /dev/null
+++ b/dom/workers/test/serviceworkers/test_bug1408734.html
@@ -0,0 +1,65 @@
+<!--
+  Any copyright is dedicated to the Public Domain.
+  http://creativecommons.org/publicdomain/zero/1.0/
+-->
+<!DOCTYPE HTML>
+<html>
+<head>
+  <title>Bug 1408734</title>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+  <script src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script src="/tests/SimpleTest/SpawnTask.js"></script>
+  <script src="error_reporting_helpers.js"></script>
+</head>
+<body>
+<p id="display"></p>
+<div id="content" style="display: none"></div>
+<pre id="test"></pre>
+<script class="testbody" type="text/javascript">
+
+// setup prefs
+add_task(() => {
+  return SpecialPowers.pushPrefEnv({"set": [
+    ["dom.serviceWorkers.enabled", true],
+    ["dom.serviceWorkers.testing.enabled", true],
+  ]});
+});
+
+// test for bug 1408734
+add_task(async () => {
+  let waitForControlled = new Promise((resolve) => {
+    navigator.serviceWorker.oncontrollerchange = resolve;
+  });
+
+  // register a service worker
+  let registration = await navigator.serviceWorker.register("fetch.js", {scope: "./"});
+  let worker = registration.installing || registration.active;
+
+  // wait for control changed
+  worker.postMessage('claim');
+  await waitForControlled;
+
+  // get the ServiceWorkerRegistration we just register through GetRegistration
+  registration = await navigator.serviceWorker.getRegistration("./");
+  ok(registration, "should get the registration under scope './'");
+
+  // call unregister()
+  await registration.unregister();
+
+  // access registration.updateViaCache to trigger the bug
+  // we really care that we don't crash. In the future we will fix
+  // updateViaCache to be accessible after unregister().
+  try {
+    if (registration.updateViaCache) {
+      ok(false,
+         "Expected InvalidStateError when accessing registration.updateViaCache after unregister()");
+    }
+  } catch (err) {
+    is(err.name, "InvalidStateError", "Expected InvalidStateError.");
+  }
+});
+
+</script>
+</pre>
+</body>
+</html>