From 5c5c06c9ddedfa0363f499e445893847db28ea91 Mon Sep 17 00:00:00 2001
From: Peter Van der Beken <peterv@propagandism.org>
Date: Tue, 3 May 2022 08:23:25 +0000
Subject: [PATCH] Bug 1764935 - Deal properly with Promise creation failure in
 NavigationPreloadManager. r=asuth

Differential Revision: https://phabricator.services.mozilla.com/D144341
---
 .../NavigationPreloadManager.cpp              | 45 ++++++++--------
 dom/serviceworkers/NavigationPreloadManager.h | 11 ++--
 dom/serviceworkers/test/mochitest-common.ini  |  2 +
 .../test_navigationPreload_disable_crash.html | 52 +++++++++++++++++++
 dom/webidl/NavigationPreloadManager.webidl    |  4 ++
 5 files changed, 85 insertions(+), 29 deletions(-)
 create mode 100644 dom/serviceworkers/test/test_navigationPreload_disable_crash.html

diff --git a/dom/serviceworkers/NavigationPreloadManager.cpp b/dom/serviceworkers/NavigationPreloadManager.cpp
index 01f6a179e89b..c1f56a7dbc0d 100644
--- a/dom/serviceworkers/NavigationPreloadManager.cpp
+++ b/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) {
diff --git a/dom/serviceworkers/NavigationPreloadManager.h b/dom/serviceworkers/NavigationPreloadManager.h
index 3ba801f740a9..45bcd6948e2c 100644
--- a/dom/serviceworkers/NavigationPreloadManager.h
+++ b/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;
diff --git a/dom/serviceworkers/test/mochitest-common.ini b/dom/serviceworkers/test/mochitest-common.ini
index 10f300ef00aa..507a903bc909 100644
--- a/dom/serviceworkers/test/mochitest-common.ini
+++ b/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
diff --git a/dom/serviceworkers/test/test_navigationPreload_disable_crash.html b/dom/serviceworkers/test/test_navigationPreload_disable_crash.html
new file mode 100644
index 000000000000..ea6439284dda
--- /dev/null
+++ b/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>
diff --git a/dom/webidl/NavigationPreloadManager.webidl b/dom/webidl/NavigationPreloadManager.webidl
index d69202bcab44..f58bcb745822 100644
--- a/dom/webidl/NavigationPreloadManager.webidl
+++ b/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();
 };