1210658 - Need exception handling in assemblePayloadWithMeasurements

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Vladan Djeric (:vladan)]

TelemetrySession.jsm calls into native code using XPIDL but the C++ implementations of those XPIDL interfaces sometimes return nsresult failure codes. An nsresult error rturn will trigger a JS exception in the calling code and TelemetrySession.jsm isn't set up to recover from such exceptions.

Calling code (no exception handler):

http://hg.mozilla.org/mozilla-central/annotate/2c1fb007137d/toolkit/components/telemetry/TelemetrySession.jsm#l1266

Native, exception-throwing code (only one example):

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.cpp#3555

Comment 1

[Georg Fritzsche [:gfritzsche]]

Also chromeHangs, threadHangStats, etc.:
http://hg.mozilla.org/mozilla-central/annotate/2c1fb007137d/toolkit/components/telemetry/TelemetrySession.jsm#l1254

I think in general we need an in-depth review of exception-safety of the Telemetry modules, but this seems like a path we should fix soon.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Attachment: Protect Telemetry ping assembly from exceptions in data gathering functions

This is specifically addressing the ping assembly code path. Further exception handling review will happen in bug 1177958 or other separate follow-ups.

Comment 3

[Georg Fritzsche [:gfritzsche]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2a512af0022

Comment 4

[Alessio Placitelli [:Dexter]]

Comment on attachment 8675687 [details] [diff] [review]
Protect Telemetry ping assembly from exceptions in data gathering functions

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

This looks good. Should we mention in the docs (i.e. main-ping.rst) that sections of the payload can be null due to client errors?

Comment 5

[Georg Fritzsche [:gfritzsche]]

Attachment: Explicitly document optional Telemetry ping properties

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/28a844f6cd11
https://hg.mozilla.org/integration/fx-team/rev/9a1f6c87afc8

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/28a844f6cd11
https://hg.mozilla.org/mozilla-central/rev/9a1f6c87afc8

Comment 8

[Nils Ohlmeier [:drno]]

This code here actually introduce a new bug:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetrySession.jsm#1246

this.log is undefined in that line. I'm guessing this._log was the intention. Should I re-open this bug here, or if you agree on using this._log instead should I simply land the fix together with my code in bug 1198883?

Comment 9

[Nils Ohlmeier [:drno]]

Reopening until the problem in comment #8 has been resolved. Bug fix for it can be found in bug 1198883.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Ah, thanks for the find.
This bug was merged to mozilla-central, any follow-up work should be in other bugs.
I'll open one and flag you there.

Comment 11

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #10)
> Ah, thanks for the find.
> This bug was merged to mozilla-central, any follow-up work should be in
> other bugs.
> I'll open one and flag you there.

Looking in your bug, this seems easy to handle there, lets just do that.