[devtools] Add browser-side issue storage

Design doc: https://bit.ly/browser-issue-storage

This CL adds an Issue storage in the browser process, so Issues
no longer need to be sent to the renderer process to be stored and
reported there.

Characteristics of the storage:
  - The issue count is capped at 1000 issues per WebContent
  - The storage is cleared for main frame navigations (modulo redirects)
  - When subframes get deleted, issues associated with the deleted
    subframe get "re-parented" to the main frame

Bug: chromium:1063007
Change-Id: I75f164812770e8c73fc388d27767cc9151e5e4af
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2246139
Commit-Queue: Simon Zünd <szuend@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Sigurd Schneider <sigurds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#781237}

content/browser/BUILD.gn

@@ -739,6 +739,8 @@ jumbo_source_set("browser") {
     "devtools/devtools_instrumentation.h",
     "devtools/devtools_io_context.cc",
     "devtools/devtools_io_context.h",
+    "devtools/devtools_issue_storage.cc",
+    "devtools/devtools_issue_storage.h",
     "devtools/devtools_manager.cc",
     "devtools/devtools_manager.h",
     "devtools/devtools_pipe_handler.cc",

content/browser/devtools/devtools_instrumentation.cc

@@ -7,7 +7,9 @@
 #include "components/download/public/common/download_create_info.h"
 #include "components/download/public/common/download_item.h"
 #include "content/browser/devtools/browser_devtools_agent_host.h"
+#include "content/browser/devtools/devtools_issue_storage.h"
 #include "content/browser/devtools/devtools_url_loader_interceptor.h"
+#include "content/browser/devtools/protocol/audits.h"
 #include "content/browser/devtools/protocol/audits_handler.h"
 #include "content/browser/devtools/protocol/browser_handler.h"
 #include "content/browser/devtools/protocol/emulation_handler.h"
@@ -828,12 +830,28 @@ void ReportSameSiteCookieIssue(
           std::move(details)));
 }
 
+namespace {
+
+void AddIssueToIssueStorage(
+    RenderFrameHost* frame,
+    std::unique_ptr<protocol::Audits::InspectorIssue> issue) {
+  WebContents* web_contents = WebContents::FromRenderFrameHost(frame);
+  DevToolsIssueStorage* issue_storage =
+      DevToolsIssueStorage::GetOrCreateForWebContents(web_contents);
+
+  issue_storage->AddInspectorIssue(frame->GetFrameTreeNodeId(),
+                                   std::move(issue));
+}
+
+}  // namespace
+
 void ReportBrowserInitiatedIssue(RenderFrameHostImpl* frame,
                                  protocol::Audits::InspectorIssue* issue) {
   FrameTreeNode* ftn = frame->frame_tree_node();
   if (!ftn)
     return;
 
+  AddIssueToIssueStorage(frame, issue->clone());
   DispatchToAgents(ftn, &protocol::AuditsHandler::OnIssueAdded, issue);
 }
 

content/browser/devtools/devtools_instrumentation.h

