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

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[John Daggett (:jtd)]

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.

Comment 1

[John Daggett (:jtd)]

Vladan, any chance we could get this in?

Comment 2

[John Daggett (:jtd)]

Bumping the priority to match that of the bug that depends on this.

Comment 3

[Vladan Djeric (:vladan)]

Attachment: chromehangsUptime.patch

Comment 4

[Nathan Froyd [:froydnj]]

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.

Comment 5

[John Daggett (:jtd)]

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.

Comment 6

[Vladan Djeric (:vladan)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/027074c49207

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/027074c49207