Prevent URLFetcher::AppendChunkedData from dereferencing NULL pointers.

net/url_request/url_fetcher_impl_unittest.cc

Index: net/url_request/url_fetcher_impl_unittest.cc
diff --git a/net/url_request/url_fetcher_impl_unittest.cc b/net/url_request/url_fetcher_impl_unittest.cc
index 6a79b1bf3d9d2f09cf24ae467c38e8057dfc747f..43d978be45f8c5067be3ff17f30497ab9d8703c5 100644
--- a/net/url_request/url_fetcher_impl_unittest.cc
+++ b/net/url_request/url_fetcher_impl_unittest.cc
@@ -24,7 +24,9 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/synchronization/waitable_event.h"
+#include "base/test/test_timeouts.h"
 #include "base/thread_task_runner_handle.h"
+#include "base/threading/platform_thread.h"
 #include "base/threading/thread.h"
 #include "build/build_config.h"
 #include "crypto/nss_util.h"
@@ -775,6 +777,70 @@ TEST_F(URLFetcherTest, PostWithUploadStreamFactoryAndRetries) {
   EXPECT_EQ(2u, num_upload_streams_created());
 }
 
+// Tests simple chunked POST case.
+TEST_F(URLFetcherTest, PostChunked) {
+  scoped_refptr<FetcherTestURLRequestContextGetter> context_getter(
+      CreateCrossThreadContextGetter());
+
+  WaitingURLFetcherDelegate delegate;
+  delegate.CreateFetcher(test_server_->GetURL("/echo"), URLFetcher::POST,
+                         CreateCrossThreadContextGetter());
+
+  delegate.fetcher()->SetChunkedUpload("text/plain");
+
+  // This posts a task to start the fetcher.
+  delegate.fetcher()->Start();
+
+  delegate.fetcher()->AppendChunkToUpload(kCreateUploadStreamBody, false);
+  delegate.fetcher()->AppendChunkToUpload(kCreateUploadStreamBody, true);
+
+  delegate.WaitForComplete();
+
+  EXPECT_TRUE(delegate.fetcher()->GetStatus().is_success());
+  EXPECT_EQ(200, delegate.fetcher()->GetResponseCode());
+  std::string data;
+  ASSERT_TRUE(delegate.fetcher()->GetResponseAsString(&data));
+  EXPECT_EQ(std::string(kCreateUploadStreamBody) +
+                std::string(kCreateUploadStreamBody),
+            data);
+}
+
+// Tests that data can be appended to a request after it fails. This is needed
+// because the consumer may try to append data to a request after it failed, but
+// before the consumer learns that it failed.
+TEST_F(URLFetcherTest, PostAppendChunkAfterError) {
+  // This test has to use a single thread, so it can easily wait until the
+  // request has reached the HostResolver, at which point network changes will
+  // cause a failure.
+  scoped_refptr<FetcherTestURLRequestContextGetter> context_getter(
+      CreateCrossThreadContextGetter());
+
+  WaitingURLFetcherDelegate delegate;
+  // Request that will fail almost immediately after being started, due to using
+  // a reserved port.
+  delegate.CreateFetcher(GURL("http://127.0.0.1:7"), URLFetcher::POST,
+                         context_getter);
+
+  delegate.fetcher()->SetChunkedUpload("text/plain");
+
+  // This posts a task to start the fetcher.
+  delegate.fetcher()->Start();
+
+  // Give the request a chance to fail, and inform the fetcher of the failure,
+  // while blocking the current thread so the error doesn't reach the delegate.
+  base::PlatformThread::Sleep(TestTimeouts::tiny_timeout());

[eroman, 2016/03/26 00:55:16]

Hmm this is subtle.

Is there a way to run this test on the IO thread and pump the IO loop as needed
to reach this condition?

[mmenke, 2016/03/28 17:33:11]

This test doesn't really need to be cross-thread, though there is same-thread
magic in the case of cancellation by the consumer, which does make me a bit
nervous about the change.

There's currently no way to know when the URLFetcher failed but has not yet been
notified of the failure, and I don't want to call into the Fetcher after it's
been notified of the failure.

So that leaves us needing to wait for some sort of notification the request
failed, but that we haven't been told yet.  I don't want to subclass the
URLFetcher, since that depends too much on URLFetcher implementation.  We could
subclass URLRequestJob, and have it notify us after after the URLFetcherCore is
notified of the change, but before the URLFetcherCore notifies the URLFetcher. 
The problem is that URLRequestJob::NotifyStartFailure currently synchronously
calls into URLRequest, which synchronously calls into URLFetcher.  I've been
thinking of making it asynchronous, so it could be call on
URLRequestJob::Start() immediately, which would break the test.

I don't see how we can do this in a way that doesn't have similar fragility
issues, unfortunately.  Going to land as-is, but open to other ideas, happy to
go back and revise the test in a followup CL.

+
+  // Try to append data.
+  delegate.fetcher()->AppendChunkToUpload("kCreateUploadStreamBody", false);
+  delegate.fetcher()->AppendChunkToUpload("kCreateUploadStreamBody", true);
+
+  delegate.WaitForComplete();
+
+  // Make sure the request failed, as expected.
+  EXPECT_FALSE(delegate.fetcher()->GetStatus().is_success());
+  EXPECT_EQ(ERR_UNSAFE_PORT, delegate.fetcher()->GetStatus().error());
+}
+
 // Checks that upload progress increases over time, never exceeds what's already
 // been sent, and adds a chunk whenever all previously appended chunks have
 // been uploaded.
