Fix a SimpleUrlLoader stream resume-after-timeout crash

Add a null check in DownloadAsStreamBodyHandler::Resume to protect
against a rare case where Resume is called right after hitting the
timeout.

To demonstrate the crash, add a unit test that explicitly keeps hold of
the resume closure until after a timeout is hit. Without the added fix,
the test crashes accessing body_reader_. While it looks odd in the
test, this case seems possible in valid-looking
SimpleURLLoaderStreamConsumer code that happens to get tasks queued in
an unlucky way right when the timeout is hit.

Credit to mruszczynski@vewd.com for investigation and initial UT.

Bug: 1169018
Change-Id: Iff38fb1e3a4404ae006e1f6b76479cdf6ffafa85
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2641033
Commit-Queue: Tomasz Śniatowski <tsniatowski@vewd.com>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Auto-Submit: Tomasz Śniatowski <tsniatowski@vewd.com>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#846019}

services/network/public/cpp/simple_url_loader.cc

@@ -1135,6 +1135,10 @@ class DownloadAsStreamBodyHandler : public BodyHandler,
                                     weak_ptr_factory_.GetWeakPtr()));
       return;
     }
+    if (!body_reader_) {
+      // If Resume was delayed, body_reader_ could have been deleted.
+      return;
+    }
     body_reader_->Resume();
   }
 

services/network/public/cpp/simple_url_loader_unittest.cc

@@ -251,6 +251,17 @@ class SimpleLoaderTestHelper : public SimpleURLLoaderStreamConsumer {
     download_to_stream_async_resume_ = download_to_stream_async_resume;
   }
 
+  // Sets whether the resume-reading closure should be captured and later
+  // available in TakeCapturedStreamResume()
+  void set_download_to_stream_capture_resume(
+      bool download_to_stream_capture_resume) {
+    download_to_stream_capture_resume_ = download_to_stream_capture_resume;
+  }
+
+  base::OnceClosure TakeCapturedStreamResume() {
+    return std::move(captured_stream_resume_);
+  }
+
   // Sets whether the helper should destroy the SimpleURLLoader in
   // OnDataReceived.
   void set_download_to_stream_destroy_on_data_received(
@@ -417,6 +428,14 @@ class SimpleLoaderTestHelper : public SimpleURLLoaderStreamConsumer {
       return;
     }
 
+    if (download_to_stream_capture_resume_) {
+      if (captured_stream_resume_) {
+        std::move(captured_stream_resume_).Run();
+      }
+      captured_stream_resume_ = std::move(resume);
+      return;
+    }
+
     if (download_to_stream_async_resume_) {
       base::SequencedTaskRunnerHandle::Get()->PostTask(FROM_HERE,
                                                        std::move(resume));
@@ -496,6 +515,8 @@ class SimpleLoaderTestHelper : public SimpleURLLoaderStreamConsumer {
   bool download_to_stream_destroy_on_data_received_ = false;
   bool download_to_stream_async_retry_ = false;
   bool download_to_stream_destroy_on_retry_ = false;
+  bool download_to_stream_capture_resume_ = false;
+  base::OnceClosure captured_stream_resume_;
 
   bool destroy_loader_on_complete_ = false;
 
@@ -3421,6 +3442,17 @@ class SimpleURLLoaderMockTimeTest : public testing::Test {
         SimpleLoaderTestHelper::DownloadType::TO_STRING);
   }
 
+  std::unique_ptr<SimpleLoaderTestHelper> CreateStreamHelper() {
+    std::unique_ptr<network::ResourceRequest> resource_request =
+        std::make_unique<network::ResourceRequest>();
+    resource_request->url = GURL("foo://bar/");
+    resource_request->method = "GET";
+    resource_request->enable_upload_progress = true;
+    return std::make_unique<SimpleLoaderTestHelper>(
+        std::move(resource_request),
+        SimpleLoaderTestHelper::DownloadType::AS_STREAM);
+  }
+
  protected:
   base::test::TaskEnvironment task_environment_;
   std::unique_ptr<base::ScopedDisallowBlocking> disallow_blocking_;
@@ -3444,6 +3476,32 @@ TEST_F(SimpleURLLoaderMockTimeTest, TimeoutTriggered) {
   EXPECT_FALSE(test_helper->simple_url_loader()->CompletionStatus());
 }
 
+// Request fails with a timeout like in TimeoutTriggered, and the stream resume
+// closure is called after the timeout. The loader is alive throughout.
+TEST_F(SimpleURLLoaderMockTimeTest, StreamResumeAfterTimeout) {
+  MockURLLoaderFactory loader_factory(&task_environment_);
+  loader_factory.AddEvents(
+      {TestLoaderEvent::kReceivedResponse, TestLoaderEvent::kBodyBufferReceived,
+       TestLoaderEvent::kBodyDataRead, TestLoaderEvent::kAdvanceOneSecond,
+       TestLoaderEvent::kResponseComplete, TestLoaderEvent::kBodyBufferClosed});
+  std::unique_ptr<SimpleLoaderTestHelper> test_helper = CreateStreamHelper();
+  test_helper->simple_url_loader()->SetTimeoutDuration(
+      base::TimeDelta::FromSeconds(1));
+  test_helper->set_download_to_stream_capture_resume(true);
+
+  loader_factory.RunTest(test_helper.get());
+
+  EXPECT_EQ(net::ERR_TIMED_OUT, test_helper->simple_url_loader()->NetError());
+  EXPECT_FALSE(test_helper->simple_url_loader()->CompletionStatus());
+
+  base::OnceClosure captured_resume = test_helper->TakeCapturedStreamResume();
+  ASSERT_TRUE(captured_resume);
+  base::SequencedTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                   std::move(captured_resume));
+  // Make sure no pending task results in a crash.
+  base::RunLoop().RunUntilIdle();
+}
+
 // Less time is simulated passing than the timeout value, so this request should
 // succeed normally.
 TEST_F(SimpleURLLoaderMockTimeTest, TimeoutNotTriggered) {