Add window type to CrashReportPrivate JS crash reports.

JS crash reports may occur from a regular tab, app window or SWA
window. This is not currently logged.

Logging this will help us track down a potential race condition where a
SWA url may not be recognised which may be the root cause of
b/162791595.

A note about tests: It is a little weird getting tests to work for
window type as we can't grant a |WebContent| permissions to use
|CrashReportPrivate| on the fly. This cl works around that by opening
the target window window type, then navigating a url that has access to
|CrashReportPrivate|.

Bug: b/162791595, b/169635499
Change-Id: I358a380b6742bd44f56a5a46f51182b9df26c693
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2489123
Commit-Queue: David Lei <dlei@google.com>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Ian Barkley-Yeung <iby@chromium.org>
Reviewed-by: dstockwell <dstockwell@google.com>
Auto-Submit: David Lei <dlei@google.com>
Cr-Commit-Position: refs/heads/master@{#824314}

chrome/browser/error_reporting/chrome_js_error_report_processor.cc

@@ -32,6 +32,10 @@
 namespace {
 
 constexpr char kCrashEndpointUrl[] = "https://clients2.google.com/cr/report";
+constexpr char kNoBrowserNoWindow[] = "NO_BROWSER";
+constexpr char kRegularTabbedWindow[] = "REGULAR_TABBED";
+constexpr char kWebAppWindow[] = "WEB_APP";
+constexpr char kSystemWebAppWindow[] = "SYSTEM_WEB_APP";
 
 // Sometimes, the stack trace will contain an error message as the first line,
 // which confuses the Crash server. This function deletes it if it is present.
@@ -71,6 +75,19 @@ std::string BuildPostRequestQueryString(const ParameterMap& params) {
   return base::JoinString(query_parts, "&");
 }
 
+std::string MapWindowTypeToString(WindowType window_type) {
+  switch (window_type) {
+    case WindowType::kRegularTabbed:
+      return kRegularTabbedWindow;
+    case WindowType::kWebApp:
+      return kWebAppWindow;
+    case WindowType::kSystemWebApp:
+      return kSystemWebAppWindow;
+    default:
+      return kNoBrowserNoWindow;
+  }
+}
+
 }  // namespace
 
 ChromeJsErrorReportProcessor::ChromeJsErrorReportProcessor() = default;
@@ -249,6 +266,12 @@ void ChromeJsErrorReportProcessor::OnConsentCheckCompleted(
       base::NumberToString(browser_process_uptime.InMilliseconds());
   params["renderer_process_uptime_ms"] =
       base::NumberToString(error_report->renderer_process_uptime_ms);
+  if (error_report->window_type.has_value()) {
+    std::string window_type =
+        MapWindowTypeToString(error_report->window_type.value());
+    if (window_type != kNoBrowserNoWindow)
+      params["window_type"] = window_type;
+  }
   if (error_report->app_locale)
     params["app_locale"] = std::move(*error_report->app_locale);
   const GURL url(base::StrCat(

chrome/browser/error_reporting/chrome_js_error_report_processor_unittest.cc

@@ -102,6 +102,7 @@ TEST_F(ChromeJsErrorReportProcessorTest, AllFields) {
   report.column_number = 14;
   report.stack_trace = "bad_func(1, 2)\nonclick()\n";
   report.renderer_process_uptime_ms = 1234;
+  report.window_type = WindowType::kSystemWebApp;
 
   processor_->SendErrorReport(
       std::move(report),
@@ -119,6 +120,7 @@ TEST_F(ChromeJsErrorReportProcessorTest, AllFields) {
   EXPECT_THAT(actual_report->query, HasSubstr("browser_process_uptime_ms="));
   EXPECT_THAT(actual_report->query,
               HasSubstr("renderer_process_uptime_ms=1234"));
+  EXPECT_THAT(actual_report->query, HasSubstr("window_type=SYSTEM_WEB_APP"));
   // TODO(iby) research why URL is repeated...
   EXPECT_THAT(actual_report->query,
               HasSubstr("src=https%3A%2F%2Fwww.chromium.org%2FHome"));

chrome/browser/extensions/api/crash_report_private/crash_report_private_api.cc

@@ -7,6 +7,7 @@
 #include "base/time/default_clock.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/metrics/renderer_uptime_tracker.h"
+#include "chrome/browser/ui/browser_finder.h"
 #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/public/browser/devtools_agent_host.h"
@@ -27,6 +28,17 @@ base::Clock*& GetClock() {
   return clock;
 }
 
+WindowType GetWindowType(content::WebContents* web_contents) {
+  Browser* browser = chrome::FindBrowserWithWebContents(web_contents);
+  if (!browser)
+    return WindowType::kNoBrowser;
+  if (!browser->app_controller())
+    return WindowType::kRegularTabbed;
+  if (browser->app_controller()->is_for_system_web_app())
+    return WindowType::kSystemWebApp;
+  return WindowType::kWebApp;
+}
+
 }  // namespace
 
 CrashReportPrivateReportErrorFunction::CrashReportPrivateReportErrorFunction() =
@@ -83,16 +95,20 @@ ExtensionFunction::ResponseAction CrashReportPrivateReportErrorFunction::Run() {
     error_report.stack_trace = std::move(*params->info.stack_trace);
   }
 
-  if (web_contents && web_contents->GetMainFrame() &&
-      web_contents->GetMainFrame()->GetProcess()) {
-    int pid = web_contents->GetMainFrame()->GetProcess()->GetID();
-    base::TimeDelta render_process_uptime =
-        metrics::RendererUptimeTracker::Get()->GetProcessUptime(pid);
-    // Note: This can be 0 in tests or if the process can't be found (implying
-    // process fails to start up or terminated). Report this anyways as it can
-    // hint at race conditions.
-    error_report.renderer_process_uptime_ms =
-        render_process_uptime.InMilliseconds();
+  if (web_contents) {
+    error_report.window_type = GetWindowType(web_contents);
+
+    if (web_contents->GetMainFrame() &&
+        web_contents->GetMainFrame()->GetProcess()) {
+      int pid = web_contents->GetMainFrame()->GetProcess()->GetID();
+      base::TimeDelta render_process_uptime =
+          metrics::RendererUptimeTracker::Get()->GetProcessUptime(pid);
+      // Note: This can be 0 in tests or if the process can't be found (implying
+      // process fails to start up or terminated). Report this anyways as it can
+      // hint at race conditions.
+      error_report.renderer_process_uptime_ms =
+          render_process_uptime.InMilliseconds();
+    }
   }
 
   error_report.app_locale = g_browser_process->GetApplicationLocale();

chrome/browser/extensions/api/crash_report_private/crash_report_private_apitest.cc

@@ -3,11 +3,15 @@
 // found in the LICENSE file.
 
 #include "base/system/sys_info.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/simple_test_clock.h"
+#include "chrome/browser/chromeos/web_applications/system_web_app_integration_test.h"
 #include "chrome/browser/devtools/devtools_window_testing.h"
 #include "chrome/browser/error_reporting/mock_chrome_js_error_report_processor.h"
 #include "chrome/browser/extensions/api/crash_report_private/crash_report_private_api.h"
 #include "chrome/browser/extensions/extension_apitest.h"
+#include "chrome/browser/ui/web_applications/test/web_app_browsertest_util.h"
+#include "chromeos/constants/chromeos_features.h"
 #include "components/crash/content/browser/error_reporting/mock_crash_endpoint.h"
 #include "content/public/test/browser_task_environment.h"
 #include "content/public/test/browser_test.h"
@@ -22,6 +26,7 @@ namespace extensions {
 
 namespace {
 
+using ::testing::HasSubstr;
 using ::testing::MatchesRegex;
 
 constexpr const char* kTestExtensionId = "jjeoclcdfjddkdjokiejckgcildcflpp";
@@ -84,6 +89,18 @@ class CrashReportPrivateApiTest : public ExtensionApiTest {
   DISALLOW_COPY_AND_ASSIGN(CrashReportPrivateApiTest);
 };
 
+class CrashReportPrivateCalledFromSwaTest : public SystemWebAppIntegrationTest {
+ public:
+  CrashReportPrivateCalledFromSwaTest() {
+    // Enable this for tests so they still pass if "--disable-features=MediaApp"
+    // is present.
+    scoped_feature_list_.InitWithFeatures({chromeos::features::kMediaApp}, {});
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
 IN_PROC_BROWSER_TEST_F(CrashReportPrivateApiTest, Basic) {
   constexpr char kTestScript[] = R"(
     chrome.crashReportPrivate.reportError({
@@ -289,4 +306,129 @@ IN_PROC_BROWSER_TEST_F(CrashReportPrivateApiTest, NoConsent) {
   EXPECT_FALSE(report);
 }
 
+// Test REGULAR_TABBED is detected when |CrashReportPrivate| is called from a
+// tab's |web_contents|.
+IN_PROC_BROWSER_TEST_F(CrashReportPrivateApiTest, CalledFromWebContentsInTab) {
+  // Navigate to the text |extension_| that has access to |CrashReportPrivate|.
+  const GURL extension_context_url(
+      "chrome-extension://jjeoclcdfjddkdjokiejckgcildcflpp/"
+      "_generated_background_page.html");
+  content::WebContents* web_content =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_TRUE(NavigateToURL(web_content, extension_context_url));
+
+  constexpr char kTestScript[] = R"(
+    chrome.crashReportPrivate.reportError({
+        message: "hi",
+        url: "http://www.test.com",
+      },
+      () => window.domAutomationController.send(""));
+  )";
+  // Run the script in the |web_content| that has loaded |extension_| instead of
+  // |ExecuteScriptInBackgroundPage| so
+  // |chrome::FindBrowserWithWebContents(web_contents)| is not |nullptr|.
+  EXPECT_EQ(true, ExecuteScript(web_content, kTestScript));
+
+  auto report = crash_endpoint_->WaitForReport();
+  EXPECT_THAT(
+      report.query,
+      MatchesRegex("app_locale=en-US&browser=Chrome&browser_process_uptime_ms="
+                   "\\d+&browser_"
+                   "version=1.2.3.4&channel=Stable&"
+                   "error_message=hi&full_url=http%3A%2F%2Fwww.test.com%2F&"
+                   "os=ChromeOS&os_version=7.20.1"
+                   "&prod=Chrome_ChromeOS&renderer_process_uptime_ms=\\d+&src="
+                   "http%3A%2F%2Fwww.test."
+                   "com%2F&type=JavascriptError&url=%2F&ver=1.2.3.4&window_"
+                   "type=REGULAR_TABBED"));
+  EXPECT_EQ(report.content, "");
+}
+
+// Test WEB_APP is detected when |CrashReportPrivate| is called from an app
+// window.
+IN_PROC_BROWSER_TEST_P(CrashReportPrivateCalledFromSwaTest,
+                       CalledFromWebContentsInWebAppWindow) {
+  WaitForTestSystemAppInstall();
+  // Set up test server to listen to handle crash reports & serve fake web app
+  // content. Note: Creating a |MockCrashEndpoint| starts the server.
+  MockCrashEndpoint endpoint(embedded_test_server());
+  ScopedMockChromeJsErrorReportProcessor processor(endpoint);
+  ASSERT_TRUE(embedded_test_server()->Started());
+  // Create and launch a test web app, opens in an app window.
+  GURL start_url = embedded_test_server()->GetURL("/test_app.html");
+  auto web_app_info = std::make_unique<WebApplicationInfo>();
+  web_app_info->start_url = start_url;
+  web_app::AppId app_id =
+      web_app::InstallWebApp(profile(), std::move(web_app_info));
+  Browser* app_browser = web_app::LaunchWebAppBrowserAndWait(profile(), app_id);
+
+  content::WebContents* web_content =
+      app_browser->tab_strip_model()->GetActiveWebContents();
+  // Navigate to chrome://media-app which was access to |CrashReportPrivate|
+  // from the |WebContents| in the web app window.
+  const GURL extension_context_url("chrome://media-app");
+  EXPECT_TRUE(NavigateToURL(web_content, extension_context_url));
+
+  constexpr char kTestScript[] = R"(
+    chrome.crashReportPrivate.reportError({
+        message: "hi",
+        url: "http://www.test.com",
+      },
+      () => window.domAutomationController.send(""));
+  )";
+  EXPECT_EQ(true, ExecuteScript(web_content, kTestScript));
+
+  auto report = endpoint.WaitForReport();
+
+  EXPECT_THAT(
+      report.query,
+      MatchesRegex("app_locale=en-US&browser=Chrome&browser_process_uptime_ms="
+                   "\\d+&browser_"
+                   "version=1.2.3.4&channel=Stable&"
+                   "error_message=hi&full_url=http%3A%2F%2Fwww.test.com%2F&"
+                   "os=ChromeOS&os_version=7.20.1"
+                   "&prod=Chrome_ChromeOS&renderer_process_uptime_ms=\\d+&src="
+                   "http%3A%2F%2Fwww.test."
+                   "com%2F&type=JavascriptError&url=%2F&ver=1.2.3.4&window_"
+                   "type=WEB_APP"));
+  EXPECT_EQ(report.content, "");
+}
+
+// Test SWA_WINDOW is detected when |CrashReportPrivate| is called from a
+// System Web App window |web_contents|.
+IN_PROC_BROWSER_TEST_P(CrashReportPrivateCalledFromSwaTest,
+                       CalledFromWebContentsInSwaWindow) {
+  WaitForTestSystemAppInstall();
+  content::WebContents* web_content = LaunchApp(web_app::SystemAppType::MEDIA);
+  MockCrashEndpoint endpoint(embedded_test_server());
+  ScopedMockChromeJsErrorReportProcessor processor(endpoint);
+
+  constexpr char kTestScript[] = R"(
+    chrome.crashReportPrivate.reportError({
+        message: "hi",
+        url: "http://www.test.com",
+      },
+      () => window.domAutomationController.send(""));
+  )";
+  EXPECT_EQ(true, ExecuteScript(web_content, kTestScript));
+
+  auto report = endpoint.WaitForReport();
+
+  EXPECT_THAT(
+      report.query,
+      MatchesRegex("app_locale=en-US&browser=Chrome&browser_process_uptime_ms="
+                   "\\d+&browser_"
+                   "version=1.2.3.4&channel=Stable&"
+                   "error_message=hi&full_url=http%3A%2F%2Fwww.test.com%2F&"
+                   "os=ChromeOS&os_version=7.20.1"
+                   "&prod=Chrome_ChromeOS&renderer_process_uptime_ms=\\d+&src="
+                   "http%3A%2F%2Fwww.test."
+                   "com%2F&type=JavascriptError&url=%2F&ver=1.2.3.4&window_"
+                   "type=SYSTEM_WEB_APP"));
+  EXPECT_EQ(report.content, "");
+}
+
+INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_WEB_APP_INFO_INSTALL_P(
+    CrashReportPrivateCalledFromSwaTest);
+
 }  // namespace extensions

components/crash/content/browser/error_reporting/javascript_error_report.h

@@ -10,6 +10,15 @@
 #include "base/component_export.h"
 #include "base/optional.h"
 
+enum class WindowType {
+  // No browser found, thus no window exists.
+  kNoBrowser,
+  // Valid window types.
+  kRegularTabbed,
+  kWebApp,
+  kSystemWebApp,
+};
+
 // A report about a JavaScript error that we might want to send back to Google
 // so it can be fixed. Fill in the fields and then call
 // SendJavaScriptErrorReport.
@@ -54,6 +63,9 @@ struct COMPONENT_EXPORT(JS_ERROR_REPORTING) JavaScriptErrorReport {
   // context) or the renderer process doesn't exist (possible due to termination
   // / failure to start).
   int renderer_process_uptime_ms = 0;
+
+  // The window type of the JS context that reported this error.
+  base::Optional<WindowType> window_type;
 };
 
 #endif  // COMPONENTS_CRASH_CONTENT_BROWSER_ERROR_REPORTING_JAVASCRIPT_ERROR_REPORT_H_