@@ -1043,6 +1109,43 @@ TEST_F(URLFetcherTest, ThrottleOnRepeatedFetches) {
   EXPECT_GE(Time::Now() - start_time, base::TimeDelta::FromSeconds(1));
 }
 
+// If throttling kicks in for a chunked upload, there should be no crash.
+TEST_F(URLFetcherTest, ThrottleChunkedUpload) {
+  base::Time start_time = Time::Now();
+  GURL url(test_server_->GetURL("/echo"));
+
+  scoped_refptr<FetcherTestURLRequestContextGetter> context_getter(
+      CreateSameThreadContextGetter());
+
+  // Registers an entry for test url. It only allows 3 requests to be sent
+  // in 200 milliseconds.
+  context_getter->AddThrottlerEntry(
+      url, std::string() /* url_id */, 200 /* sliding_window_period_ms */,
+      3 /* max_send_threshold */, 1 /* initial_backoff_ms */,
+      2.0 /* multiply_factor */, 0.0 /* jitter_factor */,
+      256 /* maximum_backoff_ms */,
+      false /* reserve_sending_time_for_next_request*/);
+
+  for (int i = 0; i < 20; ++i) {
+    WaitingURLFetcherDelegate delegate;
+    delegate.CreateFetcher(url, URLFetcher::POST, context_getter);
+    delegate.fetcher()->SetChunkedUpload("text/plain");
+    delegate.fetcher()->Start();
+    delegate.fetcher()->AppendChunkToUpload(kCreateUploadStreamBody, true);
+    delegate.WaitForComplete();
+
+    EXPECT_TRUE(delegate.fetcher()->GetStatus().is_success());
+    EXPECT_EQ(200, delegate.fetcher()->GetResponseCode());
+    std::string data;
+    ASSERT_TRUE(delegate.fetcher()->GetResponseAsString(&data));
+    EXPECT_EQ(kCreateUploadStreamBody, data);
+  }
+
+  // 20 requests were sent. Due to throttling, they should have collectively
+  // taken over 1 second.
+  EXPECT_GE(Time::Now() - start_time, base::TimeDelta::FromSeconds(1));

[eroman, 2016/03/26 00:55:16]

Is this timing exact, or can this fail?
Would it be better to leave this off?

[mmenke, 2016/03/28 17:33:11]

Removed.  This was copied from the above test...I don't think it's needed on
this one, though.  I'd really like to get rid of this throttling stuff
completely, but we have one or two consumers that use it, now that we removed
the main consumer.  :(

+}
+
 TEST_F(URLFetcherTest, ThrottleOn5xxRetries) {
   base::Time start_time = Time::Now();
   GURL url(test_server_->GetURL("/server-unavailable.html"));