Don't arm watcher in WebDataConsumerHandleImpl until it's fully read

WebDataConsumerHandle::DidGetReadable is expected to be called when it "stops to
wait for data". However, WebDataConsumerHandleImpl::DidGetReadable is called
when it's "ready to be read". It caused excessive calls of DidGetReadable and
results CPU hogging during streaming the data.

Bug: 843811
Change-Id: I213f4b5cb7004b4ae21aaedda229a7bc16d0c68b
Reviewed-on: https://chromium-review.googlesource.com/1062985Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559483}

content/renderer/loader/web_data_consumer_handle_impl.cc

@@ -81,7 +81,7 @@ Result WebDataConsumerHandleImpl::ReaderImpl::Read(void* data,
       context_->handle()->ReadData(data, &size_to_pass, flags_to_pass);
   if (rv == MOJO_RESULT_OK)
     *read_size = size_to_pass;
-  if (rv == MOJO_RESULT_OK || rv == MOJO_RESULT_SHOULD_WAIT)
+  if (rv == MOJO_RESULT_SHOULD_WAIT)
     handle_watcher_.ArmOrNotify();
 
   return HandleReadResult(rv);
@@ -104,13 +104,13 @@ Result WebDataConsumerHandleImpl::ReaderImpl::BeginRead(const void** buffer,
       context_->handle()->BeginReadData(buffer, &size_to_pass, flags_to_pass);
   if (rv == MOJO_RESULT_OK)
     *available = size_to_pass;
+  if (rv == MOJO_RESULT_SHOULD_WAIT)
+    handle_watcher_.ArmOrNotify();
   return HandleReadResult(rv);
 }
 
 Result WebDataConsumerHandleImpl::ReaderImpl::EndRead(size_t read_size) {
   MojoResult rv = context_->handle()->EndReadData(read_size);
-  if (rv == MOJO_RESULT_OK)
-    handle_watcher_.ArmOrNotify();
   return rv == MOJO_RESULT_OK ? kOk : kUnexpectedError;
 }
 

content/renderer/loader/web_data_consumer_handle_impl_unittest.cc

@@ -326,7 +326,7 @@ class CountDidGetReadableClient : public blink::WebDataConsumerHandle::Client {
 
 TEST_F(WebDataConsumerHandleImplTest, DidGetReadable) {
   static constexpr size_t kBlockSize = kDataPipeCapacity / 3;
-  static constexpr size_t kTotalSize = kBlockSize * 3;
+  static constexpr size_t kTotalSize = kBlockSize * 2;
 
   std::unique_ptr<CountDidGetReadableClient> client =
       std::make_unique<CountDidGetReadableClient>();
@@ -337,7 +337,7 @@ TEST_F(WebDataConsumerHandleImplTest, DidGetReadable) {
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(0, client->num_did_get_readable_called());
 
-  // Push three blocks.
+  // Push two blocks.
   {
     std::string expected;
     int index = 0;
@@ -365,10 +365,10 @@ TEST_F(WebDataConsumerHandleImplTest, DidGetReadable) {
     EXPECT_EQ(sizeof(buffer), size);
   }
   base::RunLoop().RunUntilIdle();
-  // |client| is notified the pipe is still ready.
-  EXPECT_EQ(2, client->num_did_get_readable_called());
+  // |client| is NOT notified since the data is still available.
+  EXPECT_EQ(1, client->num_did_get_readable_called());
 
-  // Read one more block.
+  // Read the other block.
   {
     const void* buffer = nullptr;
     size_t size = sizeof(buffer);
@@ -378,28 +378,38 @@ TEST_F(WebDataConsumerHandleImplTest, DidGetReadable) {
     EXPECT_TRUE(buffer);
     EXPECT_EQ(kTotalSize - kBlockSize, size);
     base::RunLoop().RunUntilIdle();
-    // |client| is NOT notified until EndRead is called.
-    EXPECT_EQ(2, client->num_did_get_readable_called());
 
     rv = reader->EndRead(kBlockSize);
     EXPECT_EQ(Result::kOk, rv);
   }
   base::RunLoop().RunUntilIdle();
-  // |client| is notified the pipe is still ready.
-  EXPECT_EQ(3, client->num_did_get_readable_called());
+  // |client| is NOT notified the pipe is still waiting for more data.
+  EXPECT_EQ(1, client->num_did_get_readable_called());
 
-  // Read the final block.
+  // Read one more.
   {
     char buffer[kBlockSize];
     size_t size = 0;
     Result rv = reader->Read(&buffer, sizeof(buffer),
                              WebDataConsumerHandle::kFlagNone, &size);
-    EXPECT_EQ(Result::kOk, rv);
-    EXPECT_EQ(sizeof(buffer), size);
+    EXPECT_EQ(Result::kShouldWait, rv);
+  }
+  base::RunLoop().RunUntilIdle();
+  // |client| is NOT notified because the pipe is still waiting for more data.
+  EXPECT_EQ(1, client->num_did_get_readable_called());
+
+  // Push one more block.
+  {
+    std::string expected(kBlockSize, 'x');
+    uint32_t size = expected.size();
+    MojoResult rv =
+        producer_->WriteData(expected.data(), &size, MOJO_WRITE_DATA_FLAG_NONE);
+    EXPECT_EQ(MOJO_RESULT_OK, rv);
+    EXPECT_EQ(expected.size(), size);
   }
   base::RunLoop().RunUntilIdle();
-  // |client| is NOT notified because the pipe doesn't have any data.
-  EXPECT_EQ(3, client->num_did_get_readable_called());
+  // |client| is notified the pipe gets ready.
+  EXPECT_EQ(2, client->num_did_get_readable_called());
 }
 
 }  // namespace