Bug 1538979 - Part 1: Clear mManager in ActorDestroy and disallow sending message while Destroying; r=nika
☠☠ backed out by 012ce6437a4c ☠ ☠
authorJohn Dai <jdai@mozilla.com>
Fri, 10 May 2019 09:19:27 +0000
changeset 473364 9df2b856b6550578f75d862a1fb4579b5665742e
parent 473363 e661822f67309406c695209d4be605ff281a2777
child 473365 8e065761738c199fc06388dcfc22df7e92827152
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() {