UTF-8 with BOM breaks the JSON Viewer

VERIFIED FIXED in Firefox 57

Status

defect
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: yfdyh000, Assigned: Oriol)

Tracking

({regression})

55 Branch
Firefox 57
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 wontfix, firefox56 wontfix, firefox57 verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

2 years ago
STR:
Open the https://jsonview.com/example.json


Actual results:
SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data
Reporter

Updated

2 years ago
Has Regression Range: --- → yes
Has STR: --- → yes
Keywords: regression
Assignee

Comment 1

2 years ago
U+FEFF ZERO WIDTH NO-BREAK SPACE is prepended for some reason, this breaks the JSON data.
Assignee

Comment 2

2 years ago
Ah, that's because the file is encoded in UTF-8 with BOM.
Assignee

Comment 3

2 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)
(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

2 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
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

2 years ago
Posted 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)
Assignee

Comment 9

2 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?
(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 12

2 years ago
Posted 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+
Assignee

Comment 14

2 years ago
Fixing nit.
Attachment #8904303 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 16

2 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

2 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 20

2 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

2 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

2 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
https://hg.mozilla.org/mozilla-central/rev/11a7e286a5b7
Status: ASSIGNED → RESOLVED
Closed: 2 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)
Assignee

Comment 27

2 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 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]
Assignee

Comment 32

2 years ago
Verifying per comment 31.
Status: RESOLVED → VERIFIED

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.