Make console.error report to crash reporter for chrome://media-app.

Wraps calls to the crash reporter in reportCrashError that is called
whenever there is a console.error, in doing so we need to build up
the error from any number of arguments since console.error can be
called with (...*).

Also adds this for onerror & unhandledrejection handlers in trusted
context & tests.

I think there might be some errors in the trusted context we aren't
picking up on because I don't think we guarantee the app initializes
before these errors occur and so the error reporter (in index.ts)
wouldn't be initialized. This cl makes console.error use
crashReportPrivate.reportError() so we can keep track of such errors.

This may lead to launch code such as LaunchWithDirectory not erroring
which may be related to b/160104958.

Bugs: b/156205603
Change-Id: I404e870a9786ebe5371409c963a5140381d59126
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2301652Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: David Lei <dlei@google.com>
Cr-Commit-Position: refs/heads/master@{#790636}

chrome/browser/chromeos/web_applications/media_app_integration_browsertest.cc

@@ -48,6 +48,19 @@ constexpr char kFileJpeg640x480[] = "image3.jpg";
 // A 1-second long 648x486 VP9-encoded video with stereo Opus-encoded audio.
 constexpr char kFileVideoVP9[] = "world.webm";
 
+constexpr char kUnhandledRejectionScript[] =
+    "window.dispatchEvent("
+    "new CustomEvent('simulate-unhandled-rejection-for-test'));";
+
+constexpr char kTypeErrorScript[] =
+    "window.dispatchEvent("
+    "new CustomEvent('simulate-type-error-for-test'));";
+
+constexpr char kDomExceptionScript[] =
+    "window.dispatchEvent("
+    "new "
+    "CustomEvent('simulate-unhandled-rejection-with-dom-exception-for-test'));";
+
 class MediaAppIntegrationTest : public SystemWebAppIntegrationTest {
  public:
   MediaAppIntegrationTest() {
@@ -228,30 +241,116 @@ IN_PROC_BROWSER_TEST_P(MediaAppIntegrationTest, HiddenInLauncherAndSearch) {
   EXPECT_FALSE(GetManager().ShouldShowInSearch(web_app::SystemAppType::MEDIA));
 }
 
-IN_PROC_BROWSER_TEST_P(MediaAppIntegrationTest, ReportsUnhandledExceptions) {
+// Note: Error reporting tests are limited to one per test instance otherwise we
+// run into "Too many calls to this API" error.
+IN_PROC_BROWSER_TEST_P(MediaAppIntegrationTest,
+                       TrustedContextReportsConsoleErrors) {
+  MockCrashEndpoint endpoint(embedded_test_server());
+
+  content::WebContents* web_ui =
+      WaitForSystemAppInstallAndLoad(web_app::SystemAppType::MEDIA);
+
+  // Pass multiple arguments to console.error() to also check they are parsed
+  // and captured in the error message correctly.
+  constexpr char kConsoleError[] =
+      "console.error('YIKES', {data: 'something'}, new Error('deep error'));";
+  EXPECT_EQ(true, ExecuteScript(web_ui, kConsoleError));
+  auto report = endpoint.WaitForReport();
+  EXPECT_NE(std::string::npos,
+            report.query.find(
+                "error_message=Unexpected%3A%20%22YIKES%22%0A%7B%22data%22%"
+                "3A%22something%22%7D%0AError%3A%20deep%20error"))
+      << report.query;
+  EXPECT_NE(std::string::npos, report.query.find("prod=ChromeOS_MediaApp"));
+}
+
+IN_PROC_BROWSER_TEST_P(MediaAppIntegrationTest,
+                       TrustedContextReportsDomExceptions) {
+  MockCrashEndpoint endpoint(embedded_test_server());
+
+  content::WebContents* web_ui =
+      WaitForSystemAppInstallAndLoad(web_app::SystemAppType::MEDIA);
+
+  EXPECT_EQ(true, ExecuteScript(web_ui, kDomExceptionScript));
+  auto report = endpoint.WaitForReport();
+  EXPECT_NE(std::string::npos,
+            report.query.find("error_message=Unhandled%20rejection%3A%20Error%"
+                              "3A%20NotAFile%3A%20Not%20a%20file."))
+      << report.query;
+  EXPECT_NE(std::string::npos, report.query.find("prod=ChromeOS_MediaApp"));
+}
+
+IN_PROC_BROWSER_TEST_P(MediaAppIntegrationTest,
+                       UntrustedContextReportsDomExceptions) {
+  MockCrashEndpoint endpoint(embedded_test_server());
+
+  content::WebContents* app =
+      WaitForSystemAppInstallAndLoad(web_app::SystemAppType::MEDIA);
+
+  EXPECT_EQ(true,
+            MediaAppUiBrowserTest::EvalJsInAppFrame(app, kDomExceptionScript));
+  auto report = endpoint.WaitForReport();
+  EXPECT_NE(std::string::npos,
+            report.query.find("error_message=Not%20a%20file."))
+      << report.query;
+  EXPECT_NE(std::string::npos, report.query.find("prod=ChromeOS_MediaApp"));
+}
+
+IN_PROC_BROWSER_TEST_P(MediaAppIntegrationTest,
+                       TrustedContextReportsUnhandledExceptions) {
+  MockCrashEndpoint endpoint(embedded_test_server());
+
+  content::WebContents* web_ui =
+      WaitForSystemAppInstallAndLoad(web_app::SystemAppType::MEDIA);
+
+  EXPECT_EQ(true, ExecuteScript(web_ui, kUnhandledRejectionScript));
+  auto report = endpoint.WaitForReport();
+  EXPECT_NE(std::string::npos,
+            report.query.find("error_message=Unhandled%20rejection%3A%20Error%"
+                              "3A%20FakeErrorName%3A%20fake_throw"))
+      << report.query;
+  EXPECT_NE(std::string::npos, report.query.find("prod=ChromeOS_MediaApp"));
+}
+
+IN_PROC_BROWSER_TEST_P(MediaAppIntegrationTest,
+                       UntrustedContextReportsUnhandledExceptions) {
   MockCrashEndpoint endpoint(embedded_test_server());
+
   content::WebContents* app =
       WaitForSystemAppInstallAndLoad(web_app::SystemAppType::MEDIA);
 
-  const char kScript[] =
-      "window.dispatchEvent("
-      "new CustomEvent('simulate-unhandled-rejection-for-test'));";
-  EXPECT_EQ(true, MediaAppUiBrowserTest::EvalJsInAppFrame(app, kScript));
+  EXPECT_EQ(true, MediaAppUiBrowserTest::EvalJsInAppFrame(
+                      app, kUnhandledRejectionScript));
   auto report = endpoint.WaitForReport();
   EXPECT_NE(std::string::npos, report.query.find("error_message=fake_throw"))
       << report.query;
   EXPECT_NE(std::string::npos, report.query.find("prod=ChromeOS_MediaApp"));
 }
 
-IN_PROC_BROWSER_TEST_P(MediaAppIntegrationTest, ReportsTypeErrors) {
+IN_PROC_BROWSER_TEST_P(MediaAppIntegrationTest,
+                       TrustedContextReportsTypeErrors) {
+  MockCrashEndpoint endpoint(embedded_test_server());
+  content::WebContents* web_ui =
+      WaitForSystemAppInstallAndLoad(web_app::SystemAppType::MEDIA);
+
+  EXPECT_EQ(true, ExecuteScript(web_ui, kTypeErrorScript));
+  auto report = endpoint.WaitForReport();
+  EXPECT_NE(std::string::npos,
+            report.query.find(
+                "error_message=Error%3A%20ErrorEvent%3A%20Uncaught%20TypeError%"
+                "3A%20event.notAFunction%20is%20not%20a%20function"))
+      << report.query;
+  EXPECT_NE(std::string::npos, report.query.find("prod=ChromeOS_MediaApp"));
+}
+
+IN_PROC_BROWSER_TEST_P(MediaAppIntegrationTest,
+                       UntrustedContextReportsTypeErrors) {
   MockCrashEndpoint endpoint(embedded_test_server());
   content::WebContents* app =
       WaitForSystemAppInstallAndLoad(web_app::SystemAppType::MEDIA);
 
-  const char kScript[] =
-      "window.dispatchEvent("
-      "new CustomEvent('simulate-type-error-for-test'));";
-  EXPECT_EQ(true, MediaAppUiBrowserTest::EvalJsInAppFrame(app, kScript));
+  EXPECT_EQ(true,
+            MediaAppUiBrowserTest::EvalJsInAppFrame(app, kTypeErrorScript));
   auto report = endpoint.WaitForReport();
   EXPECT_NE(std::string::npos,
             report.query.find(

chromeos/components/media_app_ui/resources/js/BUILD.gn

@@ -41,6 +41,7 @@ js_library("launch") {
     "media_app.externs.js",
   ]
   deps = [
+    ":error_reporter",
     ":message_types",
     ":mojo_api_bootstrap",
     "//chromeos/components/system_apps/public/js:message_pipe",
@@ -50,6 +51,10 @@ js_library("launch") {
 js_library("message_types") {
 }
 
+js_library("error_reporter") {
+  externs_list = [ "$externs_path/crash_report_private.js" ]
+}
+
 js_library("receiver") {
   externs_list = [ "media_app.externs.js" ]
   deps = [

chromeos/components/media_app_ui/resources/js/app_context_test_support.js

@@ -15,8 +15,18 @@
 window.addEventListener('simulate-type-error-for-test', event => {
   /** @type {{notAFunction: function()}} */ (event).notAFunction();
 });
+
 window.addEventListener('simulate-unhandled-rejection-for-test', event => {
   new Promise(resolve => {
-    throw new Error('fake_throw');
+    const error = new Error('fake_throw');
+    error.name = 'FakeErrorName';
+    throw error;
   });
 });
+
+window.addEventListener(
+    'simulate-unhandled-rejection-with-dom-exception-for-test', event => {
+      new Promise(resolve => {
+        throw new DOMException('Not a file.', 'NotAFile');
+      });
+    });

chromeos/components/media_app_ui/resources/js/dom_draft.externs.js

@@ -9,3 +9,16 @@
 
 /** @type {function(): !Promise<!ArrayBuffer>} */
 Blob.prototype.arrayBuffer;
+
+/**
+ * @see https://www.w3.org/TR/2016/WD-html51-20160310/webappapis.html#the-promiserejectionevent-interface
+ * @extends {Event}
+ * @constructor
+ */
+const PromiseRejectionEvent = function() {};
+
+/** @type {Promise<*>} */
+PromiseRejectionEvent.prototype.promise;
+
+/** @type {*} */
+PromiseRejectionEvent.prototype.reason;

chromeos/components/media_app_ui/resources/js/error_reporter.js

+// 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.
+
+const PRODUCT_NAME = 'ChromeOS_MediaApp';
+
+/**
+ * Handles reporting errors creating a new Error() to get a stacktrace. If > 1
+ * arguments are provided, builds up the error message using the other [1, n-1]
+ * errors and appends that to the first error.
+ * @param {...*} errors
+ */
+function reportCrashError(...errors) {
+  if (!errors || errors.length === 0) {
+    return;
+  }
+
+  // Build up the error message using errors[1, n-1].
+  let message = '';
+  for (let i = 1; i < errors.length; i++) {
+    const errorArg = errors[i];
+    if (errorArg !== undefined) {
+      if (errorArg instanceof Error) {
+        message += `\n${errorArg.name}: ${errorArg.message}`;
+      } else {
+        message += '\n' + JSON.stringify(errorArg);
+      }
+    }
+  }
+
+  // Base the error on the first error.
+  let firstError = errors[0];
+  /**
+   * @type {{
+   *    lineNumber: (number|undefined),
+   *    columnNumber: (number|undefined),
+   *    stack: (string|undefined)
+   * }}
+   */
+  let errorObject;
+  let errorMessage;
+  let prefix = '';
+
+  // Parse out the reason for the error.
+  if (firstError instanceof PromiseRejectionEvent) {
+    prefix = 'Unhandled rejection: ';
+    firstError = firstError.reason;
+  }
+
+  if (firstError instanceof Error || firstError instanceof ErrorEvent ||
+      firstError instanceof DOMException) {
+    // Note: `ErrorEvent` doesn't have a name field, `DOMException`s are routed
+    // through 'onerror' and treated as `ErrorEvents`, we can also have
+    // `DOMExceptions` inside a unhandled rejection.
+    errorMessage =
+        `Error: ${firstError.name || 'ErrorEvent'}: ${firstError.message}`;
+    errorObject = firstError;
+  } else {
+    // Should just be a regular object.
+    errorMessage = `Unexpected: ${JSON.stringify(firstError)}`;
+  }
+
+  if (!errorObject) {
+    // Create a new error to get a stacktrace.
+    errorObject = new Error();
+  }
+
+  /** @type {!chrome.crashReportPrivate.ErrorInfo} */
+  const params = {
+    product: PRODUCT_NAME,
+    url: self.location.href,
+    message: prefix + errorMessage + message,
+    lineNumber: errorObject.lineNumber || 0,
+    stackTrace: errorObject.stack || '',
+    columnNumber: errorObject.columnNumber || 0,
+  };
+
+  // TODO(crbug/996088): Add useful callback when the error is reported, handle
+  // if it crashes while reporting an error.
+  chrome.crashReportPrivate.reportError(params, () => {});
+}
+
+/**
+ * Redirect calls to `console.error` to also call `report` so we can report
+ * console.errors. Pass `realConsoleError` into this so we can mock it out for
+ * testing.
+ * @param {function(...*)} realConsoleError
+ * @param {function(...*)} report
+ */
+function captureConsoleErrors(realConsoleError, report) {
+  /**
+   * @param {...*} errors
+   */
+  console.error = (...errors) => {
+    // Still call real console.error.
+    realConsoleError(...errors);
+    // Send to error reporter.
+    report(...errors);
+  };
+}
+
+captureConsoleErrors(console.error, reportCrashError);
+
+window.addEventListener('error', reportCrashError);
+window.addEventListener('unhandledrejection', reportCrashError);

chromeos/components/media_app_ui/resources/js/media_app_index_scripts.js

@@ -4,6 +4,8 @@
 
 /** @fileoverview Concatenation of the JS files we use in index.html. */
 
+// <include src="error_reporter.js">
 // <include src="../../../system_apps/public/js/message_pipe.js">
 // <include src="message_types.js">
 // <include src="launch.js">
+// <include src="app_context_test_support.js">

chromeos/components/media_app_ui/test/media_app_ui_browsertest.js

@@ -181,6 +181,65 @@ TEST_F('MediaAppUIBrowserTest', 'LaunchFile', async () => {
   testDone();
 });
 
+// Tests that console.error()s in the trusted context are sent to the crash
+// reporter. This is also useful to ensure when multiple arguments are provided
+// to console.error, the error message is built up by appending all arguments to
+// the first arguments.
+// Note: unhandledrejection & onerror tests throw JS Errors regardless and are
+// tested in media_app_integration_browsertest.cc.
+TEST_F('MediaAppUIBrowserTest', 'ReportsErrorsFromTrustedContext', async () => {
+  const originalConsoleError = console.error;
+  const reportedErrors = [];
+
+  /**
+   * In tests stub out `chrome.crashReportPrivate.reportError`, check
+   *`reportedErrors` to make sure they are "sent" to the crash reporter.
+   */
+  function suppressConsoleErrorsForErrorTesting() {
+    chrome.crashReportPrivate.reportError = function(e) {
+      reportedErrors.push(e);
+    };
+    // Set `realConsoleError` in `captureConsoleErrors` to console.log to
+    // prevent console.error crashing tests.
+    captureConsoleErrors(console.log, reportCrashError);
+  }
+
+  suppressConsoleErrorsForErrorTesting();
+
+  assertEquals(0, reportedErrors.length);
+
+  const error = new Error('yikes message');
+  error.name = 'yikes error';
+  const extraData = {b: 'b'};
+
+  console.error('a');
+  console.error(error);
+  console.error('b', extraData);
+  console.error(extraData, extraData, extraData);
+  console.error(error, 'foo', extraData, {e: error});
+
+  assertEquals(5, reportedErrors.length);
+  // Test handles console.error(string).
+  assertEquals('Unexpected: "a"', reportedErrors[0].message);
+  // Test handles console.error(Error).
+  assertEquals('Error: yikes error: yikes message', reportedErrors[1].message);
+  // Test handles console.error(string, Object).
+  assertEquals('Unexpected: "b"\n{"b":"b"}', reportedErrors[2].message);
+  // Test handles console.error(Object, Object, Object).
+  assertEquals(
+      'Unexpected: {"b":"b"}\n{"b":"b"}\n{"b":"b"}', reportedErrors[3].message);
+  // Test handles console.error(string, Object, Error, Object).
+  assertEquals(
+      'Error: yikes error: yikes message\n"foo"\n{"b":"b"}\n' +
+          '{"e":{"name":"yikes error"}}',
+      reportedErrors[4].message);
+  // Note: This is not needed i.e. tests pass without this but it is good
+  // practice to reset it since we stub it out for this test.
+
+  console.error = originalConsoleError;
+  testDone();
+});
+
 // Tests that we can launch the MediaApp with the selected (first) file,
 // interact with it by invoking IPC (deletion) that doesn't re-launch the
 // MediaApp i.e. doesn't call `launchWithDirectory`, then the rest of the files

extensions/common/api/_api_features.json

@@ -140,7 +140,17 @@
     "dependencies": ["permission:crashReportPrivate"],
     "contexts": ["blessed_extension"],
     "default_parent": true
-  },{
+  },
+  {
+    "channel": "stable",
+    "contexts": [
+      "webui"
+    ],
+    "matches": [
+      "chrome://media-app/*"
+    ]
+  },
+  {
     "channel": "stable",
     "contexts": ["webui_untrusted"],
     "matches": [

extensions/common/api/generated_externs_list.txt

@@ -5,6 +5,7 @@ automation.idl
 bluetooth.idl
 bluetooth_private.idl
 clipboard.idl
+crash_report_private.idl
 management.json
 metrics_private.json
 mime_handler_private.idl

third_party/closure_compiler/externs/crash_report_private.js

+// 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.
+
+// This file was generated by:
+//   tools/json_schema_compiler/compiler.py.
+// NOTE: The format of types has changed. 'FooType' is now
+//   'chrome.crashReportPrivate.FooType'.
+// Please run the closure compiler before committing changes.
+// See
+// https://chromium.googlesource.com/chromium/src/+/master/docs/closure_compilation.md
+
+/** @fileoverview Externs generated from namespace: crashReportPrivate */
+
+/** @const */
+chrome.crashReportPrivate = {};
+
+/**
+ * @typedef {{
+ *   message: string,
+ *   url: string,
+ *   product: (string|undefined),
+ *   version: (string|undefined),
+ *   lineNumber: (number|undefined),
+ *   columnNumber: (number|undefined),
+ *   stackTrace: (string|undefined)
+ * }}
+ */
+chrome.crashReportPrivate.ErrorInfo;
+
+/**
+ * Report and upload an error to Crash.
+ * @param {!chrome.crashReportPrivate.ErrorInfo} info Information about the
+ *     error.
+ * @param {function(): void} callback Called when the error has been uploaded.
+ */
+chrome.crashReportPrivate.reportError = function(info, callback) {};