CORB/ORB: Report issue to DevTools for CORB/ORB errors.

CORB (and ORB) currently use a console message to highlight CORB/ORB errors. This adds a DevTools "issue", which has a nicer UI with UI translation, links to explainers, and hyperlinks to the offending network request in the "network" tab. This requires a companion CL in the devtools frontend to do something useful, and a follow-on to actually remove the console message. This CL contains browser_protocol.pdl and needs to land first. Removing the console message requires this + devtools CL to land first. Devtools companion CL: crrev.com/c/4618976 Remove console message + cleanup: crrev.com/c/4359633 Bug: 1178928 Change-Id: I2cd5a8f54cc180943f0fecd8b2aec59ff485580d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4624106 Commit-Queue: Daniel Vogelheim <vogelheim@chromium.org> Reviewed-by: Alex Rudenko <alexrudenko@chromium.org> Reviewed-by: Andrey Kosyakov <caseq@chromium.org> Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org> Cr-Commit-Position: refs/heads/main@{#1164727}
Denger-Network · Jun 30, 2023 · 12764f4 · 12764f4
1 parent 0055602
commit 12764f4
Show file tree

Hide file tree

Showing 13 changed files with 131 additions and 21 deletions.
diff --git a/content/browser/devtools/devtools_instrumentation.cc b/content/browser/devtools/devtools_instrumentation.cc
@@ -1912,6 +1912,9 @@ protocol::Audits::GenericIssueErrorType GenericIssueErrorTypeToProtocol(
         kFormInputHasWrongButWellIntendedAutocompleteValueError:
       return protocol::Audits::GenericIssueErrorTypeEnum::
           FormInputHasWrongButWellIntendedAutocompleteValueError;
+    case blink::mojom::GenericIssueErrorType::kResponseWasBlockedByORB:
+      return protocol::Audits::GenericIssueErrorTypeEnum::
+          ResponseWasBlockedByORB;
   }
 }
 

diff --git a/content/browser/devtools/network_service_devtools_observer.cc b/content/browser/devtools/network_service_devtools_observer.cc
@@ -27,6 +27,16 @@ void DispatchToAgents(DevToolsAgentHostImpl* agent_host,
     (h->*method)(std::forward<Args>(args)...);
 }
 
