1395507 - Escape special characters in .extra file annotations before parsing them as JSON

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Gabriele Svelto [:gsvelto]]

I've noticed while running tests that we often encounter this error:

10 INFO Console message: [JavaScript Error: "JSON.parse: bad escaped character at line 1 column 1045 of the JSON data" {file: "resource://gre/modules/CrashManager.jsm" line: 77}]

This is caused by trying to parse a field we've read from the .extra file which contains unescaped special characters (notably in the TelemetryEnvironment annotation the system.gfx.adapters[0].description field which often contains newline characters).

This is not a problem of the annotation per se, because those characters are indeed escaped before writing it out and the annotation is valid JSON; but parseKeyValuePairs() and friends un-escapes them again when it reads them, so that needs to be fixed.

Comment 1

[(not currently active) Ted Mielczarek]

I really wish I had just made the .extra file JSON when I originally implemented it. :-/

Comment 2

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1395507 - Re-escape annotations holding JSON strings with newlines;

Review commit: https://reviewboard.mozilla.org/r/183414/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183414/

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8912034 [details]
Bug 1395507 - Re-escape annotations holding JSON strings with newlines;

https://reviewboard.mozilla.org/r/183414/#review191078

::: toolkit/components/crashes/CrashService.js:125
(Diff revision 1)
> +      for (var field in keyValuePairs) {
> +        if ((field === "TelemetryEnvironment") || (field === "StackTraces")) {
> +          let escaped = keyValuePairs[field].replace(/\n/g, "n");
> +          keyValuePairs[field] = escaped;
> +        }
> +      }

Not 100% sure it's necessary to loop through all of the keys to do this, since we seem to know exactly which keys might be affected. Perhaps we could do:

```js
// Untested
let newlineEscape = s => s.replace(/\n/g, "n");
["TelemetryEnvironment", "StackTraces"].forEach(key => {
  keyValuePairs[key] = newlineEscape(keyValuePairs[key]);
});
```

or something along those lines. I don't feel super strongly about it though, so if you feel like it's not worth it, feel free to drop this.

Comment 4

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8912034 [details]
Bug 1395507 - Re-escape annotations holding JSON strings with newlines;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183414/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c15f613d623c
Re-escape annotations holding JSON strings with newlines; r=mconley

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c15f613d623c

Comment 7

[(not currently active) Ted Mielczarek]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> I really wish I had just made the .extra file JSON when I originally
> implemented it. :-/

Gabriele: what would you think about just changing things so we write out the extra data as JSON? It might be kind of a pain to do safely in the in-process case, but we could perhaps work around that a bit by serializing all the values to JSON when we create the hashtable. The .extra data format is...not exactly well-defined (which is my fault).

Comment 8

[Gabriele Svelto [:gsvelto]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> Gabriele: what would you think about just changing things so we write out
> the extra data as JSON? It might be kind of a pain to do safely in the
> in-process case, but we could perhaps work around that a bit by serializing
> all the values to JSON when we create the hashtable. The .extra data format
> is...not exactly well-defined (which is my fault).

I'm OK with the idea but I think we must really think it through. Right now when we append annotations to the .extra file we do just that which is very easy, but when we change one we overwrite the entire file. With JSON we'd need a way to not re-serialize everything every time we touch an annotation but the format will make it slightly harder since we can't just append another line to the file like we do now. Anyway, it's worth considering and it would allow us to cut down on all the ad-hoc code we have for parsing .extra files.