1358516 - about:telemetry won't show archived pings

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Benjamin Smedberg]

My about:telemetry refuses to show the archived ping selector.

STR:
* open about:telemetry
* click "archived ping data"

EXPECTED:
* expect to see previous/next/date selector UI to pick a ping to view

ACTUAL:
* none of that UI appears
* the following message appears in the browser console: TypeError: stack.forEach is not a function  aboutTelemetry.js:1192:7

At https://dxr.mozilla.org/mozilla-central/rev/8b854986038cf3f3f240697e27ef48ea65914c13/toolkit/content/aboutTelemetry.js#1192
In a debugger, the value of stack is not an array. It is:

{
    "memoryMap": [
        ["ucrtbase.pdb", "5347FBF2D7FD4C99A15D54787482262A1"],
        ["xul.pdb", "91F8980E383E4AB0B495E2177D6803F62"]
    ],
    "stacks": [
        [
            [0, 629608],
            [0, 507184],
            [0, 500479],
            [0, 497066],
            [0, 492379],
            [0, 517777],
            [1, 6305320],
            [1, 6305260],
            [1, 9985755],
            [1, 10181787],
            [1, 1560675],
            [1, 1455567],
            [1, 1451684],
            [1, 1451484],
            [1, 1560762],
            [1, 1519764],
            [1, 1518169],
            [1, 1525288],
            [1, 1526041],
            [1, 1526398],
            [1, 1526123],
            [1, 1526785],
            [1, 5686313],
            [1, 5685908],
            [1, 5680548]
        ]
    ]
}

mystor or dthayer I know you're doing BHR work, did the threadhang stack format change recently in ways that would require an about:telemetry update?

Comment 1

[Nika Layzell [:nika] (ni? for response)]

The person responsible for this change in layout is mconley who added unsymbolicated native stacks IIRC.

So, yes. about:telemetry will need to be updated to support seeing this type of native stack on windows machines.

Comment 2

[Alex Thayer [:alexical] (she/her)]

I'll also add that this will likely cause about:telemetry to break much more often once Bug 1346415 lands, since this should only be breaking right now if you've had a hang that lasted longer than eight seconds.

Should we just strip the `nativeStack ||` out of there? We'd have to symbolicate the stack in order for it to be useful at a UI level.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

(In reply to Doug Thayer [:dthayer] from comment #2)
> I'll also add that this will likely cause about:telemetry to break much more
> often once Bug 1346415 lands, since this should only be breaking right now
> if you've had a hang that lasted longer than eight seconds.
> 
> Should we just strip the `nativeStack ||` out of there? We'd have to
> symbolicate the stack in order for it to be useful at a UI level.

That seems like a reasonable approach to take here.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Don't try to display an unsymbolicated native stack in about:telemetry

MozReview-Commit-ID: FHh0NScq8Jl

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8861030 [details] [diff] [review]
Don't try to display an unsymbolicated native stack in about:telemetry

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

Some questions below.
Please go ahead and land this if the data structure concern is addressed.

::: toolkit/content/aboutTelemetry.js
@@ +1187,5 @@
>        let hangName = aThread.name + "-Hang-" + (index + 1);
>        let hangDiv = Histogram.render(
>          div, hangName, hang.histogram, {exponential: true}, true);
>        let stackDiv = document.createElement("div");
> +      hang.stack.forEach((frame) => {

Does `hang.stack` always exist?
Even when `nativeStack` is set?
Even before the recent `threadHangstats` changes, i.e. in "old" ping data?

@@ -1187,5 @@
>        let hangName = aThread.name + "-Hang-" + (index + 1);
>        let hangDiv = Histogram.render(
>          div, hangName, hang.histogram, {exponential: true}, true);
>        let stackDiv = document.createElement("div");
> -      let stack = hang.nativeStack || hang.stack;

Ok, removing `hang.nativeStack` fixes things for now.
Should we have a follow-up bug on also rendering `nativeStack`? We can set that up as a mentored bug.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> Comment on attachment 8861030 [details] [diff] [review]
> Don't try to display an unsymbolicated native stack in about:telemetry
> 
> Review of attachment 8861030 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some questions below.
> Please go ahead and land this if the data structure concern is addressed.
> 
> ::: toolkit/content/aboutTelemetry.js
> @@ +1187,5 @@
> >        let hangName = aThread.name + "-Hang-" + (index + 1);
> >        let hangDiv = Histogram.render(
> >          div, hangName, hang.histogram, {exponential: true}, true);
> >        let stackDiv = document.createElement("div");
> > +      hang.stack.forEach((frame) => {
> 
> Does `hang.stack` always exist?
> Even when `nativeStack` is set?
> Even before the recent `threadHangstats` changes, i.e. in "old" ping data?

Yes, hang.stack should always exist even in old pings. The stack is the value which hangs are keyed on and thus must be present even when the native stack is available.

> 
> @@ -1187,5 @@
> >        let hangName = aThread.name + "-Hang-" + (index + 1);
> >        let hangDiv = Histogram.render(
> >          div, hangName, hang.histogram, {exponential: true}, true);
> >        let stackDiv = document.createElement("div");
> > -      let stack = hang.nativeStack || hang.stack;
> 
> Ok, removing `hang.nativeStack` fixes things for now.
> Should we have a follow-up bug on also rendering `nativeStack`? We can set
> that up as a mentored bug.

It might be nice, but it will be a bit tricky. nativeStack is stored unsymbolicated, and thus is pretty much unreadable to anyone until it is symbolicated, which doesn't seem like something about:telemetry should be doing.

Comment 7

[Georg Fritzsche [:gfritzsche]]

(In reply to Michael Layzell [:mystor] from comment #6)
> > @@ -1187,5 @@
> > >        let hangName = aThread.name + "-Hang-" + (index + 1);
> > >        let hangDiv = Histogram.render(
> > >          div, hangName, hang.histogram, {exponential: true}, true);
> > >        let stackDiv = document.createElement("div");
> > > -      let stack = hang.nativeStack || hang.stack;
> > 
> > Ok, removing `hang.nativeStack` fixes things for now.
> > Should we have a follow-up bug on also rendering `nativeStack`? We can set
> > that up as a mentored bug.
> 
> It might be nice, but it will be a bit tricky. nativeStack is stored
> unsymbolicated, and thus is pretty much unreadable to anyone until it is
> symbolicated, which doesn't seem like something about:telemetry should be
> doing.

We have a symbolication implementation in aboutTelemetry.js already, which can be re-used.
Certainly not high priority, but would be "nice to have" as a mentored bug.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9056e0664ad
Don't try to display an unsymbolicated native stack in about:telemetry, r=gfritzsche

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c9056e0664ad