Failure in reading Blob URL should result in network error.

Intent to Implement and Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/zm0ZUCINXBk

Bug: 732750
Change-Id: I07b45276f3e0ddf991f1f8a6c390bf5b843ec22e
Reviewed-on: https://chromium-review.googlesource.com/893942Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533340}

chrome/test/data/extensions/api_test/mime_handler_view/index.js

@@ -20,13 +20,17 @@ var streamDetails;
 function fetchUrl(url) {
   return new Promise(function(resolve, reject) {
     var request = new XMLHttpRequest();
-    request.onreadystatechange = function() {
-      if (request.readyState == 4) {
-        resolve({
-          status: request.status,
-          data: request.responseText,
-        });
-      }
+    request.onload = function() {
+      resolve({
+        status: request.status,
+        data: request.responseText,
+      });
+    };
+    request.onerror = function() {
+      resolve({
+        status: request.status,
+        data: 'error',
+      });
     };
     request.open('GET', streamDetails.streamUrl, true);
     request.send();
@@ -88,8 +92,8 @@ var tests = [
     checkStreamDetails('testAbort.csv', false);
     chrome.mimeHandlerPrivate.abortStream(function() {
       fetchUrl(streamDetails.streamUrl).then(function(response) {
-        chrome.test.assertEq(404, response.status);
-        chrome.test.assertEq('', response.data);
+        chrome.test.assertEq(0, response.status);
+        chrome.test.assertEq('error', response.data);
         chrome.test.succeed();
       });
     });

chrome/test/data/extensions/api_test/streams_private/handle_mime_type/background.js

@@ -75,14 +75,15 @@ chrome.streamsPrivate.onExecuteMimeTypeHandler.addListener(
       chrome.streamsPrivate.abort(params.streamUrl, function() {
         var xhr = new XMLHttpRequest();
         xhr.open("GET", params.streamUrl, false);
-        xhr.send(null);
-        if (xhr.status == 404) {
+        try {
+          xhr.send(null);
+        } catch (e) {
           chrome.test.notifyPass();
-        } else {
-          chrome.test.notifyFail(
-              'Expected a stream URL response of 404, got ' + xhr.status + '.');
-          hasFailed = true;
+          return;
         }
+        chrome.test.notifyFail(
+            'Expected a network error, got ' + xhr.status + '.');
+        hasFailed = true;
       });
     }
     return;

content/browser/blob_storage/blob_url_unittest.cc

@@ -162,6 +162,7 @@ class BlobURLRequestJobTest : public testing::TestWithParam<RequestTestType> {
         blob_data_(new BlobDataBuilder("uuid")),
         blob_uuid_(blob_data_->uuid()),
         response_error_code_(net::OK),
+        expected_error_code_(net::OK),
         expected_status_code_(0) {}
 
   void SetUp() override {
@@ -260,14 +261,15 @@ class BlobURLRequestJobTest : public testing::TestWithParam<RequestTestType> {
 
   void TestSuccessNonrangeRequest(const std::string& expected_response,
                                   int64_t expected_content_length) {
+    expected_error_code_ = net::OK;
     expected_status_code_ = 200;
     expected_response_ = expected_response;
     TestRequest("GET", net::HttpRequestHeaders());
     EXPECT_EQ(expected_content_length, response_headers_->GetContentLength());
   }
 
-  void TestErrorRequest(int expected_status_code) {
-    expected_status_code_ = expected_status_code;
+  void TestErrorRequest(int expected_error_code) {
+    expected_error_code_ = expected_error_code;
     expected_response_ = "";
     TestRequest("GET", net::HttpRequestHeaders());
     EXPECT_TRUE(response_metadata_.empty());
@@ -355,9 +357,11 @@ class BlobURLRequestJobTest : public testing::TestWithParam<RequestTestType> {
     }
 
     // Verify response.
-    EXPECT_EQ(net::OK, response_error_code_);
-    EXPECT_EQ(expected_status_code_, response_headers_->response_code());
-    EXPECT_EQ(expected_response_, response_);
+    EXPECT_EQ(expected_error_code_, response_error_code_);
+    if (response_error_code_ == net::OK) {
+      EXPECT_EQ(expected_status_code_, response_headers_->response_code());
+      EXPECT_EQ(expected_response_, response_);
+    }
   }
 
   void BuildComplicatedData(std::string* expected_result) {
@@ -446,6 +450,7 @@ class BlobURLRequestJobTest : public testing::TestWithParam<RequestTestType> {
   scoped_refptr<net::HttpResponseHeaders> response_headers_;
   std::string response_metadata_;
 
+  int expected_error_code_;
   int expected_status_code_;
   std::string expected_response_;
 };
@@ -481,14 +486,14 @@ TEST_P(BlobURLRequestJobTest, TestGetNonExistentFileRequest) {
       temp_file1_.InsertBeforeExtension(FILE_PATH_LITERAL("-na"));
   blob_data_->AppendFile(non_existent_file, 0,
                          std::numeric_limits<uint64_t>::max(), base::Time());
-  TestErrorRequest(404);
+  TestErrorRequest(net::ERR_FILE_NOT_FOUND);
 }
 
 TEST_P(BlobURLRequestJobTest, TestGetChangedFileRequest) {
   base::Time old_time =
       temp_file_modification_time1_ - base::TimeDelta::FromSeconds(10);
   blob_data_->AppendFile(temp_file1_, 0, 3, old_time);
-  TestErrorRequest(404);
+  TestErrorRequest(net::ERR_FILE_NOT_FOUND);
 }
 
 TEST_P(BlobURLRequestJobTest, TestGetSlicedFileRequest) {
@@ -528,7 +533,7 @@ TEST_P(BlobURLRequestJobTest, TestGetNonExistentFileSystemFileRequest) {
   blob_data_->AppendFileSystemFile(non_existent_file, 0,
                                    std::numeric_limits<uint64_t>::max(),
                                    base::Time(), file_system_context_);
-  TestErrorRequest(404);
+  TestErrorRequest(net::ERR_FILE_NOT_FOUND);
 }
 
 TEST_P(BlobURLRequestJobTest, TestGetInvalidFileSystemFileRequest) {
@@ -537,7 +542,7 @@ TEST_P(BlobURLRequestJobTest, TestGetInvalidFileSystemFileRequest) {
   blob_data_->AppendFileSystemFile(invalid_file, 0,
                                    std::numeric_limits<uint64_t>::max(),
                                    base::Time(), file_system_context_);
-  TestErrorRequest(500);
+  TestErrorRequest(net::ERR_FAILED);
 }
 
 TEST_P(BlobURLRequestJobTest, TestGetChangedFileSystemFileRequest) {
@@ -546,7 +551,7 @@ TEST_P(BlobURLRequestJobTest, TestGetChangedFileSystemFileRequest) {
                         base::TimeDelta::FromSeconds(10);
   blob_data_->AppendFileSystemFile(temp_file_system_file1_, 0, 3, old_time,
                                    file_system_context_);
-  TestErrorRequest(404);
+  TestErrorRequest(net::ERR_FILE_NOT_FOUND);
 }
 
 TEST_P(BlobURLRequestJobTest, TestGetSlicedFileSystemFileRequest) {
@@ -693,7 +698,7 @@ TEST_P(BlobURLRequestJobTest, TestZeroSizeSideData) {
 TEST_P(BlobURLRequestJobTest, BrokenBlob) {
   blob_handle_ = blob_context_.AddBrokenBlob(
       "uuid", "", "", storage::BlobStatus::ERR_INVALID_CONSTRUCTION_ARGUMENTS);
-  TestErrorRequest(500);
+  TestErrorRequest(net::ERR_FAILED);
 }
 
 // The parameter's value determines whether BlobURLLoaderFactory is used.

storage/browser/blob/blob_url_loader.cc

@@ -140,18 +140,7 @@ void BlobURLLoader::DidRead(int num_bytes) {
 
 void BlobURLLoader::OnComplete(net::Error error_code,
                                uint64_t total_written_bytes) {
-  if (error_code != net::OK && !sent_headers_) {
-    net::HttpStatusCode status_code =
-        storage::BlobURLRequestJob::NetErrorToHttpStatusCode(error_code);
-    network::ResourceResponseHead response;
-    response.headers = storage::BlobURLRequestJob::GenerateHeaders(
-        status_code, nullptr, nullptr, 0, 0);
-    client_->OnReceiveResponse(response, base::nullopt, nullptr);
-  }
-  network::URLLoaderCompletionStatus status;
-  // TODO(kinuko): We should probably set the error_code here,
-  // while it makes existing tests fail. https://crbug.com/732750
-  status.completion_time = base::TimeTicks::Now();
+  network::URLLoaderCompletionStatus status(error_code);
   status.encoded_body_length = total_written_bytes;
   status.decoded_body_length = total_written_bytes;
   client_->OnComplete(status);

storage/browser/blob/blob_url_request_job.cc

@@ -182,37 +182,6 @@ scoped_refptr<net::HttpResponseHeaders> BlobURLRequestJob::GenerateHeaders(
   return headers;
 }
 
-net::HttpStatusCode BlobURLRequestJob::NetErrorToHttpStatusCode(
-    int error_code) {
-  net::HttpStatusCode status_code = net::HTTP_INTERNAL_SERVER_ERROR;
-  switch (error_code) {
-    case net::ERR_ACCESS_DENIED:
-      status_code = net::HTTP_FORBIDDEN;
-      break;
-    case net::ERR_FILE_NOT_FOUND:
-      status_code = net::HTTP_NOT_FOUND;
-      break;
-    case net::ERR_METHOD_NOT_SUPPORTED:
-      status_code = net::HTTP_METHOD_NOT_ALLOWED;
-      break;
-    case net::ERR_REQUEST_RANGE_NOT_SATISFIABLE:
-      status_code = net::HTTP_REQUESTED_RANGE_NOT_SATISFIABLE;
-      break;
-    case net::ERR_INVALID_ARGUMENT:
-      status_code = net::HTTP_BAD_REQUEST;
-      break;
-    case net::ERR_CACHE_READ_FAILURE:
-    case net::ERR_CACHE_CHECKSUM_READ_FAILURE:
-    case net::ERR_UNEXPECTED:
-    case net::ERR_FAILED:
-      break;
-    default:
-      DCHECK(false) << "Error code not supported: " << error_code;
-      break;
-  }
-  return status_code;
-}
-
 BlobURLRequestJob::~BlobURLRequestJob() {
   TRACE_EVENT_ASYNC_END1("Blob", "BlobRequest", this, "uuid",
                          blob_handle_ ? blob_handle_->uuid() : "NotFound");
@@ -314,7 +283,7 @@ void BlobURLRequestJob::NotifyFailure(int error_code) {
   // now. Instead, we just error out.
   DCHECK(!response_info_) << "Cannot NotifyFailure after headers.";
 
-  HeadersCompleted(NetErrorToHttpStatusCode(error_code));
+  NotifyStartError(net::URLRequestStatus::FromError(error_code));
 }
 
 void BlobURLRequestJob::HeadersCompleted(net::HttpStatusCode status_code) {

storage/browser/blob/blob_url_request_job.h

@@ -55,9 +55,6 @@ class STORAGE_EXPORT BlobURLRequestJob
       uint64_t total_size,
       uint64_t content_size);
 
-  // Helper method to map from a net error to an http status code.
-  static net::HttpStatusCode NetErrorToHttpStatusCode(int error_code);
-
  protected:
   ~BlobURLRequestJob() override;
 

third_party/WebKit/LayoutTests/external/wpt/FileAPI/url/url-with-xhr.any-expected.txt

 This is a testharness.js-based test.
 PASS Blob URLs can be used in XHR
 PASS XHR with a fragment should succeed
-FAIL XHR of a revoked URL should fail assert_equals: expected 0 but got 404
+PASS XHR of a revoked URL should fail
 PASS Only exact matches should revoke URLs, using XHR
-FAIL Appending a query string should cause XHR to fail assert_equals: expected 0 but got 404
-FAIL Appending a path should cause XHR to fail assert_equals: expected 0 but got 404
+PASS Appending a query string should cause XHR to fail
+PASS Appending a path should cause XHR to fail
 PASS XHR with method "HEAD" should fail
 PASS XHR with method "POST" should fail
 PASS XHR with method "DELETE" should fail
@@ -12,6 +12,6 @@ PASS XHR with method "OPTIONS" should fail
 PASS XHR with method "PUT" should fail
 PASS XHR with method "CUSTOM" should fail
 PASS XHR should return Content-Type from Blob
-FAIL Revoke blob URL after open(), will fetch assert_equals: expected "test blob contents" but got ""
+FAIL Revoke blob URL after open(), will fetch assert_unreached: Got unexpected error event Reached unreachable code
 Harness: the test ran to completion.
 

third_party/WebKit/LayoutTests/fast/files/apply-blob-url-to-xhr-expected.txt

@@ -6,8 +6,7 @@ Response: Hello
 Test that sync XMLHttpRequest POST fails.
 Received exception, code: 19, name: NetworkError, message: Failed to execute 'send' on 'XMLHttpRequest': Failed to load 'blob:file:///UUID': 'GET' is the only method allowed for 'blob:' URLs.
 Test that sync XMLHttpRequest GET fails after the blob URL is revoked.
-Status: 404
-Response: 
+Received exception, code: 19, name: NetworkError, message: Failed to execute 'send' on 'XMLHttpRequest': Failed to load 'blob:file:///UUID'.
 Test that async XMLHttpRequest GET succeeds.
 Status: 200
 Response: Hello
@@ -17,7 +16,6 @@ Test the slicing the blob response doesn't crash the browser.
 Status: 200
 First byte of response: H
 Test that async XMLHttpRequest GET fails after the blob URL is revoked.
-Status: 404
-Response: 
+Error event is dispatched
 DONE
 

third_party/WebKit/LayoutTests/fast/files/workers/worker-apply-blob-url-to-xhr-expected.txt

@@ -5,8 +5,7 @@ Response: Hello
 Test that sync XMLHttpRequest POST fails.
 Received exception, code: 19, name: NetworkError, message: Failed to execute 'send' on 'XMLHttpRequest': Failed to load 'blob:file:///UUID': 'GET' is the only method allowed for 'blob:' URLs.
 Test that sync XMLHttpRequest GET fails after the blob URL is revoked.
-Status: 404
-Response: 
+Received exception, code: 19, name: NetworkError, message: Failed to execute 'send' on 'XMLHttpRequest': Failed to load 'blob:file:///UUID'.
 Test that async XMLHttpRequest GET succeeds.
 Status: 200
 Response: Hello
@@ -16,7 +15,6 @@ Test the slicing the blob response doesn't crash the browser.
 Status: 200
 First byte of response: H
 Test that async XMLHttpRequest GET fails after the blob URL is revoked.
-Status: 404
-Response: 
+Error event is dispatched
 DONE
 

third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/xhr-to-blob-in-isolated-world-expected.txt

 CONSOLE WARNING: line 1: Synchronous XMLHttpRequest on the main thread is deprecated because of its detrimental effects to the end user's experience. For more help, check https://xhr.spec.whatwg.org/.
+CONSOLE ERROR: line 1: Uncaught NetworkError: Failed to execute 'send' on 'XMLHttpRequest': Failed to load 'blob:https://cloud-cuckoo-land.google:2112/456789'.
 This tests an isolated script's ability to XHR a blob that is in its security origin, which is not the same as the document's security origin.
-We pass if there are no console errors.
+We pass if there are no 'Not allowed to load' console errors. (This will still show one console error as the blob is not available)

third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/xhr-to-blob-in-isolated-world.html

 <body>
 This tests an isolated script's ability to XHR a blob that is in its security origin, which is not the same as the document's security origin.<br>
-We pass if there are no console errors.
+We pass if there are no 'Not allowed to load' console errors.
+(This will still show one console error as the blob is not available)
 <script>
 if (!window.testRunner) {
     document.body.appendChild(document.createTextNode("This test requires window.testRunner"));

third_party/WebKit/Source/core/fetch/FetchManager.cpp

@@ -347,18 +347,6 @@ void FetchManager::Loader::DidReceiveResponse(
   ScriptState* script_state = resolver_->GetScriptState();
   ScriptState::Scope scope(script_state);
 
-  if (response.Url().ProtocolIs("blob") && response.HttpStatusCode() == 404) {
-    // "If |blob| is null, return a network error."
-    // https://fetch.spec.whatwg.org/#concept-scheme-fetch
-    PerformNetworkError("Blob not found.");
-    return;
-  }
-
-  if (response.Url().ProtocolIs("blob") && response.HttpStatusCode() == 405) {
-    PerformNetworkError("Only 'GET' method is allowed for blob URLs.");
-    return;
-  }
-
   response_http_status_code_ = response.HttpStatusCode();
   FetchRequestData::Tainting tainting = request_->ResponseTainting();