Bug 1633935 - P2 remove OnStartRequestSent since OnStartRequest is passed thru pBg, r=mayhemer,necko-reviewers

Depends on D73529

Differential Revision: https://phabricator.services.mozilla.com/D76969

netwerk/protocol/http/HttpBackgroundChannelChild.cpp

@@ -132,33 +132,15 @@ bool HttpBackgroundChannelChild::CreateBackgroundChannel() {
   return true;
 }
 
-bool HttpBackgroundChannelChild::IsWaitingOnStartRequest(
-    bool aDataFromSocketProcess) {
+bool HttpBackgroundChannelChild::IsWaitingOnStartRequest() {
   MOZ_ASSERT(OnSocketThread());
 
-  // When data is from socket process, it is possible that both mStartSent and
-  // mStartReceived are false here. We need to wait until OnStartRequest sent
-  // from parent process.
-  // TODO: We can remove this code when diversion is removed in bug 1604448.
-  if (aDataFromSocketProcess) {
-    return !mStartReceived;
-  }
-
   // Need to wait for OnStartRequest if it is sent by
   // parent process but not received by content process.
-  return (mStartSent && !mStartReceived);
+  return !mStartReceived;
 }
 
 // PHttpBackgroundChannelChild
-IPCResult HttpBackgroundChannelChild::RecvOnStartRequestSent() {
-  LOG(("HttpBackgroundChannelChild::RecvOnStartRequestSent [this=%p]\n", this));
-  MOZ_ASSERT(OnSocketThread());
-  MOZ_ASSERT(!mStartSent);  // Should only receive this message once.
-
-  mStartSent = true;
-  return IPC_OK();
-}
-
 IPCResult HttpBackgroundChannelChild::RecvOnStartRequest(
     const nsHttpResponseHead& aResponseHead, const bool& aUseResponseHead,
     const nsHttpHeaderArray& aRequestHeaders,
@@ -171,11 +153,6 @@ IPCResult HttpBackgroundChannelChild::RecvOnStartRequest(
     return IPC_OK();
   }
 
-  // TODO: OnStartRequest is off-main-thread so it's unnecessary to dispatch
-  // another IPC message for sync reason. Directly call here to behave the same
-  // as before. This is no longer needed and removed in the next patches.
-  RecvOnStartRequestSent();
-
   mChannelChild->ProcessOnStartRequest(aResponseHead, aUseResponseHead,
                                        aRequestHeaders, aArgs);
   // Allow to queue other runnable since OnStartRequest Event already hits the
@@ -208,7 +185,8 @@ IPCResult HttpBackgroundChannelChild::RecvOnTransportAndData(
     return IPC_OK();
   }
 
-  if (IsWaitingOnStartRequest(aDataFromSocketProcess)) {
+  // Bug 1641336: Race only happens if the data is from socket process.
+  if (IsWaitingOnStartRequest()) {
     LOG(("  > pending until OnStartRequest [offset=%" PRIu64 " count=%" PRIu32
          "]\n",
          aOffset, aCount));

netwerk/protocol/http/HttpBackgroundChannelChild.h

@@ -73,8 +73,6 @@ class HttpBackgroundChannelChild final : public PHttpBackgroundChannelChild {
 
   IPCResult RecvDivertMessages();
 
-  IPCResult RecvOnStartRequestSent();
-
   void ActorDestroy(ActorDestroyReason aWhy) override;
 
   void CreateDataBridge();
@@ -93,12 +91,7 @@ class HttpBackgroundChannelChild final : public PHttpBackgroundChannelChild {
   // OnStartRequestReceived.
   // return true after both RecvOnStartRequestSend and OnStartRequestReceived
   // are invoked.
-  // When ODA message is from socket process, it is possible that both
-  // RecvOnStartRequestSent and OnStartRequestReceived are not invoked, but
-  // RecvOnTransportAndData is already invoked. In this case, we only need to
-  // check if OnStartRequestReceived is invoked to make sure ODA doesn't happen
-  // before OnStartRequest.
-  bool IsWaitingOnStartRequest(bool aDataFromSocketProcess = false);
+  bool IsWaitingOnStartRequest();
 
   // Associated HttpChannelChild for handling the channel events.
   // Will be removed while failed to create background channel,
@@ -110,10 +103,6 @@ class HttpBackgroundChannelChild final : public PHttpBackgroundChannelChild {
   // Should only access on STS thread.
   bool mStartReceived = false;
 
-  // True if OnStartRequest is sent by HttpChannelParent.
-  // Should only access on STS thread.
-  bool mStartSent = false;
-
   // Store pending messages that require to be handled after OnStartRequest.
   // Should be flushed after OnStartRequest is received and handled.
   // Should only access on STS thread.

netwerk/protocol/http/HttpBackgroundChannelParent.cpp

@@ -142,30 +142,6 @@ void HttpBackgroundChannelParent::OnChannelClosed() {
   Unused << NS_WARN_IF(NS_FAILED(rv));
 }
 
-bool HttpBackgroundChannelParent::OnStartRequestSent() {
-  LOG(("HttpBackgroundChannelParent::OnStartRequestSent [this=%p]\n", this));
-  AssertIsInMainProcess();
-
-  if (NS_WARN_IF(!mIPCOpened)) {
-    return false;
-  }
-
-  if (!IsOnBackgroundThread()) {
-    MutexAutoLock lock(mBgThreadMutex);
-    nsresult rv = mBackgroundThread->Dispatch(
-        NewRunnableMethod(
-            "net::HttpBackgroundChannelParent::OnStartRequestSent", this,
-            &HttpBackgroundChannelParent::OnStartRequestSent),
-        NS_DISPATCH_NORMAL);
-
-    MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(rv));
-
-    return NS_SUCCEEDED(rv);
-  }
-
-  return SendOnStartRequestSent();
-}
-
 bool HttpBackgroundChannelParent::OnStartRequest(
     const nsHttpResponseHead& aResponseHead, const bool& aUseResponseHead,
     const nsHttpHeaderArray& aRequestHeaders,

netwerk/protocol/http/HttpBackgroundChannelParent.h

@@ -39,9 +39,6 @@ class HttpBackgroundChannelParent final : public PHttpBackgroundChannelParent {
   // IPC channel.
   void OnChannelClosed();
 
-  // To send OnStartRequestSend message over background channel.
-  bool OnStartRequestSent();
-
   // To send OnStartRequest message over background channel.
   bool OnStartRequest(const nsHttpResponseHead& aResponseHead,
                       const bool& aUseResponseHead,

netwerk/protocol/http/HttpChannelParent.cpp

@@ -1535,8 +1535,6 @@ HttpChannelParent::OnStartRequest(nsIRequest* aRequest) {
   if (!mIPCClosed) {
     // TODO: For multipart channel, we would deliever everything across
     // pBackground as well.
-    // TODO: OnStartRequestSent is no longer needed since
-    // OnStartRequest/ODA/OnStopRequest are on the same thread.
     if (!mIsMultiPart) {
       ipcResult = mBgParent->OnStartRequest(
           *responseHead, useResponseHead,
@@ -1554,19 +1552,6 @@ HttpChannelParent::OnStartRequest(nsIRequest* aRequest) {
   if (mIPCClosed || !ipcResult) {
     rv = NS_ERROR_UNEXPECTED;
   }
-
-  // OnStartRequest is sent to content process successfully.
-  // Notify PHttpBackgroundChannelChild that all following IPC mesasges
-  // should be run after OnStartRequest is handled.
-  // We don't send this if it's multipart, since we don't use the
-  // background channel in that case.
-  if (NS_SUCCEEDED(rv) && !multiPartID) {
-    MOZ_ASSERT(mBgParent);
-    if (!mBgParent->OnStartRequestSent()) {
-      rv = NS_ERROR_UNEXPECTED;
-    }
-  }
-
   return rv;
 }
 

netwerk/protocol/http/PHttpBackgroundChannel.ipdl

@@ -20,12 +20,6 @@ async refcounted protocol PHttpBackgroundChannel
   manager PBackground;
 
 child:
-  // OnStartRequest is sent over main thread IPC. The following
-  // OnTransportAndData/OnStopRequest/OnProgress/OnStatus/FlushForDiversion/
-  // DivertMessages needs to wait in content process until OnStartRequest
-  // is processed. For synchronizing the event sequence.
-  async OnStartRequestSent();
-
   async OnStartRequest(nsHttpResponseHead  responseHead,
                        bool                useResponseHead,
                        nsHttpHeaderArray   requestHeaders,