Reland "[fuchsia] Remove usage of URLRequest::is_pending()"

This is a reland of e58e84dd7a

Original change's description:
> [fuchsia] Remove usage of URLRequest::is_pending()
>
> * Switch to tracking the URL request state internally.
> * Update tests to ensure QueryStatus() is implemented properly.
>
> Bug: 651108
> Change-Id: Idae45a2a3be933f82eb08393923f0ad776f750f8
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2657857
> Reviewed-by: Matt Menke <mmenke@chromium.org>
> Reviewed-by: Sergey Ulanov <sergeyu@chromium.org>
> Commit-Queue: Fabrice de Gans-Riberi <fdegans@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#848712}

Bug: 651108, 1173449
Change-Id: Ifa7ae6ba43177705091fe396cd4d67e4fca2e8b1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2670031
Commit-Queue: Sergey Ulanov <sergeyu@chromium.org>
Reviewed-by: Sergey Ulanov <sergeyu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#851436}

fuchsia/http/http_service_unittest.cc

@@ -135,6 +135,26 @@ void CheckResponseError(const oldhttp::URLResponse& response,
       << response.error->description;
 }
 
+void CheckQueryStatus(const oldhttp::URLLoaderPtr& url_loader,
+                      bool is_loading,
+                      int expected_network_error) {
+  base::RunLoop run_loop;
+  url_loader->QueryStatus([&run_loop, &is_loading, &expected_network_error](
+                              oldhttp::URLLoaderStatus status) {
+    EXPECT_EQ(status.is_loading, is_loading);
+    if (status.is_loading || expected_network_error == net::OK) {
+      EXPECT_FALSE(status.error);
+    } else {
+      EXPECT_TRUE(status.error);
+      if (status.error) {
+        EXPECT_EQ(status.error->code, expected_network_error);
+      }
+    }
+    run_loop.Quit();
+  });
+  run_loop.Run();
+}
+
 void CheckResponseStream(const oldhttp::URLResponse& response,
                          const std::string& expected_response) {
   EXPECT_TRUE(response.body->is_stream());
@@ -235,6 +255,7 @@ TEST_F(HttpServiceTest, BasicRequestStream) {
   CheckResponseError(url_response(), net::OK);
   EXPECT_EQ(url_response().status_code, 200u);
   CheckResponseStream(url_response(), "hello");
+  CheckQueryStatus(url_loader, false, net::OK);
 }
 
 // Check a basic end-to-end request resolution with the response being
@@ -252,6 +273,7 @@ TEST_F(HttpServiceTest, BasicRequestBuffer) {
   CheckResponseError(url_response(), net::OK);
   EXPECT_EQ(url_response().status_code, 200u);
   CheckResponseBuffer(url_response(), "hello");
+  CheckQueryStatus(url_loader, false, net::OK);
 }
 
 // Check network request headers are received properly.
@@ -277,6 +299,7 @@ TEST_F(HttpServiceTest, RequestWithHeaders) {
       {"X-Multiple-Entries", "b"},
   };
   CheckResponseHeaders(url_response(), &expected_headers);
+  CheckQueryStatus(url_loader, false, net::OK);
 }
 
 // Check duplicate network request headers are received properly.
@@ -304,6 +327,7 @@ TEST_F(HttpServiceTest, RequestWithDuplicateHeaders) {
       {"X-Multiple-Entries", "b"},
   };
   CheckResponseHeaders(url_response(), &expected_headers);
+  CheckQueryStatus(url_loader, false, net::OK);
 }
 
 // Check a request with automatic redirect resolution is handled properly.
@@ -323,6 +347,10 @@ TEST_F(HttpServiceTest, AutoRedirect) {
   ASSERT_TRUE(url_response().url.has_value());
   EXPECT_EQ(url_response().url.value(),
             http_test_server()->GetURL("/with-headers.html").spec());
+  CheckResponseStream(
+      url_response(),
+      "This file is boring; all the action's in the .mock-http-headers.\n");
+  CheckQueryStatus(url_loader, false, net::OK);
 }
 
 // Check a request with manual redirect resolution is handled properly.
