Closed
Bug 1395507
Opened 7 years ago
Closed 7 years ago
Escape special characters in .extra file annotations before parsing them as JSON
Categories
(Toolkit :: Crash Reporting, defect)
Toolkit
Crash Reporting
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.
Comment 1•7 years ago
|
||
I really wish I had just made the .extra file JSON when I originally implemented it. :-/
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c15f613d623c
Re-escape annotations holding JSON strings with newlines; r=mconley
Comment 6•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 7•7 years ago
|
||
(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).
Assignee | ||
Comment 8•7 years ago
|
||
(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.
Description
•