Show BundleResponseParseError to DevTools

Bug: 969596
Change-Id: I80ce8fe371f3d8d04cfc45146398976aa693f2de
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1866118Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706832}

content/browser/web_package/bundled_exchanges_browsertest.cc

@@ -508,6 +508,64 @@ IN_PROC_BROWSER_TEST_P(BundledExchangesFileBrowserTest,
             console_delegate.message());
 }
 
+IN_PROC_BROWSER_TEST_P(BundledExchangesFileBrowserTest,
+                       ResponseParseErrorInMainResource) {
+  const GURL test_data_url = GetTestUrlForFile(
+      GetTestDataPath("broken_bundle_broken_first_entry.wbn"));
+
+  WebContents* web_contents = shell()->web_contents();
+  ConsoleObserverDelegate console_delegate(web_contents, "*");
+  web_contents->SetDelegate(&console_delegate);
+
+  base::RunLoop run_loop;
+  FinishNavigationObserver finish_navigation_observer(web_contents,
+                                                      run_loop.QuitClosure());
+  EXPECT_FALSE(NavigateToURL(web_contents, test_data_url));
+  run_loop.Run();
+  ASSERT_TRUE(finish_navigation_observer.error_code());
+  EXPECT_EQ(net::ERR_INVALID_BUNDLED_EXCHANGES,
+            *finish_navigation_observer.error_code());
+
+  if (console_delegate.messages().empty())
+    console_delegate.Wait();
+
+  EXPECT_FALSE(console_delegate.messages().empty());
+  EXPECT_EQ(
+      "Failed to read response header of Web Bundle file: Response headers map "
+      "must have exactly one pseudo-header, :status.",
+      console_delegate.message());
+}
+
+IN_PROC_BROWSER_TEST_P(BundledExchangesFileBrowserTest,
+                       ResponseParseErrorInSubresource) {
+  const GURL test_data_url = GetTestUrlForFile(
+      GetTestDataPath("broken_bundle_broken_script_entry.wbn"));
+  NavigateToBundleAndWaitForReady(
+      test_data_url,
+      bundled_exchanges_utils::GetSynthesizedUrlForBundledExchanges(
+          test_data_url, GURL(kTestPageUrl)));
+
+  WebContents* web_contents = shell()->web_contents();
+  ConsoleObserverDelegate console_delegate(web_contents, "*");
+  web_contents->SetDelegate(&console_delegate);
+
+  ExecuteScriptAndWaitForTitle(R"(
+    const script = document.createElement("script");
+    script.onerror = () => { document.title = "load failed";};
+    script.src = "script.js";
+    document.body.appendChild(script);)",
+                               "load failed");
+
+  if (console_delegate.messages().empty())
+    console_delegate.Wait();
+
+  EXPECT_FALSE(console_delegate.messages().empty());
+  EXPECT_EQ(
+      "Failed to read response header of Web Bundle file: Response headers map "
+      "must have exactly one pseudo-header, :status.",
+      console_delegate.message());
+}
+
 IN_PROC_BROWSER_TEST_P(BundledExchangesFileBrowserTest, NoLocalFileScheme) {
   const GURL test_data_url =
       GetTestUrlForFile(GetTestDataPath("bundled_exchanges_browsertest.wbn"));

content/browser/web_package/bundled_exchanges_handle.cc

@@ -214,8 +214,8 @@ class InterceptorForFile final : public NavigationLoaderInterceptor {
     }
     DCHECK(reader_);
     primary_url_ = reader_->GetPrimaryURL();
-    url_loader_factory_ =
-        std::make_unique<BundledExchangesURLLoaderFactory>(std::move(reader_));
+    url_loader_factory_ = std::make_unique<BundledExchangesURLLoaderFactory>(
+        std::move(reader_), frame_tree_node_id_);
 
     const GURL new_url =
         bundled_exchanges_utils::GetSynthesizedUrlForBundledExchanges(
@@ -349,7 +349,7 @@ class InterceptorForTrustableFile final : public NavigationLoaderInterceptor {
     } else {
       primary_url_ = reader_->GetPrimaryURL();
       url_loader_factory_ = std::make_unique<BundledExchangesURLLoaderFactory>(
-          std::move(reader_));
+          std::move(reader_), frame_tree_node_id_);
     }
 
     if (pending_request_) {
@@ -395,9 +395,11 @@ class InterceptorForTrackedNavigationFromTrustableFile final
  public:
   InterceptorForTrackedNavigationFromTrustableFile(
       scoped_refptr<BundledExchangesReader> reader,
-      DoneCallback done_callback)
+      DoneCallback done_callback,
+      int frame_tree_node_id)
       : url_loader_factory_(std::make_unique<BundledExchangesURLLoaderFactory>(
-            std::move(reader))),
+            std::move(reader),
+            frame_tree_node_id)),
         done_callback_(std::move(done_callback)) {}
   ~InterceptorForTrackedNavigationFromTrustableFile() override {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -456,9 +458,11 @@ class InterceptorForTrackedNavigationFromFile final
  public:
   InterceptorForTrackedNavigationFromFile(
       scoped_refptr<BundledExchangesReader> reader,
-      DoneCallback done_callback)
+      DoneCallback done_callback,
+      int frame_tree_node_id)
       : url_loader_factory_(std::make_unique<BundledExchangesURLLoaderFactory>(
-            std::move(reader))),
+            std::move(reader),
+            frame_tree_node_id)),
         done_callback_(std::move(done_callback)) {}
   ~InterceptorForTrackedNavigationFromFile() override {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -601,7 +605,7 @@ class InterceptorForNavigationInfo final : public NavigationLoaderInterceptor {
       metadata_error_ = std::move(error);
     } else {
       url_loader_factory_ = std::make_unique<BundledExchangesURLLoaderFactory>(
-          std::move(reader_));
+          std::move(reader_), frame_tree_node_id_);
     }
 
     if (pending_request_) {
@@ -661,7 +665,8 @@ BundledExchangesHandle::CreateForTrustableFile(
 // static
 std::unique_ptr<BundledExchangesHandle>
 BundledExchangesHandle::CreateForTrackedNavigation(
-    scoped_refptr<BundledExchangesReader> reader) {
+    scoped_refptr<BundledExchangesReader> reader,
+    int frame_tree_node_id) {
   auto handle = base::WrapUnique(new BundledExchangesHandle());
   if (reader->source().is_trusted()) {
     handle->SetInterceptor(
@@ -669,14 +674,16 @@ BundledExchangesHandle::CreateForTrackedNavigation(
             std::move(reader),
             base::BindOnce(
                 &BundledExchangesHandle::OnBundledExchangesFileLoaded,
-                handle->weak_factory_.GetWeakPtr())));
+                handle->weak_factory_.GetWeakPtr()),
+            frame_tree_node_id));
   } else {
     handle->SetInterceptor(
         std::make_unique<InterceptorForTrackedNavigationFromFile>(
             std::move(reader),
             base::BindOnce(
                 &BundledExchangesHandle::OnBundledExchangesFileLoaded,
-                handle->weak_factory_.GetWeakPtr())));
+                handle->weak_factory_.GetWeakPtr()),
+            frame_tree_node_id));
   }
   return handle;
 }

