Bug 1662771, remove willDestroy callback from JSActors, r=nika

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

dom/chrome-webidl/JSActor.webidl

@@ -30,27 +30,20 @@ callback interface MozObserverCallback {
 };
 
 /**
- * WebIDL callback interface calling the `willDestroy`, `didDestroy`, and
+ * WebIDL callback interface calling the `didDestroy`, and
  * `actorCreated` methods on JSActors.
  */
 [MOZ_CAN_RUN_SCRIPT_BOUNDARY]
 callback MozJSActorCallback = void();
 
 /**
- * The willDestroy method, if present, will be called at the last opportunity
- * to send messages to the remote side, giving implementers the chance to clean
- * up and send final messages.
  * The didDestroy method, if present, will be called after the actor is no
  * longer able to receive any more messages.
  * The actorCreated method, if present, will be called immediately after the
  * actor has been created and initialized.
- *
- * NOTE: Messages may be received between willDestroy and didDestroy, but they
- * may not be sent.
  */
 [GenerateInit]
 dictionary MozJSActorCallbacks {
-  [ChromeOnly] MozJSActorCallback willDestroy;
   [ChromeOnly] MozJSActorCallback didDestroy;
   [ChromeOnly] MozJSActorCallback actorCreated;
 };

dom/docs/ipc/jsactors.rst

@@ -201,19 +201,6 @@ If you register your Actor to listen for content events, implement a ``handleEve
 
 This method is called immediately *after* a child actor is created and initialized. Unlike the actor's constructor, it is possible to do things like access the actor's content window and send messages from this callback.
 
