Bug 1407384 - P3: Handle the case if the channel is suspended or canceled by "http-on-examine-merged-response" observer r=mayhemer

1. This patch somehow sets a "breakpoint" in ProcessPartialContent() and ProcessNotModified() to really stop doing things after ProcessPartialContent() and ProcessNotModified(), when the channel is suspended. 2. Add a test for this. Differential Revision: https://phabricator.services.mozilla.com/D13277 --HG-- extra : moz-landing-system : lando
2019-01-15 16:17:48 +00:00 · 2019-01-15 16:17:48 +00:00 · 8711ee817e
--- a/netwerk/protocol/http/nsHttpChannel.cpp
+++ b/netwerk/protocol/http/nsHttpChannel.cpp
@ -2460,9 +2460,6 @@ nsresult nsHttpChannel::ContinueProcessResponse2(nsresult rv) {

  uint32_t httpStatus = mResponseHead->Status();

-  bool successfulReval = false;
-  bool partialContentUsed = false;
-
  // handle different server response categories.  Note that we handle
  // caching or not caching of error pages in
  // nsHttpResponseHead::MustValidate; if you change this switch, update that
@ -2486,10 +2483,16 @@ nsresult nsHttpChannel::ContinueProcessResponse2(nsresult rv) {
      break;
    case 206:
      if (mCachedContentIsPartial) {  // an internal byte range request...
-        rv = ProcessPartialContent();
-        if (NS_SUCCEEDED(rv)) {
-          partialContentUsed = true;
+        auto func = [](auto *self, nsresult aRv) {
+          return self->ContinueProcessResponseAfterPartialContent(aRv);
+        };
+        rv = ProcessPartialContent(func);
+        // Directly call ContinueProcessResponseAfterPartialContent if channel
+        // is not suspended or ProcessPartialContent throws.
+        if (!mSuspendCount || NS_FAILED(rv)) {
+          return ContinueProcessResponseAfterPartialContent(rv);
        }
+        return NS_OK;
      } else {
        mCacheInputStream.CloseAndRelease();
        rv = ProcessNormal();
@ -2524,29 +2527,16 @@ nsresult nsHttpChannel::ContinueProcessResponse2(nsresult rv) {
      break;
    case 304:
      if (!ShouldBypassProcessNotModified()) {
-        rv = ProcessNotModified();
-        if (NS_SUCCEEDED(rv)) {
-          successfulReval = true;
-          break;
-        }
-
-        LOG(("ProcessNotModified failed [rv=%" PRIx32 "]\n",
-             static_cast<uint32_t>(rv)));
-
-        // We cannot read from the cache entry, it might be in an
-        // incosistent state.  Doom it and redirect the channel
-        // to the same URI to reload from the network.
-        mCacheInputStream.CloseAndRelease();
-        if (mCacheEntry) {
-          mCacheEntry->AsyncDoom(nullptr);
-          mCacheEntry = nullptr;
-        }
-
-        rv = StartRedirectChannelToURI(mURI,
-                                       nsIChannelEventSink::REDIRECT_INTERNAL);
-        if (NS_SUCCEEDED(rv)) {
-          return NS_OK;
+        auto func = [](auto *self, nsresult aRv) {
+          return self->ContinueProcessResponseAfterNotModified(aRv);
+        };
+        rv = ProcessNotModified(func);
+        // Directly call ContinueProcessResponseAfterNotModified if channel
+        // is not suspended or ProcessNotModified throws.
+        if (!mSuspendCount || NS_FAILED(rv)) {
+          return ContinueProcessResponseAfterNotModified(rv);
        }
+        return NS_OK;
      }

      // Don't cache uninformative 304
@ -2617,9 +2607,69 @@ nsresult nsHttpChannel::ContinueProcessResponse2(nsresult rv) {
      break;
  }

+  UpdateCacheDisposition(false, false);
+  return rv;
+}
+
+nsresult nsHttpChannel::ContinueProcessResponseAfterPartialContent(
+    nsresult aRv) {
+  LOG(
+      ("nsHttpChannel::ContinueProcessResponseAfterPartialContent "
+       "[this=%p, rv=%" PRIx32 "]",
+       this, static_cast<uint32_t>(aRv)));
+
+  UpdateCacheDisposition(false, NS_SUCCEEDED(aRv));
+  return aRv;
+}
+
+nsresult nsHttpChannel::ContinueProcessResponseAfterNotModified(nsresult aRv) {
+  LOG(
+      ("nsHttpChannel::ContinueProcessResponseAfterNotModified "
+       "[this=%p, rv=%" PRIx32 "]",
+       this, static_cast<uint32_t>(aRv)));
+
+  if (NS_SUCCEEDED(aRv)) {
+    mTransactionReplaced = true;
+    UpdateCacheDisposition(true, false);
+    return NS_OK;
+  }
+
+  LOG(("ProcessNotModified failed [rv=%" PRIx32 "]\n",
+       static_cast<uint32_t>(aRv)));
+
+  // We cannot read from the cache entry, it might be in an
+  // incosistent state.  Doom it and redirect the channel
+  // to the same URI to reload from the network.
+  mCacheInputStream.CloseAndRelease();
+  if (mCacheEntry) {
+    mCacheEntry->AsyncDoom(nullptr);
+    mCacheEntry = nullptr;
+  }
+
+  nsresult rv =
+      StartRedirectChannelToURI(mURI, nsIChannelEventSink::REDIRECT_INTERNAL);
+  if (NS_SUCCEEDED(rv)) {
+    return NS_OK;
+  }
+
+  // Don't cache uninformative 304
+  if (mCustomConditionalRequest) {
+    CloseCacheEntry(false);
+  }
+
+  if (ShouldBypassProcessNotModified() || NS_FAILED(rv)) {
+    rv = ProcessNormal();
+  }
+
+  UpdateCacheDisposition(false, false);
+  return rv;
+}
+
+void nsHttpChannel::UpdateCacheDisposition(bool aSuccessfulReval,
+                                           bool aPartialContentUsed) {
  if (mRaceDelay && !mRaceCacheWithNetwork &&
      (mCachedContentIsPartial || mDidReval)) {
-    if (successfulReval || partialContentUsed) {
+    if (aSuccessfulReval || aPartialContentUsed) {
      AccumulateCategorical(
          Telemetry::LABELS_NETWORK_RACE_CACHE_VALIDATION::CachedContentUsed);
    } else {
@ -2632,7 +2682,7 @@ nsresult nsHttpChannel::ContinueProcessResponse2(nsresult rv) {
    CacheDisposition cacheDisposition;
    if (!mDidReval) {
      cacheDisposition = kCacheMissed;
-    } else if (successfulReval) {
+    } else if (aSuccessfulReval) {
      cacheDisposition = kCacheHitViaReval;
    } else {
      cacheDisposition = kCacheMissedViaReval;
@ -2656,7 +2706,6 @@ nsresult nsHttpChannel::ContinueProcessResponse2(nsresult rv) {
      Telemetry::Accumulate(Telemetry::HTTP_09_INFO, v09Info);
    }
  }
-  return rv;
 }

 nsresult nsHttpChannel::ContinueProcessResponse3(nsresult rv) {
@ -3267,7 +3316,9 @@ void nsHttpChannel::UntieByteRangeRequest() {
  MOZ_ASSERT(NS_SUCCEEDED(rv));
 }

-nsresult nsHttpChannel::ProcessPartialContent() {
+nsresult nsHttpChannel::ProcessPartialContent(
+    const std::function<nsresult(nsHttpChannel *, nsresult)>
+        &aContinueProcessResponseFunc) {
  // ok, we've just received a 206
  //
  // we need to stream whatever data is in the cache out first, and then
@ -3367,15 +3418,16 @@ nsresult nsHttpChannel::ProcessPartialContent() {
    // to prevent duplicate OnStartRequest call on the target listener
    // in case this channel is canceled before it gets its OnStartRequest
    // from the http transaction.
-
-    // Now we continue reading the network response.
-  } else {
-    // the cached content is valid, although incomplete.
-    mCachedContentIsValid = true;
-    rv = ReadFromCache(false);
+    return rv;
  }

-  return rv;
+  // Now we continue reading the network response.
+  // the cached content is valid, although incomplete.
+  mCachedContentIsValid = true;
+  return CallOrWaitForResume([aContinueProcessResponseFunc](auto *self) {
+    nsresult rv = self->ReadFromCache(false);
+    return aContinueProcessResponseFunc(self, rv);
+  });
 }

 nsresult nsHttpChannel::OnDoneReadingPartialCacheEntry(bool *streamDone) {
@ -3441,7 +3493,9 @@ bool nsHttpChannel::ShouldBypassProcessNotModified() {
  return false;
 }

-nsresult nsHttpChannel::ProcessNotModified() {
+nsresult nsHttpChannel::ProcessNotModified(
+    const std::function<nsresult(nsHttpChannel *, nsresult)>
+        &aContinueProcessResponseFunc) {
  nsresult rv;

  LOG(("nsHttpChannel::ProcessNotModified [this=%p]\n", this));
@ -3511,11 +3565,10 @@ nsresult nsHttpChannel::ProcessNotModified() {
  rv = mCacheEntry->SetValid();
  if (NS_FAILED(rv)) return rv;

-  rv = ReadFromCache(false);
-  if (NS_FAILED(rv)) return rv;
-
-  mTransactionReplaced = true;
-  return NS_OK;
+  return CallOrWaitForResume([aContinueProcessResponseFunc](auto *self) {
+    nsresult rv = self->ReadFromCache(false);
+    return aContinueProcessResponseFunc(self, rv);
+  });
 }

 nsresult nsHttpChannel::ProcessFallback(bool *waitingForRedirectCallback) {
--- a/netwerk/protocol/http/nsHttpChannel.h
+++ b/netwerk/protocol/http/nsHttpChannel.h
@ -337,12 +337,17 @@ class nsHttpChannel final : public HttpBaseChannel,
  void AsyncContinueProcessResponse();
  MOZ_MUST_USE nsresult ContinueProcessResponse1();
  MOZ_MUST_USE nsresult ContinueProcessResponse2(nsresult);
+  void UpdateCacheDisposition(bool aSuccessfulReval, bool aPartialContentUsed);
  MOZ_MUST_USE nsresult ContinueProcessResponse3(nsresult);
  MOZ_MUST_USE nsresult ProcessNormal();
  MOZ_MUST_USE nsresult ContinueProcessNormal(nsresult);
  void ProcessAltService();
  bool ShouldBypassProcessNotModified();
-  MOZ_MUST_USE nsresult ProcessNotModified();
+  MOZ_MUST_USE nsresult ProcessNotModified(
+      const std::function<nsresult(nsHttpChannel *, nsresult)>
+          &aContinueProcessResponseFunc);
+  MOZ_MUST_USE nsresult ContinueProcessResponseAfterNotModified(nsresult aRv);
+
  MOZ_MUST_USE nsresult AsyncProcessRedirection(uint32_t httpStatus);
  MOZ_MUST_USE nsresult ContinueProcessRedirection(nsresult);
  MOZ_MUST_USE nsresult ContinueProcessRedirectionAfterFallback(nsresult);
@ -414,7 +419,11 @@ class nsHttpChannel final : public HttpBaseChannel,
  void ClearBogusContentEncodingIfNeeded();

  // byte range request specific methods
-  MOZ_MUST_USE nsresult ProcessPartialContent();
+  MOZ_MUST_USE nsresult ProcessPartialContent(
+      const std::function<nsresult(nsHttpChannel *, nsresult)>
+          &aContinueProcessResponseFunc);
+  MOZ_MUST_USE nsresult
+  ContinueProcessResponseAfterPartialContent(nsresult aRv);
  MOZ_MUST_USE nsresult OnDoneReadingPartialCacheEntry(bool *streamDone);

  MOZ_MUST_USE nsresult
--- a/netwerk/test/unit/test_suspend_channel_on_examine_merged_response.js
+++ b/netwerk/test/unit/test_suspend_channel_on_examine_merged_response.js
@ -0,0 +1,193 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+// This file tests async handling of a channel suspended in
+// notifying http-on-examine-merged-response observers.
+// Note that this test is developed based on test_bug482601.js.
+
+
+ChromeUtils.import("resource://testing-common/httpd.js");
+ChromeUtils.import("resource://gre/modules/NetUtil.jsm");
+
+var httpserv = null;
+var test_nr = 0;
+var buffer = "";
+var observerCalled = false;
+var channelResumed = false;
+
+var observer = {
+  QueryInterface: function (aIID) {
+    if (aIID.equals(Ci.nsISupports) ||
+        aIID.equals(Ci.nsIObserver))
+      return this;
+    throw Cr.NS_ERROR_NO_INTERFACE;
+  },
+
+  observe: function(subject, topic, data) {
+    if (topic === "http-on-examine-merged-response" &&
+        subject instanceof Ci.nsIHttpChannel) {
+      var chan = subject.QueryInterface(Ci.nsIHttpChannel);
+      executeSoon(() => {
+        Assert.equal(channelResumed,false);
+        channelResumed = true;
+        chan.resume();
+      });
+      Assert.equal(observerCalled,false);
+      observerCalled = true;
+      chan.suspend();
+    }
+  }
+};
+
+var listener = {
+  onStartRequest: function (request, ctx) {
+    buffer = "";
+  },
+
+  onDataAvailable: function (request, ctx, stream, offset, count) {
+    buffer = buffer.concat(read_stream(stream, count));
+  },
+
+  onStopRequest: function (request, ctx, status) {
+    Assert.equal(status, Cr.NS_OK);
+    Assert.equal(buffer, "0123456789");
+    Assert.equal(channelResumed, true);
+    Assert.equal(observerCalled, true);
+    test_nr++;
+    do_timeout(0, do_test);
+  }
+};
+
+function run_test() {
+  httpserv = new HttpServer();
+  httpserv.registerPathHandler("/path/partial", path_partial);
+  httpserv.registerPathHandler("/path/cached", path_cached);
+  httpserv.start(-1);
+
+  var obs = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
+  obs.addObserver(observer, "http-on-examine-merged-response");
+
+  do_timeout(0, do_test);
+  do_test_pending();
+}
+
+function do_test() {
+  if (test_nr < tests.length) {
+    tests[test_nr]();
+  }
+  else {
+    var obs = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
+    obs.removeObserver(observer, "http-on-examine-merged-response");
+    httpserv.stop(do_test_finished);
+  }
+}
+
+var tests = [test_partial,
+             test_cached];
+
+function makeChan(url) {
+  return NetUtil.newChannel({uri: url, loadUsingSystemPrincipal: true})
+                .QueryInterface(Ci.nsIHttpChannel);
+}
+
+function storeCache(aCacheEntry, aResponseHeads, aContent) {
+  aCacheEntry.setMetaDataElement("request-method", "GET");
+  aCacheEntry.setMetaDataElement("response-head", aResponseHeads);
+  aCacheEntry.setMetaDataElement("charset", "ISO-8859-1");
+
+  var oStream = aCacheEntry.openOutputStream(0, aContent.length);
+  var written = oStream.write(aContent, aContent.length);
+  if (written != aContent.length) {
+    do_throw("oStream.write has not written all data!\n" +
+             "  Expected: " + written  + "\n" +
+             "  Actual: " + aContent.length + "\n");
+  }
+  oStream.close();
+  aCacheEntry.close();
+}
+
+function test_partial() {
+  observerCalled = false;
+  channelResumed = false;
+  asyncOpenCacheEntry("http://localhost:" + httpserv.identity.primaryPort +
+                      "/path/partial",
+                      "disk", Ci.nsICacheStorage.OPEN_NORMALLY, null,
+                      test_partial2);
+}
+
+function test_partial2(status, entry) {
+  Assert.equal(status, Cr.NS_OK);
+  storeCache(entry,
+             "HTTP/1.1 200 OK\r\n" +
+             "Date: Thu, 1 Jan 2009 00:00:00 GMT\r\n" +
+             "Server: httpd.js\r\n" +
+             "Last-Modified: Thu, 1 Jan 2009 00:00:00 GMT\r\n" +
+             "Accept-Ranges: bytes\r\n" +
+             "Content-Length: 10\r\n" +
+             "Content-Type: text/plain\r\n",
+             "0123");
+
+  observers_called = "";
+
+  var chan = makeChan("http://localhost:" + httpserv.identity.primaryPort +
+                      "/path/partial");
+  chan.asyncOpen2(listener);
+}
+
+function test_cached() {
+  observerCalled = false;
+  channelResumed = false;
+  asyncOpenCacheEntry("http://localhost:" + httpserv.identity.primaryPort +
+                      "/path/cached",
+                      "disk", Ci.nsICacheStorage.OPEN_NORMALLY, null,
+                      test_cached2);
+}
+
+function test_cached2(status, entry) {
+  Assert.equal(status, Cr.NS_OK);
+  storeCache(entry,
+             "HTTP/1.1 200 OK\r\n" +
+             "Date: Thu, 1 Jan 2009 00:00:00 GMT\r\n" +
+             "Server: httpd.js\r\n" +
+             "Last-Modified: Thu, 1 Jan 2009 00:00:00 GMT\r\n" +
+             "Accept-Ranges: bytes\r\n" +
+             "Content-Length: 10\r\n" +
+             "Content-Type: text/plain\r\n",
+             "0123456789");
+
+  observers_called = "";
+
+  var chan = makeChan("http://localhost:" + httpserv.identity.primaryPort +
+                      "/path/cached");
+  chan.loadFlags = Ci.nsIRequest.VALIDATE_ALWAYS;
+  chan.asyncOpen2(listener);
+}
+
+// PATHS
+
+// /path/partial
+function path_partial(metadata, response) {
+  Assert.ok(metadata.hasHeader("If-Range"));
+  Assert.equal(metadata.getHeader("If-Range"),
+               "Thu, 1 Jan 2009 00:00:00 GMT");
+  Assert.ok(metadata.hasHeader("Range"));
+  Assert.equal(metadata.getHeader("Range"), "bytes=4-");
+
+  response.setStatusLine(metadata.httpVersion, 206, "Partial Content");
+  response.setHeader("Content-Range", "bytes 4-9/10", false);
+  response.setHeader("Content-Type", "text/plain", false);
+  response.setHeader("Last-Modified", "Thu, 1 Jan 2009 00:00:00 GMT");
+
+  var body = "456789";
+  response.bodyOutputStream.write(body, body.length);
+}
+
+// /path/cached
+function path_cached(metadata, response) {
+  Assert.ok(metadata.hasHeader("If-Modified-Since"));
+  Assert.equal(metadata.getHeader("If-Modified-Since"),
+               "Thu, 1 Jan 2009 00:00:00 GMT");
+
+  response.setStatusLine(metadata.httpVersion, 304, "Not Modified");
+}
--- a/netwerk/test/unit/xpcshell.ini
+++ b/netwerk/test/unit/xpcshell.ini
@ -426,3 +426,4 @@ skip-if = os == "android"
 [test_network_connectivity_service.js]
 skip-if = os == "android" # DNSv6 issues on android
 [test_suspend_channel_on_authRetry.js]
+[test_suspend_channel_on_examine_merged_response.js]