1363828 - Make about:telemetry more exception-safe

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Georg Fritzsche [:gfritzsche]]

Per bug 1363759, a missing addons section in a ping is breaking the whole about:telemetry page.

In displayPingData() [1], we should make sure to catch errors from any of the renderers.
If any of them fail we should still be able to:
- show the raw data
- not break the top-level controls

> STR:
> Run a build with an active SHIELD experiment
> Quit the build
> Run it again
> Visit about:telemetry
> Choose "Archived Ping Data"
> 
> Actual:
> Archived ping data is broken and there is no selector for an archived ping to load
> Message in the JS console: TypeError: addons is undefined[Learn More]  aboutTelemetry.js:720:5
> 
> This is https://hg.mozilla.org/mozilla-central/annotate/b21b974d60d3075ae24f6fb1bae75d0f122f28fc/toolkit/content/aboutTelemetry.js#l720
> 
> I loaded the ping by hand using TelemetryArchive, and it's a shield-study ping. This ping has an environment section, but no addons block. I'm going to attach the entire ping contents.
> ...
> * unexpected exceptions in aboutTelemetry.js rendering shouldn't affect the history view or prevent me from at least viewing the raw data

1: https://dxr.mozilla.org/mozilla-central/rev/b21b974d60d3075ae24f6fb1bae75d0f122f28fc/toolkit/content/aboutTelemetry.js#2084

Comment 1

[Georg Fritzsche [:gfritzsche]]

flyingrub, would you be interested in taking this?

Comment 2

[flyingrub]

It seems to totally fit the about:telemetry redesign.

I'm taking it. :)

Comment 3

[flyingrub]

Attachment: Bug 1363828 - Replace custom isArray() by Array.isArray()

Review commit: https://reviewboard.mozilla.org/r/143232/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/143232/

Comment 4

[Chris H-C :chutten]

Comment on attachment 8871774 [details]
Bug 1363828 - Replace custom isArray() by Array.isArray()

https://reviewboard.mozilla.org/r/143232/#review146976

Did you run this through the linter to make sure it's clean? (It probably is)

`mach lint toolkit/components/telemetry`

Comment 5

[flyingrub]

I just tried to run it with `./mach lint toolkit/content/aboutTelemetry.js` (it's the only file I changed) and it went fine.

Comment 6

[flyingrub]

Attachment: Bug 1363828 - First attempt at making about:telemetry more exception

safe

We could also test ping.environment.addons before using it.

Review commit: https://reviewboard.mozilla.org/r/143280/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/143280/

Comment 7

[Chris H-C :chutten]

Comment on attachment 8871816 [details]
Bug 1363828 - First attempt at making about:telemetry more exception

https://reviewboard.mozilla.org/r/143280/#review147018

::: toolkit/content/aboutTelemetry.js:2087
(Diff revision 1)
>    // Render raw ping data.
>    let pre = document.getElementById("raw-ping-data");
>    pre.textContent = JSON.stringify(gPingData, null, 2);
>  
> +  try {
> +    tryDdisplayPingData(ping, updatePayloadList);

Ugh. Naming things is hard.

Maybe call it `displayRichPingData` or something that calls out that it's only doing the "fancy" stuff?

::: toolkit/content/aboutTelemetry.js:2093
(Diff revision 1)
> +  } catch (err) {
> +    PingPicker._showRawPingData();
> +  }
> +}
> +
> +function tryDdisplayPingData(ping, updatePayloadList = false) {

This doesn't require a default value as it will always have a value.

Comment 8

[flyingrub]

Comment on attachment 8871816 [details]
Bug 1363828 - First attempt at making about:telemetry more exception

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143280/diff/1-2/

Comment 9

[flyingrub]

Comment on attachment 8871816 [details]
Bug 1363828 - First attempt at making about:telemetry more exception

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143280/diff/2-3/

Comment 10

[Chris H-C :chutten]

Comment on attachment 8871816 [details]
Bug 1363828 - First attempt at making about:telemetry more exception

https://reviewboard.mozilla.org/r/143280/#review147052

This brings up an interesting question for the redesign:

What should we do if something goes wrong? Should we log to console? Show something to the user? 

The current aboutTelemetry.js just swallows known errors.

Something to think about.

Comment 11

[Chris H-C :chutten]

Is this all the work necessary for this bug? If so, we can start landing these.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c9139008c3c
Replace custom isArray() by Array.isArray() r=chutten
https://hg.mozilla.org/integration/autoland/rev/f50f39148516
First attempt at making about:telemetry more exception r=chutten

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2c9139008c3c
https://hg.mozilla.org/mozilla-central/rev/f50f39148516