Closed
Bug 1395313
Opened 7 years ago
Closed 7 years ago
UTF-8 with BOM breaks the JSON Viewer
Categories
(DevTools :: JSON Viewer, defect)
Tracking
(firefox-esr52 unaffected, firefox55 wontfix, firefox56 wontfix, firefox57 verified)
VERIFIED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | wontfix |
firefox56 | --- | wontfix |
firefox57 | --- | verified |
People
(Reporter: yfdyh000, Assigned: Oriol)
References
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
4.03 KB,
patch
|
lizzard
:
approval-mozilla-release-
|
Details | Diff | Splinter Review |
STR: Open the https://jsonview.com/example.json Actual results: SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data
Assignee | ||
Comment 1•7 years ago
|
||
U+FEFF ZERO WIDTH NO-BREAK SPACE is prepended for some reason, this breaks the JSON data.
Assignee | ||
Comment 2•7 years ago
|
||
Ah, that's because the file is encoded in UTF-8 with BOM.
Assignee | ||
Comment 3•7 years ago
|
||
From https://tools.ietf.org/html/rfc7159#section-8.1 > Implementations MUST NOT add a byte order mark to the beginning of a > JSON text. In the interests of interoperability, implementations > that parse JSON texts MAY ignore the presence of a byte order mark > rather than treating it as an error. Honza, should the JSON Viewer be strict or ignore the BOM?
Flags: needinfo?(odvarko)
Comment 4•7 years ago
|
||
(In reply to Oriol Brufau [:Oriol] from comment #3) > From https://tools.ietf.org/html/rfc7159#section-8.1 > > > Implementations MUST NOT add a byte order mark to the beginning of a > > JSON text. In the interests of interoperability, implementations > > that parse JSON texts MAY ignore the presence of a byte order mark > > rather than treating it as an error. > > Honza, should the JSON Viewer be strict or ignore the BOM? Can we ignore the BOM and indicate in the UI that BOM is there? (e.g. there could be a message at the top of the JSON tab content) Honza
Flags: needinfo?(odvarko)
Assignee | ||
Comment 5•7 years ago
|
||
If we ignore the BOM, we could support UTF-16 and UTF-32, which are allowed by RFC 7159. - No BOM: UTF-8 - EF BB BF: UTF-8 with BOM - FE FF: UTF-16BE - FF FE: UTF-16LE - 00 00 FE FF: UTF-32BE - FF FE 00 00: UTF-32BE Then, somewhere (Headers tab?) it could say that encoding.
Summary: json viewer broken after Bug 1368899 → UTF-8 with BOM breaks the JSON Viewer
Comment 6•7 years ago
|
||
Sounds good to me. Here is also what I found on stackoverflow: https://stackoverflow.com/questions/4990095/json-specification-and-usage-of-bom-charset-encoding (it's inline with what we want to do here) Honza
Assignee | ||
Comment 7•7 years ago
|
||
Just a fix for the UTF-8 BOM. Supporting UTF-16 and UTF-32, and telling the encoding in the frontend can be done in other bugs.
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
Attachment #8903952 -
Flags: review?(odvarko)
Assignee | ||
Comment 8•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0a4e30c768277109f7c68a8b439a95963c5f61b
Assignee | ||
Comment 9•7 years ago
|
||
Comment on attachment 8903952 [details] [diff] [review] json-utf8-bom.patch Review of attachment 8903952 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/jsonview/converter-child.js @@ +72,5 @@ > > onDataAvailable: function (request, context, inputStream, offset, count) { > + // If it's the first call, remove the BOM. > + let bom = [0xEF, 0xBB, 0xBF]; > + if (!offset && count >= bom.length) { The BOM won't be removed if the first segment has less than 3 bytes. Is this reasonable, or should I store the bytes somewhere until I have 3 of them?
Comment 10•7 years ago
|
||
(In reply to Oriol Brufau [:Oriol] from comment #9) > The BOM won't be removed if the first segment has less than 3 bytes. > Is this reasonable, or should I store the bytes somewhere until I have 3 of > them? @michal: Is this ok? Can the first segment be smaller than 3 bytes? Honza
Flags: needinfo?(michal.novotny)
Assignee | ||
Comment 11•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=252a636a6bde6fde6349c1fa8bcfedf16ccab786
Assignee | ||
Comment 12•7 years ago
|
||
I thought that instead of removing the bytes of the BOM encoded in UTF-8, it would be easier to remove the U+FEFF in the frontend.
Attachment #8903952 -
Attachment is obsolete: true
Attachment #8903952 -
Flags: review?(odvarko)
Flags: needinfo?(michal.novotny)
Attachment #8904303 -
Flags: review?(odvarko)
Updated•7 years ago
|
Comment 13•7 years ago
|
||
Comment on attachment 8904303 [details] [diff] [review] json-bom.patch Review of attachment 8904303 [details] [diff] [review]: ----------------------------------------------------------------- Agree, removing that in the UI looks better, thanks for working on this! R+, assuming try is green & and my nit comment is resolved. Honza ::: devtools/client/jsonview/test/browser_jsonview_utf8.js @@ +4,5 @@ > * http://creativecommons.org/publicdomain/zero/1.0/ */ > > "use strict"; > > +const encodedChar = "%E2%9D%A4"; // In UTF-8 this is a heavy black heart nit: please put the comment above the lin (not at the end of the same line)
Attachment #8904303 -
Flags: review?(odvarko) → review+
Assignee | ||
Comment 15•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcbc94e1fadff95a54fb5d4752ad93354e5de6e6
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 16•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0c0dfeb3692a Let JSON Viewer ignore the BOM. r=Honza
Keywords: checkin-needed
This (or the other bug from this checkin-needed push) seems to have caused failures like https://treeherder.mozilla.org/logviewer.html#?job_id=130440527&repo=mozilla-inbound Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f30873510385aeaaa810fda203edb77e74a007d2
Flags: needinfo?(oriol-bugzilla)
Assignee | ||
Comment 18•7 years ago
|
||
The test passes with my patch on my PC, and I don't see how a JSON Viewer change could interfere with the find toolbar.
Flags: needinfo?(oriol-bugzilla)
Assignee | ||
Comment 19•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=72c7b40082892a249c2804e82a51766103560a90
Assignee | ||
Comment 20•7 years ago
|
||
That failed on Windows 7, so I did a try push to Windows 7, how come Windows 2012 is being tested instead? And if I try to add new jobs, I get Taskcluster: Supplied credentials do not satisfy authorizedScopes; credentials have scopes
(In reply to Oriol Brufau [:Oriol] from comment #20) > That failed on Windows 7, so I did a try push to Windows 7, how come Windows > 2012 is being tested instead? The builds are done on Windows2012, but the tests that run against that build should properly show up on a Windows 7 line. > And if I try to add new jobs, I get > Taskcluster: Supplied credentials do not satisfy authorizedScopes; > credentials have scopes You'd need to talk with the #taskcluster people to make sure your scopes are set properly. It's possible you're signing into Treeherder with a different account than what has your commit access scopes?
Assignee | ||
Comment 22•7 years ago
|
||
(In reply to Wes Kocher (:KWierso) from comment #21) > The builds are done on Windows2012, but the tests that run against that > build should properly show up on a Windows 7 line. Thanks for the explanation, makes sense. > You'd need to talk with the #taskcluster people to make sure your scopes are > set properly. It's possible you're signing into Treeherder with a different > account than what has your commit access scopes? No, it's the same email. Will talk to them if this happens again. Anyways, bc7 on Windows 7 pgo is green, so my patch didn't cause that fail.
Keywords: checkin-needed
Comment 23•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/11a7e286a5b7 Let JSON Viewer ignore the BOM. r=Honza
Keywords: checkin-needed
Comment 24•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/11a7e286a5b7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 25•7 years ago
|
||
Honza, do you think this is safe enough for beta? We are shipping the RC build next week, so please request approval if you think we should get it in.
Flags: needinfo?(odvarko)
Comment 26•7 years ago
|
||
(In reply to Panos Astithas [:past] (56 Regression Engineering Owner) (please ni?) from comment #25) > Honza, do you think this is safe enough for beta? We are shipping the RC > build next week, so please request approval if you think we should get it in. ok, let me double check first. Oriol: can you pleases provide a simple example of JSON with BOM that shows the problem when the patch is *not* applied? I am not sure if the example from comment #0 is still valid? Thanks! Honza
Flags: needinfo?(oriol-bugzilla)
Assignee | ||
Comment 27•7 years ago
|
||
Yes, the example from comment #0 is still valid. An offline example would be data:application/json,%EF%BB%BF[1,2,3]
Flags: needinfo?(oriol-bugzilla)
Comment 28•7 years ago
|
||
Comment on attachment 8907050 [details] [diff] [review] json-bom.patch Approval Request Comment [Feature/Bug causing the regression]: n-a [User impact if declined]: The user can't see prettified JSON with BOM [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: n-a [Is the change risky?]: low risk [Why is the change risky/not risky?]: small amount of changes and only related to developers who load/preview application/json documents. [String changes made/needed]: n-a
Flags: needinfo?(odvarko)
Attachment #8907050 -
Flags: approval-mozilla-beta?
Comment 29•7 years ago
|
||
Comment on attachment 8907050 [details] [diff] [review] json-bom.patch 56 is on m-r now.
Attachment #8907050 -
Flags: approval-mozilla-beta? → approval-mozilla-release?
Comment 30•7 years ago
|
||
Comment on attachment 8907050 [details] [diff] [review] json-bom.patch We are building the RC for 56 on Monday, it not a new issue in 56 and is too last minute, sorry. It would be nice to have some verification of the fix in 57.
Attachment #8907050 -
Flags: approval-mozilla-release? → approval-mozilla-release-
Updated•7 years ago
|
Comment 31•6 years ago
|
||
I have reproduced this bug with Nightly 57.0a1 (2017-08-30) on Windows 7, 64 Bit! This bug's fix is Verified with latest Beta! Build ID 20171026211016 User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20171101]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•