Bug 854082 - Cleanup plugin frame ownership, prevent losing our frame due to re-entrance. r=bsmedberg
☠☠ backed out by 58011469a3c9 ☠ ☠
authorJohn Schoenick <jschoenick@mozilla.com>
Wed, 17 Apr 2013 15:11:21 -0700
changeset 140451 e3eaea876a18bf1eb2dc252e08a31772af515813
parent 140450 3aaf738a04d8d72d57245af2bef04f1c683f6bd8
child 140452 9423207656dd9f2f76379a9dd1f7e70d9ad0b2e4
push id2579
push userakeybl@mozilla.com
push dateMon, 24 Jun 2013 18:52:47 +0000
treeherdermozilla-beta@b69b7de8a05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbsmedberg
bugs854082
milestone23.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 854082 - Cleanup plugin frame ownership, prevent losing our frame due to re-entrance. r=bsmedberg
content/base/src/nsObjectLoadingContent.cpp
dom/plugins/base/nsPluginInstanceOwner.cpp
layout/generic/nsObjectFrame.cpp
--- a/content/base/src/nsObjectLoadingContent.cpp
+++ b/content/base/src/nsObjectLoadingContent.cpp
@@ -765,16 +765,23 @@ nsObjectLoadingContent::InstantiatePlugi
   nsCOMPtr<nsIAppShell> appShell = do_GetService(kAppShellCID);
   if (appShell) {
     appShell->SuspendNative();
   }
 
   rv = pluginHost->InstantiatePluginInstance(mContentType.get(),
                                              mURI.get(), this,
                                              getter_AddRefs(mInstanceOwner));
+  // Ensure the frame did not change during instantiation re-entry (common).
+  // HasNewFrame would not have mInstanceOwner yet, so the new frame would be
+  // dangling. (Bug 854082)
+  nsIFrame* frame = thisContent->GetPrimaryFrame();
+  if (frame && mInstanceOwner) {
+    mInstanceOwner->SetFrame(static_cast<nsObjectFrame*>(frame));
+  }
 
   if (appShell) {
     appShell->ResumeNative();
   }
 
   if (NS_FAILED(rv)) {
     return rv;
   }
