Closed Bug 863122 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: General, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: jtd, Assigned: vladan)

References

(Depends on 1 open bug)

Details

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

Attachments

(1 file)

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.
Whiteboard: [Snappy]
Assignee: nobody → vdjeric
Vladan, any chance we could get this in?
Blocks: 705287
Bumping the priority to match that of the bug that depends on this.
Severity: normal → critical
Whiteboard: [Snappy] → [Snappy:P1]
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+
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.
Depends on: 963780
https://hg.mozilla.org/mozilla-central/rev/027074c49207
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [Snappy:P1] → [Snappy:P1][qa-]
You need to log in before you can comment on or make changes to this bug.