Improve nsNPAPIPluginInstance's stream management. Streams shouldn't keep strong references to their instances. Replace ugly linked list storage. b=554524 r=jst
authorJosh Aas <joshmoz@gmail.com>
Wed, 07 Apr 2010 16:30:32 -0400
changeset 40553 10d2046d2b641734e5d94da84ffc61698460b5e3
parent 40552 5f626bbd860a28b05f55e380ea8b3c9b729926e1
child 40554 0ed67564770063b0a5b83236e019b9efe933ecaa
push id1
push userroot
push dateTue, 26 Apr 2011 22:38:44 +0000
treeherdermozilla-beta@bfdb6e623a36 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjst
bugs554524
milestone1.9.3a5pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
Improve nsNPAPIPluginInstance's stream management. Streams shouldn't keep strong references to their instances. Replace ugly linked list storage. b=554524 r=jst
modules/plugin/base/src/nsNPAPIPluginInstance.cpp
modules/plugin/base/src/nsNPAPIPluginInstance.h
modules/plugin/base/src/nsNPAPIPluginStreamListener.h
--- a/modules/plugin/base/src/nsNPAPIPluginInstance.cpp
+++ b/modules/plugin/base/src/nsNPAPIPluginInstance.cpp
@@ -199,54 +199,32 @@ nsNPAPIPluginStreamListener::nsNPAPIPlug
     mCallNotify(PR_FALSE),
     mIsSuspended(PR_FALSE),
     mIsPluginInitJSStream(mInst->mInPluginInitCall &&
                           aURL && strncmp(aURL, "javascript:",
                                           sizeof("javascript:") - 1) == 0),
     mResponseHeaderBuf(nsnull)
 {
   memset(&mNPStream, 0, sizeof(mNPStream));
-
-  NS_IF_ADDREF(mInst);
 }
 
-nsNPAPIPluginStreamListener::~nsNPAPIPluginStreamListener(void)
+nsNPAPIPluginStreamListener::~nsNPAPIPluginStreamListener()
 {
-  // remove itself from the instance stream list
-  nsNPAPIPluginInstance *inst = mInst;
-  if (inst) {
-    nsInstanceStream * prev = nsnull;
-    for (nsInstanceStream *is = inst->mStreams; is != nsnull; is = is->mNext) {
-      if (is->mPluginStreamListener == this) {
-        if (!prev)
-          inst->mStreams = is->mNext;
-        else
-          prev->mNext = is->mNext;
-
-        delete is;
-        break;
-      }
-      prev = is;
-    }
-  }
-
   // For those cases when NewStream is never called, we still may need
   // to fire a notification callback. Return network error as fallback
   // reason because for other cases, notify should have already been
   // called for other reasons elsewhere.
   CallURLNotify(NPRES_NETWORK_ERR);
 
   // lets get rid of the buffer
   if (mStreamBuffer) {
     PR_Free(mStreamBuffer);
     mStreamBuffer=nsnull;
   }
 
-  NS_IF_RELEASE(inst);
-
   if (mNotifyURL)
     PL_strfree(mNotifyURL);
 
   if (mResponseHeaderBuf)
     PL_strfree(mResponseHeaderBuf);
 }
 
 nsresult nsNPAPIPluginStreamListener::CleanUpStream(NPReason reason)
@@ -260,17 +238,17 @@ nsresult nsNPAPIPluginStreamListener::Cl
 
   StopDataPump();
 
   // Seekable streams have an extra addref when they are created which must
   // be matched here.
   if (NP_SEEK == mStreamType)
     NS_RELEASE_THIS();
 
-  if (!mInst || !mInst->CanFireNotifications())
+  if (!mInst->CanFireNotifications())
     return rv;
 
   mStreamInfo = NULL;
 
   PluginDestructionGuard guard(mInst);
 
   const NPPluginFuncs *callbacks = nsnull;
   mInst->GetCallbacks(&callbacks);
