Bug 787778 - Add additional re-entrance checks in plugin object code. r=josh
authorJohn Schoenick <jschoenick@mozilla.com>
Thu, 04 Oct 2012 13:50:49 -0700
changeset 118150 576c66cb9f2f9e7f3c3418ebb677a709a1e555a3
parent 118149 6ea044d85c1e3ef0dddcc636890536ff7801cd43
child 118151 d9e070c062f0dba3d724ec9f7cfc82f151dfd919
push id1997
push userakeybl@mozilla.com
push dateMon, 07 Jan 2013 21:25:26 +0000
treeherdermozilla-beta@4baf45cdcf21 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjosh
bugs787778
milestone19.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 787778 - Add additional re-entrance checks in plugin object code. r=josh
content/base/src/nsObjectLoadingContent.cpp
--- a/content/base/src/nsObjectLoadingContent.cpp
+++ b/content/base/src/nsObjectLoadingContent.cpp
@@ -849,18 +849,20 @@ nsObjectLoadingContent::OnStopRequest(ns
 
   if (aRequest != mChannel) {
     return NS_BINDING_ABORTED;
   }
 
   mChannel = nullptr;
 
   if (mFinalListener) {
-    mFinalListener->OnStopRequest(aRequest, aContext, aStatusCode);
+    // This may re-enter in the case of plugin listeners
+    nsCOMPtr<nsIStreamListener> listenerGrip(mFinalListener);
     mFinalListener = nullptr;
+    listenerGrip->OnStopRequest(aRequest, aContext, aStatusCode);
   }
 
   // Return value doesn't matter
   return NS_OK;
 }
 
 
 // nsIStreamListener
@@ -872,18 +874,20 @@ nsObjectLoadingContent::OnDataAvailable(
 {
   NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome(), NS_ERROR_NOT_AVAILABLE);
 
   if (aRequest != mChannel) {
     return NS_BINDING_ABORTED;
   }
 
   if (mFinalListener) {
-    return mFinalListener->OnDataAvailable(aRequest, aContext, aInputStream,
-                                           aOffset, aCount);
+    // This may re-enter in the case of plugin listeners
+    nsCOMPtr<nsIStreamListener> listenerGrip(mFinalListener);
+    return listenerGrip->OnDataAvailable(aRequest, aContext, aInputStream,
+                                         aOffset, aCount);
   }
 
   // We shouldn't have a connected channel with no final listener
   NS_NOTREACHED("Got data for channel with no connected final listener");
   mChannel = nullptr;
 
   return NS_ERROR_UNEXPECTED;
 }
@@ -1673,37 +1677,31 @@ nsObjectLoadingContent::LoadObject(bool 
     NS_NOTREACHED("Trying to load with bad channel state");
     rv = NS_ERROR_UNEXPECTED;
     return NS_OK;
   }
 
   ///
   /// Attempt to load new type
   ///
