Make about:telemetry more exception-safe

RESOLVED FIXED in Firefox 55

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gfritzsche, Assigned: flyingrub, Mentored)

Tracking

(Blocks 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(2 attachments)

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
Reporter

Comment 1

2 years ago
flyingrub, would you be interested in taking this?
Flags: needinfo?(flyinggrub)
Assignee

Comment 2

2 years ago
It seems to totally fit the about:telemetry redesign.

I'm taking it. :)
Flags: needinfo?(flyinggrub)
Assignee

Updated

2 years ago
Assignee: nobody → flyinggrub
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
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`
Attachment #8871774 - Flags: review?(chutten) → review+
Assignee

Comment 5

2 years ago
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 hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
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.
Attachment #8871816 - Flags: review?(chutten) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

2 years ago
mozreview-review
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.
Attachment #8871816 - Flags: review?(chutten) → review+
Is this all the work necessary for this bug? If so, we can start landing these.

Updated

2 years ago
Keywords: checkin-needed

Comment 12

2 years ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2c9139008c3c
https://hg.mozilla.org/mozilla-central/rev/f50f39148516
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Reporter

Updated

2 years ago
Blocks: 1384534
You need to log in before you can comment on or make changes to this bug.