Bug 1764935 - Deal properly with Promise creation failure in NavigationPreloadManager. r=asuth

Differential Revision: https://phabricator.services.mozilla.com/D144341

dom/serviceworkers/NavigationPreloadManager.cpp

@@ -40,13 +40,12 @@ JSObject* NavigationPreloadManager::WrapObject(
   return NavigationPreloadManager_Binding::Wrap(aCx, this, aGivenProto);
 }
 
-already_AddRefed<Promise> NavigationPreloadManager::SetEnabled(bool aEnabled) {
-  ErrorResult result;
-  RefPtr<Promise> promise = Promise::Create(GetParentObject(), result);
+already_AddRefed<Promise> NavigationPreloadManager::SetEnabled(
+    bool aEnabled, ErrorResult& aError) {
+  RefPtr<Promise> promise = Promise::Create(GetParentObject(), aError);
 
-  if (NS_WARN_IF(result.Failed())) {
-    result.SuppressException();
-    return promise.forget();
+  if (NS_WARN_IF(aError.Failed())) {
+    return nullptr;
   }
 
   if (!mInner) {
@@ -68,27 +67,26 @@ already_AddRefed<Promise> NavigationPreloadManager::SetEnabled(bool aEnabled) {
   return promise.forget();
 }
 
-already_AddRefed<Promise> NavigationPreloadManager::Enable() {
-  return SetEnabled(true);
+already_AddRefed<Promise> NavigationPreloadManager::Enable(
+    ErrorResult& aError) {
+  return SetEnabled(true, aError);
 }
 
-already_AddRefed<Promise> NavigationPreloadManager::Disable() {
-  return SetEnabled(false);
+already_AddRefed<Promise> NavigationPreloadManager::Disable(
+    ErrorResult& aError) {
+  return SetEnabled(false, aError);
 }
 
 already_AddRefed<Promise> NavigationPreloadManager::SetHeaderValue(
-    const nsACString& aHeader) {
-  ErrorResult result;
-  RefPtr<Promise> promise = Promise::Create(GetParentObject(), result);
+    const nsACString& aHeader, ErrorResult& aError) {
+  RefPtr<Promise> promise = Promise::Create(GetParentObject(), aError);
 
-  if (NS_WARN_IF(result.Failed())) {
-    result.SuppressException();
-    return promise.forget();
+  if (NS_WARN_IF(aError.Failed())) {
+    return nullptr;
   }
 
   if (!IsValidHeader(aHeader)) {
-    result.ThrowTypeError<MSG_INVALID_HEADER_VALUE>(aHeader);
-    promise->MaybeReject(std::move(result));
+    promise->MaybeRejectWithTypeError<MSG_INVALID_HEADER_VALUE>(aHeader);
     return promise.forget();
   }
 
@@ -111,13 +109,12 @@ already_AddRefed<Promise> NavigationPreloadManager::SetHeaderValue(
   return promise.forget();
 }
 
-already_AddRefed<Promise> NavigationPreloadManager::GetState() {
-  ErrorResult result;
-  RefPtr<Promise> promise = Promise::Create(GetParentObject(), result);
+already_AddRefed<Promise> NavigationPreloadManager::GetState(
+    ErrorResult& aError) {
+  RefPtr<Promise> promise = Promise::Create(GetParentObject(), aError);
 
-  if (NS_WARN_IF(result.Failed())) {
-    result.SuppressException();
-    return promise.forget();
+  if (NS_WARN_IF(aError.Failed())) {
+    return nullptr;
   }
 
   if (!mInner) {

dom/serviceworkers/NavigationPreloadManager.h

@@ -39,19 +39,20 @@ class NavigationPreloadManager final : public nsISupports,
                        JS::Handle<JSObject*> aGivenProto) override;
 
   // WebIdl implementation
-  already_AddRefed<Promise> Enable();
+  already_AddRefed<Promise> Enable(ErrorResult& aError);
 
-  already_AddRefed<Promise> Disable();
+  already_AddRefed<Promise> Disable(ErrorResult& aError);
 
-  already_AddRefed<Promise> SetHeaderValue(const nsACString& aHeader);
+  already_AddRefed<Promise> SetHeaderValue(const nsACString& aHeader,
+                                           ErrorResult& aError);
 
-  already_AddRefed<Promise> GetState();
+  already_AddRefed<Promise> GetState(ErrorResult& aError);
 
  private:
   ~NavigationPreloadManager() = default;
 
   // General method for Enable()/Disable()
-  already_AddRefed<Promise> SetEnabled(bool aEnabled);
+  already_AddRefed<Promise> SetEnabled(bool aEnabled, ErrorResult& aError);
 
   nsCOMPtr<nsIGlobalObject> mGlobal;
   RefPtr<ServiceWorkerRegistration::Inner> mInner;

dom/serviceworkers/test/mochitest-common.ini

@@ -270,6 +270,8 @@ skip-if = xorigin # JavaScript error: http://mochi.xorigin-test:8888/tests/Simpl
 skip-if = toolkit == 'android' && !is_fennec
 [test_match_all_client_properties.html]
 skip-if = toolkit == 'android' && !is_fennec
+[test_navigationPreload_disable_crash.html]
+scheme = https
 [test_navigator.html]
 [test_not_intercept_plugin.html]
 skip-if = serviceworker_e10s # leaks InterceptedHttpChannel and others things

dom/serviceworkers/test/test_navigationPreload_disable_crash.html

@@ -0,0 +1,52 @@
+<!--
+  Any copyright is dedicated to the Public Domain.
+  http://creativecommons.org/publicdomain/zero/1.0/
+-->
+<!DOCTYPE HTML>
+<html>
+<head>
+  <title>Failure to create a Promise shouldn't crash</title>
+  <script src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+<p id="display"></p>
+<div id="content" style="display: none">
+</div>
+<pre id="test"></pre>
+<script class="testbody" type="text/javascript">
+  async function runTest() {
+    const iframe = document.createElement('iframe');
+    document.getElementById("content").appendChild(iframe);
+
+    const serviceWorker = iframe.contentWindow.navigator.serviceWorker;
+    const worker = await iframe.contentWindow.navigator.serviceWorker.register("empty.js", {});
+
+    iframe.remove();
+
+    // We can't wait for this promise to settle, because the global's
+    // browsing context has been discarded when the iframe was removed.
+    // We're just checking if this call crashes, which would happen
+    // immediately, so ignoring the promise should be fine.
+    worker.navigationPreload.disable();
+    ok(true, "navigationPreload.disable() failed but didn't crash.");
+
+    SimpleTest.finish();
+  }
+
+  SimpleTest.waitForExplicitFinish();
+  // We can't call unregister on the worker after its browsing context has been
+  // discarded, so use SpecialPowers.removeAllServiceWorkerData.
+  SimpleTest.registerCleanupFunction(() => SpecialPowers.removeAllServiceWorkerData());
+  onload = function() {
+    SpecialPowers.pushPrefEnv({"set": [
+      ["dom.serviceWorkers.exemptFromPerDomainMax", true],
+      ["dom.serviceWorkers.enabled", true],
+      ["dom.serviceWorkers.navigationPreload.enabled", true],
+      ["dom.serviceWorkers.testing.enabled", true],
+    ]}, runTest);
+  };
+</script>
+</pre>
+</body>
+</html>

dom/webidl/NavigationPreloadManager.webidl

@@ -10,9 +10,13 @@
 [Pref="dom.serviceWorkers.navigationPreload.enabled", SecureContext,
  Exposed=(Window,Worker)]
 interface NavigationPreloadManager {
+  [NewObject]
   Promise<void> enable();
+  [NewObject]
   Promise<void> disable();
+  [NewObject]
   Promise<void> setHeaderValue(ByteString value);
+  [NewObject]
   Promise<NavigationPreloadState> getState();
 };