-  
-  // Remove blocker on entering into instantiate
-  mIsLoading = false;
-  
+
+  // We don't set mFinalListener until OnStartRequest has been called, to
+  // prevent re-entry ugliness with CloseChannel()
+  nsCOMPtr<nsIStreamListener> finalListener;
   switch (mType) {
     case eType_Image:
       if (!mChannel) {
         // We have a LoadImage() call, but UpdateObjectParameters requires a
         // channel for images, so this is not a valid state.
         NS_NOTREACHED("Attempting to load image without a channel?");
         rv = NS_ERROR_UNEXPECTED;
         break;
       }
-      rv = LoadImageWithChannel(mChannel, getter_AddRefs(mFinalListener));
-      if (mFinalListener) {
-        // Note that LoadObject is called from mChannel's OnStartRequest
-        // when loading with a channel
-        mSrcStreamLoading = true;
-        rv = mFinalListener->OnStartRequest(mChannel, nullptr);
-        mSrcStreamLoading = false;
-      }
+      rv = LoadImageWithChannel(mChannel, getter_AddRefs(finalListener));
+      // finalListener will receive OnStartRequest below
     break;
     case eType_Plugin:
     {
       if (mChannel) {
         nsRefPtr<nsPluginHost> pluginHost =
           already_AddRefed<nsPluginHost>(nsPluginHost::GetInst());
         if (!pluginHost) {
           NS_NOTREACHED("No pluginHost");
@@ -1720,28 +1718,18 @@ nsObjectLoadingContent::LoadObject(bool 
           // We're un-rendered, and can't instantiate a plugin. HasNewFrame will
           // re-start us when we can proceed.
           LOG(("OBJLC [%p]: Aborting load - plugin-type, but no frame", this));
           CloseChannel();
           break;
         }
         
         rv = pluginHost->NewEmbeddedPluginStreamListener(mURI, this, nullptr,
-                                                         getter_AddRefs(mFinalListener));
-        if (NS_SUCCEEDED(rv)) {
-          // Note that LoadObject is called from mChannel's OnStartRequest
-          // when loading with a channel
-
-          mSrcStreamLoading = true;
-          rv = mFinalListener->OnStartRequest(mChannel, nullptr);
-          mSrcStreamLoading = false;
-          if (NS_SUCCEEDED(rv)) {
-            NotifyContentObjectWrapper();
-          }
-        }
+                                                         getter_AddRefs(finalListener));
+        // finalListener will receive OnStartRequest below
       } else {
         rv = AsyncStartPluginInstance();
       }
     }
     break;
     case eType_Document:
     {
       if (!mChannel) {
@@ -1787,92 +1775,110 @@ nsObjectLoadingContent::LoadObject(bool 
       nsCOMPtr<nsIURILoader>
         uriLoader(do_GetService(NS_URI_LOADER_CONTRACTID, &rv));
       if (NS_FAILED(rv)) {
         NS_NOTREACHED("Failed to get uriLoader service");
         mType = eType_Null;
         break;
       }
       rv = uriLoader->OpenChannel(mChannel, nsIURILoader::DONT_RETARGET, req,
-                                  getter_AddRefs(mFinalListener));
-      if (NS_SUCCEEDED(rv)) {
-        // Note that LoadObject is called from mChannel's OnStartRequest
-        // when loading with a channel
-        mSrcStreamLoading = true;
-        rv = mFinalListener->OnStartRequest(mChannel, nullptr);
-        mSrcStreamLoading = false;
-      }
+                                  getter_AddRefs(finalListener));
+      // finalListener will receive OnStartRequest below
     }
     break;
     case eType_Loading:
       // If our type remains Loading, we need a channel to proceed
       rv = OpenChannel();
       if (NS_FAILED(rv)) {
         LOG(("OBJLC [%p]: OpenChannel returned failure (%u)", this, rv));
       }
     break;
     case eType_Null:
       // Handled below, silence compiler warnings
     break;
   };
 
+  //
+  // Loaded, handle notifications and fallback
+  //
   if (NS_FAILED(rv)) {
     // If we failed in the loading hunk above, switch to fallback
     LOG(("OBJLC [%p]: Loading failed, switching to fallback", this));
     mType = eType_Null;
   }
 
-  // Switching to fallback state
+  // If we didn't load anything, handle switching to fallback state
   if (mType == eType_Null) {
     LOG(("OBJLC [%p]: Loading fallback, type %u", this, fallbackType));
     NS_ASSERTION(!mFrameLoader && !mInstanceOwner,
                  "switched to type null but also loaded something");
 
     if (mChannel) {
       // If we were loading with a channel but then failed over, throw it away
-      // (this also closes mFinalListener)
       CloseChannel();
     }
 
     // Don't notify or send events - we'll handle those ourselves
     // (so really this is just setting mFallbackType)
     LoadFallback(fallbackType, false);
   }
 
   // Notify of our final state if we haven't already
   NotifyStateChanged(oldType, oldState, false, aNotify);
-
+  
   if (mType == eType_Null && !mContentType.IsEmpty() &&
       mFallbackType != eFallbackAlternate) {
     // if we have a content type and are not showing alternate
     // content, fire a pluginerror to trigger (we stopped LoadFallback
     // from doing so above, it doesn't know of our old state)
     FirePluginError(mFallbackType);
   }
 
+  //
+  // Pass load on to finalListener if loading with a channel
+  //
+
+  // If we re-entered and loaded something else, that load will have cleaned up
+  // our our listener.
+  if (!mIsLoading) {
+    LOG(("OBJLC [%p]: Re-entered before dispatching to final listener", this));
+  } else if (finalListener) {
+    NS_ASSERTION(mType != eType_Null && mType != eType_Loading,
+                 "We should not have a final listener with a non-loaded type");
+    // Note that we always enter into LoadObject() from ::OnStartRequest when
+    // loading with a channel.
+    mSrcStreamLoading = true;
+    // Remove blocker on entering into instantiate
+    // (this is otherwise unset by the stack class)
+    mIsLoading = false;
+    mFinalListener = finalListener;
+    finalListener->OnStartRequest(mChannel, nullptr);
+    mSrcStreamLoading = false;
+  }
+
   return NS_OK;
 }
 
+// This call can re-enter when dealing with plugin listeners
 nsresult
 nsObjectLoadingContent::CloseChannel()
 {
   if (mChannel) {
     LOG(("OBJLC [%p]: Closing channel\n", this));
-    // These three statements are carefully ordered:
-    // - onStopRequest should get a channel whose status is the same as the
-    //   status argument
-    // - onStopRequest must get a non-null channel
-    mChannel->Cancel(NS_BINDING_ABORTED);
-    if (mFinalListener) {
-      // NOTE mFinalListener is only created when we load with a channel, which
-      //      LoadObject() requires come from a OnStartRequest call
-      mFinalListener->OnStopRequest(mChannel, nullptr, NS_BINDING_ABORTED);
-      mFinalListener = nullptr;
+    // Null the values before potentially-reentering, and ensure they survive
+    // the call
+    nsCOMPtr<nsIChannel> channelGrip(mChannel);
+    nsCOMPtr<nsIStreamListener> listenerGrip(mFinalListener);
+    mChannel = nullptr;
+    mFinalListener = nullptr;
+    channelGrip->Cancel(NS_BINDING_ABORTED);
+    if (listenerGrip) {
+      // mFinalListener is only set by LoadObject after OnStartRequest
+      listenerGrip->OnStopRequest(channelGrip, nullptr, NS_BINDING_ABORTED);
     }
-    mChannel = nullptr;
   }
   return NS_OK;
 }
 
 nsresult
 nsObjectLoadingContent::OpenChannel()
 {
   nsCOMPtr<nsIContent> thisContent = 
@@ -1968,17 +1974,21 @@ nsObjectLoadingContent::UnloadObject(boo
   // state
   CancelImageRequests(false);
   if (mFrameLoader) {
     mFrameLoader->Destroy();
     mFrameLoader = nullptr;
   }
 
   if (aResetState) {
-    CloseChannel();
+    if (mType != eType_Plugin) {
+      // This can re-enter when dealing with plugins, and StopPluginInstance
+      // will handle it
+      CloseChannel();
+    }
     mChannelLoaded = false;
     mType = eType_Loading;
     mURI = mOriginalURI = mBaseURI = nullptr;
     mContentType.Truncate();
     mOriginalContentType.Truncate();
   }
 
   // This call should be last as it may re-enter