content/browser/web_package/bundled_exchanges_handle.h

@@ -33,7 +33,8 @@ class BundledExchangesHandle {
       std::unique_ptr<BundledExchangesSource> source,
       int frame_tree_node_id);
   static std::unique_ptr<BundledExchangesHandle> CreateForTrackedNavigation(
-      scoped_refptr<BundledExchangesReader> reader);
+      scoped_refptr<BundledExchangesReader> reader,
+      int frame_tree_node_id);
   static std::unique_ptr<BundledExchangesHandle> CreateForNavigationInfo(
       std::unique_ptr<BundledExchangesNavigationInfo> navigation_info,
       int frame_tree_node_id);

content/browser/web_package/bundled_exchanges_handle_tracker.cc

@@ -25,8 +25,10 @@ std::unique_ptr<BundledExchangesHandle>
 BundledExchangesHandleTracker::MaybeCreateBundledExchangesHandle(
     const GURL& url,
     int frame_tree_node_id) {
-  if (reader_->HasEntry(url))
-    return BundledExchangesHandle::CreateForTrackedNavigation(reader_);
+  if (reader_->HasEntry(url)) {
+    return BundledExchangesHandle::CreateForTrackedNavigation(
+        reader_, frame_tree_node_id);
+  }
   if (!reader_->source().is_trusted() &&
       url == bundled_exchanges_utils::GetSynthesizedUrlForBundledExchanges(
                  reader_->source().url(), target_inner_url_)) {

content/browser/web_package/bundled_exchanges_reader.cc

@@ -163,7 +163,13 @@ void BundledExchangesReader::ReadResponse(const GURL& url,
 
   auto it = entries_.find(net::SimplifyUrlForRequest(url));
   if (it == entries_.end() || it->second->response_locations.empty()) {
-    PostTask(FROM_HERE, base::BindOnce(std::move(callback), nullptr));
+    PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            std::move(callback), nullptr,
+            data_decoder::mojom::BundleResponseParseError::New(
+                data_decoder::mojom::BundleParseErrorType::kParserInternalError,
+                "Not found in Web Bundle file.")));
     return;
   }
 
@@ -264,8 +270,7 @@ void BundledExchangesReader::OnResponseParsed(
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(metadata_ready_);
 
-  // TODO(crbug.com/966753): Handle |error|.
-  std::move(callback).Run(std::move(response));
+  std::move(callback).Run(std::move(response), std::move(error));
 }
 
 }  // namespace content

