From f68338bc51e1e7ddce0008ceab4b666c50b35941 Mon Sep 17 00:00:00 2001 From: Benjamin Smedberg Date: Wed, 23 Feb 2011 10:47:25 -0500 Subject: [PATCH] Bug 634534 - Firefox 4 (and probably earlier versions) crash when they make a byte-range request and then call NPN_DestroyStream while both the "main" request and the byte-range request are active. This affects Silverlight on mac, and almost certainly Acrobat on Windows. Instead of tracking a single request in nsPluginStreamListenerPeer::mRequest, track all the currently pending requests with nsPluginStreamListenerPeer::mRequests, and make sure that they are all cancelled when the stream is destroyed. r=josh sr=bz a=blocker --HG-- extra : rebase_source : e6da19a0edb2cd283bdb92f14e925ac9b96157ec --- .../base/src/nsNPAPIPluginStreamListener.cpp | 28 ++-- .../base/src/nsNPAPIPluginStreamListener.h | 47 +++++- modules/plugin/base/src/nsPluginHost.cpp | 2 + .../base/src/nsPluginStreamListenerPeer.cpp | 138 +++++++++++++++--- .../base/src/nsPluginStreamListenerPeer.h | 5 +- 5 files changed, 177 insertions(+), 43 deletions(-) diff --git a/modules/plugin/base/src/nsNPAPIPluginStreamListener.cpp b/modules/plugin/base/src/nsNPAPIPluginStreamListener.cpp index 53ea6684c5f..b6789ec3e96 100644 --- a/modules/plugin/base/src/nsNPAPIPluginStreamListener.cpp +++ b/modules/plugin/base/src/nsNPAPIPluginStreamListener.cpp @@ -361,7 +361,7 @@ nsNPAPIPluginStreamListener::OnStartBinding(nsIPluginStreamInfo* pluginInfo) return NS_OK; } -nsresult +void nsNPAPIPluginStreamListener::SuspendRequest() { NS_ASSERTION(!mIsSuspended, @@ -369,19 +369,18 @@ nsNPAPIPluginStreamListener::SuspendRequest() nsCOMPtr pluginInfoNPAPI = do_QueryInterface(mStreamInfo); - nsIRequest *request; - if (!pluginInfoNPAPI || !(request = pluginInfoNPAPI->GetRequest())) { - NS_ERROR("Trying to suspend a non-suspendable stream!"); - return NS_ERROR_FAILURE; + if (!pluginInfoNPAPI) { + return; } nsresult rv = StartDataPump(); - NS_ENSURE_SUCCESS(rv, rv); + if (NS_FAILED(rv)) + return; mIsSuspended = PR_TRUE; - return request->Suspend(); + pluginInfoNPAPI->SuspendRequests(); } void @@ -390,11 +389,7 @@ nsNPAPIPluginStreamListener::ResumeRequest() nsCOMPtr pluginInfoNPAPI = do_QueryInterface(mStreamInfo); - nsIRequest *request = pluginInfoNPAPI->GetRequest(); - - // request can be null if the network stream is done. - if (request) - request->Resume(); + pluginInfoNPAPI->ResumeRequests(); mIsSuspended = PR_FALSE; } @@ -609,7 +604,7 @@ nsNPAPIPluginStreamListener::OnDataAvailable(nsIPluginStreamInfo* pluginInfo, if (numtowrite <= 0 || (!mIsPluginInitJSStream && PluginInitJSLoadInProgress())) { if (!mIsSuspended) { - rv = SuspendRequest(); + SuspendRequest(); } // Break out of the inner loop, but keep going through the @@ -676,7 +671,7 @@ nsNPAPIPluginStreamListener::OnDataAvailable(nsIPluginStreamInfo* pluginInfo, // resume the request. if (mIsSuspended || ++zeroBytesWriteCount == 3) { if (!mIsSuspended) { - rv = SuspendRequest(); + SuspendRequest(); } // Break out of the for loop, but keep going through the @@ -758,9 +753,8 @@ nsNPAPIPluginStreamListener::OnStopBinding(nsIPluginStreamInfo* pluginInfo, nsCOMPtr pluginInfoNPAPI = do_QueryInterface(mStreamInfo); - nsIRequest *request; - if (pluginInfoNPAPI && (request = pluginInfoNPAPI->GetRequest())) { - request->Cancel(status); + if (pluginInfoNPAPI) { + pluginInfoNPAPI->CancelRequests(status); } } diff --git a/modules/plugin/base/src/nsNPAPIPluginStreamListener.h b/modules/plugin/base/src/nsNPAPIPluginStreamListener.h index 8cd8ac1c69f..7c20c12b03d 100644 --- a/modules/plugin/base/src/nsNPAPIPluginStreamListener.h +++ b/modules/plugin/base/src/nsNPAPIPluginStreamListener.h @@ -45,7 +45,7 @@ #include "nsIRequest.h" #include "nsITimer.h" #include "nsAutoPtr.h" -#include "nsCOMPtr.h" +#include "nsCOMArray.h" #include "nsIOutputStream.h" #include "nsIPluginInstanceOwner.h" #include "nsString.h" @@ -68,14 +68,49 @@ class nsINPAPIPluginStreamInfo : public nsIPluginStreamInfo { public: NS_DECLARE_STATIC_IID_ACCESSOR(NS_INPAPIPLUGINSTREAMINFO_IID) - - nsIRequest *GetRequest() + + void TrackRequest(nsIRequest* request) { - return mRequest; + mRequests.AppendObject(request); + } + + void ReplaceRequest(nsIRequest* oldRequest, nsIRequest* newRequest) + { + PRInt32 i = mRequests.IndexOfObject(oldRequest); + if (i == -1) { + NS_ASSERTION(mRequests.Count() == 0, + "Only our initial stream should be unknown!"); + mRequests.AppendObject(oldRequest); + } + else { + mRequests.ReplaceObjectAt(newRequest, i); + } + } + + void CancelRequests(nsresult status) + { + // Copy the array to avoid modification during the loop. + nsCOMArray requestsCopy(mRequests); + for (PRInt32 i = 0; i < requestsCopy.Count(); ++i) + requestsCopy[i]->Cancel(status); + } + + void SuspendRequests() { + nsCOMArray requestsCopy(mRequests); + for (PRInt32 i = 0; i < requestsCopy.Count(); ++i) + requestsCopy[i]->Suspend(); + } + + void ResumeRequests() { + nsCOMArray requestsCopy(mRequests); + for (PRInt32 i = 0; i < requestsCopy.Count(); ++i) + requestsCopy[i]->Resume(); } protected: - nsCOMPtr mRequest; + friend class nsPluginByteRangeStreamListener; + + nsCOMArray mRequests; }; NS_DEFINE_STATIC_IID_ACCESSOR(nsINPAPIPluginStreamInfo, @@ -121,7 +156,7 @@ public: nsresult CleanUpStream(NPReason reason); void CallURLNotify(NPReason reason); void SetCallNotify(PRBool aCallNotify) { mCallNotify = aCallNotify; } - nsresult SuspendRequest(); + void SuspendRequest(); void ResumeRequest(); nsresult StartDataPump(); void StopDataPump(); diff --git a/modules/plugin/base/src/nsPluginHost.cpp b/modules/plugin/base/src/nsPluginHost.cpp index 0ef1eef5e71..dd92396cbf3 100644 --- a/modules/plugin/base/src/nsPluginHost.cpp +++ b/modules/plugin/base/src/nsPluginHost.cpp @@ -3341,6 +3341,8 @@ nsresult nsPluginHost::NewPluginURLStream(const nsString& aURL, rv = AddHeadersToChannel(aHeadersData, aHeadersDataLen, httpChannel); } rv = channel->AsyncOpen(listenerPeer, nsnull); + if (NS_SUCCEEDED(rv)) + listenerPeer->TrackRequest(channel); return rv; } diff --git a/modules/plugin/base/src/nsPluginStreamListenerPeer.cpp b/modules/plugin/base/src/nsPluginStreamListenerPeer.cpp index 78b3460633e..b09b6320063 100644 --- a/modules/plugin/base/src/nsPluginStreamListenerPeer.cpp +++ b/modules/plugin/base/src/nsPluginStreamListenerPeer.cpp @@ -51,6 +51,7 @@ #include "nsIURI.h" #include "nsPluginHost.h" #include "nsIByteRangeRequest.h" +#include "nsIMultiPartChannel.h" #include "nsIInputStreamTee.h" #include "nsPrintfCString.h" #include "nsIContentUtils.h" @@ -62,7 +63,10 @@ // nsPluginByteRangeStreamListener -class nsPluginByteRangeStreamListener : public nsIStreamListener { +class nsPluginByteRangeStreamListener + : public nsIStreamListener + , public nsIInterfaceRequestor +{ public: nsPluginByteRangeStreamListener(nsIWeakReference* aWeakPtr); virtual ~nsPluginByteRangeStreamListener(); @@ -70,6 +74,7 @@ public: NS_DECL_ISUPPORTS NS_DECL_NSIREQUESTOBSERVER NS_DECL_NSISTREAMLISTENER + NS_DECL_NSIINTERFACEREQUESTOR private: nsCOMPtr mStreamConverter; @@ -77,7 +82,11 @@ private: PRBool mRemoveMagicNumber; }; -NS_IMPL_ISUPPORTS1(nsPluginByteRangeStreamListener, nsIStreamListener) +NS_IMPL_ISUPPORTS3(nsPluginByteRangeStreamListener, + nsIRequestObserver, + nsIStreamListener, + nsIInterfaceRequestor) + nsPluginByteRangeStreamListener::nsPluginByteRangeStreamListener(nsIWeakReference* aWeakPtr) { mWeakPtrPluginStreamListenerPeer = aWeakPtr; @@ -90,6 +99,22 @@ nsPluginByteRangeStreamListener::~nsPluginByteRangeStreamListener() mWeakPtrPluginStreamListenerPeer = 0; } +/** + * Unwrap any byte-range requests so that we can check whether the base channel + * is being tracked properly. + */ +static nsCOMPtr +GetBaseRequest(nsIRequest* r) +{ + nsCOMPtr mp = do_QueryInterface(r); + if (!mp) + return r; + + nsCOMPtr base; + mp->GetBaseChannel(getter_AddRefs(base)); + return already_AddRefed(base.forget()); +} + NS_IMETHODIMP nsPluginByteRangeStreamListener::OnStartRequest(nsIRequest *request, nsISupports *ctxt) { @@ -99,6 +124,12 @@ nsPluginByteRangeStreamListener::OnStartRequest(nsIRequest *request, nsISupports if (!finalStreamListener) return NS_ERROR_FAILURE; + nsPluginStreamListenerPeer *pslp = + static_cast(finalStreamListener.get()); + + NS_ASSERTION(pslp->mRequests.IndexOfObject(GetBaseRequest(request)) != -1, + "Untracked byte-range request?"); + nsCOMPtr serv = do_GetService(NS_STREAMCONVERTERSERVICE_CONTRACTID, &rv); if (NS_SUCCEEDED(rv)) { rv = serv->AsyncConvertData(MULTIPART_BYTERANGES, @@ -125,10 +156,6 @@ nsPluginByteRangeStreamListener::OnStartRequest(nsIRequest *request, nsISupports return NS_ERROR_FAILURE; } - // get nsPluginStreamListenerPeer* ptr from finalStreamListener - nsPluginStreamListenerPeer *pslp = - reinterpret_cast(finalStreamListener.get()); - if (responseCode != 200) { PRBool bWantsAllNetworkStreams = PR_FALSE; pslp->GetPluginInstance()-> @@ -159,6 +186,13 @@ nsPluginByteRangeStreamListener::OnStopRequest(nsIRequest *request, nsISupports if (!finalStreamListener) return NS_ERROR_FAILURE; + nsPluginStreamListenerPeer *pslp = + static_cast(finalStreamListener.get()); + PRBool found = pslp->mRequests.RemoveObject(request); + if (!found) { + NS_ERROR("OnStopRequest received for untracked byte-range request!"); + } + if (mRemoveMagicNumber) { // remove magic number from container nsCOMPtr container = do_QueryInterface(ctxt); @@ -222,6 +256,18 @@ nsPluginByteRangeStreamListener::OnDataAvailable(nsIRequest *request, nsISupport return mStreamConverter->OnDataAvailable(request, ctxt, inStr, sourceOffset, count); } +NS_IMETHODIMP +nsPluginByteRangeStreamListener::GetInterface(const nsIID& aIID, void** result) +{ + // Forward interface requests to our parent + nsCOMPtr finalStreamListener = do_QueryReferent(mWeakPtrPluginStreamListenerPeer); + if (!finalStreamListener) + return NS_ERROR_FAILURE; + + return finalStreamListener->GetInterface(aIID, result); +} + + // nsPRUintKey class nsPRUintKey : public nsHashKey { @@ -297,8 +343,7 @@ nsPluginStreamListenerPeer::~nsPluginStreamListenerPeer() // Called as a result of GetURL and PostURL nsresult nsPluginStreamListenerPeer::Initialize(nsIURI *aURL, nsNPAPIPluginInstance *aInstance, - nsIPluginStreamListener* aListener, - PRInt32 requestCount) + nsIPluginStreamListener* aListener) { #ifdef PLUGIN_LOGGING nsCAutoString urlSpec; @@ -317,7 +362,7 @@ nsresult nsPluginStreamListenerPeer::Initialize(nsIURI *aURL, mPStreamListener = static_cast(aListener); mPStreamListener->SetStreamListenerPeer(this); - mPendingRequests = requestCount; + mPendingRequests = 1; mDataForwardToRequest = new nsHashtable(16, PR_FALSE); if (!mDataForwardToRequest) @@ -471,6 +516,12 @@ nsPluginStreamListenerPeer::OnStartRequest(nsIRequest *request, nsISupports* aContext) { nsresult rv = NS_OK; + + if (mRequests.IndexOfObject(GetBaseRequest(request)) == -1) { + NS_ASSERTION(mRequests.Count() == 0, + "Only our initial stream should be unknown!"); + TrackRequest(request); + } if (mHaveFiredOnStartRequest) { return NS_OK; @@ -550,8 +601,6 @@ nsPluginStreamListenerPeer::OnStartRequest(nsIRequest *request, mLength = length; } - mRequest = request; - nsCAutoString aContentType; // XXX but we already got the type above! rv = channel->GetContentType(aContentType); if (NS_FAILED(rv)) @@ -775,7 +824,10 @@ nsPluginStreamListenerPeer::RequestRead(NPByteRange* rangeList) if (NS_FAILED(rv)) return rv; - return channel->AsyncOpen(converter, container); + rv = channel->AsyncOpen(converter, container); + if (NS_SUCCEEDED(rv)) + TrackRequest(channel); + return rv; } NS_IMETHODIMP @@ -864,6 +916,9 @@ NS_IMETHODIMP nsPluginStreamListenerPeer::OnDataAvailable(nsIRequest *request, PRUint32 sourceOffset, PRUint32 aLength) { + NS_ASSERTION(mRequests.IndexOfObject(GetBaseRequest(request)) != -1, + "Received OnDataAvailable for untracked request."); + if (mRequestFailed) return NS_ERROR_FAILURE; @@ -885,8 +940,6 @@ NS_IMETHODIMP nsPluginStreamListenerPeer::OnDataAvailable(nsIRequest *request, if (!mPStreamListener) return NS_ERROR_FAILURE; - mRequest = request; - const char * url = nsnull; GetURL(&url); @@ -968,6 +1021,14 @@ NS_IMETHODIMP nsPluginStreamListenerPeer::OnStopRequest(nsIRequest *request, nsresult aStatus) { nsresult rv = NS_OK; + + nsCOMPtr mp = do_QueryInterface(request); + if (!mp) { + PRBool found = mRequests.RemoveObject(request); + if (!found) { + NS_ERROR("Received OnStopRequest for untracked request."); + } + } PLUGIN_LOG(PLUGIN_LOG_NOISY, ("nsPluginStreamListenerPeer::OnStopRequest this=%p aStatus=%d request=%p\n", @@ -1073,7 +1134,6 @@ NS_IMETHODIMP nsPluginStreamListenerPeer::OnStopRequest(nsIRequest *request, if (NS_SUCCEEDED(aStatus)) { mStreamComplete = PR_TRUE; } - mRequest = NULL; return NS_OK; } @@ -1284,6 +1344,47 @@ nsPluginStreamListenerPeer::GetInterface(const nsIID& aIID, void** result) return GetInterfaceGlobal(aIID, result); } +/** + * Proxy class which forwards async redirect notifications back to the necko + * callback, keeping nsPluginStreamListenerPeer::mRequests in sync with + * which channel is active. + */ +class ChannelRedirectProxyCallback : public nsIAsyncVerifyRedirectCallback +{ +public: + ChannelRedirectProxyCallback(nsINPAPIPluginStreamInfo* listener, + nsIAsyncVerifyRedirectCallback* parent, + nsIChannel* oldChannel, + nsIChannel* newChannel) + : mWeakListener(do_GetWeakReference(listener)) + , mParent(parent) + , mOldChannel(oldChannel) + , mNewChannel(newChannel) + { + } + + NS_DECL_ISUPPORTS + + NS_IMETHODIMP OnRedirectVerifyCallback(nsresult result) + { + if (NS_SUCCEEDED(result)) { + nsCOMPtr listener = do_QueryReferent(mWeakListener); + if (listener) + listener->ReplaceRequest(mOldChannel, mNewChannel); + } + return mParent->OnRedirectVerifyCallback(result); + } + +private: + nsWeakPtr mWeakListener; + nsCOMPtr mParent; + nsCOMPtr mOldChannel; + nsCOMPtr mNewChannel; +}; + +NS_IMPL_ISUPPORTS1(ChannelRedirectProxyCallback, nsIAsyncVerifyRedirectCallback) + + NS_IMETHODIMP nsPluginStreamListenerPeer::AsyncOnChannelRedirect(nsIChannel *oldChannel, nsIChannel *newChannel, PRUint32 flags, nsIAsyncVerifyRedirectCallback* callback) @@ -1293,8 +1394,11 @@ nsPluginStreamListenerPeer::AsyncOnChannelRedirect(nsIChannel *oldChannel, nsICh return NS_ERROR_FAILURE; } + nsCOMPtr proxyCallback = + new ChannelRedirectProxyCallback(this, callback, oldChannel, newChannel); + // Give NPAPI a chance to control redirects. - bool notificationHandled = mPStreamListener->HandleRedirectNotification(oldChannel, newChannel, callback); + bool notificationHandled = mPStreamListener->HandleRedirectNotification(oldChannel, newChannel, proxyCallback); if (notificationHandled) { return NS_OK; } @@ -1331,5 +1435,5 @@ nsPluginStreamListenerPeer::AsyncOnChannelRedirect(nsIChannel *oldChannel, nsICh return rv; } - return channelEventSink->AsyncOnChannelRedirect(oldChannel, newChannel, flags, callback); + return channelEventSink->AsyncOnChannelRedirect(oldChannel, newChannel, flags, proxyCallback); } diff --git a/modules/plugin/base/src/nsPluginStreamListenerPeer.h b/modules/plugin/base/src/nsPluginStreamListenerPeer.h index 2d04f31a845..7cba1e4de86 100644 --- a/modules/plugin/base/src/nsPluginStreamListenerPeer.h +++ b/modules/plugin/base/src/nsPluginStreamListenerPeer.h @@ -109,15 +109,14 @@ public: // Called by GetURL and PostURL (via NewStream) nsresult Initialize(nsIURI *aURL, nsNPAPIPluginInstance *aInstance, - nsIPluginStreamListener *aListener, - PRInt32 requestCount = 1); + nsIPluginStreamListener *aListener); nsresult InitializeEmbedded(nsIURI *aURL, nsNPAPIPluginInstance* aInstance, nsIPluginInstanceOwner *aOwner = nsnull); nsresult InitializeFullPage(nsIURI* aURL, nsNPAPIPluginInstance *aInstance); - + nsresult OnFileAvailable(nsIFile* aFile); nsresult ServeStreamAsFile(nsIRequest *request, nsISupports *ctxt);