Bug 1633935 - P9 send OnAfterLastPart via pBg IPC to avoid race betwe…

…en OS*R, r=mayhemer,necko-reviewers Depends on D77750 Differential Revision: https://phabricator.services.mozilla.com/D77751
Brli · Jun 30, 2020 · 295be98 · 295be98
1 parent 06e6e24
commit 295be98
Show file tree

Hide file tree

Showing 9 changed files with 67 additions and 11 deletions.
diff --git a/netwerk/protocol/http/HttpBackgroundChannelChild.cpp b/netwerk/protocol/http/HttpBackgroundChannelChild.cpp
@@ -133,6 +133,20 @@ bool HttpBackgroundChannelChild::CreateBackgroundChannel() {
   return true;
 }
 
+IPCResult HttpBackgroundChannelChild::RecvOnAfterLastPart(
+    const nsresult& aStatus) {
+  LOG(("HttpBackgroundChannelChild::RecvOnAfterLastPart [this=%p]\n", this));
+  MOZ_ASSERT(OnSocketThread());
+
+  MOZ_ASSERT(mChannelChild, "no channel child in RecvOnAfterLastPart");
+  if (NS_WARN_IF(!mChannelChild)) {
+    return IPC_OK();
+  }
+
+  mChannelChild->ProcessOnAfterLastPart(aStatus);
+  return IPC_OK();
+}
+
 IPCResult HttpBackgroundChannelChild::RecvOnProgress(
     const int64_t& aProgress, const int64_t& aProgressMax) {
   LOG(("HttpBackgroundChannelChild::RecvOnProgress [this=%p]\n", this));

diff --git a/netwerk/protocol/http/HttpBackgroundChannelChild.h b/netwerk/protocol/http/HttpBackgroundChannelChild.h
@@ -69,6 +69,8 @@ class HttpBackgroundChannelChild final : public PHttpBackgroundChannelChild {
       const nsHttpHeaderArray& aResponseTrailers,
       const nsTArray<ConsoleReportCollected>& aConsoleReports);
 
+  IPCResult RecvOnAfterLastPart(const nsresult& aStatus);
+
   IPCResult RecvOnProgress(const int64_t& aProgress,
                            const int64_t& aProgressMax);
 

diff --git a/netwerk/protocol/http/HttpBackgroundChannelParent.cpp b/netwerk/protocol/http/HttpBackgroundChannelParent.cpp
@@ -239,6 +239,30 @@ bool HttpBackgroundChannelParent::OnStopRequest(
                            aResponseTrailers, aConsoleReports);
 }
 