content/browser/web_package/bundled_exchanges_reader.h

@@ -49,8 +49,9 @@ class CONTENT_EXPORT BundledExchangesReader final
   // Gets data_decoder::mojom::BundleResponsePtr for the given |url| that
   // contains response headers and range information for its body.
   // Should be called after ReadMetadata finishes.
-  using ResponseCallback =
-      base::OnceCallback<void(data_decoder::mojom::BundleResponsePtr)>;
+  using ResponseCallback = base::OnceCallback<void(
+      data_decoder::mojom::BundleResponsePtr,
+      data_decoder::mojom::BundleResponseParseErrorPtr)>;
   void ReadResponse(const GURL& url, ResponseCallback callback);
 
   // Starts loading response body. |response| should be obtained by

content/browser/web_package/bundled_exchanges_reader_unittest.cc

@@ -88,14 +88,17 @@ TEST_F(BundledExchangesReaderTest, ReadResponse) {
 
   GetMockFactory()->ReadAndFullfillResponse(
       GetReader(), GetPrimaryURL(), std::move(response),
-      base::BindOnce([](data_decoder::mojom::BundleResponsePtr response) {
-        EXPECT_TRUE(response);
-        if (response) {
-          EXPECT_EQ(200, response->response_code);
-          EXPECT_EQ(0xdeadu, response->payload_offset);
-          EXPECT_EQ(0xbeafu, response->payload_length);
-        }
-      }));
+      base::BindOnce(
+          [](data_decoder::mojom::BundleResponsePtr response,
+             data_decoder::mojom::BundleResponseParseErrorPtr error) {
+            EXPECT_TRUE(response);
+            EXPECT_FALSE(error);
+            if (response) {
+              EXPECT_EQ(200, response->response_code);
+              EXPECT_EQ(0xdeadu, response->payload_offset);
+              EXPECT_EQ(0xbeafu, response->payload_length);
+            }
+          }));
 }
 
 TEST_F(BundledExchangesReaderTest, ReadResponseForURLContainingUserAndPass) {
@@ -112,14 +115,17 @@ TEST_F(BundledExchangesReaderTest, ReadResponseForURLContainingUserAndPass) {
 
   GetMockFactory()->ReadAndFullfillResponse(
       GetReader(), url, std::move(response),
-      base::BindOnce([](data_decoder::mojom::BundleResponsePtr response) {
-        EXPECT_TRUE(response);
-        if (response) {
-          EXPECT_EQ(200, response->response_code);
-          EXPECT_EQ(0xdeadu, response->payload_offset);
-          EXPECT_EQ(0xbeafu, response->payload_length);
-        }
-      }));
+      base::BindOnce(
+          [](data_decoder::mojom::BundleResponsePtr response,
+             data_decoder::mojom::BundleResponseParseErrorPtr error) {
+            EXPECT_TRUE(response);
+            EXPECT_FALSE(error);
+            if (response) {
+              EXPECT_EQ(200, response->response_code);
+              EXPECT_EQ(0xdeadu, response->payload_offset);
+              EXPECT_EQ(0xbeafu, response->payload_length);
+            }
+          }));
 }
 
 TEST_F(BundledExchangesReaderTest, ReadResponseForURLContainingFragment) {
@@ -136,14 +142,17 @@ TEST_F(BundledExchangesReaderTest, ReadResponseForURLContainingFragment) {
 
   GetMockFactory()->ReadAndFullfillResponse(
       GetReader(), url, std::move(response),
-      base::BindOnce([](data_decoder::mojom::BundleResponsePtr response) {
-        EXPECT_TRUE(response);
-        if (response) {
-          EXPECT_EQ(200, response->response_code);
-          EXPECT_EQ(0xdeadu, response->payload_offset);
-          EXPECT_EQ(0xbeafu, response->payload_length);
-        }
-      }));
+      base::BindOnce(
+          [](data_decoder::mojom::BundleResponsePtr response,
+             data_decoder::mojom::BundleResponseParseErrorPtr error) {
+            EXPECT_TRUE(response);
+            EXPECT_FALSE(error);
+            if (response) {
+              EXPECT_EQ(200, response->response_code);
+              EXPECT_EQ(0xdeadu, response->payload_offset);
+              EXPECT_EQ(0xbeafu, response->payload_length);
+            }
+          }));
 }
 
 TEST_F(BundledExchangesReaderTest, ReadResponseBody) {

content/browser/web_package/bundled_exchanges_url_loader_factory.cc

@@ -12,6 +12,8 @@
 #include "base/strings/string_util.h"
 #include "base/task/post_task.h"
 #include "content/browser/web_package/bundled_exchanges_reader.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/web_contents.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/receiver.h"
@@ -58,6 +60,19 @@ network::ResourceResponseHead CreateResourceResponse(
   return response_head;
 }
 
+void AddResponseParseErrorMessageToConsole(
+    int frame_tree_node_id,
+    const data_decoder::mojom::BundleResponseParseErrorPtr& error) {
+  WebContents* web_contents =
+      WebContents::FromFrameTreeNodeId(frame_tree_node_id);
+  if (!web_contents)
+    return;
+  web_contents->GetMainFrame()->AddMessageToConsole(
+      blink::mojom::ConsoleMessageLevel::kError,
+      std::string("Failed to read response header of Web Bundle file: ") +
+          error->message);
+}
+
 }  // namespace
 
 // TODO(crbug.com/966753): Consider security models, i.e. plausible CORS