@@ -301,47 +279,43 @@ nsresult nsNPAPIPluginStreamListener::Cl
   // fire notification back to plugin, just like before
   CallURLNotify(reason);
 
   return rv;
 }
 
 void nsNPAPIPluginStreamListener::CallURLNotify(NPReason reason)
 {
-  if (!mCallNotify || !mInst || !mInst->CanFireNotifications())
+  if (!mCallNotify || !mInst->CanFireNotifications())
     return;
 
   PluginDestructionGuard guard(mInst);
 
   mCallNotify = PR_FALSE; // only do this ONCE and prevent recursion
 
   const NPPluginFuncs *callbacks = nsnull;
   mInst->GetCallbacks(&callbacks);
   if (!callbacks)
     return;
   
   if (callbacks->urlnotify) {
-
     NPP npp;
     mInst->GetNPP(&npp);
 
     NS_TRY_SAFE_CALL_VOID((*callbacks->urlnotify)(npp, mNotifyURL, reason, mNotifyData), mInst->mLibrary, mInst);
 
     NPP_PLUGIN_LOG(PLUGIN_LOG_NORMAL,
     ("NPP URLNotify called: this=%p, npp=%p, notify=%p, reason=%d, url=%s\n",
     this, npp, mNotifyData, reason, mNotifyURL));
   }
 }
 
 NS_IMETHODIMP
 nsNPAPIPluginStreamListener::OnStartBinding(nsIPluginStreamInfo* pluginInfo)
 {
-  if (!mInst)
-    return NS_ERROR_FAILURE;
-
   PluginDestructionGuard guard(mInst);
 
   NPP npp;
   const NPPluginFuncs *callbacks = nsnull;
 
   mInst->GetCallbacks(&callbacks);
   mInst->GetNPP(&npp);
 
@@ -468,18 +442,18 @@ nsNPAPIPluginStreamListener::StopDataPum
   }
 }
 
 // Return true if a javascript: load that was started while the plugin
 // was being initialized is still in progress.
 PRBool
 nsNPAPIPluginStreamListener::PluginInitJSLoadInProgress()
 {
-  for (nsInstanceStream *is = mInst->mStreams; is; is = is->mNext) {
-    if (is->mPluginStreamListener->mIsPluginInitJSStream) {
+  for (unsigned int i = 0; i < mInst->mStreams.Length(); i++) {
+    if (mInst->mStreams[i]->mIsPluginInitJSStream) {
       return PR_TRUE;
     }
   }
 
   return PR_FALSE;
 }
 
 // This method is called when there's more data available off the
@@ -489,17 +463,17 @@ nsNPAPIPluginStreamListener::PluginInitJ
 // the plugin pump calls this method, the input argument will be null,
 // and the length will be the number of bytes available in our
 // internal buffer.
 NS_IMETHODIMP
 nsNPAPIPluginStreamListener::OnDataAvailable(nsIPluginStreamInfo* pluginInfo,
                                           nsIInputStream* input,
                                           PRUint32 length)
 {
-  if (!mInst || !mInst->CanFireNotifications())
+  if (!mInst->CanFireNotifications())
     return NS_ERROR_FAILURE;
 
   PluginDestructionGuard guard(mInst);
 
   // Just in case the caller switches plugin info on us.
   mStreamInfo = pluginInfo;
 
   const NPPluginFuncs *callbacks = nsnull;
@@ -754,17 +728,17 @@ nsNPAPIPluginStreamListener::OnDataAvail
 
   return rv;
 }
 
 NS_IMETHODIMP
 nsNPAPIPluginStreamListener::OnFileAvailable(nsIPluginStreamInfo* pluginInfo, 
                                              const char* fileName)
 {
-  if (!mInst || !mInst->CanFireNotifications())
+  if (!mInst->CanFireNotifications())
     return NS_ERROR_FAILURE;
 
   PluginDestructionGuard guard(mInst);
 
   const NPPluginFuncs *callbacks = nsnull;
   mInst->GetCallbacks(&callbacks);
   if (!callbacks || !callbacks->asfile)
     return NS_ERROR_FAILURE;
@@ -797,17 +771,17 @@ nsNPAPIPluginStreamListener::OnStopBindi
       do_QueryInterface(mStreamInfo);
 
     nsIRequest *request;
     if (pluginInfoNPAPI && (request = pluginInfoNPAPI->GetRequest())) {
       request->Cancel(status);
     }
   }
 
-  if (!mInst || !mInst->CanFireNotifications())
+  if (!mInst->CanFireNotifications())
     return NS_ERROR_FAILURE;
 
   // check if the stream is of seekable type and later its destruction
   // see bug 91140    
   nsresult rv = NS_OK;
   NPReason reason = NS_FAILED(status) ? NPRES_NETWORK_ERR : NPRES_DONE;
   if (mStreamType != NP_SEEK ||
       (NP_SEEK == mStreamType && NS_BINDING_ABORTED == status)) {
@@ -868,26 +842,16 @@ nsNPAPIPluginStreamListener::NewResponse
 {
   mResponseHeaders.Append(headerName);
   mResponseHeaders.Append(": ");
   mResponseHeaders.Append(headerValue);
   mResponseHeaders.Append('\n');
   return NS_OK;
 }
 
-nsInstanceStream::nsInstanceStream()
-{
-  mNext = nsnull;
-  mPluginStreamListener = nsnull;
-}
-
-nsInstanceStream::~nsInstanceStream()
-{
-}
-
 NS_IMPL_ISUPPORTS1(nsNPAPIPluginInstance, nsIPluginInstance)
 
 nsNPAPIPluginInstance::nsNPAPIPluginInstance(NPPluginFuncs* callbacks,
                                              PluginLibrary* aLibrary)
   : mCallbacks(callbacks),
 #ifdef XP_MACOSX
 #ifdef NP_NO_QUICKDRAW
     mDrawingModel(NPDrawingModelCoreGraphics),
@@ -913,27 +877,20 @@ nsNPAPIPluginInstance::nsNPAPIPluginInst
   // Initialize the NPP structure.
 
   mNPP.pdata = NULL;
   mNPP.ndata = this;
 
   PLUGIN_LOG(PLUGIN_LOG_BASIC, ("nsNPAPIPluginInstance ctor: this=%p\n",this));
 }
 
-nsNPAPIPluginInstance::~nsNPAPIPluginInstance(void)
+nsNPAPIPluginInstance::~nsNPAPIPluginInstance()
 {
   PLUGIN_LOG(PLUGIN_LOG_BASIC, ("nsNPAPIPluginInstance dtor: this=%p\n",this));
 
-  // clean the stream list if any
-  for (nsInstanceStream *is = mStreams; is != nsnull;) {
-    nsInstanceStream * next = is->mNext;
-    delete is;
-    is = next;
-  }
-
   if (mMIMEType) {
     PR_Free((void *)mMIMEType);
     mMIMEType = nsnull;
   }
 }
 
 TimeStamp
 nsNPAPIPluginInstance::LastStopTime()
@@ -999,29 +956,20 @@ NS_IMETHODIMP nsNPAPIPluginInstance::Sto
   EnterAsyncPluginThreadCallLock();
   mRunning = DESTROYING;
   mStopTime = TimeStamp::Now();
   ExitAsyncPluginThreadCallLock();
 
   OnPluginDestroy(&mNPP);
 
   // clean up open streams
-  for (nsInstanceStream *is = mStreams; is != nsnull;) {
-    nsRefPtr<nsNPAPIPluginStreamListener> listener = is->mPluginStreamListener;
-
-    nsInstanceStream *next = is->mNext;
-    delete is;
-    is = next;
-    mStreams = is;
-
-    // Clean up our stream after removing it from the list because 
-    // it may be released and destroyed at this point.
-    if (listener)
-      listener->CleanUpStream(NPRES_USER_BREAK);
+  for (unsigned int i = 0; i < mStreams.Length(); i++) {
+    mStreams[i]->CleanUpStream(NPRES_USER_BREAK);
   }
+  mStreams.Clear();
 
   NPError error = NPERR_GENERIC_ERROR;
   if (mCallbacks->destroy) {
     NPSavedData *sdata = 0;
 
     NS_TRY_SAFE_CALL_RETURN(error, (*mCallbacks->destroy)(&mNPP, &sdata), mLibrary, this);
 
     NPP_PLUGIN_LOG(PLUGIN_LOG_NORMAL,
@@ -1291,33 +1239,21 @@ nsNPAPIPluginInstance::NewStreamFromPlug
 nsresult nsNPAPIPluginInstance::NewNotifyStream(nsIPluginStreamListener** listener, 
                                                 void* notifyData,
                                                 PRBool aCallNotify,
                                                 const char* aURL)
 {
   nsNPAPIPluginStreamListener* stream = new nsNPAPIPluginStreamListener(this, notifyData, aURL);
   NS_ENSURE_TRUE(stream, NS_ERROR_OUT_OF_MEMORY);
 
-  // add it to the list
-  nsInstanceStream * is = new nsInstanceStream();
-  NS_ENSURE_TRUE(is, NS_ERROR_OUT_OF_MEMORY);
+  mStreams.AppendElement(stream);
 
-  is->mNext = mStreams;
-  is->mPluginStreamListener = stream;
-  mStreams = is;
   stream->SetCallNotify(aCallNotify); // set flag in stream to call URLNotify
 
-  NS_ADDREF(stream); // Stabilize
-    
-  nsresult res = stream->QueryInterface(kIPluginStreamListenerIID, (void**)listener);
-
-  // Destabilize and avoid leaks. Avoid calling delete <interface pointer>
-  NS_RELEASE(stream);
-
-  return res;
+  return stream->QueryInterface(kIPluginStreamListenerIID, (void**)listener);
 }
 
 NS_IMETHODIMP nsNPAPIPluginInstance::Print(NPPrint* platformPrint)
 {
   NS_ENSURE_TRUE(platformPrint, NS_ERROR_NULL_POINTER);
 
   PluginDestructionGuard guard(this);
 
--- a/modules/plugin/base/src/nsNPAPIPluginInstance.h
+++ b/modules/plugin/base/src/nsNPAPIPluginInstance.h
@@ -36,40 +36,32 @@
  * the terms of any one of the MPL, the GPL or the LGPL.
  *
  * ***** END LICENSE BLOCK ***** */
 
 #ifndef nsNPAPIPluginInstance_h_
 #define nsNPAPIPluginInstance_h_
 
 #include "nsCOMPtr.h"
+#include "nsAutoPtr.h"
 #include "nsTArray.h"
 #include "nsIPlugin.h"
 #include "nsIPluginInstance.h"
 #include "nsIPluginTagInfo.h"
 #include "nsPIDOMWindow.h"
 #include "nsIPluginInstanceOwner.h"
 #include "nsITimer.h"
 #include "mozilla/TimeStamp.h"
 
 #include "npfunctions.h"
 #include "mozilla/PluginLibrary.h"
 
 class nsNPAPIPluginStreamListener;
 class nsPIDOMWindow;
 
-struct nsInstanceStream
-{
-  nsInstanceStream *mNext;
-  nsNPAPIPluginStreamListener *mPluginStreamListener;
-
-  nsInstanceStream();
-  ~nsInstanceStream();
-};
-
 class nsNPAPITimer
 {
 public:
   NPP npp;
   uint32_t id;
   nsCOMPtr<nsITimer> timer;
   void (*callback)(NPP npp, uint32_t timerID);
 };
@@ -175,17 +167,17 @@ protected:
   PRPackedBool mTransparent;
   PRPackedBool mCached;
   PRPackedBool mWantsAllNetworkStreams;
 
 public:
   // True while creating the plugin, or calling NPP_SetWindow() on it.
   PRPackedBool mInPluginInitCall;
   PluginLibrary* mLibrary;
-  nsInstanceStream *mStreams;
+  nsTArray< nsRefPtr<nsNPAPIPluginStreamListener> > mStreams;
 
 private:
   nsTArray<PopupControlState> mPopupStates;
 
   char* mMIMEType;
 
   // Weak pointer to the owner. The owner nulls this out (by calling
   // InvalidateOwner()) when it's no longer our owner.
--- a/modules/plugin/base/src/nsNPAPIPluginStreamListener.h
+++ b/modules/plugin/base/src/nsNPAPIPluginStreamListener.h
@@ -80,17 +80,17 @@ public:
   void StopDataPump();
 
   PRBool PluginInitJSLoadInProgress();
 
 protected:
   void* mNotifyData;
   char* mStreamBuffer;
   char* mNotifyURL;
-  nsNPAPIPluginInstance* mInst;
+  nsNPAPIPluginInstance* mInst; // weak, must always be valid
   NPStream mNPStream;
   PRUint32 mStreamBufferSize;
   PRInt32 mStreamBufferByteCount;
   PRInt32 mStreamType;
   PRPackedBool mStreamStarted;
   PRPackedBool mStreamCleanedUp;
   PRPackedBool mCallNotify;
   PRPackedBool mIsSuspended;