@@ -1006,26 +1013,20 @@ nsObjectLoadingContent::HasNewFrame(nsIO
   if (!mInstanceOwner) {
     // We are successfully setup as type plugin, but have not spawned an
     // instance due to a lack of a frame.
     AsyncStartPluginInstance();
     return NS_OK;
   }
 
   // Otherwise, we're just changing frames
-  mInstanceOwner->SetFrame(nullptr);
-
   // Set up relationship between instance owner and frame.
   nsObjectFrame *objFrame = static_cast<nsObjectFrame*>(aFrame);
   mInstanceOwner->SetFrame(objFrame);
 
-  // Set up new frame to draw.
-  objFrame->FixupWindow(objFrame->GetContentRectRelativeToSelf().Size());
-  objFrame->InvalidateFrame();
-
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsObjectLoadingContent::GetPluginInstance(nsNPAPIPluginInstance** aInstance)
 {
   *aInstance = nullptr;
 
@@ -2289,17 +2290,16 @@ nsObjectLoadingContent::GetPrintFrame(ns
 
 NS_IMETHODIMP
 nsObjectLoadingContent::PluginDestroyed()
 {
   // Called when our plugin is destroyed from under us, usually when reloading
   // plugins in plugin host. Invalidate instance owner / prototype but otherwise
   // don't take any action.
   TeardownProtoChain();
-  mInstanceOwner->SetFrame(nullptr);
   mInstanceOwner->Destroy();
   mInstanceOwner = nullptr;
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsObjectLoadingContent::PluginCrashed(nsIPluginTag* aPluginTag,
                                       const nsAString& pluginDumpID,
@@ -2573,16 +2573,18 @@ nsObjectLoadingContent::StopPluginInstan
     // re-open it to handle instantiating again, even if we don't invalidate
     // our loaded state.
     /// XXX(johns): Except currently, we don't, just leaving re-opening channels
     ///             to plugins...
     LOG(("OBJLC [%p]: StopPluginInstance - Closing used channel", this));
     CloseChannel();
   }
 
+  // We detach the instance owner's frame before destruction, but don't destroy
+  // the instance owner until the plugin is stopped.
   mInstanceOwner->SetFrame(nullptr);
 
   bool delayedStop = false;
 #ifdef XP_WIN
   // Force delayed stop for Real plugin only; see bug 420886, 426852.
   nsRefPtr<nsNPAPIPluginInstance> inst;
   mInstanceOwner->GetInstance(getter_AddRefs(inst));
   if (inst) {
--- a/dom/plugins/base/nsPluginInstanceOwner.cpp
+++ b/dom/plugins/base/nsPluginInstanceOwner.cpp
@@ -2499,18 +2499,17 @@ nsEventStatus nsPluginInstanceOwner::Pro
 #endif
  
   return rv;
 }
 
 nsresult
 nsPluginInstanceOwner::Destroy()
 {
-  if (mObjectFrame)
-    mObjectFrame->SetInstanceOwner(nullptr);
+  SetFrame(nullptr);
 
 #ifdef XP_MACOSX
   RemoveFromCARefreshTimer();
   if (mColorProfile)
     ::CGColorSpaceRelease(mColorProfile);
 #endif
 
   // unregister context menu listener
@@ -3372,17 +3371,17 @@ void nsPluginInstanceOwner::SetFrame(nsO
     mObjectFrame->SetInstanceOwner(this);
     // Can only call PrepForDrawing on an object frame once. Don't do it here unless
     // widget creation is complete. Doesn't matter if we actually have a widget.
     if (mWidgetCreationComplete) {
       mObjectFrame->PrepForDrawing(mWidget);
     }
     mObjectFrame->FixupWindow(mObjectFrame->GetContentRectRelativeToSelf().Size());
     mObjectFrame->InvalidateFrame();
-    
+
     nsFocusManager* fm = nsFocusManager::GetFocusManager();
     const nsIContent* content = aFrame->GetContent();
     if (fm && content) {
       mContentFocused = (content == fm->GetFocusedContent());
     }
   }
 }
 
--- a/layout/generic/nsObjectFrame.cpp
+++ b/layout/generic/nsObjectFrame.cpp
@@ -304,27 +304,28 @@ nsObjectFrame::DestroyFrom(nsIFrame* aDe
   if (mReflowCallbackPosted) {
     PresContext()->PresShell()->CancelReflowCallback(this);
   }
 
   // Tell content owner of the instance to disconnect its frame.
   nsCOMPtr<nsIObjectLoadingContent> objContent(do_QueryInterface(mContent));
   NS_ASSERTION(objContent, "Why not an object loading content?");
 
+  // The content might not have a reference to the instance owner any longer in
+  // the case of re-entry during instantiation or teardown, so make sure we're
+  // dissociated.
   if (mInstanceOwner) {
     mInstanceOwner->SetFrame(nullptr);
   }
   objContent->HasNewFrame(nullptr);
 
   if (mBackgroundSink) {
     mBackgroundSink->Destroy();
   }
 
-  SetInstanceOwner(nullptr);
-
   nsObjectFrameSuper::DestroyFrom(aDestructRoot);
 }
 
 /* virtual */ void
 nsObjectFrame::DidSetStyleContext(nsStyleContext* aOldStyleContext)
 {
   if (HasView()) {
     nsView* view = GetView();
@@ -774,16 +775,20 @@ nsObjectFrame::UnregisterPluginForGeomet
   }
   mRootPresContextRegisteredWith->UnregisterPluginForGeometryUpdates(mContent);
   mRootPresContextRegisteredWith = nullptr;
 }
 
 void
 nsObjectFrame::SetInstanceOwner(nsPluginInstanceOwner* aOwner)
 {
+  // The ownership model here is historically fuzzy. This should only be called
+  // by nsPluginInstanceOwner when it is given a new frame, and
+  // nsObjectLoadingContent should be arbitrating frame-ownership via its
+  // HasNewFrame callback.
   mInstanceOwner = aOwner;
   if (mInstanceOwner) {
     return;
   }
   UnregisterPluginForGeometryUpdates();
   if (mWidget && mInnerView) {
     mInnerView->DetachWidgetEventHandler(mWidget);
     // Make sure the plugin is hidden in case an update of plugin geometry
@@ -873,17 +878,17 @@ nsObjectFrame::DidReflow(nsPresContext* 
     NS_ASSERTION(objContent, "Why not an object loading content?");
     objContent->HasNewFrame(this);
   }
 
   nsresult rv = nsObjectFrameSuper::DidReflow(aPresContext, aReflowState, aStatus);
 
   // The view is created hidden; once we have reflowed it and it has been
   // positioned then we show it.
-  if (aStatus != nsDidReflowStatus::FINISHED) 
+  if (aStatus != nsDidReflowStatus::FINISHED)
     return rv;
 
   if (HasView()) {
     nsView* view = GetView();
     nsViewManager* vm = view->GetViewManager();
     if (vm)
       vm->SetViewVisibility(view, IsHidden() ? nsViewVisibility_kHide : nsViewVisibility_kShow);
   }