Bug 1538979 - Part 1: Clear mManager in ActorDestroy and disallow sending message while Destroying; r=nika
authorJohn Dai <jdai@mozilla.com>
Fri, 10 May 2019 15:01:33 +0000
changeset 473408 c30426096b2734fe2aa49fb6fd45129327bdfe04
parent 473407 840bfd02106371c14080e27d0e4820cbc42c63be
child 473409 0e3d9d0c32602bf7a0847cac48b3622d57a46247
push id35996
push userdvarga@mozilla.com
push dateFri, 10 May 2019 21:46:48 +0000
treeherdermozilla-central@362df4629f8f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnika
bugs1538979
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1538979 - Part 1: Clear mManager in ActorDestroy and disallow sending message while Destroying; r=nika Differential Revision: https://phabricator.services.mozilla.com/D30195
dom/ipc/JSWindowActorChild.cpp
dom/ipc/JSWindowActorChild.h
dom/ipc/JSWindowActorParent.cpp
dom/ipc/JSWindowActorParent.h
dom/ipc/WindowGlobalChild.cpp
dom/ipc/WindowGlobalParent.cpp
--- a/dom/ipc/JSWindowActorChild.cpp
+++ b/dom/ipc/JSWindowActorChild.cpp
@@ -10,16 +10,18 @@
 #include "mozilla/dom/WindowGlobalParent.h"
 #include "mozilla/dom/MessageManagerBinding.h"
 #include "mozilla/dom/BrowsingContext.h"
 #include "nsGlobalWindowInner.h"
 
 namespace mozilla {
 namespace dom {
 
+JSWindowActorChild::~JSWindowActorChild() { MOZ_ASSERT(!mManager); }
+
 JSObject* JSWindowActorChild::WrapObject(JSContext* aCx,
                                          JS::Handle<JSObject*> aGivenProto) {
   return JSWindowActorChild_Binding::Wrap(aCx, this, aGivenProto);
 }
 
 WindowGlobalChild* JSWindowActorChild::Manager() const { return mManager; }
 
 void JSWindowActorChild::Init(const nsAString& aName,
@@ -53,17 +55,17 @@ class AsyncMessageToParent : public Runn
   RefPtr<WindowGlobalParent> mParent;
 };
 
 }  // anonymous namespace
 
 void JSWindowActorChild::SendRawMessage(const JSWindowActorMessageMeta& aMeta,
                                         ipc::StructuredCloneData&& aData,
                                         ErrorResult& aRv) {
-  if (NS_WARN_IF(!mManager || mManager->IsClosed())) {
+  if (NS_WARN_IF(!mCanSend || !mManager || mManager->IsClosed())) {
     aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
     return;
   }
 
   if (mManager->IsInProcess()) {
     RefPtr<WindowGlobalParent> wgp = mManager->GetParentActor();
     nsCOMPtr<nsIRunnable> runnable =
         new AsyncMessageToParent(aMeta, std::move(aData), wgp);
@@ -107,16 +109,24 @@ BrowsingContext* JSWindowActorChild::Get
 Nullable<WindowProxyHolder> JSWindowActorChild::GetContentWindow(
     ErrorResult& aRv) {
   if (BrowsingContext* bc = GetBrowsingContext(aRv)) {
     return WindowProxyHolder(bc);
   }
   return nullptr;
 }
 
+void JSWindowActorChild::StartDestroy() {
+  mCanSend = false;
+}
+
+void JSWindowActorChild::AfterDestroy() {
+  mManager = nullptr;
+}
+
 NS_IMPL_CYCLE_COLLECTION_INHERITED(JSWindowActorChild, JSWindowActor, mManager)
 
 NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(JSWindowActorChild,
                                                JSWindowActor)
 NS_IMPL_CYCLE_COLLECTION_TRACE_END
 
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(JSWindowActorChild)
 NS_INTERFACE_MAP_END_INHERITING(JSWindowActor)
--- a/dom/ipc/JSWindowActorChild.h
+++ b/dom/ipc/JSWindowActorChild.h
@@ -42,28 +42,30 @@ class JSWindowActorChild final : public 
 
   static already_AddRefed<JSWindowActorChild> Constructor(GlobalObject& aGlobal,
                                                           ErrorResult& aRv) {
     return MakeAndAddRef<JSWindowActorChild>();
   }
 
   WindowGlobalChild* Manager() const;
   void Init(const nsAString& aName, WindowGlobalChild* aManager);
-
+  void StartDestroy();
+  void AfterDestroy();
   Document* GetDocument(ErrorResult& aRv);
   BrowsingContext* GetBrowsingContext(ErrorResult& aRv);
   Nullable<WindowProxyHolder> GetContentWindow(ErrorResult& aRv);
 
  protected:
   void SendRawMessage(const JSWindowActorMessageMeta& aMeta,
                       ipc::StructuredCloneData&& aData,
                       ErrorResult& aRv) override;
 
  private:
-  ~JSWindowActorChild() = default;
+  ~JSWindowActorChild();
 
+  bool mCanSend = true;
   RefPtr<WindowGlobalChild> mManager;
 };
 
 }  // namespace dom
 }  // namespace mozilla
 
 #endif  // mozilla_dom_JSWindowActorChild_h
--- a/dom/ipc/JSWindowActorParent.cpp
+++ b/dom/ipc/JSWindowActorParent.cpp
@@ -7,16 +7,18 @@
 #include "mozilla/dom/JSWindowActorBinding.h"
 #include "mozilla/dom/JSWindowActorParent.h"
 #include "mozilla/dom/WindowGlobalParent.h"
 #include "mozilla/dom/MessageManagerBinding.h"
 
 namespace mozilla {
 namespace dom {
 
+JSWindowActorParent::~JSWindowActorParent() { MOZ_ASSERT(!mManager); }
+
 JSObject* JSWindowActorParent::WrapObject(JSContext* aCx,
                                           JS::Handle<JSObject*> aGivenProto) {
   return JSWindowActorParent_Binding::Wrap(aCx, this, aGivenProto);
 }
 
 WindowGlobalParent* JSWindowActorParent::Manager() const { return mManager; }
 
 void JSWindowActorParent::Init(const nsAString& aName,
@@ -50,17 +52,17 @@ class AsyncMessageToChild : public Runna
   RefPtr<WindowGlobalChild> mChild;
 };
 
 }  // anonymous namespace
 
 void JSWindowActorParent::SendRawMessage(const JSWindowActorMessageMeta& aMeta,
                                          ipc::StructuredCloneData&& aData,
                                          ErrorResult& aRv) {
-  if (NS_WARN_IF(!mManager || mManager->IsClosed())) {
+  if (NS_WARN_IF(!mCanSend || !mManager || mManager->IsClosed())) {
     aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
     return;
   }
 
   if (mManager->IsInProcess()) {
     RefPtr<WindowGlobalChild> wgc = mManager->GetChildActor();
     nsCOMPtr<nsIRunnable> runnable =
         new AsyncMessageToChild(aMeta, std::move(aData), wgc);
@@ -78,16 +80,24 @@ void JSWindowActorParent::SendRawMessage
   }
 
   if (NS_WARN_IF(!mManager->SendRawMessage(aMeta, msgData))) {
     aRv.Throw(NS_ERROR_UNEXPECTED);
     return;
   }
 }
 
+void JSWindowActorParent::StartDestroy() {
+  mCanSend = false;
+}
+
+void JSWindowActorParent::AfterDestroy() {
+  mManager = nullptr;
+}
+
 NS_IMPL_CYCLE_COLLECTION_INHERITED(JSWindowActorParent, JSWindowActor, mManager)
 
 NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(JSWindowActorParent,
                                                JSWindowActor)
 NS_IMPL_CYCLE_COLLECTION_TRACE_END
 
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(JSWindowActorParent)
 NS_INTERFACE_MAP_END_INHERITING(JSWindowActor)
--- a/dom/ipc/JSWindowActorParent.h
+++ b/dom/ipc/JSWindowActorParent.h
@@ -37,24 +37,27 @@ class JSWindowActorParent final : public
 
   static already_AddRefed<JSWindowActorParent> Constructor(
       GlobalObject& aGlobal, ErrorResult& aRv) {
     return MakeAndAddRef<JSWindowActorParent>();
   }
 
   WindowGlobalParent* Manager() const;
   void Init(const nsAString& aName, WindowGlobalParent* aManager);
+  void StartDestroy();
+  void AfterDestroy();
 
  protected:
   void SendRawMessage(const JSWindowActorMessageMeta& aMeta,
                       ipc::StructuredCloneData&& aData,
                       ErrorResult& aRv) override;
 
  private:
-  ~JSWindowActorParent() = default;
+  ~JSWindowActorParent();
 
+  bool mCanSend = true;
   RefPtr<WindowGlobalParent> mManager;
 };
 
 }  // namespace dom
 }  // namespace mozilla
 
 #endif  // mozilla_dom_JSWindowActorParent_h
