Bug 1638652 - Only disable content conversion in the parent process if we're going to be delivering data to a content process. r=jya,necko-reviewers
authorMatt Woodrow <mwoodrow@mozilla.com>
Tue, 02 Jun 2020 23:43:19 +0000
changeset 533639 865edc41179afc8f6f8a345d8c17a0a63597d6d8
parent 533638 7e03470bab9eb0bb72081cbbbb3217304dbd6d80
child 533640 c47cf57dab71d81c60b5a735277c39dd719ce2ba
push id37475
push userbtara@mozilla.com
push dateWed, 03 Jun 2020 16:12:12 +0000
treeherdermozilla-central@ea34fd156c89 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjya, necko-reviewers
bugs1638652
milestone79.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1638652 - Only disable content conversion in the parent process if we're going to be delivering data to a content process. r=jya,necko-reviewers Differential Revision: https://phabricator.services.mozilla.com/D77399
netwerk/ipc/DocumentLoadListener.cpp
netwerk/ipc/DocumentLoadListener.h
--- a/netwerk/ipc/DocumentLoadListener.cpp
+++ b/netwerk/ipc/DocumentLoadListener.cpp
@@ -1153,22 +1153,24 @@ void DocumentLoadListener::SerializeRedi
   aArgs.loadStateLoadType() = mLoadStateLoadType;
   if (mSessionHistoryInfo) {
     aArgs.sessionHistoryInfo().emplace(
         mSessionHistoryInfo->mId, MakeUnique<mozilla::dom::SessionHistoryInfo>(
                                       *mSessionHistoryInfo->mInfo));
   }
 }
 
-bool DocumentLoadListener::MaybeTriggerProcessSwitch() {
+bool DocumentLoadListener::MaybeTriggerProcessSwitch(
+    bool* aWillSwitchToRemote) {
   MOZ_ASSERT(XRE_IsParentProcess());
   MOZ_DIAGNOSTIC_ASSERT(!mDoingProcessSwitch,
                         "Already in the middle of switching?");
   MOZ_DIAGNOSTIC_ASSERT(mChannel);
   MOZ_DIAGNOSTIC_ASSERT(mParentChannelListener);
+  MOZ_DIAGNOSTIC_ASSERT(aWillSwitchToRemote);
 
   LOG(("DocumentLoadListener MaybeTriggerProcessSwitch [this=%p]", this));
 
   // Get the BrowsingContext which will be switching processes.
   RefPtr<CanonicalBrowsingContext> browsingContext =
       mParentChannelListener->GetBrowsingContext();
   if (NS_WARN_IF(!browsingContext)) {
     LOG(("Process Switch Abort: no browsing context"));
@@ -1321,16 +1323,18 @@ bool DocumentLoadListener::MaybeTriggerP
          NS_ConvertUTF16toUTF8(remoteType).get()));
     return false;
   }
   if (NS_WARN_IF(remoteType.IsEmpty())) {
     LOG(("Process Switch Abort: non-remote target process"));
     return false;
   }
 
+  *aWillSwitchToRemote = !remoteType.IsEmpty();
+
   LOG(("Process Switch: Changing Remoteness from '%s' to '%s'",
        NS_ConvertUTF16toUTF8(currentRemoteType).get(),
        NS_ConvertUTF16toUTF8(remoteType).get()));
 
   // XXX: This is super hacky, and we should be able to do something better.
   static uint64_t sNextCrossProcessRedirectIdentifier = 0;
   mCrossProcessRedirectIdentifier = ++sNextCrossProcessRedirectIdentifier;
   mDoingProcessSwitch = true;
@@ -1614,38 +1618,48 @@ DocumentLoadListener::OnStartRequest(nsI
   } else {
     // This can be called multiple time if we have a multipart
     // decoder. Since we've already added the reqest to
     // mStreamListenerFunctions, we don't need to do anything else.
     return NS_OK;
   }
   mInitiatedRedirectToRealChannel = true;
 
+  // Determine if a new process needs to be spawned. If it does, this will
+  // trigger a cross process switch, and we should hold off on redirecting to
+  // the real channel.
+  bool willBeRemote = false;
+  if (status == NS_BINDING_ABORTED ||
+      !MaybeTriggerProcessSwitch(&willBeRemote)) {
+    TriggerRedirectToRealChannel();
+
+    // If we're not switching, then check if we're currently remote.
+    if (GetBrowsingContext() && GetBrowsingContext()->GetContentParent()) {
+      willBeRemote = true;
+    }
+  }
+
+  // If we're going to be delivering this channel to a remote content
+  // process, then we want to install any required content conversions
+  // in the content process.
   // The caller of this OnStartRequest will install a conversion
   // helper after we return if we haven't disabled conversion. Normally
   // HttpChannelParent::OnStartRequest would disable conversion, but we're
   // defering calling that until later. Manually disable it now to prevent the
   // converter from being installed (since we want the child to do it), and
   // also save the value so that when we do call
   // HttpChannelParent::OnStartRequest, we can have the value as it originally
   // was.
   if (httpChannel) {
     Unused << httpChannel->GetApplyConversion(&mOldApplyConversion);
-    httpChannel->SetApplyConversion(false);
+    if (willBeRemote) {
+      httpChannel->SetApplyConversion(false);
+    }
   }
 
-  // Determine if a new process needs to be spawned. If it does, this will
-  // trigger a cross process switch, and we should hold off on redirecting to
-  // the real channel.
-  if (status != NS_BINDING_ABORTED && MaybeTriggerProcessSwitch()) {
-    return NS_OK;
-  }
-
-  TriggerRedirectToRealChannel();
-
   return NS_OK;
 }
 
 NS_IMETHODIMP
 DocumentLoadListener::OnStopRequest(nsIRequest* aRequest,
                                     nsresult aStatusCode) {
   LOG(("DocumentLoadListener OnStopRequest [this=%p]", this));
   mStreamListenerFunctions.AppendElement(StreamListenerFunction{
--- a/netwerk/ipc/DocumentLoadListener.h
+++ b/netwerk/ipc/DocumentLoadListener.h
@@ -240,17 +240,19 @@ class DocumentLoadListener : public nsII
   // This redirects the ParentChannelListener to forward any future
   // messages to the new channel, manually forwards any being held
   // by us, and resumes the underlying source channel.
   void FinishReplacementChannelSetup(nsresult aResult);
 
   // Called from `OnStartRequest` to make the decision about whether or not to
   // change process. This method will return `nullptr` if the current target
   // process is appropriate.
-  bool MaybeTriggerProcessSwitch();
+  // aWillSwitchToRemote is set to true if we initiate a process switch,
+  // and that the new remote type will be something other than NOT_REMOTE
+  bool MaybeTriggerProcessSwitch(bool* aWillSwitchToRemote);
 
   // A helper for TriggerRedirectToRealChannel that abstracts over
   // the same-process and cross-process switch cases and returns
   // a single promise to wait on.
   RefPtr<PDocumentChannelParent::RedirectToRealChannelPromise>
   RedirectToRealChannel(uint32_t aRedirectFlags, uint32_t aLoadFlags,
                         const Maybe<uint64_t>& aDestinationProcess,
                         nsTArray<ParentEndpoint>&& aStreamFilterEndpoints);