Bug 1132874 - Improve PPluginWidget shutdown logic. r=aklotz
authorJim Mathies <jmathies@mozilla.com>
Thu, 19 Feb 2015 07:05:12 -0600
changeset 229884 e19b2d82fee01ab795fa2d10600113e4f24e17aa
parent 229883 c419d483d834783633c8b60240619a3f7a4f5ddf
child 229885 eff950c4b958a1f42f63bea029b3ee7dbb0bddc3
push id28300
push userryanvm@gmail.com
push dateThu, 19 Feb 2015 23:52:47 +0000
treeherdermozilla-central@56f090df5480 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaklotz
bugs1132874
milestone38.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 1132874 - Improve PPluginWidget shutdown logic. r=aklotz
dom/ipc/PPluginWidget.ipdl
dom/plugins/ipc/PluginWidgetChild.cpp
dom/plugins/ipc/PluginWidgetChild.h
dom/plugins/ipc/PluginWidgetParent.cpp
dom/plugins/ipc/PluginWidgetParent.h
widget/PluginWidgetProxy.cpp
--- a/dom/ipc/PPluginWidget.ipdl
+++ b/dom/ipc/PPluginWidget.ipdl
@@ -43,22 +43,20 @@ parent:
    * window for plugins. On Linux, this returns an XID for a socket widget
    * embedded in the chrome side native window. On Windows this returns the
    * native HWND of the plugin widget.
    */
   sync GetNativePluginPort() returns (uintptr_t value);
 
 child:
   /**
-   * Sent from content when a plugin is unloaded via layout. We shut down
-   * some things in response to this so that we don't end up firing async
-   * msgs after the child is destroyed. We know that after this call
-   * the child actor may not be on the other side.
+   * Event indicating the parent is shutting down.
+   * aWhichSide - indicates which side intititated the shutdown.
    */
-  async ParentShutdown();
+  async ParentShutdown(uint16_t aWhichSide);
 
   /**
    * Requests a full update of the plugin window.
    */
   async UpdateWindow(uintptr_t aChildId);
 };
 
 }
--- a/dom/plugins/ipc/PluginWidgetChild.cpp
+++ b/dom/plugins/ipc/PluginWidgetChild.cpp
@@ -1,65 +1,77 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "mozilla/plugins/PluginWidgetChild.h"
+#include "mozilla/plugins/PluginWidgetParent.h"
 #include "PluginWidgetProxy.h"
 #include "mozilla/DebugOnly.h"
 #include "nsDebug.h"
 
 #if defined(XP_WIN)
 #include "mozilla/plugins/PluginInstanceParent.h"
 using mozilla::plugins::PluginInstanceParent;
 #endif
 