--- a/dom/ipc/WindowGlobalChild.cpp
+++ b/dom/ipc/WindowGlobalChild.cpp
@@ -139,16 +139,26 @@ already_AddRefed<BrowserChild> WindowGlo
 }
 
 void WindowGlobalChild::Destroy() {
   // 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()) {
+    // Make a copy so that we can avoid potential iterator invalidation when
+    // calling the user-provided Destroy() methods.
+    nsTArray<RefPtr<JSWindowActorChild>> windowActors(mWindowActors.Count());
+    for (auto iter = mWindowActors.Iter(); !iter.Done(); iter.Next()) {
+      windowActors.AppendElement(iter.UserData());
+    }
+
+    for (auto& windowActor : windowActors) {
+      windowActor->StartDestroy();
+    }
     SendDestroy();
   }
 
   mIPCClosed = true;
 }
 
 static nsresult ChangeFrameRemoteness(WindowGlobalChild* aWgc,
                                       BrowsingContext* aBc,
@@ -291,22 +301,25 @@ void WindowGlobalChild::ActorDestroy(Act
   mIPCClosed = true;
   gWindowGlobalChildById->Remove(mInnerWindowId);
 
   // Destroy our JSWindowActors, and reject any pending queries.
   nsRefPtrHashtable<nsStringHashKey, JSWindowActorChild> windowActors;
   mWindowActors.SwapElements(windowActors);
   for (auto iter = windowActors.Iter(); !iter.Done(); iter.Next()) {
     iter.Data()->RejectPendingQueries();
+    iter.Data()->AfterDestroy();
   }
+  windowActors.Clear();
 }
 
 WindowGlobalChild::~WindowGlobalChild() {
   MOZ_ASSERT(!gWindowGlobalChildById ||
              !gWindowGlobalChildById->Contains(mInnerWindowId));
+  MOZ_ASSERT(!mWindowActors.Count());
 }
 
 JSObject* WindowGlobalChild::WrapObject(JSContext* aCx,
                                         JS::Handle<JSObject*> aGivenProto) {
   return WindowGlobalChild_Binding::Wrap(aCx, this, aGivenProto);
 }
 
 nsISupports* WindowGlobalChild::GetParentObject() {
--- a/dom/ipc/WindowGlobalParent.cpp
+++ b/dom/ipc/WindowGlobalParent.cpp
@@ -165,16 +165,26 @@ IPCResult WindowGlobalParent::RecvBecome
   mBrowsingContext->SetCurrentWindowGlobal(this);
   return IPC_OK();
 }
 
 IPCResult WindowGlobalParent::RecvDestroy() {
   if (!mIPCClosed) {
     RefPtr<BrowserParent> browserParent = GetRemoteTab();
     if (!browserParent || !browserParent->IsDestroyed()) {
+      // Make a copy so that we can avoid potential iterator invalidation when
+      // calling the user-provided Destroy() methods.
+      nsTArray<RefPtr<JSWindowActorParent>> windowActors(mWindowActors.Count());
+      for (auto iter = mWindowActors.Iter(); !iter.Done(); iter.Next()) {
+        windowActors.AppendElement(iter.UserData());
+      }
+
+      for (auto& windowActor : windowActors) {
+        windowActor->StartDestroy();
+      }
       Unused << Send__delete__(this);
     }
   }
   return IPC_OK();
 }
 
 IPCResult WindowGlobalParent::RecvRawMessage(
     const JSWindowActorMessageMeta& aMeta, const ClonedMessageData& aData) {
@@ -294,27 +304,30 @@ void WindowGlobalParent::ActorDestroy(Ac
   gWindowGlobalParentsById->Remove(mInnerWindowId);
   mBrowsingContext->UnregisterWindowGlobal(this);
 
   // Destroy our JSWindowActors, and reject any pending queries.
   nsRefPtrHashtable<nsStringHashKey, JSWindowActorParent> windowActors;
   mWindowActors.SwapElements(windowActors);
   for (auto iter = windowActors.Iter(); !iter.Done(); iter.Next()) {
     iter.Data()->RejectPendingQueries();
+    iter.Data()->AfterDestroy();
   }
+  windowActors.Clear();
 
   nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
   if (obs) {
     obs->NotifyObservers(this, "window-global-destroyed", nullptr);
   }
 }
 
 WindowGlobalParent::~WindowGlobalParent() {
   MOZ_ASSERT(!gWindowGlobalParentsById ||
              !gWindowGlobalParentsById->Contains(mInnerWindowId));
+  MOZ_ASSERT(!mWindowActors.Count());
 }
 
 JSObject* WindowGlobalParent::WrapObject(JSContext* aCx,
                                          JS::Handle<JSObject*> aGivenProto) {
   return WindowGlobalParent_Binding::Wrap(aCx, this, aGivenProto);
 }
 
 nsISupports* WindowGlobalParent::GetParentObject() {