@@ -195,8 +195,9 @@ void ReportSameSiteCookieIssue(
 //
 // DevTools must be attached, otherwise issues reported through
 // |ReportBrowserInitiatedIssue| are lost.
-void ReportBrowserInitiatedIssue(RenderFrameHostImpl* frame,
-                                 protocol::Audits::InspectorIssue* issue);
+void CONTENT_EXPORT
+ReportBrowserInitiatedIssue(RenderFrameHostImpl* frame,
+                            protocol::Audits::InspectorIssue* issue);
 
 // Produces a Heavy Ad Issue based on the parameters passed in.
 std::unique_ptr<protocol::Audits::InspectorIssue> GetHeavyAdIssue(

content/browser/devtools/devtools_issue_storage.cc

+// Copyright 2020 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.
+
+#include "content/browser/devtools/devtools_issue_storage.h"
+
+#include "content/browser/devtools/protocol/audits.h"
+#include "content/public/browser/navigation_handle.h"
+#include "ui/base/page_transition_types.h"
+
+namespace content {
+
+static const unsigned kMaxIssueCount = 1000;
+
+WEB_CONTENTS_USER_DATA_KEY_IMPL(DevToolsIssueStorage)
+
+DevToolsIssueStorage::DevToolsIssueStorage(WebContents* contents)
+    : WebContentsObserver(contents) {}
+DevToolsIssueStorage::~DevToolsIssueStorage() = default;
+
+void DevToolsIssueStorage::AddInspectorIssue(
+    int frame_tree_node_id,
+    std::unique_ptr<protocol::Audits::InspectorIssue> issue) {
+  DCHECK_LE(issues_.size(), kMaxIssueCount);
+  if (issues_.size() == kMaxIssueCount) {
+    issues_.pop_front();
+  }
+  issues_.emplace_back(frame_tree_node_id, std::move(issue));
+}
+
+std::vector<const protocol::Audits::InspectorIssue*>
+DevToolsIssueStorage::FilterIssuesBy(
+    const base::flat_set<int>& frame_tree_node_ids) const {
+  std::vector<const protocol::Audits::InspectorIssue*> issues;
+  for (const auto& entry : issues_) {
+    if (frame_tree_node_ids.contains(entry.first)) {
+      issues.push_back(entry.second.get());
+    }
+  }
+  return issues;
+}
+
+void DevToolsIssueStorage::DidFinishNavigation(
+    NavigationHandle* navigation_handle) {
+  if (!navigation_handle->IsInMainFrame())
+    return;
+  if (!navigation_handle->HasCommitted())
+    return;
+
+  const auto transition = navigation_handle->GetPageTransition();
+  if (ui::PageTransitionIsRedirect(transition))
+    return;
+
+  issues_.clear();
+}
+
+void DevToolsIssueStorage::FrameDeleted(RenderFrameHost* render_frame_host) {
+  // Deletion of the main frame causes the DevToolsIssueStorage to be cleaned
+  // up. Also there would no longer be a root frame we could re-parent issues
+  // on.
+  auto* const main_frame = render_frame_host->GetMainFrame();
+  if (render_frame_host == main_frame)
+    return;
+
+  // Reassign issues from the deleted frame to the root frame.
+  const int frame_id = render_frame_host->GetFrameTreeNodeId();
+  const int main_frame_id = main_frame->GetFrameTreeNodeId();
+  for (auto& entry : issues_) {
+    if (entry.first == frame_id) {
+      entry.first = main_frame_id;
+    }
+  }
+}
+
+}  // namespace content

content/browser/devtools/devtools_issue_storage.h

+// Copyright 2020 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.
+
+#ifndef CONTENT_BROWSER_DEVTOOLS_DEVTOOLS_ISSUE_STORAGE_H_
+#define CONTENT_BROWSER_DEVTOOLS_DEVTOOLS_ISSUE_STORAGE_H_
+
+#include <deque>
+
+#include "base/unguessable_token.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/browser/web_contents_user_data.h"
+
+namespace content {
+
+namespace protocol {
+namespace Audits {
+class InspectorIssue;
+}  // namespace Audits
+}  // namespace protocol
+
+class DevToolsIssueStorage
+    : public content::WebContentsUserData<DevToolsIssueStorage>,
+      public WebContentsObserver {
+ public:
+  ~DevToolsIssueStorage() override;
+
+  static DevToolsIssueStorage* GetOrCreateForWebContents(
+      WebContents* contents) {
+    content::WebContentsUserData<DevToolsIssueStorage>::CreateForWebContents(
+        contents);
+    return FromWebContents(contents);
+  }
+
+  void AddInspectorIssue(
+      int frame_tree_node_id,
+      std::unique_ptr<protocol::Audits::InspectorIssue> issue);
+  std::vector<const protocol::Audits::InspectorIssue*> FilterIssuesBy(
+      const base::flat_set<int>& frame_tree_node_ids) const;
+
+ private:
+  explicit DevToolsIssueStorage(content::WebContents* contents);
+  friend class content::WebContentsUserData<DevToolsIssueStorage>;
+  WEB_CONTENTS_USER_DATA_KEY_DECL();
+
+  // WebContentsObserver overrides.
+  void DidFinishNavigation(NavigationHandle* navigation_handle) override;
+  void FrameDeleted(RenderFrameHost* render_frame_host) override;
+
+  using FrameAssociatedIssue =
+      std::pair<int, std::unique_ptr<protocol::Audits::InspectorIssue>>;
+  std::deque<FrameAssociatedIssue> issues_;
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_DEVTOOLS_DEVTOOLS_ISSUE_STORAGE_H_

content/browser/devtools/devtools_issue_storage_browsertest.cc

+// Copyright 2020 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.
+
+#include "content/browser/devtools/devtools_instrumentation.h"
+#include "content/browser/devtools/protocol/audits.h"
+#include "content/browser/devtools/protocol/devtools_protocol_test_support.h"
+#include "content/browser/devtools/render_frame_devtools_agent_host.h"
+#include "content/browser/frame_host/frame_tree_node.h"
+#include "content/browser/frame_host/render_frame_host_impl.h"
+#include "content/browser/web_contents/web_contents_impl.h"
+#include "content/public/browser/devtools_agent_host.h"
+#include "content/public/browser/devtools_agent_host_client.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/common/url_constants.h"
+#include "content/public/test/browser_test.h"
+#include "content/public/test/browser_test_base.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/public/test/content_browser_test.h"
+#include "content/public/test/content_browser_test_utils.h"
+#include "content/shell/browser/shell.h"
+#include "net/dns/mock_host_resolver.h"
+
+namespace content {
+
+class DevToolsIssueStorageBrowserTest : public DevToolsProtocolTest {
+ public:
+  void SetUpOnMainThread() override {
+    host_resolver()->AddRule("*", "127.0.0.1");
+    SetupCrossSiteRedirector(embedded_test_server());
+  }
+};
+
+namespace {
+
+void ReportDummyIssue(RenderFrameHostImpl* rfh) {
+  auto issueDetails = protocol::Audits::InspectorIssueDetails::Create();
+  auto inspector_issue =
+      protocol::Audits::InspectorIssue::Create()
+          .SetCode(
+              protocol::Audits::InspectorIssueCodeEnum::SameSiteCookieIssue)
+          .SetDetails(issueDetails.Build())
+          .Build();
+  devtools_instrumentation::ReportBrowserInitiatedIssue(rfh,
+                                                        inspector_issue.get());
+}
+
+}  // namespace
+
+IN_PROC_BROWSER_TEST_F(DevToolsIssueStorageBrowserTest,
+                       DevToolsReceivesBrowserIssues) {
+  // 1) Navigate to about:blank.
+  EXPECT_TRUE(NavigateToURL(shell(), GURL("about:blank")));
+
+  // 2) Report an empty SameSite cookie issue.
+  WebContentsImpl* web_contents_impl =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+  RenderFrameHostImpl* root = web_contents_impl->GetFrameTree()->GetMainFrame();
+  ReportDummyIssue(root);
+
+  // 3) Open DevTools.
+  Attach();
+
+  // 4) Verify we haven't received any Issues yet.
+  ASSERT_TRUE(notifications_.empty());
+
+  // 5) Enable audits domain.
+  SendCommand("Audits.enable", std::make_unique<base::DictionaryValue>());
+
+  // 6) Verify we have received the SameSite issue.
+  WaitForNotification("Audits.issueAdded", true);
+}
+
+IN_PROC_BROWSER_TEST_F(DevToolsIssueStorageBrowserTest,
+                       DevToolsReceivesBrowserIssuesWhileAttached) {
+  // 1) Navigate to about:blank.
+  EXPECT_TRUE(NavigateToURL(shell(), GURL("about:blank")));
+
+  // 2) Open DevTools and enable Audits domain.
+  Attach();
+  SendCommand("Audits.enable", std::make_unique<base::DictionaryValue>());
+
+  // 3) Verify we haven't received any Issues yet.
+  ASSERT_TRUE(notifications_.empty());
+
+  // 4) Report an empty SameSite cookie issue.
+  WebContentsImpl* web_contents_impl =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+  RenderFrameHostImpl* root = web_contents_impl->GetFrameTree()->GetMainFrame();
+  ReportDummyIssue(root);
+
+  // 5) Verify we have received the SameSite issue.
+  WaitForNotification("Audits.issueAdded", true);
+}
+
+IN_PROC_BROWSER_TEST_F(DevToolsIssueStorageBrowserTest,
+                       DeleteSubframeWithIssue) {
+  // 1) Navigate to a page with an OOP iframe.
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL test_url =
+      embedded_test_server()->GetURL("/devtools/page-with-oopif.html");
+  EXPECT_TRUE(NavigateToURL(shell(), test_url));
+
+  // 2) Report an empty SameSite cookie issue in the iframe.
+  WebContentsImpl* web_contents_impl =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+  RenderFrameHostImpl* root = web_contents_impl->GetFrameTree()->GetMainFrame();
+  EXPECT_EQ(root->child_count(), static_cast<unsigned>(1));
+  RenderFrameHostImpl* iframe = root->child_at(0)->current_frame_host();
+  EXPECT_FALSE(iframe->is_main_frame());
+
+  ReportDummyIssue(iframe);
+
+  // 3) Delete the iframe from the page. This should cause the issue to be
+  // re-assigned
+  //    to the root frame.
+  root->RemoveChild(iframe->frame_tree_node());
+
+  // 4) Open DevTools and enable Audits domain.
+  Attach();
+  SendCommand("Audits.enable", std::make_unique<base::DictionaryValue>());
+
+  // 5) Verify we have received the SameSite issue on the main target.
+  WaitForNotification("Audits.issueAdded", true);
+}
+
+IN_PROC_BROWSER_TEST_F(DevToolsIssueStorageBrowserTest,
+                       MainFrameNavigationClearsIssues) {
+  // 1) Navigate to about:blank.
+  EXPECT_TRUE(NavigateToURL(shell(), GURL("about:blank")));
+
+  // 2) Report an empty SameSite cookie issue.
+  WebContentsImpl* web_contents_impl =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+  RenderFrameHostImpl* root = web_contents_impl->GetFrameTree()->GetMainFrame();
+  ReportDummyIssue(root);
+
+  // 3) Navigate to /devtools/navigation.html
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL test_url = embedded_test_server()->GetURL("/devtools/navigation.html");
+  EXPECT_TRUE(NavigateToURL(shell(), test_url));
+
+  // 4) Open DevTools and enable Audits domain.
+  Attach();
+  SendCommand("Audits.enable", std::make_unique<base::DictionaryValue>());
+
+  // 5) Verify that we haven't received any notifications.
+  ASSERT_TRUE(notifications_.empty());
+}
+
+}  // namespace content

content/browser/devtools/protocol/audits_handler.cc

@@ -5,6 +5,9 @@
 #include "content/browser/devtools/protocol/audits_handler.h"
 
 #include "content/browser/devtools/devtools_agent_host_impl.h"
+#include "content/browser/devtools/devtools_issue_storage.h"
+#include "content/browser/devtools/render_frame_devtools_agent_host.h"
+#include "content/browser/frame_host/frame_tree_node.h"
 
 namespace content {
 namespace protocol {
@@ -19,6 +22,11 @@ std::vector<AuditsHandler*> AuditsHandler::ForAgentHost(
   return host->HandlersByName<AuditsHandler>(Audits::Metainfo::domainName);
 }
 
+void AuditsHandler::SetRenderer(int process_host_id,
+                                RenderFrameHostImpl* frame_host) {
+  host_ = frame_host;
+}
+
 void AuditsHandler::Wire(UberDispatcher* dispatcher) {
   frontend_ = std::make_unique<Audits::Frontend>(dispatcher->channel());
   Audits::Dispatcher::wire(dispatcher, this);
@@ -29,12 +37,53 @@ DispatchResponse AuditsHandler::Disable() {
   return Response::FallThrough();
 }
 
+namespace {
+
+void SendStoredIssuesForFrameToAgent(RenderFrameHostImpl* rfh,
+                                     protocol::AuditsHandler* handler) {
+  // Check the storage first. No need to do any work in case its empty.
+  WebContents* web_contents = WebContents::FromRenderFrameHost(rfh);
+  DevToolsIssueStorage* issue_storage =
+      DevToolsIssueStorage::FromWebContents(web_contents);
+  if (!issue_storage)
+    return;
+
+  FrameTreeNode* local_root = rfh->frame_tree_node();
+
+  std::vector<int> frame_tree_node_ids;
+  for (FrameTreeNode* node : rfh->frame_tree()->SubtreeNodes(local_root)) {
+    // For each child we find the child's local root. Should the child's local
+    // root match |local_root|, the provided |AuditsHandler| is responsible and
+    // we collect the devtools_frame_token.
+    if (local_root == GetFrameTreeNodeAncestor(node)) {
+      frame_tree_node_ids.push_back(node->frame_tree_node_id());
+    }
+  }
+
+  base::flat_set<int> frame_ids_set(frame_tree_node_ids);
+  auto issues = issue_storage->FilterIssuesBy(std::move(frame_ids_set));
+  for (auto* const issue : issues) {
+    handler->OnIssueAdded(issue);
+  }
+}
+
+}  // namespace
+
 DispatchResponse AuditsHandler::Enable() {
+  if (enabled_) {
+    return Response::FallThrough();
+  }
+
   enabled_ = true;
+  if (host_) {
+    SendStoredIssuesForFrameToAgent(host_, this);
+  }
+
   return Response::FallThrough();
 }
 
-void AuditsHandler::OnIssueAdded(protocol::Audits::InspectorIssue* issue) {
+void AuditsHandler::OnIssueAdded(
+    const protocol::Audits::InspectorIssue* issue) {
   if (enabled_) {
     frontend_->IssueAdded(issue->clone());
   }

content/browser/devtools/protocol/audits_handler.h

@@ -8,6 +8,7 @@
 #include "base/macros.h"
 #include "content/browser/devtools/protocol/audits.h"
 #include "content/browser/devtools/protocol/devtools_domain_handler.h"
+#include "content/browser/frame_host/render_frame_host_impl.h"
 #include "mojo/public/cpp/bindings/remote.h"
 
 namespace content {
@@ -25,17 +26,20 @@ class AuditsHandler final : public DevToolsDomainHandler,
   static std::vector<AuditsHandler*> ForAgentHost(DevToolsAgentHostImpl* host);
 
   // DevToolsDomainHandler implementation.
+  void SetRenderer(int process_host_id,
+                   RenderFrameHostImpl* frame_host) override;
   void Wire(UberDispatcher* dispatcher) override;
 
   // Audits::Backend implementation.
   DispatchResponse Disable() override;
   DispatchResponse Enable() override;
 
-  void OnIssueAdded(protocol::Audits::InspectorIssue* issue);
+  void OnIssueAdded(const protocol::Audits::InspectorIssue* issue);
 
  private:
   std::unique_ptr<Audits::Frontend> frontend_;
   bool enabled_ = false;
+  RenderFrameHostImpl* host_;
 
   DISALLOW_COPY_AND_ASSIGN(AuditsHandler);
 };

content/browser/devtools/render_frame_devtools_agent_host.cc

@@ -96,6 +96,8 @@ bool ShouldCreateDevToolsForNode(FrameTreeNode* ftn) {
           ftn->current_frame_host()->IsCrossProcessSubframe());
 }
 
+}  // namespace
+
 FrameTreeNode* GetFrameTreeNodeAncestor(FrameTreeNode* frame_tree_node) {
   while (frame_tree_node && !ShouldCreateDevToolsForNode(frame_tree_node))
     frame_tree_node = FrameTreeNode::From(frame_tree_node->parent());
@@ -103,8 +105,6 @@ FrameTreeNode* GetFrameTreeNodeAncestor(FrameTreeNode* frame_tree_node) {
   return frame_tree_node;
 }
 
-}  // namespace
-
 // static
 scoped_refptr<DevToolsAgentHost> DevToolsAgentHost::GetOrCreateFor(
     WebContents* web_contents) {

content/browser/devtools/render_frame_devtools_agent_host.h

@@ -168,6 +168,10 @@ class CONTENT_EXPORT RenderFrameDevToolsAgentHost
   DISALLOW_COPY_AND_ASSIGN(RenderFrameDevToolsAgentHost);
 };
 
+// Returns the ancestor FrameTreeNode* for which a RenderFrameDevToolsAgentHost
+// should be created (i.e. the next local root).
+FrameTreeNode* GetFrameTreeNodeAncestor(FrameTreeNode* frame_tree_node);
+
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_DEVTOOLS_RENDER_FRAME_DEVTOOLS_AGENT_HOST_H_

content/test/BUILD.gn

@@ -933,6 +933,7 @@ test("content_browsertests") {
     "../browser/data_decoder_browsertest.cc",
     "../browser/database_browsertest.cc",
     "../browser/device_sensors/device_sensor_browsertest.cc",
+    "../browser/devtools/devtools_issue_storage_browsertest.cc",
     "../browser/devtools/devtools_video_consumer_browsertest.cc",
     "../browser/devtools/protocol/devtools_protocol_browsertest.cc",
     "../browser/devtools/protocol/devtools_protocol_test_support.cc",