+#define PWLOG(...)
+// #define PWLOG(...) printf_stderr(__VA_ARGS__)
+
 namespace mozilla {
 namespace plugins {
 
 PluginWidgetChild::PluginWidgetChild() :
   mWidget(nullptr)
 {
   MOZ_COUNT_CTOR(PluginWidgetChild);
 }
 
 PluginWidgetChild::~PluginWidgetChild()
 {
   MOZ_COUNT_DTOR(PluginWidgetChild);
 }
 
-/*
- * Tear down scenarios
- * layout (plugin content unloading):
- *  - PluginWidgetProxy nsIWidget Destroy()
- *  - PluginWidgetProxy->PluginWidgetChild->SendDestroy()
- *  - PluginWidgetParent::RecvDestroy(), sends async Destroyed() to PluginWidgetChild
- *  - PluginWidgetChild::RecvDestroyed() calls Send__delete__()
- *  - PluginWidgetParent::ActorDestroy() called in response to __delete__.
- * PBrowser teardown (tab closing):
- *  - PluginWidgetParent::ParentDestroy() called by TabParent::Destroy()
- *  - PluginWidgetParent::ActorDestroy()
- *  - PluginWidgetParent::~PluginWidgetParent() in response to PBrowserParent::DeallocSubtree()
- *  - PluginWidgetChild::ActorDestroy() from PPluginWidgetChild::DestroySubtree
- *  - ~PluginWidgetChild() in response to PBrowserChild::DeallocSubtree()
- **/
+// Called by the proxy widget when it is destroyed by layout. Only gets
+// called once.
+void
+PluginWidgetChild::ProxyShutdown()
+{
+  PWLOG("PluginWidgetChild::ProxyShutdown()\n");
+  if (mWidget) {
+    SendDestroy();
+    mWidget = nullptr;
+  }
+}
 
 void
-PluginWidgetChild::ActorDestroy(ActorDestroyReason aWhy)
+PluginWidgetChild::KillWidget()
 {
+  PWLOG("PluginWidgetChild::KillWidget()\n");
   if (mWidget) {
     mWidget->ChannelDestroyed();
   }
   mWidget = nullptr;
 }
 
+void
+PluginWidgetChild::ActorDestroy(ActorDestroyReason aWhy)
+{
+  PWLOG("PluginWidgetChild::ActorDestroy()\n");
+  KillWidget();
+}
+
 bool
-PluginWidgetChild::RecvParentShutdown()
+PluginWidgetChild::RecvParentShutdown(const uint16_t& aType)
 {
-  Send__delete__(this);
+  PWLOG("PluginWidgetChild::RecvParentShutdown()\n");
+  KillWidget();
+  if (aType == PluginWidgetParent::CONTENT) {
+    Send__delete__(this);
+  }
   return true;
 }
 
 bool
 PluginWidgetChild::RecvUpdateWindow(const uintptr_t& aChildId)
 {
 #if defined(XP_WIN)
   NS_ASSERTION(aChildId, "Expected child hwnd value for remote plugin instance.");
--- a/dom/plugins/ipc/PluginWidgetChild.h
+++ b/dom/plugins/ipc/PluginWidgetChild.h
@@ -16,17 +16,25 @@ namespace plugins {
 class PluginWidgetChild : public PPluginWidgetChild
 {
 public:
   PluginWidgetChild();
   virtual ~PluginWidgetChild();
 
   virtual bool RecvUpdateWindow(const uintptr_t& aChildId) MOZ_OVERRIDE;
   virtual void ActorDestroy(ActorDestroyReason aWhy) MOZ_OVERRIDE;
-  virtual bool RecvParentShutdown() MOZ_OVERRIDE;
+  virtual bool RecvParentShutdown(const uint16_t& aType) MOZ_OVERRIDE;
+
+  void SetWidget(mozilla::widget::PluginWidgetProxy* aWidget) {
+    mWidget = aWidget;
+  }
+  void ProxyShutdown();
+
+private:
+  void KillWidget();
 
   mozilla::widget::PluginWidgetProxy* mWidget;
 };
 
 } // namespace plugins
 } // namespace mozilla
 
 #endif // mozilla_plugins_PluginWidgetChild_h
--- a/dom/plugins/ipc/PluginWidgetParent.cpp
+++ b/dom/plugins/ipc/PluginWidgetParent.cpp
@@ -36,18 +36,33 @@ static NS_DEFINE_CID(kWidgetCID, NS_CHIL
 // ipc message delivery fails.
 #define ENSURE_CHANNEL {                                      \
   if (!mWidget) {                                             \
     NS_WARNING("called on an invalid remote widget.");        \
     return true;                                              \
   }                                                           \
 }
 
-PluginWidgetParent::PluginWidgetParent() :
-  mActorDestroyed(false)
+/*
+ * Tear down scenarios
+ * layout (plugin content unloading):
+ *  - PluginWidgetProxy::Destroy() calls PluginWidgetChild::ProxyShutdown(), calls SendDestroy()
+ *  - PluginWidgetParent::RecvDestroy(), sends async ParentShutdown(CONTENT)
+ *  - PluginWidgetChild::RecvParentShutdown(CONTENT), calls Send__delete__()
+ *  - PluginWidgetParent::ActorDestroy() called in response to __delete__
+ * PBrowser teardown (tab closing):
+ *  - PluginWidgetParent::ParentDestroy() called by TabParent::Destroy(), sends async ParentShutdown(TAB_CLOSURE)
+ *  - PluginWidgetChild::RecvParentShutdown(TAB_CLOSURE) (PluginWidgetProxy disabled)
+ *  - PluginWidgetParent::ActorDestroy()
+ *  - PluginWidgetParent::~PluginWidgetParent() in response to PBrowserParent::DeallocSubtree()
+ *  - PluginWidgetChild::ActorDestroy() from PPluginWidgetChild::DestroySubtree
+ *  - ~PluginWidgetChild() in response to PBrowserChild::DeallocSubtree()
+ **/
+
+PluginWidgetParent::PluginWidgetParent()
 {
   PWLOG("PluginWidgetParent::PluginWidgetParent()\n");
   MOZ_COUNT_CTOR(PluginWidgetParent);
 }
 
 PluginWidgetParent::~PluginWidgetParent()
 {
   PWLOG("PluginWidgetParent::~PluginWidgetParent()\n");
@@ -172,51 +187,50 @@ PluginWidgetParent::RecvCreate(nsresult*
   // visibility updates from the compositor, which ships this data
   // over with corresponding layer updates.
   mWidget->RegisterPluginWindowForRemoteUpdates();
 
   return true;
 }
 
 void
+PluginWidgetParent::Shutdown(ShutdownType aType)
+{
+  if (mWidget) {
+    mWidget->UnregisterPluginWindowForRemoteUpdates();
+    DebugOnly<nsresult> rv = mWidget->Destroy();
+    NS_ASSERTION(NS_SUCCEEDED(rv), "widget destroy failure");
+    mWidget = nullptr;
+    unused << SendParentShutdown(aType);
+  }
+}
+
+void
 PluginWidgetParent::ActorDestroy(ActorDestroyReason aWhy)
 {
-  mActorDestroyed = true;
   PWLOG("PluginWidgetParent::ActorDestroy()\n");
 }
 
 // Called by TabParent's Destroy() in response to an early tear down (Early
 // in that this is happening before layout in the child has had a chance
-// to destroy the child widget.) when the tab is closing. We will not receive
-// RecvDestroy here.
+// to destroy the child widget.) when the tab is closing.
 void
 PluginWidgetParent::ParentDestroy()
 {
-  if (mActorDestroyed || !mWidget) {
-    return;
-  }
   PWLOG("PluginWidgetParent::ParentDestroy()\n");
-  mWidget->UnregisterPluginWindowForRemoteUpdates();
-  DebugOnly<nsresult> rv = mWidget->Destroy();
-  NS_ASSERTION(NS_SUCCEEDED(rv), "widget destroy failure");
-  mWidget = nullptr;
-  mActorDestroyed = true;
-  return;
+  Shutdown(TAB_CLOSURE);
 }
 
 // Called by the child when a plugin is torn down within a tab
-// normally.
+// normally. Messages back via ParentShutdown().
 bool
 PluginWidgetParent::RecvDestroy()
 {
-  bool destroyed = mActorDestroyed;
-  ParentDestroy();
-  if (!destroyed) {
-    unused << SendParentShutdown();
-  }
+  PWLOG("PluginWidgetParent::RecvDestroy()\n");
+  Shutdown(CONTENT);
   return true;
 }
 
 bool
 PluginWidgetParent::RecvSetFocus(const bool& aRaise)
 {
   ENSURE_CHANNEL;
   PWLOG("PluginWidgetParent::RecvSetFocus(%d)\n", aRaise);
--- a/dom/plugins/ipc/PluginWidgetParent.h
+++ b/dom/plugins/ipc/PluginWidgetParent.h
@@ -27,43 +27,52 @@ public:
   /**
    * Windows helper for firing off an update window request to a plugin.
    *
    * aWidget - the eWindowType_plugin_ipc_chrome widget associated with
    *           this plugin window.
    */
   static void SendAsyncUpdate(nsIWidget* aWidget);
 
-public:
   PluginWidgetParent();
   virtual ~PluginWidgetParent();
 
   virtual void ActorDestroy(ActorDestroyReason aWhy) MOZ_OVERRIDE;
   virtual bool RecvCreate(nsresult* aResult) MOZ_OVERRIDE;
   virtual bool RecvDestroy() MOZ_OVERRIDE;
   virtual bool RecvSetFocus(const bool& aRaise) MOZ_OVERRIDE;
   virtual bool RecvGetNativePluginPort(uintptr_t* value) MOZ_OVERRIDE;
 
   // Helper for compositor checks on the channel
-  bool ActorDestroyed() { return mActorDestroyed; }
+  bool ActorDestroyed() { return !mWidget; }
 
   // Called by PBrowser when it receives a Destroy() call from the child.
   void ParentDestroy();
 
   // Sets mWidget's parent
   void SetParent(nsIWidget* aParent);
 
 private:
   // The tab our connection is associated with.
   mozilla::dom::TabParent* GetTabParent();
+
+public:
+  // Identifies the side of the connection that initiates shutdown.
+  enum ShutdownType {
+    TAB_CLOSURE = 1,
+    CONTENT     = 2
+  };
+
+private:
+  void Shutdown(ShutdownType aType);
+
   // The chrome side native widget.
   nsCOMPtr<nsIWidget> mWidget;
 #if defined(MOZ_WIDGET_GTK)
   nsAutoPtr<nsPluginNativeWindowGtk> mWrapper;
 #endif
-  bool mActorDestroyed;
 };
 
 } // namespace plugins
 } // namespace mozilla
 
 #endif // mozilla_plugins_PluginWidgetParent_h
 
--- a/widget/PluginWidgetProxy.cpp
+++ b/widget/PluginWidgetProxy.cpp
@@ -33,17 +33,17 @@ NS_IMPL_ISUPPORTS_INHERITED(PluginWidget
 } while (0)
 
 PluginWidgetProxy::PluginWidgetProxy(dom::TabChild* aTabChild,
                                      mozilla::plugins::PluginWidgetChild* aActor) :
   PuppetWidget(aTabChild),
   mActor(aActor)
 {
   // See ChannelDestroyed() in the header
-  mActor->mWidget = this;
+  mActor->SetWidget(this);
 }
 
 PluginWidgetProxy::~PluginWidgetProxy()
 {
   PWLOG("PluginWidgetProxy::~PluginWidgetProxy()\n");
 }
 
 NS_IMETHODIMP
@@ -94,32 +94,19 @@ PluginWidgetProxy::GetParent(void)
 }
 
 NS_IMETHODIMP
 PluginWidgetProxy::Destroy()
 {
   PWLOG("PluginWidgetProxy::Destroy()\n");
 
   if (mActor) {
-   /**
-    * We need to communicate that the sub protocol is going to be torn down
-    * before the sub protocol dies. Otherwise we can end up with async events
-    * in transit from chrome to content, which on arrival will trigger an abort
-    * in the content process, crashing all tabs.
-    *
-    * Note, this is one of two ways PluginWidget tear down initiates. Here we
-    * are a plugin in content and content has just unloaded us for some reason,
-    * usually due to swap out for flash ads or the user simply loaded a
-    * different page. The other involves a full tear down of the tab (PBrowser)
-    * which happens prior to widgets getting collected by ref counting in
-    * layout. We still get this Destroy call, but in all likelyhood mActor is
-    * already null via a call on ChannelDestroyed from PluginWidgetChild.
-    */
-    mActor->SendDestroy();
-    mActor->mWidget = nullptr;
+    // Communicate that the layout widget has been torn down before the sub
+    // protocol.
+    mActor->ProxyShutdown();
     mActor = nullptr;
   }
 
   return PuppetWidget::Destroy();
 }
 
 void
 PluginWidgetProxy::GetWindowClipRegion(nsTArray<nsIntRect>* aRects)