Skip to content

GitLab

T
tangled
Commit 5ce3f773 authored by David's avatar David Committed by Commit Bot
Browse files
Options

Add browser_process_uptime_ms to JS crash reports. 

Currently we don't record this for JS crash reports.
chrome://media-app wants to record the uptime of the browser process
to help track down potential race conditions.

We think this can be useful in general for JS errors.

Doc: http://go/backlight-better-crash-report#heading=h.1fkxhdmqpt47

Note: we have Process uptime & Process type fields in chrome crashes
and I believe we plan to add this in for JS crashes in crbug/1121816.

Change-Id: Ide154b49d2f9791cb044885054a5ff929f720618
Bug: b/169635499
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2463094
Commit-Queue: David Lei <dlei@google.com>
Reviewed-by: default avatarGabriel Charette <gab@chromium.org>
Reviewed-by: default avatarGiovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: default avatarIan Barkley-Yeung <iby@chromium.org>
Reviewed-by: default avatarBugs Nash <bugsnash@chromium.org>
Auto-Submit: David Lei <dlei@google.com>
Cr-Commit-Position: refs/heads/master@{#818717}
parent 4dad6dba
Hide whitespace changes
Inline Side-by-side
Showing with 61 additions and 31 deletions
components/crash/content/browser/error_reporting/BUILD.gn
View file @ 5ce3f773
......@@ -21,6 +21,7 @@ static_library("error_reporting") {
"//base",
"//components/crash/core/app",
"//components/feedback",
"//components/startup_metric_utils/browser",
"//content/public/browser",
"//net",
"//services/network:network_service",
......
components/crash/content/browser/error_reporting/DEPS
View file @ 5ce3f773
include_rules = [
"+components/feedback",
"+components/startup_metric_utils",
"+net",
"+services/network",
]
components/crash/content/browser/error_reporting/send_javascript_error_report.cc
View file @ 5ce3f773
......@@ -24,6 +24,7 @@
#include "components/crash/content/browser/error_reporting/javascript_error_report.h"
#include "components/crash/core/app/client_upload_info.h"
#include "components/feedback/redaction_tool.h"
#include "components/startup_metric_utils/browser/startup_metric_utils.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/storage_partition.h"
......@@ -236,6 +237,7 @@ void SendReport(const GURL& url,
void OnConsentCheckCompleted(
base::ScopedClosureRunner callback_runner,
scoped_refptr<network::SharedURLLoaderFactory> loader_factory,
base::TimeDelta browser_process_uptime,
base::Optional<JavaScriptErrorReport> error_report) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
if (!error_report) {
......@@ -282,6 +284,11 @@ void OnConsentCheckCompleted(
if (error_report->column_number)
params["column"] = base::NumberToString(*error_report->column_number);
// TODO(crbug/1121816): Chrome crashes have "Process uptime" and "Process
// type" fields, eventually consider using that for process uptime.
params["browser_process_uptime_ms"] =
base::NumberToString(browser_process_uptime.InMilliseconds());
const GURL url(base::StrCat(
{crash_endpoint_string, "?", BuildPostRequestQueryString(params)}));
std::string body;
......@@ -312,13 +319,19 @@ void SendJavaScriptErrorReport(JavaScriptErrorReport error_report,
content::BrowserContext::GetDefaultStoragePartition(browser_context)
->GetURLLoaderFactoryForBrowserProcess();
// Get browser uptime before swapping threads to reduce lag time between the
// error report occurring and sending it off.
base::TimeTicks startup_time = startup_metric_utils::MainEntryPointTicks();
base::TimeDelta browser_process_uptime =
(base::TimeTicks::Now() - startup_time);
// Consent check needs to be done on a blockable thread. We must return to
// this thread (the UI thread) to use the loader_factory.
base::ThreadPool::PostTaskAndReplyWithResult(
FROM_HERE, {base::MayBlock()},
base::BindOnce(&CheckConsentAndRedact, std::move(error_report)),
base::BindOnce(&OnConsentCheckCompleted, std::move(callback_runner),
std::move(loader_factory)));
std::move(loader_factory), browser_process_uptime));
}
#endif // !defined(OS_WIN)
......
components/crash/content/browser/error_reporting/send_javascript_error_report_unittest.cc
View file @ 5ce3f773
......@@ -72,6 +72,7 @@ TEST_F(SendJavaScriptErrorReportTest, Basic) {
ASSERT_TRUE(actual_report);
EXPECT_THAT(actual_report->query, HasSubstr("error_message=Hello%20World"));
EXPECT_THAT(actual_report->query, HasSubstr("type=JavascriptError"));
EXPECT_THAT(actual_report->query, HasSubstr("browser_process_uptime_ms="));
// TODO(iby) research why URL is repeated...
EXPECT_THAT(actual_report->query,
HasSubstr("src=https%3A%2F%2Fwww.chromium.org%2FHome"));
......@@ -116,6 +117,7 @@ TEST_F(SendJavaScriptErrorReportTest, AllFields) {
ASSERT_TRUE(actual_report);
EXPECT_THAT(actual_report->query, HasSubstr("error_message=Hello%20World"));
EXPECT_THAT(actual_report->query, HasSubstr("type=JavascriptError"));
EXPECT_THAT(actual_report->query, HasSubstr("browser_process_uptime_ms="));
// TODO(iby) research why URL is repeated...
EXPECT_THAT(actual_report->query,
HasSubstr("src=https%3A%2F%2Fwww.chromium.org%2FHome"));
......
extensions/browser/api/crash_report_private/crash_report_private_apitest.cc
View file @ 5ce3f773
......@@ -12,6 +12,7 @@
#include "extensions/shell/test/shell_apitest.h"
#include "extensions/test/extension_test_message_listener.h"
#include "extensions/test/test_extension_dir.h"
#include "testing/gmock/include/gmock/gmock-matchers.h"
namespace extensions {
......@@ -19,6 +20,8 @@ using browsertest_util::ExecuteScriptInBackgroundPage;
namespace {
using ::testing::MatchesRegex;
constexpr const char* kTestExtensionId = "jjeoclcdfjddkdjokiejckgcildcflpp";
std::string GetOsVersion() {
......@@ -99,13 +102,15 @@ IN_PROC_BROWSER_TEST_F(CrashReportPrivateApiTest, Basic) {
const base::Optional<MockCrashEndpoint::Report>& report = last_report();
ASSERT_TRUE(report);
EXPECT_EQ(report->query,
"browser=Chrome&browser_version=1.2.3.4&channel=Stable&"
"error_message=hi&full_url=http%3A%2F%2Fwww.test.com%2F&"
"os=ChromeOS&os_version=" +
GetOsVersion() +
"&prod=Chrome_ChromeOS&src=http%3A%2F%2Fwww.test."
"com%2F&type=JavascriptError&url=%2F&ver=1.2.3.4");
EXPECT_THAT(
report->query,
MatchesRegex("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=" +
GetOsVersion() +
"&prod=Chrome_ChromeOS&src=http%3A%2F%2Fwww.test."
"com%2F&type=JavascriptError&url=%2F&ver=1.2.3.4"));
EXPECT_EQ(report->content, "");
}
......@@ -129,14 +134,16 @@ IN_PROC_BROWSER_TEST_F(CrashReportPrivateApiTest, ExtraParamsAndStackTrace) {
ASSERT_TRUE(report);
// The product name is escaped twice. The first time, it becomes
// "Chrome%20(Chrome%20OS)" and then the second escapes the '%' into '%25'.
EXPECT_EQ(report->query,
"browser=Chrome&browser_version=1.2.3.4&channel=Stable&column=456&"
"error_message=hi&full_url=http%3A%2F%2Fwww.test.com%2Ffoo"
"&line=123&os=ChromeOS&os_version=" +
GetOsVersion() +
"&prod=Chrome%2520(Chrome%2520OS)&"
"src=http%3A%2F%2Fwww.test.com%2Ffoo&"
"type=JavascriptError&url=%2Ffoo&ver=1.0.0.0");
EXPECT_THAT(
report->query,
MatchesRegex("browser=Chrome&browser_process_uptime_ms=\\d+&browser_"
"version=1.2.3.4&channel=Stable&column=456&"
"error_message=hi&full_url=http%3A%2F%2Fwww.test.com%2Ffoo"
"&line=123&os=ChromeOS&os_version=" +
GetOsVersion() +
"&prod=Chrome%2520\\(Chrome%2520OS\\)&"
"src=http%3A%2F%2Fwww.test.com%2Ffoo&"
"type=JavascriptError&url=%2Ffoo&ver=1.0.0.0"));
EXPECT_EQ(report->content, " at <anonymous>:1:1");
}
......@@ -158,13 +165,16 @@ IN_PROC_BROWSER_TEST_F(CrashReportPrivateApiTest, StackTraceWithErrorMessage) {
const base::Optional<MockCrashEndpoint::Report>& report = last_report();
ASSERT_TRUE(report);
EXPECT_EQ(report->query,
"browser=Chrome&browser_version=1.2.3.4&channel=Stable&column=456&"
"error_message=hi&full_url=http%3A%2F%2Fwww.test.com%2Ffoo&"
"line=123&os=ChromeOS&os_version=" +
GetOsVersion() +
"&prod=TestApp&src=http%3A%2F%2Fwww.test.com%2Ffoo&type="
"JavascriptError&url=%2Ffoo&ver=1.0.0.0");
EXPECT_THAT(
report->query,
MatchesRegex(
"browser=Chrome&browser_process_uptime_ms=\\d+&browser_version=1.2."
"3.4&channel=Stable&column=456&"
"error_message=hi&full_url=http%3A%2F%2Fwww.test.com%2Ffoo&"
"line=123&os=ChromeOS&os_version=" +
GetOsVersion() +
"&prod=TestApp&src=http%3A%2F%2Fwww.test.com%2Ffoo&type="
"JavascriptError&url=%2Ffoo&ver=1.0.0.0"));
EXPECT_EQ(report->content, "");
}
......@@ -187,14 +197,17 @@ IN_PROC_BROWSER_TEST_F(CrashReportPrivateApiTest, RedactMessage) {
const base::Optional<MockCrashEndpoint::Report>& report = last_report();
ASSERT_TRUE(report);
EXPECT_EQ(report->query,
"browser=Chrome&browser_version=1.2.3.4&channel=Stable&column=456&"
"error_message=%5BMAC%20OUI%3D06%3A00%3A00%20IFACE%3D1%5D&"
"full_url=http%3A%2F%2Fwww.test.com%2Ffoo&line=123&os=ChromeOS&"
"os_version=" +
GetOsVersion() +
"&prod=TestApp&src=http%3A%2F%2Fwww.test.com%2Ffoo&type="
"JavascriptError&url=%2Ffoo&ver=1.0.0.0");
EXPECT_THAT(
report->query,
MatchesRegex(
"browser=Chrome&browser_process_uptime_ms=\\d+&browser_version=1.2."
"3.4&channel=Stable&column=456&"
"error_message=%5BMAC%20OUI%3D06%3A00%3A00%20IFACE%3D1%5D&"
"full_url=http%3A%2F%2Fwww.test.com%2Ffoo&line=123&os=ChromeOS&"
"os_version=" +
GetOsVersion() +
"&prod=TestApp&src=http%3A%2F%2Fwww.test.com%2Ffoo&type="
"JavascriptError&url=%2Ffoo&ver=1.0.0.0"));
EXPECT_EQ(report->content, "");
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment