Bug 1670530 - Don't process multipart content when doing a view-source load, r=mattwoodrow,necko-reviewers,valentin

Before switching to using DocumentChannel to process multipart requests,
multipart documents loaded as a view-source load would be displayed their
plain-text data, as the multipart processing would be after the view-source
channel had wrapped the channel, and replaced the content type with
"application/x-view-source".

This change restores that behaviour, by preventing parent process multipart
processing for wrapped channels like `view-source` loads. This also allowed
removing the replaceRequest option on nsViewSourceChannel, as it was no longer
necessary, and required introducing a mechanism to get the inner http channel
for process switching.

The crash in Bug 1670530 was caused by a bad interaction between the view-source
replaceChannel logic, and the parent/content process switching logic, which
could lead to the load in the content process being initialized in a broken
state after a process switch, due to accidentally acting on a wrapped
view-source channel when an unwrapped one was expected. This patch also fixes
that issue, by removing the replaceRequest logic which caused it in the first
place.

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

docshell/test/browser/browser.ini

@@ -173,3 +173,7 @@ skip-if = (os == 'linux' && bits == 64 && os_version == '18.04') || (os == "win"
 [browser_fall_back_to_https.js]
 skip-if = (os == 'mac')
 [browser_badCertDomainFixup.js]
+[browser_viewsource_chrome_to_content.js]
+[browser_viewsource_multipart.js]
+support-files =
+  file_basic_multipart.sjs

docshell/test/browser/browser_viewsource_chrome_to_content.js

@@ -0,0 +1,20 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+"use strict";
+
+const TEST_PATH = getRootDirectory(gTestPath).replace(
+  "chrome://mochitests/content",
+  "http://example.com"
+);
+const TEST_URI = `view-source:${TEST_PATH}dummy_page.html`;
+
+add_task(async function chrome_to_content_view_source() {
+  await BrowserTestUtils.withNewTab("about:mozilla", async browser => {
+    is(browser.documentURI.spec, "about:mozilla");
+
+    // This process switch would previously crash in debug builds due to assertion failures.
+    BrowserTestUtils.loadURI(browser, TEST_URI);
+    await BrowserTestUtils.browserLoaded(browser);
+    is(browser.documentURI.spec, TEST_URI);
+  });
+});

docshell/test/browser/browser_viewsource_multipart.js

@@ -0,0 +1,44 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+"use strict";
+
+const TEST_PATH = getRootDirectory(gTestPath).replace(
+  "chrome://mochitests/content",
+  "http://example.com"
+);
+const MULTIPART_URI = `${TEST_PATH}file_basic_multipart.sjs`;
+
+add_task(async function viewsource_multipart_uri() {
+  await BrowserTestUtils.withNewTab("about:blank", async browser => {
+    BrowserTestUtils.loadURI(browser, MULTIPART_URI);
+    await BrowserTestUtils.browserLoaded(browser);
+    is(browser.currentURI.spec, MULTIPART_URI);
+
+    // Continue probing the URL until we find the h1 we're expecting. This
+    // should handle cases where we somehow beat the second document having
+    // loaded.
+    await TestUtils.waitForCondition(async () => {
+      let value = await SpecialPowers.spawn(browser, [], async () => {
+        let headers = content.document.querySelectorAll("h1");
+        is(headers.length, 1, "only one h1 should be present");
+        return headers[0].textContent;
+      });
+
+      ok(value == "First" || value == "Second", "some other value was found?");
+      return value == "Second";
+    });
+
+    // Load a view-source version of the page, which should show the full
+    // content, not handling multipart.
+    BrowserTestUtils.loadURI(browser, `view-source:${MULTIPART_URI}`);
+    await BrowserTestUtils.browserLoaded(browser);
+
+    let viewSourceContent = await SpecialPowers.spawn(browser, [], async () => {
+      return content.document.body.textContent;
+    });
+
+    ok(viewSourceContent.includes("<h1>First</h1>"), "first header");
+    ok(viewSourceContent.includes("<h1>Second</h1>"), "second header");
+    ok(viewSourceContent.includes("BOUNDARY"), "boundary");
+  });
+});

docshell/test/browser/file_basic_multipart.sjs

@@ -0,0 +1,24 @@
+"use strict";
+
+function handleRequest(request, response) {
+  response.setHeader(
+    "Content-Type",
+    "multipart/x-mixed-replace;boundary=BOUNDARY",
+    false
+  );
+  response.setStatusLine(request.httpVersion, 200, "OK");
+
+  response.write(`--BOUNDARY
+Content-Type: text/html
+
+<h1>First</h1>
+Will be replaced
+--BOUNDARY
+Content-Type: text/html
+
+<h1>Second</h1>
+This will stick around
+--BOUNDARY
+--BOUNDARY--
+`);
+}

netwerk/ipc/DocumentLoadListener.cpp

@@ -539,15 +539,6 @@ auto DocumentLoadListener::Open(nsDocShellLoadState* aLoadState,
     }
   }
 