+RenderFrameHostImpl* GetRenderFrameHostImplFrom(int frame_tree_node_id) {
+  auto* ftn = FrameTreeNode::GloballyFindByID(frame_tree_node_id);
+  if (!ftn) {
+    return nullptr;
+  }
+
+  RenderFrameHostImpl* rfhi = ftn->current_frame_host();
+  return rfhi;
+}
+
 }  // namespace
 
 NetworkServiceDevToolsObserver::NetworkServiceDevToolsObserver(
@@ -193,14 +203,7 @@ void NetworkServiceDevToolsObserver::OnCorsError(
     const GURL& url,
     const network::CorsErrorStatus& cors_error_status,
     bool is_warning) {
-  if (frame_tree_node_id_ == FrameTreeNode::kFrameTreeNodeInvalidId)
-    return;
-
-  auto* ftn = FrameTreeNode::GloballyFindByID(frame_tree_node_id_);
-  if (!ftn)
-    return;
-
-  RenderFrameHostImpl* rfhi = ftn->current_frame_host();
+  RenderFrameHostImpl* rfhi = GetRenderFrameHostImplFrom(frame_tree_node_id_);
   if (!rfhi)
     return;
 
@@ -256,6 +259,36 @@ void NetworkServiceDevToolsObserver::OnCorsError(
   devtools_instrumentation::ReportBrowserInitiatedIssue(rfhi, issue.get());
 }
 
+void NetworkServiceDevToolsObserver::OnCorbError(
+    const absl::optional<std::string>& devtools_request_id,
+    const GURL& url) {
+  RenderFrameHostImpl* rfhi = GetRenderFrameHostImplFrom(frame_tree_node_id_);
+  if (!rfhi) {
+    return;
+  }
+
+  std::unique_ptr<protocol::Audits::AffectedRequest> affected_request =
+      protocol::Audits::AffectedRequest::Create()
+          .SetRequestId(devtools_request_id.value_or(""))
+          .SetUrl(url.spec())
+          .Build();
+  auto generic_details =
+      protocol::Audits::GenericIssueDetails::Create()
+          .SetErrorType(protocol::Audits::GenericIssueErrorTypeEnum::
+                            ResponseWasBlockedByORB)
+          .SetRequest(std::move(affected_request))
+          .Build();
+  auto details = protocol::Audits::InspectorIssueDetails::Create()
+                     .SetGenericIssueDetails(std::move(generic_details))
+                     .Build();
+  auto issue =
+      protocol::Audits::InspectorIssue::Create()
+          .SetCode(protocol::Audits::InspectorIssueCodeEnum::GenericIssue)
+          .SetDetails(std::move(details))
+          .Build();
+  devtools_instrumentation::ReportBrowserInitiatedIssue(rfhi, issue.get());
+}
+
 void NetworkServiceDevToolsObserver::OnSubresourceWebBundleMetadata(
     const std::string& devtools_request_id,
     const std::vector<GURL>& urls) {

diff --git a/content/browser/devtools/network_service_devtools_observer.h b/content/browser/devtools/network_service_devtools_observer.h
@@ -78,6 +78,8 @@ class NetworkServiceDevToolsObserver : public network::mojom::DevToolsObserver {
                    const GURL& url,
                    const network::CorsErrorStatus& status,
                    bool is_warning) override;
+  void OnCorbError(const absl::optional<std::string>& devtools_request_id,
+                   const GURL& url) override;
   void OnSubresourceWebBundleMetadata(const std::string& devtools_request_id,
                                       const std::vector<GURL>& urls) override;
   void OnSubresourceWebBundleMetadataError(

diff --git a/services/network/cors/cors_url_loader.cc b/services/network/cors/cors_url_loader.cc
@@ -818,6 +818,10 @@ void CorsURLLoader::ReportCorsErrorToDevTools(const CorsErrorStatus& status,
       CloneClientSecurityState(), request_.url, status, is_warning);
 }
 
+void CorsURLLoader::ReportCorbErrorToDevTools() {
+  devtools_observer_->OnCorbError(request_.devtools_request_id, request_.url);
+}
+
 absl::optional<URLLoaderCompletionStatus> CorsURLLoader::ConvertPreflightResult(
     int net_error,
     absl::optional<CorsErrorStatus> status) {
@@ -1000,6 +1004,9 @@ void CorsURLLoader::HandleComplete(URLLoaderCompletionStatus status) {
   if (devtools_observer_ && status.cors_error_status) {
     ReportCorsErrorToDevTools(*status.cors_error_status);
   }
+  if (devtools_observer_ && status.should_report_corb_blocking) {
+    ReportCorbErrorToDevTools();
+  }
 
   // If we detect a private network access when we were not expecting one, we
   // restart the request and force a preflight request. This preflight and the

diff --git a/services/network/cors/cors_url_loader.h b/services/network/cors/cors_url_loader.h
@@ -152,6 +152,9 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CorsURLLoader
   void ReportCorsErrorToDevTools(const CorsErrorStatus& status,
                                  bool is_warning = false);
 
+  // Reports a Corb/ORB error for `request_` to DevTools, if possible.
+  void ReportCorbErrorToDevTools();
+
   // Handles OnComplete() callback.
   void HandleComplete(URLLoaderCompletionStatus status);
 

diff --git a/services/network/cors/preflight_controller_unittest.cc b/services/network/cors/preflight_controller_unittest.cc
@@ -373,6 +373,9 @@ class MockDevToolsObserver : public mojom::DevToolsObserver {
                    const network::CorsErrorStatus& status,
                    bool is_warning) override {}
 
+  void OnCorbError(const absl::optional<std::string>& devtools_request_id,
+                   const GURL& url) override {}
+
   void Clone(mojo::PendingReceiver<DevToolsObserver> observer) override {
     receivers_.Add(this, std::move(observer));
   }

diff --git a/services/network/public/mojom/devtools_observer.mojom b/services/network/public/mojom/devtools_observer.mojom
@@ -152,6 +152,15 @@ interface DevToolsObserver {
     CorsErrorStatus status,
     bool is_warning);
 
+  // Called to report a CORB or ORB error. This callback is intended to
+  // deliver a DevTools issue.
+  //
+  // |devtools_request_id| id of the affected request.
+  // |url| is the URL that was blocked by CORB.
+  OnCorbError(
+    string? devtools_request_id,
+    url.mojom.Url url);
+
   // Called when parsing the .wbn file has succeeded. The event
   // contains the information about the web bundle contents.
   OnSubresourceWebBundleMetadata(

diff --git a/services/network/test/mock_devtools_observer.h b/services/network/test/mock_devtools_observer.h
@@ -111,6 +111,12 @@ class MockDevToolsObserver : public mojom::DevToolsObserver {
                    const network::CorsErrorStatus& status,
                    bool is_warning) override;
 
+  MOCK_METHOD(void,
+              OnCorbError,
+              (const absl::optional<std::string>& devtools_request_id,
+               const GURL& url),
+              (override));
+
   void Clone(mojo::PendingReceiver<DevToolsObserver> observer) override;
 
   void WaitUntilRawResponse(size_t goal);

diff --git a/third_party/blink/public/devtools_protocol/browser_protocol.pdl b/third_party/blink/public/devtools_protocol/browser_protocol.pdl
@@ -737,6 +737,7 @@ experimental domain Audits
       FormLabelHasNeitherForNorNestedInput
       FormLabelForMatchesNonExistingIdError
       FormInputHasWrongButWellIntendedAutocompleteValueError
+      ResponseWasBlockedByORB
 
   # Depending on the concrete errorType, different properties are set.
   type GenericIssueDetails extends object
@@ -746,6 +747,7 @@ experimental domain Audits
       optional Page.FrameId frameId
       optional DOM.BackendNodeId violatingNodeId
       optional string violatingNodeAttribute
+      optional AffectedRequest request
 
   # This issue tracks information needed to print a deprecation message.
   # https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/third_party/blink/renderer/core/frame/deprecation/README.md

diff --git a/third_party/blink/public/mojom/devtools/inspector_issue.mojom b/third_party/blink/public/mojom/devtools/inspector_issue.mojom
@@ -253,6 +253,7 @@ enum GenericIssueErrorType {
   kFormLabelHasNeitherForNorNestedInput,
   kFormLabelForMatchesNonExistingIdError,
   kFormInputHasWrongButWellIntendedAutocompleteValueError,
+  kResponseWasBlockedByORB,
 };
 
 struct GenericIssueDetails {

diff --git a/third_party/blink/renderer/core/inspector/inspector_audits_issue.cc b/third_party/blink/renderer/core/inspector/inspector_audits_issue.cc
@@ -135,6 +135,9 @@ AuditsIssue::GenericIssueErrorTypeToProtocol(
         kFormInputHasWrongButWellIntendedAutocompleteValueError:
       return protocol::Audits::GenericIssueErrorTypeEnum::
           FormInputHasWrongButWellIntendedAutocompleteValueError;
+    case mojom::blink::GenericIssueErrorType::kResponseWasBlockedByORB:
+      return protocol::Audits::GenericIssueErrorTypeEnum::
+          ResponseWasBlockedByORB;
   }
 }
 

diff --git a/...b_tests/http/tests/inspector-protocol/network/block_cross_site_document_load-expected.txt b/...b_tests/http/tests/inspector-protocol/network/block_cross_site_document_load-expected.txt
@@ -1,4 +1,4 @@
-Tests the blockedCrossSiteDocumentLoad bit available as a part of the loading finished signal.
+Tests that CORB/ORB blocking gets reported as an issue to DevTools.
 Blocking cross-site document at http://devtools.oopif.test:8000/inspector-protocol/network/resources/nosniff.pl: shouldReportCorbBlocking=true.
 Blocking cross-site document at http://devtools.oopif.test:8000/inspector-protocol/network/resources/simple-iframe.html: shouldReportCorbBlocking=true.
 Blocking, but not reporting cross-site document at http://devtools.oopif.test:8000/inspector-protocol/network/resources/204.pl: shouldReportCorbBlocking=false.

diff --git a/...y/blink/web_tests/http/tests/inspector-protocol/network/block_cross_site_document_load.js b/...y/blink/web_tests/http/tests/inspector-protocol/network/block_cross_site_document_load.js
@@ -1,21 +1,61 @@
 (async function(testRunner) {
   var {page, session, dp} = await testRunner.startURL(
       '../resources/test-page.html',
-      `Tests the blockedCrossSiteDocumentLoad bit available as a part of the loading finished signal.`);
+      `Tests that CORB/ORB blocking gets reported as an issue to DevTools.`);
 
-  const responses = new Map();
-  await dp.Network.enable();
+  await dp.Audits.enable();
+
+  // Returns a promise that returns true if a ResponseWasBlockedByORB issue
+  // was added. It expectes a "probe" issue for a fetch to `probe_url` in
+  // order to determine (without a race condition) at what point in time the
+  // issue should have been received.
+  // We do some sanity checks on the issue parameters while we're here.
+  function expectIssueAdded(issueHasBeenAdded) {
+    return dp.Audits.onceIssueAdded().then(issue => {
+      const genericIssueDetails = issue?.params?.issue?.details?.genericIssueDetails;
+      if (!genericIssueDetails)
+        throw new Error('Unexpected issue shape.');
+      if (genericIssueDetails.errorType !== 'ResponseWasBlockedByORB')
+        throw new Error('Unexpected issue errorType.');
+      if (!genericIssueDetails?.request?.url)
+        throw new Error('Unexpected issue shape.');
+      if (genericIssueDetails.request.url.includes('probe.test'))
+        return issueHasBeenAdded;
+      return expectIssueAdded(true);
+    });
+  }
+
+  function shouldReportCorbBlocking() {
+    // This returns a promise which returns whether a DevTools issue (other
+    // than for "probe.test") was added.
+    return expectIssueAdded(false);
+  }
+
+  // In order to recognize whether the sub-test a completed, we'll always
+  // follow up with a URL that definitely gets (C)ORB-blocked. We'll complete
+  // the test when we receive an issue for that probe url. That gives us a
+  // non-racy point in time where the previous url - which we want to test
+  // for - must have been received.
+  const probe_url = 'http://probe.test:8000/inspector-protocol/network/resources/nosniff.pl';
+  function loadImageAndProbe(url) {
+    return `
+        probe = function() { new Image().src = '${probe_url}'; }
+        img = new Image();
+        img.src = '${url}';
+        img.onerror = probe;
+        img.onload = probe;
+    `;
+  }
 
   let blocked_urls = [
     'http://devtools.oopif.test:8000/inspector-protocol/network/resources/nosniff.pl',
     'http://devtools.oopif.test:8000/inspector-protocol/network/resources/simple-iframe.html',
   ];
   for (const url of blocked_urls) {
-    session.evaluate(`new Image().src = '${url}';`);
-    const response = await dp.Network.onceLoadingFinished();
+    session.evaluate(loadImageAndProbe(url));
     testRunner.log(
         `Blocking cross-site document at ${url}: ` +
-        `shouldReportCorbBlocking=${response.params.shouldReportCorbBlocking}.`);
+        `shouldReportCorbBlocking=${await shouldReportCorbBlocking()}.`);
   }
 
   let blocked_unreported_urls = [
@@ -24,11 +64,10 @@
     'http://devtools.oopif.test:8000/inspector-protocol/network/resources/content-length-0.pl',
   ];
   for (const url of blocked_unreported_urls) {
-    session.evaluate(`new Image().src = '${url}';`);
-    const response = await dp.Network.onceLoadingFinished();
+    session.evaluate(loadImageAndProbe(url));
     testRunner.log(
         `Blocking, but not reporting cross-site document at ${url}: ` +
-        `shouldReportCorbBlocking=${response.params.shouldReportCorbBlocking}.`);
+        `shouldReportCorbBlocking=${await shouldReportCorbBlocking()}.`);
   }
 
   let allowed_urls = [
@@ -37,11 +76,10 @@
     'http://devtools.oopif.test:8000/inspector-protocol/network/resources/test.css',
   ]
   for (const url of allowed_urls) {
-    session.evaluate(`new Image().src = '${url}';`);
-    const response = await dp.Network.onceLoadingFinished();
+    session.evaluate(loadImageAndProbe(url));
     testRunner.log(
         `Allowing cross-site document at ${url}: ` +
-        `shouldReportCorbBlocking=${response.params.shouldReportCorbBlocking}.`);
+        `shouldReportCorbBlocking=${await shouldReportCorbBlocking()}.`);
   }
 
   testRunner.completeTest();