Closed Bug 1210658 Opened 4 years ago Closed 4 years ago

Need exception handling in assemblePayloadWithMeasurements

Categories

(Toolkit :: Telemetry, defect, P1)

defect
Points:
1

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: vladan, Assigned: gfritzsche)

References

(Blocks 1 open bug)

Details

(Whiteboard: [unifiedTelemetry] [measurement:client])

Attachments

(2 files)

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
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.
Blocks: 1201022
Priority: -- → P3
Whiteboard: [unifiedTelemetry] [measurement:client]
Points: --- → 1
Priority: P3 → P1
Priority: P1 → P2
Priority: P2 → P1
Assignee: nobody → gfritzsche
Status: NEW → ASSIGNED
This is specifically addressing the ping assembly code path. Further exception handling review will happen in bug 1177958 or other separate follow-ups.
Attachment #8675687 - Flags: review?(alessio.placitelli)
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?
Attachment #8675687 - Flags: review?(alessio.placitelli) → review+
Attachment #8675751 - Flags: review?(alessio.placitelli) → review+
https://hg.mozilla.org/mozilla-central/rev/28a844f6cd11
https://hg.mozilla.org/mozilla-central/rev/9a1f6c87afc8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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?
Flags: needinfo?(gfritzsche)
Reopening until the problem in comment #8 has been resolved. Bug fix for it can be found in bug 1198883.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 1198883
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.
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Flags: needinfo?(gfritzsche)
Resolution: --- → FIXED
(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.
You need to log in before you can comment on or make changes to this bug.