Closed Bug 1395507 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Crash Reporting, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: gsvelto, Assigned: gsvelto)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
I really wish I had just made the .extra file JSON when I originally implemented it. :-/
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
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.
Attachment #8912034 - Flags: review?(mconley) → review+
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c15f613d623c
Re-escape annotations holding JSON strings with newlines; r=mconley
https://hg.mozilla.org/mozilla-central/rev/c15f613d623c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Blocks: 1416078
(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).
(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.
You need to log in before you can comment on or make changes to this bug.