-``willDestroy``
-```````````````
-
-This method is called when we know that the `JSActor` pair is going to be destroyed because the associated BrowsingContext is going away. You should override this method if you have any cleanup you need to do before going away.
-
-You can also use ``willDestroy`` as a last opportunity to send messages to the other side, as the communications channel at this point is still running.
-
-.. note::
-    This method cannot be async.
-
-.. note::
-    As a `JSProcessActorChild` is destroyed when its process dies, a `JSProcessActorChild` will never receive this call.
-
 ``didDestroy``
 ``````````````
 
@@ -568,4 +555,4 @@ And more
 .. _ContentDOMReference.jsm: https://searchfox.org/mozilla-central/source/toolkit/modules/ContentDOMReference.jsm
 .. _JSProcessActor.webidl: https://searchfox.org/mozilla-central/source/dom/chrome-webidl/JSWindowActor.webidl
 .. _JSWindowActor.webidl: https://searchfox.org/mozilla-central/source/dom/chrome-webidl/JSWindowActor.webidl
-.. _BrowserElementParent.jsm: https://searchfox.org/mozilla-central/rev/ec806131cb7bcd1c26c254d25cd5ab8a61b2aeb6/toolkit/actors/BrowserElementParent.jsm
+.. _BrowserElementParent.jsm: https://searchfox.org/mozilla-central/rev/ec806131cb7bcd1c26c254d25cd5ab8a61b2aeb6/toolkit/actors/BrowserElementParent.jsm

dom/ipc/WindowGlobalChild.cpp

@@ -328,22 +328,15 @@ void WindowGlobalChild::BeforeUnloadRemoved() {
 }
 
 void WindowGlobalChild::Destroy() {
-  // Destroying a WindowGlobalChild requires running script, so hold off on
-  // doing it until we can safely run JS callbacks.
-  nsContentUtils::AddScriptRunner(NS_NewRunnableFunction(
-      "WindowGlobalChild::Destroy", [self = RefPtr<WindowGlobalChild>(this)]() {
-        // Make a copy so that we can avoid potential iterator invalidation when
-        // calling the user-provided Destroy() methods.
-        self->JSActorWillDestroy();
+  JSActorWillDestroy();
 
-        // Perform async IPC shutdown unless we're not in-process, and our
-        // BrowserChild is in the process of being destroyed, which will destroy
-        // us as well.
-        RefPtr<BrowserChild> browserChild = self->GetBrowserChild();
-        if (!browserChild || !browserChild->IsDestroyed()) {
-          self->SendDestroy();
-        }
-      }));
+  // Perform async IPC shutdown unless we're not in-process, and our
+  // BrowserChild is in the process of being destroyed, which will destroy
+  // us as well.
+  RefPtr<BrowserChild> browserChild = GetBrowserChild();
+  if (!browserChild || !browserChild->IsDestroyed()) {
+    SendDestroy();
+  }
 }
 
 mozilla::ipc::IPCResult WindowGlobalChild::RecvMakeFrameLocal(

dom/ipc/jsactor/JSActor.cpp

@@ -60,10 +60,7 @@ JSActor::JSActor(nsISupports* aGlobal) {
   }
 }
 
-void JSActor::StartDestroy() {
-  InvokeCallback(CallbackFunction::WillDestroy);
-  mCanSend = false;
-}
+void JSActor::StartDestroy() { mCanSend = false; }
 
 void JSActor::AfterDestroy() {
   mCanSend = false;
@@ -95,11 +92,7 @@ void JSActor::InvokeCallback(CallbackFunction callback) {
   }
 
   // Destroy callback is optional.
-  if (callback == CallbackFunction::WillDestroy) {
-    if (callbacksHolder.mWillDestroy.WasPassed()) {
-      callbacksHolder.mWillDestroy.Value()->Call(this);
-    }
-  } else if (callback == CallbackFunction::DidDestroy) {
+  if (callback == CallbackFunction::DidDestroy) {
     if (callbacksHolder.mDidDestroy.WasPassed()) {
       callbacksHolder.mDidDestroy.Value()->Call(this);
     }

dom/ipc/jsactor/JSActor.h

@@ -85,8 +85,8 @@ class JSActor : public nsISupports, public nsWrapperCache {
   void StartDestroy();
   void AfterDestroy();
 
-  enum class CallbackFunction { WillDestroy, DidDestroy, ActorCreated };
-  void InvokeCallback(CallbackFunction willDestroy);
+  enum class CallbackFunction { DidDestroy, ActorCreated };
+  void InvokeCallback(CallbackFunction callback);
 
   virtual void ClearManager() = 0;
 

dom/ipc/jsactor/JSActorManager.cpp

@@ -192,20 +192,8 @@ void JSActorManager::ReceiveRawMessage(
 }
 
 void JSActorManager::JSActorWillDestroy() {
-  MOZ_ASSERT(nsContentUtils::IsSafeToRunScript());
-  CrashReporter::AutoAnnotateCrashReport autoMessageName(
-      CrashReporter::Annotation::JSActorMessage, "<WillDestroy>"_ns);
-
-  // Make a copy so that we can avoid potential iterator invalidation when
-  // calling the user-provided Destroy() methods.
-  nsTArray<RefPtr<JSActor>> actors(mJSActors.Count());
   for (auto& entry : mJSActors) {
-    actors.AppendElement(entry.GetData());
-  }
-  for (auto& actor : actors) {
-    CrashReporter::AutoAnnotateCrashReport autoActorName(
-        CrashReporter::Annotation::JSActorName, actor->Name());
-    actor->StartDestroy();
+    entry.GetData()->StartDestroy();
   }
 }
 

dom/ipc/jsactor/JSActorManager.h

@@ -42,10 +42,15 @@ class JSActorManager : public nsISupports {
 
  protected:
   /**
-   * Lifecycle methods which will fire the `willDestroy` and `didDestroy`
-   * methods on relevant actors.
+   * The actor is about to be destroyed so prevent it from sending any
+   * more messages.
    */
   void JSActorWillDestroy();
+
+  /**
+   * Lifecycle method which will fire the `didDestroy` methods on relevant
+   * actors.
+   */
   void JSActorDidDestroy();
 
   /**
@@ -85,4 +90,4 @@ class JSActorManager : public nsISupports {
 }  // namespace dom
 }  // namespace mozilla
 
-#endif  // mozilla_dom_JSActorManager_h
+#endif  // mozilla_dom_JSActorManager_h

dom/ipc/tests/JSWindowActor/browser_destroy_callbacks.js

@@ -17,17 +17,6 @@ declTest("destroy actor by iframe remove", {
       let actorChild = child.getActor("TestWindow");
       ok(actorChild, "JSWindowActorChild should have value.");
 
-      let willDestroyPromise = new Promise(resolve => {
-        const TOPIC = "test-js-window-actor-willdestroy";
-        Services.obs.addObserver(function obs(subject, topic, data) {
-          ok(data, "willDestroyCallback data should be true.");
-          is(subject, actorChild, "Should have this value");
-
-          Services.obs.removeObserver(obs, TOPIC);
-          resolve();
-        }, TOPIC);
-      });
-
       let didDestroyPromise = new Promise(resolve => {
         const TOPIC = "test-js-window-actor-diddestroy";
         Services.obs.addObserver(function obs(subject, topic, data) {
@@ -41,7 +30,7 @@ declTest("destroy actor by iframe remove", {
 
       info("Remove frame");
       content.document.getElementById("frame").remove();
-      await Promise.all([willDestroyPromise, didDestroyPromise]);
+      await didDestroyPromise;
 
       Assert.throws(
         () => child.getActor("TestWindow"),
@@ -71,17 +60,6 @@ declTest("destroy actor by page navigates", {
       let actorChild = child.getActor("TestWindow");
       ok(actorChild, "JSWindowActorChild should have value.");
 
-      let willDestroyPromise = new Promise(resolve => {
-        const TOPIC = "test-js-window-actor-willdestroy";
-        Services.obs.addObserver(function obs(subject, topic, data) {
-          ok(data, "willDestroyCallback data should be true.");
-          is(subject, actorChild, "Should have this value");
-
-          Services.obs.removeObserver(obs, TOPIC);
-          resolve();
-        }, TOPIC);
-      });
-
       let didDestroyPromise = new Promise(resolve => {
         const TOPIC = "test-js-window-actor-diddestroy";
         Services.obs.addObserver(function obs(subject, topic, data) {
@@ -94,7 +72,6 @@ declTest("destroy actor by page navigates", {
       });
 
       await Promise.all([
-        willDestroyPromise,
         didDestroyPromise,
         ContentTaskUtils.waitForEvent(frame, "load"),
       ]);
@@ -125,18 +102,6 @@ declTest("destroy actor by tab being closed", {
     // frame message manager will be being shut down at the same time. Instead
     // send messages over the per-process message manager which should still be
     // active.
-    let willDestroyPromise = new Promise(resolve => {
-      Services.ppmm.addMessageListener(
-        "test-jswindowactor-willdestroy",
-        function onmessage(msg) {
-          Services.ppmm.removeMessageListener(
-            "test-jswindowactor-willdestroy",
-            onmessage
-          );
-          resolve();
-        }
-      );
-    });
     let didDestroyPromise = new Promise(resolve => {
       Services.ppmm.addMessageListener(
         "test-jswindowactor-diddestroy",
@@ -156,15 +121,6 @@ declTest("destroy actor by tab being closed", {
       let actorChild = child.getActor("TestWindow");
       ok(actorChild, "JSWindowActorChild should have value.");
 
-      Services.obs.addObserver(function obs(subject, topic, data) {
-        if (subject != actorChild) {
-          return;
-        }
-        dump("WillDestroy called\n");
-        Services.obs.removeObserver(obs, "test-js-window-actor-willdestroy");
-        Services.cpmm.sendAsyncMessage("test-jswindowactor-willdestroy", data);
-      }, "test-js-window-actor-willdestroy");
-
       Services.obs.addObserver(function obs(subject, topic, data) {
         if (subject != actorChild) {
           return;
@@ -178,8 +134,6 @@ declTest("destroy actor by tab being closed", {
     info("removing new tab");
     await BrowserTestUtils.removeTab(newTab);
     info("waiting for destroy callbacks to fire");
-    await willDestroyPromise;
-    info("got willDestroy callback");
     await didDestroyPromise;
     info("got didDestroy callback");
   },

testing/mochitest/BrowserTestUtils/ContentEventListenerChild.jsm

@@ -16,7 +16,7 @@ class ContentEventListenerChild extends JSWindowActorChild {
     Services.cpmm.sharedData.addEventListener("change", this);
   }
 
-  willDestroy() {
+  didDestroy() {
     this._shutdown = true;
     Services.cpmm.sharedData.removeEventListener("change", this);
     this._updateContentEventListeners(/* clearListeners = */ true);

toolkit/actors/TestProcessActorChild.jsm

@@ -51,14 +51,6 @@ class TestProcessActorChild extends JSProcessActorChild {
     return "TestProcessActorChild";
   }
 
-  willDestroy() {
-    Services.obs.notifyObservers(
-      this,
-      "test-js-content-actor-willdestroy",
-      true
-    );
-  }
-
   didDestroy() {
     Services.obs.notifyObservers(
       this,

toolkit/actors/TestWindowChild.jsm

@@ -65,14 +65,6 @@ class TestWindowChild extends JSWindowActorChild {
     return "TestWindowChild";
   }
 
-  willDestroy() {
-    Services.obs.notifyObservers(
-      this,
-      "test-js-window-actor-willdestroy",
-      true
-    );
-  }
-
   didDestroy() {
     Services.obs.notifyObservers(this, "test-js-window-actor-diddestroy", true);
   }