Always use backpressure for fetch

Previously, backpressure would only be applied to fetch responses if the
Cache-Control: no-store header was set. This was to avoid blocking other
resource loads that might have to wait for the lock on the http disk
cache that was held by the resource load to which backpressure was being
applied.

As of https://chromium-review.googlesource.com/684615 the disk cache
supports multiple concurrent writers and so this protection is no longer
needed.

Remove support for the non-backpressure mode and always apply
backpressure.

The fact that the body is no longer implicitly read is visible
to tests that observe the result of the fetch() via a side
channel, specifically inspector and browser tests. Fix such tests
to explicitly read the body to completion.

Additionally, in order to satisfy the presubmit check, make
SharedMemoryDataConsumerHandle::Context::on_reader_detached_ a
OnceClosure.

The implementation already implicitly ran on_reader_detached_ at most
once, because is_on_reader_detached_valid_ would be set to false by
ResetOnReaderDetached immediately after PostTask was called. Now this
has been made explicit by using OnceClosure.

on_reader_detached_ is only reset on the writer_task_runner_
thread. When the closure is not run this is implemented by the logic in
ResetOnReaderDetached(), the same as before. When the closure is run the
behaviour has changed slightly: the OnceClosure is moved into the task
that is posted to the writer_task_runner_ thread, where it will be
destroyed automatically after running. There's no nead for a separate
reset in this case.

BUG=535793

Change-Id: I9d2b58e183b84640afc153ac99de08fdc2368240
Reviewed-on: https://chromium-review.googlesource.com/861695
Commit-Queue: Adam Rice <ricea@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530070}

chrome/test/data/password/password_fetch_submit.html

@@ -22,10 +22,11 @@ function send_fetch() {
           // Pretend like auth succeeded by hiding the login and signup forms.
           document.getElementById("testform").style.display = "none";
           document.getElementById("signup_testform").style.display = "none";
-          // Delay upload so that handler in PasswordAutofillAgent can be run
-          // first. This will happen immediately after JS execution ends, so
-          // this shouldn't introduce any timing dependent flakes.
-          setTimeout(delayedUpload, 0);
+          // Finish loading the body of the response and then give
+          // PasswordAutofillAgent a chance to run before upload. It will run as
+          // soon as JS execution ends, so this shouldn't introduce any timing
+          // dependent flakes.
+          response.arrayBuffer().then(() => setTimeout(delayedUpload, 0));
         }
       }
     )

content/renderer/loader/shared_memory_data_consumer_handle.cc

@@ -54,12 +54,12 @@ using Result = blink::WebDataConsumerHandle::Result;
 class SharedMemoryDataConsumerHandle::Context final
     : public base::RefCountedThreadSafe<Context> {
  public:
-  explicit Context(const base::Closure& on_reader_detached)
+  explicit Context(base::OnceClosure on_reader_detached)
       : result_(kOk),
         first_offset_(0),
         client_(nullptr),
         writer_task_runner_(base::ThreadTaskRunnerHandle::Get()),
-        on_reader_detached_(on_reader_detached),
+        on_reader_detached_(std::move(on_reader_detached)),
         is_on_reader_detached_valid_(!on_reader_detached_.is_null()),
         is_handle_active_(true),
         is_two_phase_read_in_progress_(false) {}
@@ -76,7 +76,9 @@ class SharedMemoryDataConsumerHandle::Context final
         // We post a task even in the writer thread in order to avoid a
         // reentrance problem as calling |on_reader_detached_| may manipulate
         // the context synchronously.
-        writer_task_runner_->PostTask(FROM_HERE, on_reader_detached_);
+        is_on_reader_detached_valid_ = false;
+        writer_task_runner_->PostTask(FROM_HERE,
+                                      std::move(on_reader_detached_));
       }
       Clear();
     }
@@ -250,7 +252,7 @@ class SharedMemoryDataConsumerHandle::Context final
   Client* client_;
   scoped_refptr<base::SingleThreadTaskRunner> notification_task_runner_;
   scoped_refptr<base::SingleThreadTaskRunner> writer_task_runner_;
-  base::Closure on_reader_detached_;
+  base::OnceClosure on_reader_detached_;
   // We need this boolean variable to remember if |on_reader_detached_| is
   // callable because we need to reset |on_reader_detached_| only on the writer
   // thread and hence |on_reader_detached_.is_null()| is untrustworthy on
