Closed Bug 1210658 Opened 4 years ago Closed 4 years ago
Need exception handling in assemble
Payload With Measurements
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.
Priority: -- → P3
Whiteboard: [unifiedTelemetry] [measurement:client]
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+
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?
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 → ---
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 ago → 4 years ago
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.