Skip to content

GitLab

T
tangled
Commit 377e9bec authored by David's avatar David Committed by Commit Bot
Browse files
Options

Add renderer_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 renderer process
to help track down potential race conditions.

The uptime is obtained from metrics::RendererUptimeTracker.

Bug: b/169635499, 1143049
Change-Id: Id12667449d9d208048af0a2dad5ed6f282033333
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2501105
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: default avatarGiovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: default avatarTrent Apted <tapted@chromium.org>
Reviewed-by: default avatarIan Barkley-Yeung <iby@chromium.org>
Auto-Submit: David Lei <dlei@google.com>
Cr-Commit-Position: refs/heads/master@{#823074}
parent a4be931c
Hide whitespace changes
Inline Side-by-side
Showing with 36 additions and 4 deletions
chrome/browser/error_reporting/chrome_js_error_report_processor.cc
View file @ 377e9bec
......@@ -247,6 +247,8 @@ void ChromeJsErrorReportProcessor::OnConsentCheckCompleted(
// type" fields, eventually consider using that for process uptime.
params["browser_process_uptime_ms"] =
base::NumberToString(browser_process_uptime.InMilliseconds());
params["renderer_process_uptime_ms"] =
base::NumberToString(error_report->renderer_process_uptime_ms);
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
View file @ 377e9bec
......@@ -70,6 +70,7 @@ TEST_F(ChromeJsErrorReportProcessorTest, Basic) {
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="));
EXPECT_THAT(actual_report->query, HasSubstr("renderer_process_uptime_ms=0"));
// TODO(iby) research why URL is repeated...
EXPECT_THAT(actual_report->query,
HasSubstr("src=https%3A%2F%2Fwww.chromium.org%2FHome"));
......@@ -100,6 +101,7 @@ TEST_F(ChromeJsErrorReportProcessorTest, AllFields) {
report.line_number = 83;
report.column_number = 14;
report.stack_trace = "bad_func(1, 2)\nonclick()\n";
report.renderer_process_uptime_ms = 1234;
processor_->SendErrorReport(
std::move(report),
......@@ -115,6 +117,8 @@ TEST_F(ChromeJsErrorReportProcessorTest, AllFields) {
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="));
EXPECT_THAT(actual_report->query,
HasSubstr("renderer_process_uptime_ms=1234"));
// 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
View file @ 377e9bec
......@@ -6,9 +6,13 @@
#include "base/time/default_clock.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/metrics/renderer_uptime_tracker.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"
#include "content/public/browser/render_process_host.h"
#include "content/public/browser/render_view_host.h"
#include "content/public/browser/web_contents.h"
namespace extensions {
namespace api {
......@@ -79,6 +83,18 @@ 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();
}
error_report.app_locale = g_browser_process->GetApplicationLocale();
processor->SendErrorReport(
......
chrome/browser/extensions/api/crash_report_private/crash_report_private_apitest.cc
View file @ 377e9bec
......@@ -103,7 +103,8 @@ IN_PROC_BROWSER_TEST_F(CrashReportPrivateApiTest, Basic) {
"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&src=http%3A%2F%2Fwww.test."
"&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"));
EXPECT_EQ(report->content, "");
}
......@@ -134,7 +135,8 @@ IN_PROC_BROWSER_TEST_F(CrashReportPrivateApiTest, ExtraParamsAndStackTrace) {
"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=7.20.1"
"&prod=Chrome%2520\\(Chrome%2520OS\\)&"
"&prod=Chrome%2520\\(Chrome%2520OS\\)&renderer_process_"
"uptime_ms=\\d+&"
"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");
......@@ -164,7 +166,8 @@ IN_PROC_BROWSER_TEST_F(CrashReportPrivateApiTest, StackTraceWithErrorMessage) {
"3.4&channel=Stable&column=456&"
"error_message=hi&full_url=http%3A%2F%2Fwww.test.com%2Ffoo&"
"line=123&os=ChromeOS&os_version=7.20.1"
"&prod=TestApp&src=http%3A%2F%2Fwww.test.com%2Ffoo&type="
"&prod=TestApp&renderer_process_uptime_ms=\\d+&src=http%3A%"
"2F%2Fwww.test.com%2Ffoo&type="
"JavascriptError&url=%2Ffoo&ver=1.0.0.0"));
EXPECT_EQ(report->content, "");
}
......@@ -196,7 +199,8 @@ IN_PROC_BROWSER_TEST_F(CrashReportPrivateApiTest, RedactMessage) {
"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=7.20.1"
"&prod=TestApp&src=http%3A%2F%2Fwww.test.com%2Ffoo&type="
"&prod=TestApp&renderer_process_uptime_ms=\\d+&src=http%3A%2F%2Fwww."
"test.com%2Ffoo&type="
"JavascriptError&url=%2Ffoo&ver=1.0.0.0"));
EXPECT_EQ(report->content, "");
}
......
components/crash/content/browser/error_reporting/javascript_error_report.h
View file @ 377e9bec
......@@ -48,6 +48,12 @@ struct COMPONENT_EXPORT(JS_ERROR_REPORTING) JavaScriptErrorReport {
// String containing the application locale. Not sent if not present.
base::Optional<std::string> app_locale;
// Uptime of the renderer process in milliseconds. 0 if the callee
// |web_contents| is null (shouldn't really happen as this is caled from a JS
// context) or the renderer process doesn't exist (possible due to termination
// / failure to start).
int renderer_process_uptime_ms = 0;
};
#endif // COMPONENTS_CRASH_CONTENT_BROWSER_ERROR_REPORTING_JAVASCRIPT_ERROR_REPORT_H_
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