From 6f0f9e969fcc343c184f02ba873bc4b1af9d87d5 Mon Sep 17 00:00:00 2001 From: Matt Woodrow Date: Wed, 4 Dec 2019 03:19:38 +0000 Subject: [PATCH] Bug 1600211 - Forward AllPartsStopped to HttpChannelChild to ensure that we notify the listeners correctly. r=mayhemer Differential Revision: https://phabricator.services.mozilla.com/D55223 --HG-- extra : moz-landing-system : lando --- netwerk/ipc/DocumentLoadListener.cpp | 15 ++++++- netwerk/ipc/DocumentLoadListener.h | 5 ++- netwerk/protocol/http/HttpChannelChild.cpp | 45 +++++++++++++++++++ netwerk/protocol/http/HttpChannelChild.h | 3 ++ netwerk/protocol/http/HttpChannelParent.cpp | 13 ++++++ netwerk/protocol/http/HttpChannelParent.h | 5 ++- netwerk/protocol/http/PHttpChannel.ipdl | 4 ++ .../protocol/http/ParentChannelListener.cpp | 14 +++--- netwerk/protocol/http/ParentChannelListener.h | 4 ++ 9 files changed, 100 insertions(+), 8 deletions(-) diff --git a/netwerk/ipc/DocumentLoadListener.cpp b/netwerk/ipc/DocumentLoadListener.cpp index d7f3195b81f2..57afee03642e 100644 --- a/netwerk/ipc/DocumentLoadListener.cpp +++ b/netwerk/ipc/DocumentLoadListener.cpp @@ -384,6 +384,17 @@ void DocumentLoadListener::ResumeSuspendedChannel( aListener->OnStopRequest(aParams.request, rv); } rv = NS_OK; + }, + [&](const OnAfterLastPartParams& aParams) { + nsCOMPtr multiListener = + do_QueryInterface(aListener); + if (multiListener) { + if (NS_SUCCEEDED(rv)) { + multiListener->OnAfterLastPart(aParams.status); + } else { + multiListener->OnAfterLastPart(rv); + } + } }); } // We don't expect to get new stream listener functions added @@ -765,7 +776,9 @@ DocumentLoadListener::OnDataAvailable(nsIRequest* aRequest, //----------------------------------------------------------------------------- NS_IMETHODIMP -DocumentLoadListener::OnAfterLastPart() { +DocumentLoadListener::OnAfterLastPart(nsresult aStatus) { + mStreamListenerFunctions.AppendElement(StreamListenerFunction{ + VariantIndex<3>{}, OnAfterLastPartParams{aStatus}}); mIsFinished = true; return NS_OK; } diff --git a/netwerk/ipc/DocumentLoadListener.h b/netwerk/ipc/DocumentLoadListener.h index c04a1170e7d2..3984f522dbca 100644 --- a/netwerk/ipc/DocumentLoadListener.h +++ b/netwerk/ipc/DocumentLoadListener.h @@ -274,8 +274,11 @@ class DocumentLoadListener : public nsIInterfaceRequestor, nsCOMPtr request; nsresult status; }; + struct OnAfterLastPartParams { + nsresult status; + }; typedef mozilla::Variant + OnStopRequestParams, OnAfterLastPartParams> StreamListenerFunction; // TODO Backtrack this. // The set of nsIStreamListener functions that got called on this diff --git a/netwerk/protocol/http/HttpChannelChild.cpp b/netwerk/protocol/http/HttpChannelChild.cpp index 78cc484b3fe3..4cd0e3769915 100644 --- a/netwerk/protocol/http/HttpChannelChild.cpp +++ b/netwerk/protocol/http/HttpChannelChild.cpp @@ -582,6 +582,51 @@ mozilla::ipc::IPCResult HttpChannelChild::RecvOnStopRequest( return IPC_OK(); } +mozilla::ipc::IPCResult HttpChannelChild::RecvOnAfterLastPart( + const nsresult& aStatus) { + mEventQ->RunOrEnqueue(new NeckoTargetChannelFunctionEvent( + this, [self = UnsafePtr(this), aStatus]() { + self->OnAfterLastPart(aStatus); + })); + return IPC_OK(); +} + +void HttpChannelChild::OnAfterLastPart(const nsresult& aStatus) { + if (mOnStopRequestCalled) { + return; + } + mOnStopRequestCalled = true; + + // notify "http-on-stop-connect" observers + gHttpHandler->OnStopRequest(this); + + ReleaseListeners(); + + // If a preferred alt-data type was set, the parent would hold a reference to + // the cache entry in case the child calls openAlternativeOutputStream(). + // (see nsHttpChannel::OnStopRequest) + if (!mPreferredCachedAltDataTypes.IsEmpty()) { + mAltDataCacheEntryAvailable = mCacheEntryAvailable; + } + mCacheEntryAvailable = false; + + if (mLoadGroup) mLoadGroup->RemoveRequest(this, nullptr, mStatus); + CleanupBackgroundChannel(); + + if (mLoadFlags & LOAD_DOCUMENT_URI) { + // Keep IPDL channel open, but only for updating security info. + // If IPDL is already closed, then do nothing. + if (CanSend()) { + mKeptAlive = true; + SendDocumentChannelCleanup(true); + } + } else { + // The parent process will respond by sending a DeleteSelf message and + // making sure not to send any more messages after that. + TrySendDeletingChannel(); + } +} + class SyntheticDiversionListener final : public nsIStreamListener { RefPtr mChannel; diff --git a/netwerk/protocol/http/HttpChannelChild.h b/netwerk/protocol/http/HttpChannelChild.h index d62600648985..6829ee805950 100644 --- a/netwerk/protocol/http/HttpChannelChild.h +++ b/netwerk/protocol/http/HttpChannelChild.h @@ -216,6 +216,8 @@ class HttpChannelChild final : public PHttpChannelChild, mozilla::ipc::IPCResult RecvSetClassifierMatchedTrackingInfo( const ClassifierInfo& info) override; + mozilla::ipc::IPCResult RecvOnAfterLastPart(const nsresult& aStatus) override; + virtual void ActorDestroy(ActorDestroyReason aWhy) override; virtual void DoNotifyListenerCleanup() override; @@ -534,6 +536,7 @@ class HttpChannelChild final : public PHttpChannelChild, void DeleteSelf(); void DoNotifyListener(); void ContinueDoNotifyListener(); + void OnAfterLastPart(const nsresult& aStatus); // Create a a new channel to be used in a redirection, based on the provided // response headers. diff --git a/netwerk/protocol/http/HttpChannelParent.cpp b/netwerk/protocol/http/HttpChannelParent.cpp index f87fe3e1b158..bdd408ae2684 100644 --- a/netwerk/protocol/http/HttpChannelParent.cpp +++ b/netwerk/protocol/http/HttpChannelParent.cpp @@ -290,6 +290,7 @@ NS_INTERFACE_MAP_BEGIN(HttpChannelParent) NS_INTERFACE_MAP_ENTRY(nsIAsyncVerifyRedirectReadyCallback) NS_INTERFACE_MAP_ENTRY(nsIChannelEventSink) NS_INTERFACE_MAP_ENTRY(nsIRedirectResultListener) + NS_INTERFACE_MAP_ENTRY(nsIMultiPartChannelListener) NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIParentRedirectingChannel) NS_INTERFACE_MAP_ENTRY_CONCRETE(HttpChannelParent) NS_INTERFACE_MAP_END @@ -1590,6 +1591,18 @@ HttpChannelParent::OnStopRequest(nsIRequest* aRequest, nsresult aStatusCode) { return NS_OK; } +//----------------------------------------------------------------------------- +// HttpChannelParent::nsIMultiPartChannelListener +//----------------------------------------------------------------------------- + +NS_IMETHODIMP +HttpChannelParent::OnAfterLastPart(nsresult aStatus) { + if (!mIPCClosed) { + Unused << SendOnAfterLastPart(aStatus); + } + return NS_OK; +} + //----------------------------------------------------------------------------- // HttpChannelParent::nsIStreamListener //----------------------------------------------------------------------------- diff --git a/netwerk/protocol/http/HttpChannelParent.h b/netwerk/protocol/http/HttpChannelParent.h index 9562cd843426..5018a608ec22 100644 --- a/netwerk/protocol/http/HttpChannelParent.h +++ b/netwerk/protocol/http/HttpChannelParent.h @@ -23,6 +23,7 @@ #include "nsIAuthPromptProvider.h" #include "mozilla/dom/ipc/IdType.h" #include "nsIDeprecationWarner.h" +#include "nsIMultiPartChannel.h" class nsICacheEntry; @@ -60,7 +61,8 @@ class HttpChannelParent final : public nsIInterfaceRequestor, public HttpChannelSecurityWarningReporter, public nsIAsyncVerifyRedirectReadyCallback, public nsIChannelEventSink, - public nsIRedirectResultListener { + public nsIRedirectResultListener, + public nsIMultiPartChannelListener { virtual ~HttpChannelParent(); public: @@ -76,6 +78,7 @@ class HttpChannelParent final : public nsIInterfaceRequestor, NS_DECL_NSIASYNCVERIFYREDIRECTREADYCALLBACK NS_DECL_NSICHANNELEVENTSINK NS_DECL_NSIREDIRECTRESULTLISTENER + NS_DECL_NSIMULTIPARTCHANNELLISTENER NS_DECLARE_STATIC_IID_ACCESSOR(HTTP_CHANNEL_PARENT_IID) diff --git a/netwerk/protocol/http/PHttpChannel.ipdl b/netwerk/protocol/http/PHttpChannel.ipdl index 453c7234462d..e645407b3b34 100644 --- a/netwerk/protocol/http/PHttpChannel.ipdl +++ b/netwerk/protocol/http/PHttpChannel.ipdl @@ -238,6 +238,10 @@ child: // Tell the child information of matched URL againts SafeBrowsing tracking list async SetClassifierMatchedTrackingInfo(ClassifierInfo info); + + // Forwards nsIMultiPartChannelListener::onAfterLastPart. Make sure we've + // disconnected our listeners, since we might not have on the last OnStopRequest. + async OnAfterLastPart(nsresult aStatus); both: // After receiving this message, the parent also calls diff --git a/netwerk/protocol/http/ParentChannelListener.cpp b/netwerk/protocol/http/ParentChannelListener.cpp index fac38ca57a16..9c89fe6e06a0 100644 --- a/netwerk/protocol/http/ParentChannelListener.cpp +++ b/netwerk/protocol/http/ParentChannelListener.cpp @@ -78,6 +78,14 @@ ParentChannelListener::OnStartRequest(nsIRequest* aRequest) { if (!mNextListener) return NS_ERROR_UNEXPECTED; + // If we're not a multi-part channel, then we can drop mListener and break the + // reference cycle. If we are, then this might be called again, so wait for + // OnAfterLastPart instead. + nsCOMPtr multiPartChannel = do_QueryInterface(aRequest); + if (multiPartChannel) { + mIsMultiPart = true; + } + LOG(("ParentChannelListener::OnStartRequest [this=%p]\n", this)); return mNextListener->OnStartRequest(aRequest); } @@ -94,11 +102,7 @@ ParentChannelListener::OnStopRequest(nsIRequest* aRequest, this, static_cast(aStatusCode))); nsresult rv = mNextListener->OnStopRequest(aRequest, aStatusCode); - // If we're not a multi-part channel, then we can drop mListener and break the - // reference cycle. If we are, then this might be called again, so wait for - // OnAfterLastPart instead. - nsCOMPtr multiPartChannel = do_QueryInterface(aRequest); - if (!multiPartChannel) { + if (!mIsMultiPart) { mNextListener = nullptr; } return rv; diff --git a/netwerk/protocol/http/ParentChannelListener.h b/netwerk/protocol/http/ParentChannelListener.h index f06b2d92bb43..5c6e070d9e8e 100644 --- a/netwerk/protocol/http/ParentChannelListener.h +++ b/netwerk/protocol/http/ParentChannelListener.h @@ -90,6 +90,10 @@ class ParentChannelListener final : public nsIInterfaceRequestor, nsCOMPtr mInterceptController; RefPtr mBrowserParent; + + // True if we received OnStartRequest for a nsIMultiPartChannel, and are + // expected AllPartsStopped to be called when complete. + bool mIsMultiPart = false; }; NS_DEFINE_STATIC_IID_ACCESSOR(ParentChannelListener, PARENT_CHANNEL_LISTENER)