Bug 579516: Patch contains a number of fixes to nsPluginStreamListenerPeer memory management. Also moves storage of the stream array for cache lookups to the plugin instance rather than the instance tag. Also stops adding streams to the cached list that shouldn't be there. r=benwa

layout/generic/nsObjectFrame.cpp

@@ -969,9 +969,7 @@ nsObjectFrame::InstantiatePlugin(nsIPluginHost* aPluginHost,
   nsresult rv;
   if (fullPageMode) {  /* full-page mode */
     nsCOMPtr<nsIStreamListener> stream;
-    rv = aPluginHost->InstantiateFullPagePlugin(aMimeType, aURI,
-          /* resulting stream listener */       *getter_AddRefs(stream),
-                                                mInstanceOwner);
+    rv = aPluginHost->InstantiateFullPagePlugin(aMimeType, aURI, mInstanceOwner, getter_AddRefs(stream));
     if (NS_SUCCEEDED(rv))
       pDoc->SetStreamListener(stream);
   } else {   /* embedded mode */

modules/plugin/base/public/nsIPluginHost.idl

@@ -62,10 +62,9 @@ interface nsIChannel;
 interface nsIPluginStreamListener;
 
 [ptr] native PRLibraryPtr(PRLibrary);
-[ref] native nsIStreamListenerRef(nsIStreamListener *);
 [ptr] native nsPluginNativeWindowPtr(nsPluginNativeWindow);
 
-[scriptable, uuid(D419142E-0571-416B-B797-2A8E6624491D)]
+[scriptable, uuid(C198DEAA-3F93-482D-A47C-85FF6514FE07)]
 interface nsIPluginHost : nsISupports
 {
   [noscript] void init();
@@ -89,8 +88,8 @@ interface nsIPluginHost : nsISupports
 
   [noscript] void instantiateFullPagePlugin(in string aMimeType,
                                             in nsIURI aURI,
-                                            in nsIStreamListenerRef aStreamListener,
-                                            in nsIPluginInstanceOwner aOwner);
+                                            in nsIPluginInstanceOwner aOwner,
+                                            out nsIStreamListener aStreamListener);
 
   /**
    * Instantiate an embedded plugin for an existing channel. The caller is

modules/plugin/base/src/nsNPAPIPluginInstance.cpp

@@ -176,10 +176,10 @@ NS_IMETHODIMP nsNPAPIPluginInstance::Stop()
   OnPluginDestroy(&mNPP);
 
   // clean up open streams
-  for (unsigned int i = 0; i < mStreamListeners.Length(); i++) {
-    mStreamListeners[i]->CleanUpStream(NPRES_USER_BREAK);
+  for (unsigned int i = 0; i < mPStreamListeners.Length(); i++) {
+    mPStreamListeners[i]->CleanUpStream(NPRES_USER_BREAK);
   }
-  mStreamListeners.Clear();
+  mPStreamListeners.Clear();
 
   NPError error = NPERR_GENERIC_ERROR;
   if (mCallbacks->destroy) {
@@ -266,6 +266,18 @@ nsNPAPIPluginInstance::GetMode(PRInt32 *result)
     return NS_ERROR_FAILURE;
 }
 
+nsTArray<nsNPAPIPluginStreamListener*>*
+nsNPAPIPluginInstance::PStreamListeners()
+{
+  return &mPStreamListeners;
+}
+
+nsTArray<nsPluginStreamListenerPeer*>*
+nsNPAPIPluginInstance::BStreamListeners()
+{
+  return &mBStreamListeners;
+}
+
 nsresult
 nsNPAPIPluginInstance::InitializePlugin()
 { 
@@ -456,7 +468,7 @@ nsresult nsNPAPIPluginInstance::NewNotifyStream(nsIPluginStreamListener** listen
   nsNPAPIPluginStreamListener* stream = new nsNPAPIPluginStreamListener(this, notifyData, aURL);
   NS_ENSURE_TRUE(stream, NS_ERROR_OUT_OF_MEMORY);
 
-  mStreamListeners.AppendElement(stream);
+  mPStreamListeners.AppendElement(stream);
   stream->SetCallNotify(aCallNotify); // set flag in stream to call URLNotify
 
   return stream->QueryInterface(kIPluginStreamListenerIID, (void**)listener);

modules/plugin/base/src/nsNPAPIPluginInstance.h

@@ -50,7 +50,8 @@
 #include "mozilla/TimeStamp.h"
 #include "mozilla/PluginLibrary.h"
 
-class nsNPAPIPluginStreamListener;
+class nsPluginStreamListenerPeer; // browser-initiated stream class
+class nsNPAPIPluginStreamListener; // plugin-initiated stream class
 class nsIPluginInstanceOwner;
 
 class nsNPAPITimer
@@ -126,6 +127,12 @@ public:
   void          UnscheduleTimer(uint32_t timerID);
   NPError       PopUpContextMenu(NPMenu* menu);
   NPBool        ConvertPoint(double sourceX, double sourceY, NPCoordinateSpace sourceSpace, double *destX, double *destY, NPCoordinateSpace destSpace);
+
+  // Returns the array of plugin-initiated streams.
+  nsTArray<nsNPAPIPluginStreamListener*> *PStreamListeners();
+  // Returns the array of browser-initiated streams.
+  nsTArray<nsPluginStreamListenerPeer*> *BStreamListeners();
+
 protected:
   nsresult InitializePlugin();
 
@@ -168,9 +175,14 @@ public:
   // True while creating the plugin, or calling NPP_SetWindow() on it.
   PRPackedBool mInPluginInitCall;
   PluginLibrary* mLibrary;
-  nsTArray<nsNPAPIPluginStreamListener*> mStreamListeners;
 
 private:
+  // array of plugin-initiated stream listeners
+  nsTArray<nsNPAPIPluginStreamListener*> mPStreamListeners;
+  
+  // array of browser-initiated stream listeners
+  nsTArray<nsPluginStreamListenerPeer*> mBStreamListeners;
+
   nsTArray<PopupControlState> mPopupStates;
 
   char* mMIMEType;

modules/plugin/base/src/nsNPAPIPluginStreamListener.cpp

@@ -168,8 +168,9 @@ mResponseHeaderBuf(nsnull)
 nsNPAPIPluginStreamListener::~nsNPAPIPluginStreamListener()
 {
   // remove this from the plugin instance's stream list
-  mInst->mStreamListeners.RemoveElement(this);
-  
+  nsTArray<nsNPAPIPluginStreamListener*> *pStreamListeners = mInst->PStreamListeners();
+  pStreamListeners->RemoveElement(this);
+
   // 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
@@ -416,9 +417,10 @@ nsNPAPIPluginStreamListener::PluginInitJSLoadInProgress()
 {
   if (!mInst)
     return PR_FALSE;
-  
-  for (unsigned int i = 0; i < mInst->mStreamListeners.Length(); i++) {
-    if (mInst->mStreamListeners[i]->mIsPluginInitJSStream) {
+
+  nsTArray<nsNPAPIPluginStreamListener*> *pStreamListeners = mInst->PStreamListeners();
+  for (unsigned int i = 0; i < pStreamListeners->Length(); i++) {
+    if (pStreamListeners->ElementAt(i)->mIsPluginInitJSStream) {
       return PR_TRUE;
     }
   }

modules/plugin/base/src/nsPluginHost.cpp

@@ -1127,8 +1127,8 @@ NS_IMETHODIMP nsPluginHost::InstantiateEmbeddedPlugin(const char *aMimeType,
 // Called by full-page case
 NS_IMETHODIMP nsPluginHost::InstantiateFullPagePlugin(const char *aMimeType,
                                                       nsIURI* aURI,
-                                                      nsIStreamListener *&aStreamListener,
-                                                      nsIPluginInstanceOwner *aOwner)
+                                                      nsIPluginInstanceOwner *aOwner,
+                                                      nsIStreamListener **aStreamListener)
 {
 #ifdef PLUGIN_LOGGING
   nsCAutoString urlSpec;
@@ -1147,7 +1147,7 @@ NS_IMETHODIMP nsPluginHost::InstantiateFullPagePlugin(const char *aMimeType,
     if (!pluginTag || !pluginTag->mIsJavaPlugin) {
       nsCOMPtr<nsIPluginInstance> instanceCOMPtr;
       aOwner->GetInstance(getter_AddRefs(instanceCOMPtr));
-      NewFullPagePluginStream(aStreamListener, aURI, static_cast<nsNPAPIPluginInstance*>(instanceCOMPtr.get()));
+      NewFullPagePluginStream(aURI, static_cast<nsNPAPIPluginInstance*>(instanceCOMPtr.get()), aStreamListener);
     }
     return NS_OK;
   }
@@ -1171,7 +1171,7 @@ NS_IMETHODIMP nsPluginHost::InstantiateFullPagePlugin(const char *aMimeType,
       if (window->window)
         window->CallSetWindow(instanceCOMPtr);
 
-      rv = NewFullPagePluginStream(aStreamListener, aURI, instance);
+      rv = NewFullPagePluginStream(aURI, instance, aStreamListener);
 
       // If we've got a native window, the let the plugin know about it.
       if (window->window)
@@ -3214,27 +3214,24 @@ nsresult nsPluginHost::NewEmbeddedPluginStream(nsIURI* aURL,
 }
 
 // Called by InstantiateFullPagePlugin()
-nsresult nsPluginHost::NewFullPagePluginStream(nsIStreamListener *&aStreamListener,
-                                               nsIURI* aURI,
-                                               nsNPAPIPluginInstance *aInstance)
+nsresult nsPluginHost::NewFullPagePluginStream(nsIURI* aURI,
+                                               nsNPAPIPluginInstance *aInstance,
+                                               nsIStreamListener **aStreamListener)
 {
-  nsPluginStreamListenerPeer  *listener = new nsPluginStreamListenerPeer();
+  NS_ASSERTION(aStreamListener, "Stream listener out param cannot be null");
+
+  nsRefPtr<nsPluginStreamListenerPeer> listener = new nsPluginStreamListenerPeer();
   if (!listener)
     return NS_ERROR_OUT_OF_MEMORY;
 
-  nsresult rv;
+  nsresult rv = listener->InitializeFullPage(aURI, aInstance);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
 
-  rv = listener->InitializeFullPage(aURI, aInstance);
+  listener.forget(aStreamListener);
 
-  aStreamListener = listener;
-  NS_ADDREF(listener);
-
-  // add peer to list of stream peers for this instance
-  nsPluginInstanceTag * p = FindInstanceTag(aInstance);
-  if (p)
-    p->mStreams.AppendObject(listener);
-
-  return rv;
+  return NS_OK;
 }
 
 NS_IMETHODIMP nsPluginHost::Observe(nsISupports *aSubject,

modules/plugin/base/src/nsPluginHost.h

@@ -192,7 +192,9 @@ private:
   NewEmbeddedPluginStream(nsIURI* aURL, nsIPluginInstanceOwner *aOwner, nsNPAPIPluginInstance* aInstance);
 
   nsresult
-  NewFullPagePluginStream(nsIStreamListener *&aStreamListener, nsIURI* aURI, nsNPAPIPluginInstance *aInstance);
+  NewFullPagePluginStream(nsIURI* aURI,
+                          nsNPAPIPluginInstance *aInstance,
+                          nsIStreamListener **aStreamListener);
 
   // Return an nsPluginTag for this type, if any.  If aCheckEnabled is
   // true, only enabled plugins will be returned.

modules/plugin/base/src/nsPluginStreamListenerPeer.cpp

@@ -279,6 +279,9 @@ nsPluginStreamListenerPeer::~nsPluginStreamListenerPeer()
     mFileCacheOutputStream = nsnull;
   
   delete mDataForwardToRequest;
+
+  if (mPluginInstance)
+    mPluginInstance->BStreamListeners()->RemoveElement(this);
 }
 
 // Called as a result of GetURL and PostURL
@@ -377,16 +380,16 @@ nsPluginStreamListenerPeer::SetupPluginCacheFile(nsIChannel* channel)
   nsresult rv = NS_OK;
   
   PRBool useExistingCacheFile = PR_FALSE;
-  
   nsRefPtr<nsPluginHost> pluginHost = dont_AddRef(nsPluginHost::GetInst());
+
+  // Look for an existing cache file for the URI.
   nsTArray< nsAutoPtr<nsPluginInstanceTag> > *instanceTags = pluginHost->InstanceTagArray();
   for (PRUint32 i = 0; i < instanceTags->Length(); i++) {
     nsPluginInstanceTag *instanceTag = (*instanceTags)[i];
-    
     // most recent streams are at the end of list
-    for (PRInt32 i = instanceTag->mStreams.Count() - 1; i >= 0; --i) {
-      nsPluginStreamListenerPeer *lp =
-      static_cast<nsPluginStreamListenerPeer*>(instanceTag->mStreams[i]);
+    nsTArray<nsPluginStreamListenerPeer*> *bStreamListeners = instanceTag->mInstance->BStreamListeners();
+    for (PRInt32 i = bStreamListeners->Length() - 1; i >= 0; --i) {
+      nsPluginStreamListenerPeer *lp = bStreamListeners->ElementAt(i);
       if (lp && lp->mLocalCachedFileHolder) {
         useExistingCacheFile = lp->UseExistingPluginCacheFile(this);
         if (useExistingCacheFile) {
@@ -398,7 +401,8 @@ nsPluginStreamListenerPeer::SetupPluginCacheFile(nsIChannel* channel)
         break;
     }
   }
-  
+
+  // Create a new cache file if one could not be found.
   if (!useExistingCacheFile) {
     nsCOMPtr<nsIFile> pluginTmp;
     rv = nsPluginHost::GetPluginTempDir(getter_AddRefs(pluginTmp));
@@ -438,17 +442,12 @@ nsPluginStreamListenerPeer::SetupPluginCacheFile(nsIChannel* channel)
       return rv;
     
     // save the file.
-    
     mLocalCachedFileHolder = new CachedFileHolder(pluginTmp);
   }
-  
+
   // add this listenerPeer to list of stream peers for this instance
-  // it'll delay release of listenerPeer until nsPluginInstanceTag::~nsPluginInstanceTag
-  // and the temp file is going to stay alive until then
-  nsPluginInstanceTag *instanceTag = pluginHost->FindInstanceTag(mPluginInstance);
-  if (instanceTag)
-    instanceTag->mStreams.AppendObject(this);
-  
+  mPluginInstance->BStreamListeners()->AppendElement(this);
+
   return rv;
 }
 
@@ -829,7 +828,7 @@ nsPluginStreamListenerPeer::UseExistingPluginCacheFile(nsPluginStreamListenerPee
   
   NS_ENSURE_ARG_POINTER(psi);
   
-  if ( psi->mLength == mLength &&
+  if (psi->mLength == mLength &&
       psi->mModified == mModified &&
       mStreamComplete &&
       mURLSpec.Equals(psi->mURLSpec))

modules/plugin/base/src/nsPluginTags.h

@@ -129,9 +129,7 @@ struct nsPluginInstanceTag
   char*                  mURL;
   nsRefPtr<nsPluginTag>  mPluginTag;
   nsNPAPIPluginInstance* mInstance; // this must always be valid
-  // Array holding all opened stream listeners for this entry
-  nsCOMArray<nsIPluginStreamInfo> mStreams; 
-  
+
   nsPluginInstanceTag(nsPluginTag* aPluginTag,
                       nsIPluginInstance* aInstance, 
                       const char * url);