@@ -67,8 +82,11 @@ class BundledExchangesURLLoaderFactory::EntryLoader final
  public:
   EntryLoader(base::WeakPtr<BundledExchangesURLLoaderFactory> factory,
               mojo::PendingRemote<network::mojom::URLLoaderClient> client,
-              const network::ResourceRequest& resource_request)
-      : factory_(std::move(factory)), loader_client_(std::move(client)) {
+              const network::ResourceRequest& resource_request,
+              int frame_tree_node_id)
+      : factory_(std::move(factory)),
+        loader_client_(std::move(client)),
+        frame_tree_node_id_(frame_tree_node_id) {
     DCHECK(factory_);
 
     // Parse the Range header if any.
@@ -100,13 +118,16 @@ class BundledExchangesURLLoaderFactory::EntryLoader final
   void PauseReadingBodyFromNet() override {}
   void ResumeReadingBodyFromNet() override {}
 
-  void OnResponseReady(data_decoder::mojom::BundleResponsePtr response) {
+  void OnResponseReady(data_decoder::mojom::BundleResponsePtr response,
+                       data_decoder::mojom::BundleResponseParseErrorPtr error) {
     if (!factory_ || !loader_client_.is_connected())
       return;
 
     // TODO(crbug.com/990733): For the initial implementation, we allow only
     // net::HTTP_OK, but we should clarify acceptable status code in the spec.
     if (!response || response->response_code != net::HTTP_OK) {
+      if (error)
+        AddResponseParseErrorMessageToConsole(frame_tree_node_id_, error);
       loader_client_->OnComplete(network::URLLoaderCompletionStatus(
           net::ERR_INVALID_BUNDLED_EXCHANGES));
       return;
@@ -170,6 +191,7 @@ class BundledExchangesURLLoaderFactory::EntryLoader final
 
   base::WeakPtr<BundledExchangesURLLoaderFactory> factory_;
   mojo::Remote<network::mojom::URLLoaderClient> loader_client_;
+  const int frame_tree_node_id_;
   base::Optional<net::HttpByteRange> byte_range_;
 
   base::WeakPtrFactory<EntryLoader> weak_factory_{this};
@@ -178,8 +200,9 @@ class BundledExchangesURLLoaderFactory::EntryLoader final
 };
 
 BundledExchangesURLLoaderFactory::BundledExchangesURLLoaderFactory(
-    scoped_refptr<BundledExchangesReader> reader)
-    : reader_(std::move(reader)) {}
+    scoped_refptr<BundledExchangesReader> reader,
+    int frame_tree_node_id)
+    : reader_(std::move(reader)), frame_tree_node_id_(frame_tree_node_id) {}
 
 BundledExchangesURLLoaderFactory::~BundledExchangesURLLoaderFactory() = default;
 
@@ -199,9 +222,9 @@ void BundledExchangesURLLoaderFactory::CreateLoaderAndStart(
   if (base::EqualsCaseInsensitiveASCII(resource_request.method,
                                        net::HttpRequestHeaders::kGetMethod) &&
       reader_->HasEntry(resource_request.url)) {
-    auto loader = std::make_unique<EntryLoader>(weak_factory_.GetWeakPtr(),
-                                                loader_client.PassInterface(),
-                                                resource_request);
+    auto loader = std::make_unique<EntryLoader>(
+        weak_factory_.GetWeakPtr(), loader_client.PassInterface(),
+        resource_request, frame_tree_node_id_);
     std::unique_ptr<network::mojom::URLLoader> url_loader = std::move(loader);
     mojo::MakeSelfOwnedReceiver(
         std::move(url_loader), mojo::PendingReceiver<network::mojom::URLLoader>(

content/browser/web_package/bundled_exchanges_url_loader_factory.h

@@ -24,7 +24,8 @@ class CONTENT_EXPORT BundledExchangesURLLoaderFactory final
     : public network::mojom::URLLoaderFactory {
  public:
   explicit BundledExchangesURLLoaderFactory(
-      scoped_refptr<BundledExchangesReader> reader);
+      scoped_refptr<BundledExchangesReader> reader,
+      int frame_tree_node_id);
   ~BundledExchangesURLLoaderFactory() override;
 
   // Set a |network::mojom::URLLoaderFactory| remote interface used for requests
@@ -55,6 +56,7 @@ class CONTENT_EXPORT BundledExchangesURLLoaderFactory final
 
   mojo::ReceiverSet<network::mojom::URLLoaderFactory> receivers_;
   scoped_refptr<BundledExchangesReader> reader_;
+  const int frame_tree_node_id_;
   mojo::Remote<network::mojom::URLLoaderFactory> fallback_factory_;
 
   base::WeakPtrFactory<BundledExchangesURLLoaderFactory> weak_factory_{this};

content/browser/web_package/bundled_exchanges_url_loader_factory_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/optional.h"
 #include "base/run_loop.h"
 #include "base/test/task_environment.h"
+#include "content/browser/frame_host/frame_tree_node.h"
 #include "content/browser/web_package/bundled_exchanges_reader.h"
 #include "content/browser/web_package/mock_bundled_exchanges_reader_factory.h"
 #include "mojo/public/cpp/bindings/remote.h"
@@ -34,8 +35,8 @@ class BundledExchangesURLLoaderFactoryTest : public testing::Test {
     mock_factory_ = MockBundledExchangesReaderFactory::Create();
     auto reader = mock_factory_->CreateReader(body_);
     reader_ = reader.get();
-    loader_factory_ =
-        std::make_unique<BundledExchangesURLLoaderFactory>(std::move(reader));
+    loader_factory_ = std::make_unique<BundledExchangesURLLoaderFactory>(
+        std::move(reader), FrameTreeNode::kFrameTreeNodeInvalidId);
 
     base::flat_map<GURL, data_decoder::mojom::BundleIndexValuePtr> items;
     data_decoder::mojom::BundleIndexValuePtr item =

content/browser/web_package/mock_bundled_exchanges_reader_factory.cc

@@ -207,8 +207,10 @@ class MockBundledExchangesReaderFactoryImpl final
         url, base::BindOnce(
                  [](base::Closure quit_closure,
                     BundledExchangesReader::ResponseCallback callback,
-                    data_decoder::mojom::BundleResponsePtr response) {
-                   std::move(callback).Run(std::move(response));
+                    data_decoder::mojom::BundleResponsePtr response,
+                    data_decoder::mojom::BundleResponseParseErrorPtr error) {
+                   std::move(callback).Run(std::move(response),
+                                           std::move(error));
                    std::move(quit_closure).Run();
                  },
                  run_loop.QuitClosure(), std::move(callback)));

content/test/data/bundled_exchanges/broken_bundle/index.html

+<!DOCTYPE html>
+<title>Ready</title>>
+<body>
+</body>

content/test/data/bundled_exchanges/broken_bundle/script.js

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+document.title = "Done";

content/test/data/bundled_exchanges/generate-test-wbns.sh

@@ -19,3 +19,29 @@ gen-bundle \
   -dir bundled_exchanges_browsertest/ \
   -manifestURL https://test.example.org/manifest.webmanifest \
   -o bundled_exchanges_browsertest.wbn
+
+# Generate a base WBN which will used to generate broken WBN.
+# This WBN must contains 3 entries:
+#   [1]: https://test.example.org/
+#   [2]: https://test.example.org/index.html
+#   [3]: https://test.example.org/script.html
+gen-bundle \
+  -version b1 \
+  -baseURL https://test.example.org/ \
+  -primaryURL https://test.example.org/ \
+  -dir broken_bundle/ \
+  -o broken_bundle_base.wbn
+
+# Rewrite ":status" (3a737461747573) header of the first entry to ":xxxxxx"
+# (3a787878787878).
+xxd -p broken_bundle_base.wbn |
+  tr -d '\n' |
+  sed 's/3a737461747573/3a787878787878/' |
+  xxd -r -p > broken_bundle_broken_first_entry.wbn
+
+# Rewrite ":status" (3a737461747573) header of the third entry (script.js) to
+# ":xxxxxx" (3a787878787878).
+xxd -p broken_bundle_base.wbn |
+  tr -d '\n' |
+  sed 's/3a737461747573/3a787878787878/3' |
+  xxd -r -p > broken_bundle_broken_script_entry.wbn