@@ -263,10 +265,8 @@ class SharedMemoryDataConsumerHandle::Context final
 };
 
 SharedMemoryDataConsumerHandle::Writer::Writer(
-    const scoped_refptr<Context>& context,
-    BackpressureMode mode)
-    : context_(context), mode_(mode) {
-}
+    const scoped_refptr<Context>& context)
+    : context_(context) {}
 
 SharedMemoryDataConsumerHandle::Writer::~Writer() {
   Close();
@@ -290,14 +290,8 @@ void SharedMemoryDataConsumerHandle::Writer::AddData(
     }
 
     needs_notification = context_->IsEmpty();
-    std::unique_ptr<RequestPeer::ThreadSafeReceivedData> data_to_pass;
-    if (mode_ == kApplyBackpressure) {
-      data_to_pass =
-          std::make_unique<DelegateThreadSafeReceivedData>(std::move(data));
-    } else {
-      data_to_pass = std::make_unique<FixedReceivedData>(data.get());
-    }
-    context_->Push(std::move(data_to_pass));
+    context_->Push(
+        std::make_unique<DelegateThreadSafeReceivedData>(std::move(data)));
   }
 
   if (needs_notification) {
@@ -436,16 +430,14 @@ Result SharedMemoryDataConsumerHandle::ReaderImpl::EndRead(size_t read_size) {
 }
 
 SharedMemoryDataConsumerHandle::SharedMemoryDataConsumerHandle(
-    BackpressureMode mode,
     std::unique_ptr<Writer>* writer)
-    : SharedMemoryDataConsumerHandle(mode, base::Closure(), writer) {}
+    : SharedMemoryDataConsumerHandle(base::OnceClosure(), writer) {}
 
 SharedMemoryDataConsumerHandle::SharedMemoryDataConsumerHandle(
-    BackpressureMode mode,
-    const base::Closure& on_reader_detached,
+    base::OnceClosure on_reader_detached,
     std::unique_ptr<Writer>* writer)
-    : context_(new Context(on_reader_detached)) {
-  writer->reset(new Writer(context_, mode));
+    : context_(new Context(std::move(on_reader_detached))) {
+  writer->reset(new Writer(context_));
 }
 
 SharedMemoryDataConsumerHandle::~SharedMemoryDataConsumerHandle() {

content/renderer/loader/shared_memory_data_consumer_handle.h

@@ -25,14 +25,9 @@ class CONTENT_EXPORT SharedMemoryDataConsumerHandle final
   class Context;
 
  public:
-  enum BackpressureMode {
-    kApplyBackpressure,
-    kDoNotApplyBackpressure,
-  };
-
   class CONTENT_EXPORT Writer final {
    public:
-    Writer(const scoped_refptr<Context>& context, BackpressureMode mode);
+    explicit Writer(const scoped_refptr<Context>& context);
     ~Writer();
     // Note: Writer assumes |AddData| is not called in a client's didGetReadable
     // callback. There isn't such assumption for |Close| and |Fail|.
@@ -43,7 +38,6 @@ class CONTENT_EXPORT SharedMemoryDataConsumerHandle final
 
    private:
     scoped_refptr<Context> context_;
-    BackpressureMode mode_;
 
     DISALLOW_COPY_AND_ASSIGN(Writer);
   };
@@ -69,15 +63,13 @@ class CONTENT_EXPORT SharedMemoryDataConsumerHandle final
 
   // Creates a handle and a writer associated with the handle. The created
   // writer should be used on the calling thread.
-  SharedMemoryDataConsumerHandle(BackpressureMode mode,
-                                 std::unique_ptr<Writer>* writer);
+  explicit SharedMemoryDataConsumerHandle(std::unique_ptr<Writer>* writer);
   // |on_reader_detached| will be called aynchronously on the calling thread
   // when the reader (including the handle) is detached (i.e. both the handle
   // and the reader are destructed). The callback will be reset in the internal
   // context when the writer is detached, i.e. |Close| or |Fail| is called,
   // and the callback will never be called.
-  SharedMemoryDataConsumerHandle(BackpressureMode mode,
-                                 const base::Closure& on_reader_detached,
+  SharedMemoryDataConsumerHandle(base::OnceClosure on_reader_detached,
                                  std::unique_ptr<Writer>* writer);
   ~SharedMemoryDataConsumerHandle() override;
 

content/renderer/loader/web_url_loader_impl.cc

@@ -849,15 +849,8 @@ void WebURLLoaderImpl::Context::OnReceivedResponse(
   }
 
   if (use_stream_on_response_) {
-    SharedMemoryDataConsumerHandle::BackpressureMode mode =
-        SharedMemoryDataConsumerHandle::kDoNotApplyBackpressure;
-    if (info.headers &&
-        info.headers->HasHeaderValue("Cache-Control", "no-store")) {
-      mode = SharedMemoryDataConsumerHandle::kApplyBackpressure;
-    }
-
     auto read_handle = std::make_unique<SharedMemoryDataConsumerHandle>(
-        mode, base::Bind(&Context::CancelBodyStreaming, this),
+        base::BindOnce(&Context::CancelBodyStreaming, this),
         &body_stream_writer_);
 
     // Here |body_stream_writer_| has an indirect reference to |this| and that

third_party/WebKit/LayoutTests/http/tests/devtools/extensions/resources/extensions-network.html

@@ -10,7 +10,8 @@ p { font-family: 'test'; }
 <img src="missing-image.png">
 <script>
 function doXHR() {
-  return fetch('extensions-network.html');
+  return fetch('extensions-network.html')
+    .then(response => response.arrayBuffer());
 }
 </script>
 <p>some text.</p>

third_party/WebKit/LayoutTests/http/tests/inspector-protocol/audits/get-encoded-response.js

@@ -11,7 +11,8 @@
   async function logResponse(url, encoding, quality, sizeOnly) {
     testRunner.log(`\nResults for ${url} encoding=${encoding} q=${quality} sizeOnly=${sizeOnly}`);
 
-    session.evaluate(`fetch(${JSON.stringify(url)})`);
+    session.evaluate(`fetch(${JSON.stringify(url)})
+                        .then(response => response.arrayBuffer())`);
 
     const requestId = (await dp.Network.onceResponseReceived()).params.requestId;
     const result = (await dp.Audits.getEncodedResponse({requestId, encoding, quality, sizeOnly})).result;

third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network-data-length.js

@@ -10,7 +10,9 @@
   var pendingRequests = 0;
 
   function sendRequest(url) {
-    dp.Runtime.evaluate({expression: `fetch('${url}')`});
+    dp.Runtime.evaluate({expression: `
+      fetch('${url}')
+        .then(response => response.arrayBuffer())`});
     pendingRequests++;
   }
 

third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/get-response-body.js

@@ -5,7 +5,8 @@
   await dp.Network.enable();
 
   async function logResponseBody(url) {
-    session.evaluate(`fetch(${JSON.stringify(url)});`);
+    session.evaluate(`fetch(${JSON.stringify(url)})
+                        .then(response => response.arrayBuffer())`);
 
     var requestWillBeSent = (await dp.Network.onceRequestWillBeSent()).params;
     testRunner.log(`Request for ${requestWillBeSent.request.url}`);

third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/override-referer-with-downgrade.js

@@ -5,7 +5,8 @@
 
   dp.Network.enable();
   await dp.Network.setExtraHTTPHeaders({headers: {'ReFeReR': 'https://127.0.0.1:8000/'}});
-  session.evaluate(`fetch('${testRunner.url('./resources/echo-headers.php?headers=HTTP_REFERER')}')`);
+  session.evaluate(`fetch('${testRunner.url('./resources/echo-headers.php?headers=HTTP_REFERER')}')
+                      .then(response => response.arrayBuffer())`);
 
   var response = (await dp.Network.onceLoadingFinished()).params;
   var content = await dp.Network.getResponseBody({requestId: response.requestId});

third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/request-interception-on-both-redirect-rewrite.js

@@ -18,7 +18,8 @@
   ]});
   testRunner.log('Request interception patterns sent.');
 
-  session.evaluate(`fetch('${testRunner.url('../resources/redirect1.php')}')`);
+  session.evaluate(`fetch('${testRunner.url('../resources/redirect1.php')}')
+                      .then(response => response.arrayBuffer())`);
 
   // Should be redirect1.php as request.
   var interceptionEvent = await waitForInterceptionEvent();

third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/request-interception-on-response-redirect.js

@@ -65,7 +65,8 @@
   await new Promise(resolve => {
     session.protocol.Network.onResponseReceived(resolve);
     session.evaluate(`
-      fetch('${testRunner.url('../resources/ping-redirect.php')}');
+      fetch('${testRunner.url('../resources/ping-redirect.php')}')
+        .then(response => response.arrayBuffer());
     `);
   });
 

third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/request-interception-patterns.js

@@ -71,11 +71,14 @@
    * @return {!Promise}
    */
   async function testUrls() {
-    session.evaluate(`fetch('../network/resources/small-test-1.txt')`);
+    session.evaluate(`fetch('../network/resources/small-test-1.txt')
+                        .then(response => response.arrayBuffer())`);
     await new Promise(resolve => responseWasReceivedCallback = resolve);
-    session.evaluate(`fetch('../network/resources/small-test-2.txt')`);
+    session.evaluate(`fetch('../network/resources/small-test-2.txt')
+                        .then(response => response.arrayBuffer())`);
     await new Promise(resolve => responseWasReceivedCallback = resolve);
-    session.evaluate(`fetch('../resources/test-page.html')`);
+    session.evaluate(`fetch('../resources/test-page.html')
+                        .then(response => response.arrayBuffer())`);
     await new Promise(resolve => responseWasReceivedCallback = resolve);
     testRunner.log('');
   }

third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/search-in-response.js

@@ -4,7 +4,8 @@
   await dp.Network.enable();
 
   var url = testRunner.url('./resources/final.js');
-  session.evaluate(`fetch("${url}");`);
+  session.evaluate(`fetch("${url}")
+                      .then(response => response.arrayBuffer());`);
 
   var requestWillBeSent = (await dp.Network.onceRequestWillBeSent()).params;
   testRunner.log(`Request for ${requestWillBeSent.request.url}`);