1491778 - Crash report has invalid JSON in the telemetry environment

Status: RESOLVED FIXED

(Socorro :: Webapp, task, P3)

Description

[Kartikaya Gupta (email:[e-mail])]

If you go to the telemetry environment tab on https://crash-stats.mozilla.com/report/index/9f215422-98ef-436e-9622-b1c210180915#tab-telemetryenvironment it says the JSON is invalid. I haven't checked the raw dump to see what's actually happening.

Comment 1

[Chris H-C :chutten]

The raw dump appears to not have any environment at all. Maybe the frontend shouldn't parseerror on an absent field?

Comment 2

[Will Kahn-Greene [:willkg] ET needinfo? me]

If I go to that page and look at the source, there's definitely data there. You can open up the console and do this:

20:53:00.842 var container = $('#telemetryenvironment-json');
20:53:12.138 var jsonData = container.data('telemetryenvironment');
20:53:14.934 jsonData;
20:53:14.906
... lots of data here ...
20:53:33.631           $('#telemetryenvironment-json').JSONView(jsonData);
20:53:33.598 SyntaxError: JSON.parse: bad character in string literal at line 1 column 1331 of the JSON data[Learn More]

Seems like there is data and it's invalid.

Maybe instead of saying just "invalid data", it could print "invalid data" and then dump the raw data.

Would that be more helpful?

Comment 3

[Will Kahn-Greene [:willkg] ET needinfo? me]

I think the bit of the JSON blob that it's dying on is this:

"""
"pository\\\\
vpcdi.inf_amd64_4b6"
"""

If I do this, then it works fine:

00:00:37.372 var js2 = jsonData.replace(/\r/g, '').replace(/\n/g, '');
00:00:37.335 undefined
00:00:39.339 $('#telemetryenvironment-json').JSONView(js2);

where "works fine" is defined as "displays the JSON all parsed nicely in the box".

This feels to me like a bug in the crash reporter where it serializes the telemetry environment as a JSON blob. I bet it's not converting CR and LF right. I'll write up a bug to cover that.

In the meantime, we can fix this in crash stats, too.

Grabbing this to do next week.

Comment 4

[Will Kahn-Greene [:willkg] ET needinfo? me]

Attachment: pr 4625: fix bug 1491778: remove \r and \n from telemetry json blob

Comment 5

[Will Kahn-Greene [:willkg] ET needinfo? me]

I looked at the raw crash in the report_index view and the TelemetryEnvironment JSON blob doesn't parse there, either. So it's malformed at that point. If I convert \r -> \\r and \n to \\n, then it parses fine.

Questions to ponder:

1. Are there other sequences that are getting unescaped?
2. At what point in the pipeline is the problem happening?
3. The collector has code to handle \n here:

   https://dxr.mozilla.org/mozilla-central/source/toolkit/components/crashes/CrashService.js#124

   That comment is interesting.

Needs more thought. I'll do that next week.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Probably unrelated code but thought I'd link it here just in case: bug 1390547

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

The issue comes from how we write out annotations. Because they're in a format reminiscent of an INI file they need to be escape so that we don't have newlines within an annotation. The problem is that this messes up annotations holding valid JSON code, so we unescape those explicitly prior to sending a crash ping [1]. We also do so prior to submitting a main process crash [2]. The code that submits content process crashes also does its implicit unescaping [3] but it's a bit different than the others so the problem might lie in there.

[1] https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/toolkit/components/crashes/CrashService.js#131
[2] https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/toolkit/crashreporter/client/crashreporter.cpp#79
[3] https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/toolkit/crashreporter/KeyValueParser.jsm#27

Comment 8

[Will Kahn-Greene [:willkg] ET needinfo? me]

Attachment: pr 4650: bug 1491778: show raw value when TelemetryEnvironment doesn't parse

Comment 9

[[github robot]]

Commits pushed to master at https://github.com/mozilla-services/socorro

https://github.com/mozilla-services/socorro/commit/03bfa8e80de71fa36f7a95db64cd4d8e4707f479
bug 1491778: show raw value when TelemetryEnvironment doesn't parse

This changes the Telemetry Environment pane to show the raw value in the
case where the JSON doesn't parse.

https://github.com/mozilla-services/socorro/commit/4aaa048a44d4956846ee73b0521bdfc70b5f9668
Merge pull request #4650 from willkg/1491778-invalid-json-2

bug 1491778: show raw value when TelemetryEnvironment doesn't parse

Comment 10

[Will Kahn-Greene [:willkg] ET needinfo? me]

I think we want to fix the crux of the problem in the crash reporter first and then see where things are at. I'm going to make this depend on that bug and make it a P3 and then sit on it a while.

Comment 11

[Will Kahn-Greene [:willkg] ET needinfo? me]

Unassigning myself since I'm not going to get to this any time soon.

Comment 12

[Gabriele Svelto [:gsvelto] (PTO)]

Will is this still happening or did we solve it with bug 1420363?

Comment 13

[Will Kahn-Greene [:willkg] ET needinfo? me]

I don't know offhand if there are still problems. I'd have to figure out how to figure it out. I'll toss this in my list of things to do.

Comment 14

[Gabriele Svelto [:gsvelto] (PTO)]

This is the lowest of the low-priority tasks, I thought it was just an easy RESOLVED > FIXED ;-)

Comment 15

[Will Kahn-Greene [:willkg] ET needinfo? me]

I couldn't figure out a good way to query "show me the TelemetryEnvironment values that don't parse as JSON" and the data for the crash reports mentioned in this bug have expired.

I think I'm going to mark this as FIXED for now and if it's still an issue, we can reopen and we'll have data to work with.