From d9a178b62b01f80dc6c6b5b7435b760d0cb3b16b Mon Sep 17 00:00:00 2001 From: Josh Aas Date: Sat, 17 Jul 2010 19:47:29 -0400 Subject: [PATCH] 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 | 4 +-- modules/plugin/base/public/nsIPluginHost.idl | 7 ++-- .../plugin/base/src/nsNPAPIPluginInstance.cpp | 20 ++++++++--- .../plugin/base/src/nsNPAPIPluginInstance.h | 16 +++++++-- .../base/src/nsNPAPIPluginStreamListener.cpp | 12 ++++--- modules/plugin/base/src/nsPluginHost.cpp | 35 +++++++++---------- modules/plugin/base/src/nsPluginHost.h | 4 ++- .../base/src/nsPluginStreamListenerPeer.cpp | 29 ++++++++------- modules/plugin/base/src/nsPluginTags.h | 4 +-- 9 files changed, 75 insertions(+), 56 deletions(-) diff --git a/layout/generic/nsObjectFrame.cpp b/layout/generic/nsObjectFrame.cpp index e745488ee5d..c0413957c5b 100644 --- a/layout/generic/nsObjectFrame.cpp +++ b/layout/generic/nsObjectFrame.cpp @@ -969,9 +969,7 @@ nsObjectFrame::InstantiatePlugin(nsIPluginHost* aPluginHost, nsresult rv; if (fullPageMode) { /* full-page mode */ nsCOMPtr 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 */ diff --git a/modules/plugin/base/public/nsIPluginHost.idl b/modules/plugin/base/public/nsIPluginHost.idl index 1232cf13264..115870ac00f 100644 --- a/modules/plugin/base/public/nsIPluginHost.idl +++ b/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 diff --git a/modules/plugin/base/src/nsNPAPIPluginInstance.cpp b/modules/plugin/base/src/nsNPAPIPluginInstance.cpp index eeec720b9eb..df758a7ca9d 100644 --- a/modules/plugin/base/src/nsNPAPIPluginInstance.cpp +++ b/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* +nsNPAPIPluginInstance::PStreamListeners() +{ + return &mPStreamListeners; +} + +nsTArray* +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); diff --git a/modules/plugin/base/src/nsNPAPIPluginInstance.h b/modules/plugin/base/src/nsNPAPIPluginInstance.h index e3ac9b66e98..458f48153d9 100644 --- a/modules/plugin/base/src/nsNPAPIPluginInstance.h +++ b/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 *PStreamListeners(); + // Returns the array of browser-initiated streams. + nsTArray *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 mStreamListeners; private: + // array of plugin-initiated stream listeners + nsTArray mPStreamListeners; + + // array of browser-initiated stream listeners + nsTArray mBStreamListeners; + nsTArray mPopupStates; char* mMIMEType; diff --git a/modules/plugin/base/src/nsNPAPIPluginStreamListener.cpp b/modules/plugin/base/src/nsNPAPIPluginStreamListener.cpp index db0d7f62610..886b151c52e 100644 --- a/modules/plugin/base/src/nsNPAPIPluginStreamListener.cpp +++ b/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 *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 *pStreamListeners = mInst->PStreamListeners(); + for (unsigned int i = 0; i < pStreamListeners->Length(); i++) { + if (pStreamListeners->ElementAt(i)->mIsPluginInitJSStream) { return PR_TRUE; } } diff --git a/modules/plugin/base/src/nsPluginHost.cpp b/modules/plugin/base/src/nsPluginHost.cpp index f31a404d768..e84d0025d78 100644 --- a/modules/plugin/base/src/nsPluginHost.cpp +++ b/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 instanceCOMPtr; aOwner->GetInstance(getter_AddRefs(instanceCOMPtr)); - NewFullPagePluginStream(aStreamListener, aURI, static_cast(instanceCOMPtr.get())); + NewFullPagePluginStream(aURI, static_cast(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 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, diff --git a/modules/plugin/base/src/nsPluginHost.h b/modules/plugin/base/src/nsPluginHost.h index 8d5940ce789..7036ba13a94 100644 --- a/modules/plugin/base/src/nsPluginHost.h +++ b/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. diff --git a/modules/plugin/base/src/nsPluginStreamListenerPeer.cpp b/modules/plugin/base/src/nsPluginStreamListenerPeer.cpp index a5d518e384e..af8b2480a5d 100644 --- a/modules/plugin/base/src/nsPluginStreamListenerPeer.cpp +++ b/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 pluginHost = dont_AddRef(nsPluginHost::GetInst()); + + // Look for an existing cache file for the URI. nsTArray< nsAutoPtr > *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(instanceTag->mStreams[i]); + nsTArray *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 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)) diff --git a/modules/plugin/base/src/nsPluginTags.h b/modules/plugin/base/src/nsPluginTags.h index 0f62a564816..74883d4c07a 100644 --- a/modules/plugin/base/src/nsPluginTags.h +++ b/modules/plugin/base/src/nsPluginTags.h @@ -129,9 +129,7 @@ struct nsPluginInstanceTag char* mURL; nsRefPtr mPluginTag; nsNPAPIPluginInstance* mInstance; // this must always be valid - // Array holding all opened stream listeners for this entry - nsCOMArray mStreams; - + nsPluginInstanceTag(nsPluginTag* aPluginTag, nsIPluginInstance* aInstance, const char * url);