Bug 854082 - Cleanup plugin frame ownership, prevent losing our frame due to re-entrance. r=bsmedberg
authorJohn Schoenick <jschoenick@mozilla.com>
Wed, 17 Apr 2013 15:11:21 -0700
changeset 129671 9844a2fe0f46a62da9c5476725a763e354ab8d93
parent 129670 2db093c411a51fdc08342b10bb4804c9cc550e06
child 129672 557f1cfca69cbd232776947f0d18343517e6b1b6
push id26946
push userjschoenick@mozilla.com
push dateTue, 23 Apr 2013 20:22:19 +0000
treeherdermozilla-inbound@39b0b62b8cd2 [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);
   }