1199662 - Crash ping environments are broken

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Benjamin Smedberg]

STR:
Crash Firefox: you can do this by typing this into the browser console:

  Cu.import("resource://gre/modules/ctypes.jsm");
  ctypes.cast(new ctypes.intptr_t(5), ctypes.int32_t.ptr).contents;

Re-open Firefox and wait a minute.
Using the future version of about:healthreport (not landed yet) or API access, check the contents of the crash ping.

EXPECTED:
payload.hasCrashEnvironment should be true, and the environment block of the ping should list the previous environment.

ACTUAL:
payload.hasCrashEnvironment is false.
Exception in the browser console:
10:18:11.597 JSON.parse: expected ',' or '}' after property value in object at line 1 column 785 of the JSON data1 CrashManager.jsm:541:0

Comment 1

[Benjamin Smedberg]

In the data submitted to crash-stats, TelemetryEnvironment is a correct annotation and appears to be valid JSON.

Comment 2

[Benjamin Smedberg]

Turns out it's confusion about escaping in the event file. Crash reporter stores and write its key-value pairs by escaping \ and \n, but when we read it we don't unescape. Going to fix this using KeyValueParser.jsm.

Comment 3

[Benjamin Smedberg]

Attachment: MozReview Request: Bug 1199662 - Crash ping environment block is broken when any string field contains a quotation mark. Unescape INI fields properly using the library that already exists for the purpose. r?ted

Bug 1199662 - Crash ping environment block is broken when any string field contains a quotation mark. Unescape INI fields properly using the library that already exists for the purpose. r?ted

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 8654980 [details]
MozReview Request: Bug 1199662 - Crash ping environment block is broken when any string field contains a quotation mark. Unescape INI fields properly using the library that already exists for the purpose. r?ted

https://reviewboard.mozilla.org/r/17783/#review15975

Wish we weren't using such a half-baked format, but at least we have a library to parse it!

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f903c7e59e0

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f903c7e59e0

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8654980 [details]
MozReview Request: Bug 1199662 - Crash ping environment block is broken when any string field contains a quotation mark. Unescape INI fields properly using the library that already exists for the purpose. r?ted

Approval Request Comment
[Feature/regressing bug #]:
Unified Telemetry
[User impact if declined]:
Part of the data in crash pings is broken and thus missing for analysis.
[Describe test coverage new/current, TreeHerder]:
Automated tests, manual confirmation.
[Risks and why]:
Low-risk, small and limited change.
[String/UUID change made/needed]:
None.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8654980 [details]
MozReview Request: Bug 1199662 - Crash ping environment block is broken when any string field contains a quotation mark. Unescape INI fields properly using the library that already exists for the purpose. r?ted

OK, let's do it.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0d91010d6dcc