Bug 723190: Fix for crash due to bad scroll listener registration for Mac Carbon plugins. r=smichaud
authorJosh Aas <joshmoz@gmail.com>
Tue, 21 Feb 2012 13:59:38 -0500
changeset 88850 428d0a52f855df747f3318574794a06008ab58eb
parent 88849 9795ad013aa9532225ad2d59d62130e2d354e8f3
child 88851 e6fd8e2be05ddb99f1c59cabd8af8246304a3c66
push id975
push userffxbld
push dateTue, 13 Mar 2012 21:39:16 +0000
treeherdermozilla-aurora@99faebf9dc36 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmichaud
bugs723190
milestone13.0a1
Bug 723190: Fix for crash due to bad scroll listener registration for Mac Carbon plugins. r=smichaud
dom/plugins/base/nsPluginInstanceOwner.cpp
--- a/dom/plugins/base/nsPluginInstanceOwner.cpp
+++ b/dom/plugins/base/nsPluginInstanceOwner.cpp
@@ -3656,17 +3656,17 @@ nsPluginInstanceOwner::CallSetWindow()
 
 void nsPluginInstanceOwner::SetFrame(nsObjectFrame *aFrame)
 {
   // Don't do anything if the frame situation hasn't changed.
   if (mObjectFrame == aFrame) {
     return;
   }
 
-  // Deal with things that depend on whether or not we used to have a frame.
+  // If we already have a frame that is changing or going away...
   if (mObjectFrame) {
     // We have an old frame.
     // Drop image reference because the child may destroy the surface after we return.
     nsRefPtr<ImageContainer> container = mObjectFrame->GetImageContainer();
     if (container) {
 #ifdef XP_MACOSX
       nsRefPtr<Image> image = container->GetCurrentImage();
       if (image && (image->GetFormat() == Image::MAC_IO_SURFACE) && mObjectFrame) {
@@ -3678,67 +3678,59 @@ void nsPluginInstanceOwner::SetFrame(nsO
         // called, so OnDestroyImage() can't yet have been called.  So we need
         // to do ourselves what OnDestroyImage() would have done.
         NS_RELEASE_THIS();
       }
 #endif
       container->SetCurrentImage(nsnull);
     }
 
+    // Scroll position listening is only required for Carbon event model plugins on Mac OS X.
 #if defined(XP_MACOSX) && !defined(NP_NO_QUICKDRAW)
-    if (!aFrame) {
-      // At this point we had a frame but it is going away and we're not getting a new one.
-      // Unregister for a scroll position listening, which is only required for Carbon
-      // event model plugins on Mac OS X. It's OK to unregister when we didn't register,
-      // so don't be strict about unregistering. Better to unregister when we didn't have to
-      // than to not unregister when we should.
-      for (nsIFrame* f = mObjectFrame; f; f = nsLayoutUtils::GetCrossDocParentFrame(f)) {
-        nsIScrollableFrame* sf = do_QueryFrame(f);
-        if (sf) {
-          sf->RemoveScrollPositionListener(this);
-        }
+    // Our frame is changing or going away, unregister for a scroll position listening.
+    // It's OK to unregister when we didn't register, so don't be strict about unregistering.
+    // Better to unregister when we didn't have to than to not unregister when we should.
+    for (nsIFrame* f = mObjectFrame; f; f = nsLayoutUtils::GetCrossDocParentFrame(f)) {
+      nsIScrollableFrame* sf = do_QueryFrame(f);
+      if (sf) {
+        sf->RemoveScrollPositionListener(this);
       }
     }
 #endif
 
     // Make sure the old frame isn't holding a reference to us.
     mObjectFrame->SetInstanceOwner(nsnull);
-  } else {
-    // Scroll position listening is only required for Carbon event model plugins on Mac OS X.
-    // Note that we probably have a crash bug in the way we register/unregister, bug 723190.
-    // Bug 723190 is mitigated by limiting registration to Carbon event model plugins.
-#if defined(XP_MACOSX) && !defined(NP_NO_QUICKDRAW)
-    if (aFrame) {
-      // We didn't have an object frame before but we do now. We need to register a scroll
-      // position listener on every scrollable frame up to the top.
-      if (GetEventModel() == NPEventModelCarbon) {
-        for (nsIFrame* f = aFrame; f; f = nsLayoutUtils::GetCrossDocParentFrame(f)) {
-          nsIScrollableFrame* sf = do_QueryFrame(f);
-          if (sf) {
-            sf->AddScrollPositionListener(this);
-          }
-        }
-      }
-    }
-#endif
   }
 
   // Swap in the new frame (or no frame)
   mObjectFrame = aFrame;
 
   // Set up a new frame
   if (mObjectFrame) {
     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->Invalidate(mObjectFrame->GetContentRectRelativeToSelf());
+
+    // Scroll position listening is only required for Carbon event model plugins on Mac OS X.
+#if defined(XP_MACOSX) && !defined(NP_NO_QUICKDRAW)
+    // We need to register as a scroll position listener on every scrollable frame up to the top.
+    if (GetEventModel() == NPEventModelCarbon) {
+      for (nsIFrame* f = aFrame; f; f = nsLayoutUtils::GetCrossDocParentFrame(f)) {
+        nsIScrollableFrame* sf = do_QueryFrame(f);
+        if (sf) {
+          sf->AddScrollPositionListener(this);
+        }
+      }
+    }
+#endif
   }
 }
 
 nsObjectFrame* nsPluginInstanceOwner::GetFrame()
 {
   return mObjectFrame;
 }