Closed Bug 1395313 Opened 7 years ago Closed 7 years ago

UTF-8 with BOM breaks the JSON Viewer

Categories

(DevTools :: JSON Viewer, defect)

55 Branch
defect
Not set
normal

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)

STR:
Open the https://jsonview.com/example.json


Actual results:
SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data
Has Regression Range: --- → yes
Has STR: --- → yes
Keywords: regression
U+FEFF ZERO WIDTH NO-BREAK SPACE is prepended for some reason, this breaks the JSON data.
Ah, that's because the file is encoded in UTF-8 with BOM.
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)
(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)
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
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
Attached patch json-utf8-bom.patch (obsolete) — Splinter Review
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)
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?
(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)
Attached patch json-bom.patch (obsolete) — Splinter Review
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)
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+
Attached patch json-bom.patchSplinter Review
Fixing nit.
Attachment #8904303 - Attachment is obsolete: true
Keywords: checkin-needed
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
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)
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?
(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
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
https://hg.mozilla.org/mozilla-central/rev/11a7e286a5b7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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)
(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)
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 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 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 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-
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]
Verifying per comment 31.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.