Bug 854082 - Cleanup plugin frame ownership, prevent losing our frame due to re-entrance. r=bsmedberg

content/base/src/nsObjectLoadingContent.cpp

@@ -770,6 +770,13 @@ nsObjectLoadingContent::InstantiatePluginInstance(bool aIsLoading)
   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();
@@ -1011,16 +1018,10 @@ nsObjectLoadingContent::HasNewFrame(nsIObjectFrame* aFrame)
   }
 
   // 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;
 }
 
@@ -2294,7 +2295,6 @@ nsObjectLoadingContent::PluginDestroyed()
   // 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;
@@ -2578,6 +2578,8 @@ nsObjectLoadingContent::StopPluginInstance()
     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;

dom/plugins/base/nsPluginInstanceOwner.cpp

@@ -2504,8 +2504,7 @@ nsEventStatus nsPluginInstanceOwner::ProcessEvent(const nsGUIEvent& anEvent)
 nsresult
 nsPluginInstanceOwner::Destroy()
 {
-  if (mObjectFrame)
-    mObjectFrame->SetInstanceOwner(nullptr);
+  SetFrame(nullptr);
 
 #ifdef XP_MACOSX
   RemoveFromCARefreshTimer();

layout/generic/nsObjectFrame.cpp

@@ -309,6 +309,9 @@ nsObjectFrame::DestroyFrom(nsIFrame* aDestructRoot)
   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);
   }
@@ -318,8 +321,6 @@ nsObjectFrame::DestroyFrom(nsIFrame* aDestructRoot)
     mBackgroundSink->Destroy();
   }
 
-  SetInstanceOwner(nullptr);
-
   nsObjectFrameSuper::DestroyFrom(aDestructRoot);
 }
 
@@ -779,6 +780,10 @@ nsObjectFrame::UnregisterPluginForGeometryUpdates()
 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;