+bool HttpBackgroundChannelParent::OnAfterLastPart(const nsresult aStatus) {
+  LOG(("HttpBackgroundChannelParent::OnAfterLastPart [this=%p]\n", this));
+  AssertIsInMainProcess();
+
+  if (NS_WARN_IF(!mIPCOpened)) {
+    return false;
+  }
+
+  if (!IsOnBackgroundThread()) {
+    MutexAutoLock lock(mBgThreadMutex);
+    nsresult rv = mBackgroundThread->Dispatch(
+        NewRunnableMethod<const nsresult>(
+            "net::HttpBackgroundChannelParent::OnAfterLastPart", this,
+            &HttpBackgroundChannelParent::OnAfterLastPart, aStatus),
+        NS_DISPATCH_NORMAL);
+
+    MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(rv));
+
+    return NS_SUCCEEDED(rv);
+  }
+
+  return SendOnAfterLastPart(aStatus);
+}
+
 bool HttpBackgroundChannelParent::OnProgress(const int64_t aProgress,
                                              const int64_t aProgressMax) {
   LOG(("HttpBackgroundChannelParent::OnProgress [this=%p]\n", this));

diff --git a/netwerk/protocol/http/HttpBackgroundChannelParent.h b/netwerk/protocol/http/HttpBackgroundChannelParent.h
@@ -57,6 +57,9 @@ class HttpBackgroundChannelParent final : public PHttpBackgroundChannelParent {
                      const nsHttpHeaderArray& aResponseTrailers,
                      const nsTArray<ConsoleReportCollected>& aConsoleReports);
 
+  // To send OnAfterLastPart message over background channel.
+  bool OnAfterLastPart(const nsresult aStatus);
+
   // To send OnProgress message over background channel.
   bool OnProgress(const int64_t aProgress, const int64_t aProgressMax);
 

diff --git a/netwerk/protocol/http/HttpChannelChild.cpp b/netwerk/protocol/http/HttpChannelChild.cpp
@@ -531,13 +531,13 @@ void HttpChannelChild::OnStartRequest(
   DoOnStartRequest(this, nullptr);
 }
 
-mozilla::ipc::IPCResult HttpChannelChild::RecvOnAfterLastPart(
-    const nsresult& aStatus) {
+void HttpChannelChild::ProcessOnAfterLastPart(const nsresult& aStatus) {
+  LOG(("HttpChannelChild::ProcessOnAfterLastPart [this=%p]\n", this));
+  MOZ_ASSERT(OnSocketThread());
   mEventQ->RunOrEnqueue(new NeckoTargetChannelFunctionEvent(
       this, [self = UnsafePtr<HttpChannelChild>(this), aStatus]() {
         self->OnAfterLastPart(aStatus);
       }));
-  return IPC_OK();
 }
 
 void HttpChannelChild::OnAfterLastPart(const nsresult& aStatus) {

diff --git a/netwerk/protocol/http/HttpChannelChild.h b/netwerk/protocol/http/HttpChannelChild.h
@@ -176,8 +176,6 @@ 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;
@@ -266,6 +264,7 @@ class HttpChannelChild final : public PHttpChannelChild,
       const nsTArray<ConsoleReportCollected>& aConsoleReports);
   void ProcessFlushedForDiversion();
   void ProcessDivertMessages();
+  void ProcessOnAfterLastPart(const nsresult& aStatus);
   void ProcessOnProgress(const int64_t& aProgress, const int64_t& aProgressMax);
 
   void ProcessOnStatus(const nsresult& aStatus);

diff --git a/netwerk/protocol/http/HttpChannelParent.cpp b/netwerk/protocol/http/HttpChannelParent.cpp
@@ -1629,9 +1629,22 @@ HttpChannelParent::OnStopRequest(nsIRequest* aRequest, nsresult aStatusCode) {
 
 NS_IMETHODIMP
 HttpChannelParent::OnAfterLastPart(nsresult aStatus) {
-  if (!mIPCClosed) {
-    Unused << SendOnAfterLastPart(aStatus);
+  LOG(("HttpChannelParent::OnAfterLastPart [this=%p]\n", this));
+  MOZ_ASSERT(NS_IsMainThread());
+
+  // If IPC channel is closed, there is nothing we can do. Just return NS_OK.
+  if (mIPCClosed) {
+    return NS_OK;
   }
+
+  // If IPC channel is open, background channel should be ready to send
+  // OnAfterLastPart.
+  MOZ_ASSERT(mBgParent);
+
+  if (!mBgParent || !mBgParent->OnAfterLastPart(aStatus)) {
+    return NS_ERROR_UNEXPECTED;
+  }
+
   return NS_OK;
 }
 

diff --git a/netwerk/protocol/http/PHttpBackgroundChannel.ipdl b/netwerk/protocol/http/PHttpBackgroundChannel.ipdl
@@ -45,6 +45,11 @@ child:
 
   async OnStatus(nsresult status);
 
+  // Forwards nsIMultiPartChannelListener::onAfterLastPart. Make sure we've
+  // disconnected our listeners, since we might not have on the last
+  // OnStopRequest.
+  async OnAfterLastPart(nsresult aStatus);
+
   // Parent has been suspended for diversion; no more events to be enqueued.
   async FlushedForDiversion();
 

diff --git a/netwerk/protocol/http/PHttpChannel.ipdl b/netwerk/protocol/http/PHttpChannel.ipdl
@@ -165,10 +165,6 @@ 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