1297243 - JSON bugzilla attachment reports charset='' (two apostrophes, which is an invalid parameter value) and can't be loaded.

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, defect, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

In bug 1297234 there is an attachment https://bug1297234.bmoattachments.org/attachment.cgi?id=8783756 which fails to load - although the URL loads fine from cURL and is valid JSON.

The viewer shows "SyntaxError: JSON.parse: unexpected end of data at line 1 column 1 of the JSON data" and the console reports "Component returned failure code: 0x80500001 [nsIConverterInputStream.init]" pointing at converter-child.js

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

The exception happens since the charset isn't properly set.

@mayhemer: We are using the following to get the charset from the current channel:

this.charset = request.QueryInterface(Ci.nsIChannel).contentCharset || "UTF-8";

https://dxr.mozilla.org/mozilla-central/rev/f5d043ce6d36a3c461cbd829d4a4a38394b7c436/devtools/client/jsonview/converter-child.js#103

But it returns "''" (two apostrophes) 

Is this expected? Shouldn't `contentCharset` be just an empty string?

Honza

Comment 2

[Honza Bambas (:mayhemer)]

Examining |wget -S| I can see there is |Content-Type: application/json; charset=''| response header.  There from the two apostrophes come.

According rfc2822 and rfc2045 the charset part is a 'parameter'.  A parameter MAY be a quoted string which MUST be surrounded by DQUOTE (which is '"') only.

see https://tools.ietf.org/html/rfc2822#section-3.2.5

hence, this is a bugzilla bug.

Comment 3

[Thom Chiovoloni [:tcsc] (ex-moco)]

While fixing Bugzilla is probably a good move, ISTM like we should be doing something better in this case.  Earlier versions of Firefox (48, for example) displayed it as plain text, for example, which is *much better* than what you get now. At the very least, the raw data should actually show up under the "Raw Data" tab.

Comment 4

[Dylan Hardison [:dylan] (he/him)]

The correct behavior is no charset= at all, surely?

Comment 5

[Honza Bambas (:mayhemer)]

Depends, if it's known to the server, then why not to provide it?  It's always better for a browser to know the encoding.  However, the charset parameter is purely optional.  But if that would be just '' then rather send nothing (because '' is no encoding).

Comment 6

[Dylan Hardison [:dylan] (he/him)]

Just an FYI, I added a charset to the attachment in bug 1297234 and it loads. Working on a proper fix which will be out next Tuesday.

Comment 7

[Dylan Hardison [:dylan] (he/him)]

1) I think we can detect encoding on application/json in addition to text/* types.
2) We need to do something else when we can't detect an a charset. Testing out options now.

Comment 8

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1297243_1.patch

I see no evidence that apache ever replaces $cgi->charset('') with anything,
so this should work. On my testing machine it results in a content type with no charset. I'm testing this on dev too: https://bug1153636.bugzilla-dev.allizom.org/attachment.cgi?id=8591901

There will be a second patch to do charset detection for JSON files.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

SGTM - this isn't blocking me in any way.

Comment 10

[David Lawrence [:dkl]]

Comment on attachment 8792026 [details] [diff] [review]
1297243_1.patch

Review of attachment 8792026 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 11

[Dylan Hardison [:dylan] (he/him)]

To [e-mail]:mozilla-bteam/bmo.git
   381c4dd..e1dc4f6  master -> master