-  // nsViewSourceChannel normally replaces the nsIRequest passed to
-  // OnStart/StopRequest with itself. We don't need this, and instead
-  // we want the original request so that we get different ones for
-  // each part of a multipart channel.
-  nsCOMPtr<nsIViewSourceChannel> viewSourceChannel;
-  if (aPid && (viewSourceChannel = do_QueryInterface(mChannel))) {
-    viewSourceChannel->SetReplaceRequest(false);
-  }
-
   // Setup a ClientChannelHelper to watch for redirects, and copy
   // across any serviceworker related data between channels as needed.
   AddClientChannelHelperInParent(mChannel, std::move(aInfo));
@@ -1795,8 +1786,12 @@ DocumentLoadListener::RedirectToRealChannel(
   nsCOMPtr<nsIRedirectChannelRegistrar> registrar =
       RedirectChannelRegistrar::GetOrCreate();
   MOZ_ASSERT(registrar);
+  nsCOMPtr<nsIChannel> chan = mChannel;
+  if (nsCOMPtr<nsIViewSourceChannel> vsc = do_QueryInterface(chan)) {
+    chan = vsc->GetInnerChannel();
+  }
   mRedirectChannelId = nsContentUtils::GenerateLoadIdentifier();
-  MOZ_ALWAYS_SUCCEEDS(registrar->RegisterChannel(mChannel, mRedirectChannelId));
+  MOZ_ALWAYS_SUCCEEDS(registrar->RegisterChannel(chan, mRedirectChannelId));
 
   if (aDestinationProcess) {
     if (!*aDestinationProcess) {

netwerk/ipc/ParentChannelWrapper.cpp

@@ -21,7 +21,16 @@ void ParentChannelWrapper::Register(uint64_t aRegistrarId) {
   nsCOMPtr<nsIChannel> dummy;
   MOZ_ALWAYS_SUCCEEDS(
       NS_LinkRedirectChannels(aRegistrarId, this, getter_AddRefs(dummy)));
+
+#ifdef DEBUG
+  // The channel registered with the RedirectChannelRegistrar will be the inner
+  // channel when dealing with view-source loads.
+  if (nsCOMPtr<nsIViewSourceChannel> viewSource = do_QueryInterface(mChannel)) {
+    MOZ_ASSERT(dummy == viewSource->GetInnerChannel());
+  } else {
     MOZ_ASSERT(dummy == mChannel);
+  }
+#endif
 }
 
 ////////////////////////////////////////////////////////////////////////////////

netwerk/protocol/http/HttpChannelParent.cpp

@@ -55,6 +55,7 @@
 #include "nsThreadUtils.h"
 #include "nsQueryObject.h"
 #include "nsIMultiPartChannel.h"
+#include "nsIViewSourceChannel.h"
 
 using mozilla::BasePrincipal;
 using namespace mozilla::dom;
@@ -1141,6 +1142,9 @@ HttpChannelParent::OnStartRequest(nsIRequest* aRequest) {
       multiPartChannel->GetPartID(&partID);
       multiPartID = Some(partID);
       multiPartChannel->GetIsLastPart(&isLastPartOfMultiPart);
+    } else if (nsCOMPtr<nsIViewSourceChannel> viewSourceChannel =
+                   do_QueryInterface(aRequest)) {
+      chan = do_QueryObject(viewSourceChannel->GetInnerChannel());
     }
   }
   MOZ_ASSERT(multiPartID || !isMultiPart, "Changed multi-part state?");

netwerk/protocol/http/nsHttpChannel.cpp

@@ -1663,13 +1663,14 @@ nsresult nsHttpChannel::CallOnStartRequest() {
   // in the pipeline to handle the content and pass it along to our
   // original listener. nsUnknownDecoder doesn't support detecting this type,
   // so we only need to insert this using the response header's mime type.
-  // We only do this for document loads, since we might want to send parts
-  // to the external protocol handler without leaving the parent process.
+  //
+  // We only do this for unwrapped document loads, since we might want to send
+  // parts to the external protocol handler without leaving the parent process.
   bool mustRunStreamFilterInParent = false;
   nsCOMPtr<nsIParentChannel> parentChannel;
   NS_QueryNotificationCallbacks(this, parentChannel);
   RefPtr<DocumentLoadListener> docListener = do_QueryObject(parentChannel);
-  if (mResponseHead && docListener) {
+  if (mResponseHead && docListener && docListener->GetChannel() == this) {
     nsAutoCString contentType;
     mResponseHead->ContentType(contentType);
 

netwerk/protocol/viewsource/nsIViewSourceChannel.idl

@@ -35,12 +35,9 @@ interface nsIViewSourceChannel : nsIChannel
     [must_use] attribute nsIURI baseURI;
 
     /**
-     * If true (default), then replaces the nsIRequest* passed to
-     * nsIStreamListener callback functions with itself, so that
-     * nsIViewSourceChannel is available.
-     * Otherwise passes through the the nsIRequest* from the inner channel.
+     * Get the inner channel wrapped by this nsIViewSourceChannel.
      */
-    attribute boolean replaceRequest;
+    [notxpcom, nostdcall] nsIChannel getInnerChannel();
 };
 
 

netwerk/protocol/viewsource/nsViewSourceChannel.cpp

@@ -647,23 +647,13 @@ nsViewSourceChannel::SetBaseURI(nsIURI* aBaseURI) {
   return NS_OK;
 }
 
+nsIChannel* nsViewSourceChannel::GetInnerChannel() { return mChannel; }
+
 NS_IMETHODIMP
 nsViewSourceChannel::GetProtocolVersion(nsACString& aProtocolVersion) {
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
-NS_IMETHODIMP
-nsViewSourceChannel::GetReplaceRequest(bool* aReplaceRequest) {
-  *aReplaceRequest = mReplaceRequest;
-  return NS_OK;
-}
-
-NS_IMETHODIMP
-nsViewSourceChannel::SetReplaceRequest(bool aReplaceRequest) {
-  mReplaceRequest = aReplaceRequest;
-  return NS_OK;
-}
-
 // nsIRequestObserver methods
 NS_IMETHODIMP
 nsViewSourceChannel::OnStartRequest(nsIRequest* aRequest) {
@@ -680,10 +670,7 @@ nsViewSourceChannel::OnStartRequest(nsIRequest* aRequest) {
     Cancel(rv);
   }
 
-  if (mReplaceRequest) {
   return mListener->OnStartRequest(static_cast<nsIViewSourceChannel*>(this));
-  }
-  return mListener->OnStartRequest(aRequest);
 }
 
 NS_IMETHODIMP
@@ -698,13 +685,8 @@ nsViewSourceChannel::OnStopRequest(nsIRequest* aRequest, nsresult aStatus) {
     }
   }
 
-  nsresult rv;
-  if (mReplaceRequest) {
-    rv = mListener->OnStopRequest(static_cast<nsIViewSourceChannel*>(this),
-                                  aStatus);
-  } else {
-    rv = mListener->OnStopRequest(aRequest, aStatus);
-  }
+  nsresult rv = mListener->OnStopRequest(
+      static_cast<nsIViewSourceChannel*>(this), aStatus);
 
   ReleaseListeners();
 
@@ -717,12 +699,8 @@ nsViewSourceChannel::OnDataAvailable(nsIRequest* aRequest,
                                      nsIInputStream* aInputStream,
                                      uint64_t aSourceOffset, uint32_t aLength) {
   NS_ENSURE_TRUE(mListener, NS_ERROR_FAILURE);
-  if (mReplaceRequest) {
   return mListener->OnDataAvailable(static_cast<nsIViewSourceChannel*>(this),
                                     aInputStream, aSourceOffset, aLength);
-  }
-  return mListener->OnDataAvailable(aRequest, aInputStream, aSourceOffset,
-                                    aLength);
 }
 
 // nsIHttpChannel methods

netwerk/protocol/viewsource/nsViewSourceChannel.h

@@ -54,10 +54,7 @@ class nsViewSourceChannel final : public nsIViewSourceChannel,
 
   // nsViewSourceChannel methods:
   nsViewSourceChannel()
-      : mIsDocument(false),
-        mOpened(false),
-        mIsSrcdocChannel(false),
-        mReplaceRequest(true) {}
+      : mIsDocument(false), mOpened(false), mIsSrcdocChannel(false) {}
 
   [[nodiscard]] nsresult Init(nsIURI* uri, nsILoadInfo* aLoadInfo);
 
@@ -98,7 +95,6 @@ class nsViewSourceChannel final : public nsIViewSourceChannel,
   bool mIsDocument;  // keeps track of the LOAD_DOCUMENT_URI flag
   bool mOpened;
   bool mIsSrcdocChannel;
-  bool mReplaceRequest;
 };
 
 #endif /* nsViewSourceChannel_h___ */