@@ -335,7 +363,7 @@ TEST_F(HttpServiceTest, ManualRedirect) {
   oldhttp::URLRequest request;
   request.url = request_url;
   request.method = "GET";
-  request.response_body_mode = oldhttp::ResponseBodyMode::STREAM;
+  request.response_body_mode = oldhttp::ResponseBodyMode::BUFFER;
   request.auto_follow_redirects = false;
 
   ExecuteRequest(url_loader, std::move(request));
@@ -345,6 +373,7 @@ TEST_F(HttpServiceTest, ManualRedirect) {
   EXPECT_EQ(url_response().status_code, 302u);
   EXPECT_EQ(url_response().url.value_or(""), request_url);
   EXPECT_EQ(url_response().redirect_url.value_or(""), final_url);
+  CheckQueryStatus(url_loader, true, net::OK);
 
   base::RunLoop run_loop;
   url_loader->FollowRedirect(
@@ -354,6 +383,7 @@ TEST_F(HttpServiceTest, ManualRedirect) {
         run_loop.Quit();
       });
   run_loop.Run();
+  CheckQueryStatus(url_loader, false, net::OK);
 }
 
 // Check HTTP error codes are properly populated.
@@ -372,6 +402,7 @@ TEST_F(HttpServiceTest, HttpErrorCode) {
   ExecuteRequest(url_loader, std::move(request));
   CheckResponseError(url_response(), net::OK);
   EXPECT_EQ(url_response().status_code, 404u);
+  CheckQueryStatus(url_loader, false, net::OK);
 }
 
 // Check network error codes are properly populated.
@@ -386,6 +417,7 @@ TEST_F(HttpServiceTest, InvalidURL) {
 
   ExecuteRequest(url_loader, std::move(request));
   CheckResponseError(url_response(), net::ERR_INVALID_URL);
+  CheckQueryStatus(url_loader, false, net::ERR_INVALID_URL);
 }
 
 // Ensure the service can handle multiple concurrent requests.
@@ -441,13 +473,7 @@ TEST_F(HttpServiceTest, QueryStatus) {
   ExecuteRequest(url_loader, std::move(request));
   CheckResponseError(url_response(), net::OK);
   EXPECT_EQ(url_response().status_code, 200u);
-
-  base::RunLoop run_loop;
-  url_loader->QueryStatus([&run_loop](oldhttp::URLLoaderStatus status) {
-    EXPECT_TRUE(status.is_loading);
-    run_loop.Quit();
-  });
-  run_loop.Run();
+  CheckQueryStatus(url_loader, true, net::OK);
 }
 
 // Check the response error is properly set if the server disconnects early.
@@ -462,4 +488,23 @@ TEST_F(HttpServiceTest, CloseSocket) {
 
   ExecuteRequest(url_loader, std::move(request));
   CheckResponseError(url_response(), net::ERR_EMPTY_RESPONSE);
+  CheckQueryStatus(url_loader, false, net::ERR_EMPTY_RESPONSE);
+}
+
+// Checks the QueryStatus gets set properly if an error occurs after the
+// response has started being processed.
+TEST_F(HttpServiceTest, ContentLengthTooLong) {
+  oldhttp::URLLoaderPtr url_loader;
+  http_service()->CreateURLLoader(url_loader.NewRequest());
+
+  oldhttp::URLRequest request;
+  request.url =
+      http_test_server()->GetURL("/content-length-too-long.html").spec();
+  request.method = "GET";
+  request.response_body_mode = oldhttp::ResponseBodyMode::STREAM;
+
+  ExecuteRequest(url_loader, std::move(request));
+  CheckResponseError(url_response(), net::OK);
+  CheckResponseStream(url_response(), "hello");
+  CheckQueryStatus(url_loader, false, net::ERR_CONTENT_LENGTH_MISMATCH);
 }

fuchsia/http/testdata/content-length-too-long.html

@@ -0,0 +1 @@
+hello

fuchsia/http/testdata/content-length-too-long.html.mock-http-headers

@@ -0,0 +1,3 @@
+HTTP/1.1 200 OK
+Content-Type: text/html
+Content-Length: 9001

fuchsia/http/url_loader_impl.cc

@@ -176,12 +176,14 @@ void URLLoaderImpl::Start(oldhttp::URLRequest request, Callback callback) {
 
   // Start the request.
   net_request_->Start();
+  is_loading_ = true;
 }
 
 void URLLoaderImpl::FollowRedirect(Callback callback) {
   if (!net_request_ || auto_follow_redirects_ ||
       !net_request_->is_redirecting()) {
     callback(BuildResponse(net::ERR_INVALID_HANDLE));
+    return;
   }
 
   done_callback_ = std::move(callback);
@@ -192,15 +194,10 @@ void URLLoaderImpl::FollowRedirect(Callback callback) {
 void URLLoaderImpl::QueryStatus(QueryStatusCallback callback) {
   oldhttp::URLLoaderStatus status;
 
-  if (!net_request_) {
-    status.is_loading = false;
-  } else if (net_request_->is_pending() || net_request_->is_redirecting()) {
-    status.is_loading = true;
-  } else {
-    status.is_loading = false;
+  status.is_loading = is_loading_;
+  if (net_request_ && !is_loading_) {
     status.error = BuildError(net_error_);
   }
-
   callback(std::move(status));
 }
 
@@ -250,6 +247,7 @@ void URLLoaderImpl::OnResponseStarted(net::URLRequest* request, int net_error) {
 
   // Return early if the request failed.
   if (net_error_ != net::OK) {
+    is_loading_ = false;
     std::move(done_callback_)(BuildResponse(net_error_));
     return;
   }
@@ -260,11 +258,13 @@ void URLLoaderImpl::OnResponseStarted(net::URLRequest* request, int net_error) {
     zx::socket read_socket;
     zx_status_t result = zx::socket::create(0, &read_socket, &write_socket_);
     if (result != ZX_OK) {
+      is_loading_ = false;
+      net_error_ = net::ERR_INSUFFICIENT_RESOURCES;
       ZX_DLOG(WARNING, result) << "zx_socket_create";
-      std::move(done_callback_)(BuildResponse(net::ERR_INSUFFICIENT_RESOURCES));
+      std::move(done_callback_)(BuildResponse(net_error_));
       return;
     }
-    oldhttp::URLResponse response = BuildResponse(net::OK);
+    oldhttp::URLResponse response = BuildResponse(net_error_);
     response.body = oldhttp::URLBody::New();
     response.body->set_stream(std::move(read_socket));
     std::move(done_callback_)(std::move(response));
@@ -311,6 +311,9 @@ void URLLoaderImpl::ReadNextBuffer() {
 
 bool URLLoaderImpl::WriteResponseBytes(int result) {
   if (result < 0) {
+    is_loading_ = false;
+    net_error_ = result;
+
     // Signal read error back to the client.
     if (write_socket_) {
       DCHECK(response_body_mode_ == oldhttp::ResponseBodyMode::STREAM ||
@@ -328,6 +331,8 @@ bool URLLoaderImpl::WriteResponseBytes(int result) {
 
   if (result == 0) {
     // Read complete.
+    is_loading_ = false;
+
     if (write_socket_) {
       DCHECK(response_body_mode_ == oldhttp::ResponseBodyMode::STREAM ||
              response_body_mode_ ==
@@ -344,8 +349,8 @@ bool URLLoaderImpl::WriteResponseBytes(int result) {
         response.body = std::move(body);
         std::move(done_callback_)(std::move(response));
       } else {
-        std::move(done_callback_)(
-            BuildResponse(net::ERR_INSUFFICIENT_RESOURCES));
+        net_error_ = net::ERR_INSUFFICIENT_RESOURCES;
+        std::move(done_callback_)(BuildResponse(net_error_));
       }
     }
     return false;
@@ -370,6 +375,8 @@ bool URLLoaderImpl::WriteResponseBytes(int result) {
       // Something went wrong, attempt to shut down the socket and close it.
       ZX_DLOG(WARNING, status) << "zx_socket_write";
       write_socket_ = zx::socket();
+      is_loading_ = false;
+      net_error_ = net::ERR_FAILED;
       return false;
     }
   } else {

fuchsia/http/url_loader_impl.h

@@ -92,6 +92,9 @@ class URLLoaderImpl : public ::fuchsia::net::oldhttp::URLLoader,
   // manually handle redirects.
   bool auto_follow_redirects_;
 
+  // Set to true when the URLRequest is loading.
+  bool is_loading_ = false;
+
   // Populated from the FIDL URLRequest. Indicates how the response body should
   // be populated.
   ::fuchsia::net::oldhttp::ResponseBodyMode response_body_mode_;