add time since launch and time since system startup to chromehang data

RESOLVED FIXED in mozilla29

Status

()

Core
General
--
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jtd, Assigned: vladan)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:P1][qa-])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
I confirmed with Vladan that the chromehang data we collect doesn't contain either (1) time since Firefox launch and (2) time since system startup (e.g. GetTickCount on Windows).  This information would be extremely helpful for understanding the environment that leads to specific hangs and categorizing them appropriately.

Updated

5 years ago
Whiteboard: [Snappy]
(Assignee)

Updated

5 years ago
Assignee: nobody → vdjeric
(Reporter)

Comment 1

4 years ago
Vladan, any chance we could get this in?
Blocks: 705287
(Reporter)

Comment 2

4 years ago
Bumping the priority to match that of the bug that depends on this.
Severity: normal → critical
Whiteboard: [Snappy] → [Snappy:P1]
(Assignee)

Comment 3

4 years ago
Created attachment 8362720 [details] [diff] [review]
chromehangsUptime.patch
Attachment #8362720 - Flags: review?(nfroyd)
Comment on attachment 8362720 [details] [diff] [review]
chromehangsUptime.patch

Review of attachment 8362720 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with a good answer for the HangMonitor.cpp bit.

::: toolkit/components/telemetry/Telemetry.cpp
@@ +1531,5 @@
>    }
>  
> +  ok = JS_DefineProperty(cx, fullReportObj, "systemUptime",
> +                         OBJECT_TO_JSVAL(systemUptimeArray),
> +                         nullptr, nullptr, JSPROP_ENUMERATE);

Can you file a followup for inverting the order that we do things in this function, so that we:

- build the arrays;
- set fullReportObj properties;
- set *ret.

in that order?  We've tried to not leave partial bits hanging around on failure in other reports; it'd be good to follow the same pattern here.

::: toolkit/components/telemetry/Telemetry.h
@@ +168,5 @@
>   *
> + * @param aDuration - Approximate duration of main thread hang in seconds
> + * @param aStack - Array of PCs from the hung call stack
> + * @param aSystemUptime - System uptime at the time of the hang
> + * @param aFirefoxUptime - Firefox uptime at the time of the hang

Nit: can you specify that these are in minutes and document the -1 value you use for all other platforms than Windows?

::: xpcom/threads/HangMonitor.cpp
@@ +205,5 @@
> +#ifdef REPORT_CHROME_HANGS
> +      if (waitCount == 2) {
> +        GetChromeHangReport(stack, systemUptime, firefoxUptime);
> +      }
> +#else

I am surprised to see this chunk of code in the patch for this bug; we want this so the *Uptime variables are for the start of a long-duration hang rather than the end of a long-duration hang, correct?  If so, a comment might be appropriate.
Attachment #8362720 - Flags: review?(nfroyd) → review+
(Reporter)

Comment 5

4 years ago
In case this helps, the need for system uptime in chrome hang reports is so that we can distinguish chrome hangs that are specific to Firefox from chrome hangs that are related to the system condition at startup.  Under Windows for example, on cold startup the system may still be initializing when Firefox is started. This is known to cause certain delays, such as DirectWrite initialization. Having more detailed data also allows us to ask folks like Microsoft to fix these problems.
(Assignee)

Updated

4 years ago
Depends on: 963780
https://hg.mozilla.org/mozilla-central/rev/027074c49207
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

4 years ago
Whiteboard: [Snappy:P1] → [Snappy:P1][qa-]
You need to log in before you can comment on or make changes to this bug.