Switch to ReadyToCommitNavigation to initialize error reporting

Per discussion on chromium-dev
(https://groups.google.com/u/1/a/chromium.org/g/chromium-dev/c/F2xhH0h36rU),
set up stack tracing and enable error reporting when
ReadyToCommitNavigation is called instead of when RenderFrameCreated is
called.

Bug: chromium:1121816
Change-Id: Ie0aaf56ced830180284f153bc43322701c95e504
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2585659
Commit-Queue: Ian Barkley-Yeung <iby@chromium.org>
Reviewed-by: dpapad <dpapad@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Auto-Submit: Ian Barkley-Yeung <iby@chromium.org>
Cr-Commit-Position: refs/heads/master@{#838262}

content/browser/webui/web_ui_main_frame_observer.cc

@@ -19,6 +19,7 @@
 #include "components/crash/content/browser/error_reporting/javascript_error_report.h"
 #include "components/crash/content/browser/error_reporting/js_error_report_processor.h"
 #include "content/browser/renderer_host/render_frame_host_impl.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui_controller.h"
 #include "content/public/common/content_features.h"
@@ -55,19 +56,19 @@ void WebUIMainFrameObserver::OnDidAddMessageToConsole(
     const base::string16& source_id,
     const base::Optional<base::string16>& untrusted_stack_trace) {
   // TODO(iby) Change all VLOGs to DVLOGs once tast tests are stable.
-  VLOG(3) << "OnDidAddMessageToConsole called for " << message;
+  DVLOG(3) << "OnDidAddMessageToConsole called for " << message;
   if (untrusted_stack_trace) {
-    VLOG(3) << "stack is " << *untrusted_stack_trace;
+    DVLOG(3) << "stack is " << *untrusted_stack_trace;
   }
 
   if (!error_reporting_enabled_) {
-    VLOG(3) << "Message not reported, error reporting disabled for this page "
-               "or experiment is off";
+    DVLOG(3) << "Message not reported, error reporting disabled for this page "
+                "or experiment is off";
     return;
   }
 
   if (log_level != blink::mojom::ConsoleMessageLevel::kError) {
-    VLOG(3) << "Message not reported, not an error";
+    DVLOG(3) << "Message not reported, not an error";
     return;
   }
 
@@ -75,7 +76,7 @@ void WebUIMainFrameObserver::OnDidAddMessageToConsole(
   // WebUIMainFrameObservers will get a callback when either page gets an error.
   // To avoid duplicates, only report on errors from this page's frame.
   if (source_frame != web_ui_->frame_host()) {
-    VLOG(3) << "Message not reported, different frame";
+    DVLOG(3) << "Message not reported, different frame";
     return;
   }
 
@@ -84,7 +85,7 @@ void WebUIMainFrameObserver::OnDidAddMessageToConsole(
 
   if (!processor) {
     // This usually means we are not on an official Google build, FYI.
-    VLOG(3) << "Message not reported, no processor";
+    DVLOG(3) << "Message not reported, no processor";
     return;
   }
 
@@ -92,7 +93,7 @@ void WebUIMainFrameObserver::OnDidAddMessageToConsole(
   // TODO(https://crbug.com/1121816) Improve redaction.
   GURL url(source_id);
   if (!url.is_valid()) {
-    VLOG(3) << "Message not reported, invalid URL";
+    DVLOG(3) << "Message not reported, invalid URL";
     return;
   }
 
@@ -102,7 +103,7 @@ void WebUIMainFrameObserver::OnDidAddMessageToConsole(
   // content because Chrome cannot control what information is being included in
   // the reports.
   if (!url.SchemeIs(kChromeUIScheme)) {
-    VLOG(3) << "Message not reported, not a chrome:// URL";
+    DVLOG(3) << "Message not reported, not a chrome:// URL";
     return;
   }
 
@@ -124,19 +125,22 @@ void WebUIMainFrameObserver::OnDidAddMessageToConsole(
   report.send_to_production_servers =
       features::kWebUIJavaScriptErrorReportsSendToProductionParam.Get();
 
-  VLOG(3) << "Error being sent to Google";
+  DVLOG(3) << "Error being sent to Google";
   processor->SendErrorReport(std::move(report), base::DoNothing(),
                              web_contents()->GetBrowserContext());
 }
 
-void WebUIMainFrameObserver::RenderFrameCreated(
-    RenderFrameHost* render_frame_host) {
+void WebUIMainFrameObserver::ReadyToCommitNavigation(
+    NavigationHandle* navigation_handle) {
+  DVLOG(3) << "WebUIMainFrameObserver::ReadyToCommitNavigation()";
   if (!base::FeatureList::IsEnabled(
           features::kSendWebUIJavaScriptErrorReports)) {
+    DVLOG(3) << "Experiment is off";
     return;
   }
 
-  if (render_frame_host != web_ui_->frame_host()) {
+  if (navigation_handle->GetRenderFrameHost() != web_ui_->frame_host()) {
+    DVLOG(3) << "Wrong frame";
     return;
   }
 
@@ -149,7 +153,10 @@ void WebUIMainFrameObserver::RenderFrameCreated(
   // frame is created, or it will lock up the communications channel. (See
   // https://crbug.com/1154866).
   if (error_reporting_enabled_) {
+    DVLOG(3) << "Enabled";
     web_ui_->frame_host()->SetWantErrorMessageStackTrace();
+  } else {
+    DVLOG(3) << "Error reporting disabled for this page";
   }
 }
 

content/browser/webui/web_ui_main_frame_observer.h

@@ -7,7 +7,6 @@
 
 #include <stdint.h>
 
-#include "base/gtest_prod_util.h"  // FRIEND_TEST_ALL_PREFIXES
 #include "base/strings/string16.h"
 #include "build/build_config.h"
 #include "content/common/content_export.h"
@@ -54,7 +53,7 @@ class CONTENT_EXPORT WebUIMainFrameObserver : public WebContentsObserver {
       int32_t line_no,
       const base::string16& source_id,
       const base::Optional<base::string16>& untrusted_stack_trace) override;
-  void RenderFrameCreated(RenderFrameHost* render_frame_host) override;
+  void ReadyToCommitNavigation(NavigationHandle* navigation_handle) override;
 #endif  // defined(OS_LINUX) || defined(OS_CHROMEOS)
 
  private:

content/browser/webui/web_ui_main_frame_observer_unittest.cc

@@ -17,6 +17,7 @@
 #include "content/public/browser/web_ui_controller.h"
 #include "content/public/common/content_features.h"
 #include "content/public/test/browser_task_environment.h"
+#include "content/public/test/navigation_simulator.h"
 #include "content/public/test/test_browser_context.h"
 #include "content/public/test/test_renderer_host.h"
 #include "content/test/test_web_contents.h"
@@ -135,11 +136,12 @@ class WebUIMainFrameObserverTest : public RenderViewHostTestHarness {
     RenderViewHostTestHarness::TearDown();
   }
 
-  // Simulate the remote renderer becoming alive.
-  void CreateRenderFrame() {
-    static_cast<TestWebContents*>(web_contents())
-        ->GetRenderViewHost()
-        ->CreateRenderView(base::nullopt, 0, false);
+  // Simulate navigating to the WebUI page. Basically so that
+  // WebUIMainFrameObserver::ReadyToCommitNavigation() gets called, since that
+  // initializes the error handling.
+  void NavigateToPage() {
+    NavigationSimulator::NavigateAndCommitFromBrowser(web_contents(),
+                                                      GURL(kSourceURL8));
   }
 
   // Calls the observer's OnDidAddMessageToConsole with the given arguments.
@@ -178,7 +180,7 @@ constexpr char WebUIMainFrameObserverTest::kSourceURL8[];
 constexpr char WebUIMainFrameObserverTest::kStackTrace8[];
 
 TEST_F(WebUIMainFrameObserverTest, ErrorReported) {
-  CreateRenderFrame();
+  NavigateToPage();
   CallOnDidAddMessageToConsole(web_ui_->frame_host(),
                                blink::mojom::ConsoleMessageLevel::kError,
                                kMessage16, 5, kSourceId16, kStackTrace16);
@@ -198,7 +200,7 @@ TEST_F(WebUIMainFrameObserverTest, ErrorReported) {
 }
 
 TEST_F(WebUIMainFrameObserverTest, NoStackTrace) {
-  CreateRenderFrame();
+  NavigateToPage();
   CallOnDidAddMessageToConsole(web_ui_->frame_host(),
                                blink::mojom::ConsoleMessageLevel::kError,
                                kMessage16, 5, kSourceId16, base::nullopt);
@@ -208,7 +210,7 @@ TEST_F(WebUIMainFrameObserverTest, NoStackTrace) {
 }
 
 TEST_F(WebUIMainFrameObserverTest, NonErrorsIgnored) {
-  CreateRenderFrame();
+  NavigateToPage();
   CallOnDidAddMessageToConsole(web_ui_->frame_host(),
                                blink::mojom::ConsoleMessageLevel::kWarning,
                                kMessage16, 5, kSourceId16, kStackTrace16);
@@ -223,7 +225,7 @@ TEST_F(WebUIMainFrameObserverTest, NonErrorsIgnored) {
 }
 
 TEST_F(WebUIMainFrameObserverTest, NoProcessorDoesntCrash) {
-  CreateRenderFrame();
+  NavigateToPage();
   FakeJsErrorReportProcessor::SetDefault(nullptr);
   CallOnDidAddMessageToConsole(web_ui_->frame_host(),
                                blink::mojom::ConsoleMessageLevel::kError,
@@ -235,7 +237,7 @@ TEST_F(WebUIMainFrameObserverTest, NotSentIfFlagDisabled) {
   scoped_feature_list_.Reset();
   scoped_feature_list_.InitAndDisableFeature(
       features::kSendWebUIJavaScriptErrorReports);
-  CreateRenderFrame();
+  NavigateToPage();
   CallOnDidAddMessageToConsole(web_ui_->frame_host(),
                                blink::mojom::ConsoleMessageLevel::kError,
                                kMessage16, 5, kSourceId16, kStackTrace16);
@@ -244,7 +246,7 @@ TEST_F(WebUIMainFrameObserverTest, NotSentIfFlagDisabled) {
 }
 
 TEST_F(WebUIMainFrameObserverTest, NotSentIfInvalidURL) {
-  CreateRenderFrame();
+  NavigateToPage();
   CallOnDidAddMessageToConsole(
       web_ui_->frame_host(), blink::mojom::ConsoleMessageLevel::kError,
       kMessage16, 5, base::UTF8ToUTF16("invalid URL"), kStackTrace16);
@@ -255,7 +257,7 @@ TEST_F(WebUIMainFrameObserverTest, NotSentIfInvalidURL) {
 TEST_F(WebUIMainFrameObserverTest, NotSentIfDisabledForPage) {
   static_cast<MockWebUIController*>(web_ui_->GetController())
       ->enable_javascript_error_reporting(false);
-  CreateRenderFrame();
+  NavigateToPage();
   CallOnDidAddMessageToConsole(web_ui_->frame_host(),
                                blink::mojom::ConsoleMessageLevel::kError,
                                kMessage16, 5, kSourceId16, kStackTrace16);
@@ -264,7 +266,7 @@ TEST_F(WebUIMainFrameObserverTest, NotSentIfDisabledForPage) {
 }
 
 TEST_F(WebUIMainFrameObserverTest, URLPathIsPreservedOtherPartsRemoved) {
-  CreateRenderFrame();
+  NavigateToPage();
   struct URLTest {
     const char* const input;
     const char* const expected;
@@ -307,7 +309,7 @@ TEST_F(WebUIMainFrameObserverTest, URLPathIsPreservedOtherPartsRemoved) {
 }
 
 TEST_F(WebUIMainFrameObserverTest, ErrorsNotReportedInOtherFrames) {
-  CreateRenderFrame();
+  NavigateToPage();
   auto another_contents =
       TestWebContents::Create(browser_context(), site_instance_);
   CHECK(another_contents->GetMainFrame());
@@ -319,7 +321,7 @@ TEST_F(WebUIMainFrameObserverTest, ErrorsNotReportedInOtherFrames) {
 }
 
 TEST_F(WebUIMainFrameObserverTest, ErrorsNotReportedForNonChromeURLs) {
-  CreateRenderFrame();
+  NavigateToPage();
   const char* const kNonChromeSourceURLs[] = {
       "chrome-untrusted://media-app",
       "chrome-error://chromewebdata/",