Closed Bug 923856 Opened 7 years ago Closed 7 years ago

`SyntaxError: JSON.parse: unexpected character` in network inspector response

Categories

(DevTools :: Netmonitor, defect, P3)

27 Branch
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: jesse, Assigned: thomas)

References

()

Details

(Whiteboard: [good first bug][mentor=vporof@mozilla.com][lang=js])

Attachments

(1 file, 4 obsolete files)

Attached image Screen Shot 2013-10-05 at 6.02.44 PM.png (obsolete) —
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20131005030203

Steps to reproduce:

Open the response tab of the network inspector for an xhr response.

Content-Type: "application/vnd.tent.posts-feed.v0+json"

Response Body: {"pages":{},"posts":[],"profiles":{}}


Actual results:

Shows "eyJwYWdlcyI6e30sInBvc3RzIjpbXSwicHJvZmlsZXMiOnt9fQ==" where the parsed JSON should be.

Shows a red bar with "SyntaxError: JSON.parse: unexpected character".


Expected results:

Should show tree of parsed JSON.
Component: Untriaged → Developer Tools: Netmonitor
See Also: → 905226
Can you please post a link so that we can reproduce this? It looks like the JSON body is base64 encoded somehow, but there's no way to tell.
Load http://still-atoll-9448.herokuapp.com/test with the network tab open.
Thanks! Yup, it's base64 encoded. For some reason, the network monitor doesn't decode it. The web console isn't able to display it at all, so I assume the backend gets confused somewhere.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mihai.sucan)
Chrome decodes it by default.
This is an unrecognized mime type. See line 427 in network-helper.js. NH_isTextMimeType() returns false, the regex for the json type check needs some tweaking.
Flags: needinfo?(mihai.sucan)
OS: Mac OS X → All
Hardware: x86 → All
Ok, thanks! That seems easy enough to fix.
Priority: -- → P3
Whiteboard: [good first bug][mentor=vporof@mozilla.com][lang=js]
Can I take this?
(In reply to Thomas Andersen from comment #7)
> Can I take this?

Sure! As Mihai said, you'll have to tweak the regex in NH_isTextMimeType in network-helper.js to better handle such json mime types. From what I can tell, it can now only handle -json and -xml, not +json etc.

Let me know if you need any help with building Firefox from source or writing a test.
Assignee: nobody → thomas
Status: NEW → ASSIGNED
Nice! I probably need some direction regarding creating automatic test/s
Would a unit test be a good start for this fix?
Testing the static boolean NetworkHeloer.isTextMimeType(aMimeType) and use the mime type from this case and some of the more distinct patterns found in network-helper/mimeCategoryMap as input.

If that sounds good, my first question is: Where are unit tests located? ${project}/toolkit/devtools/ ?
Attached patch 923856-initial.patch (obsolete) — Splinter Review
"From what I can tell, it can now only handle -json and -xml, not +json etc."

It also needs to handle dots (.) and numbers in the subtype

Some examples:

"application/vnd.tent.posts-feed.v0+json"
"application/vnd.mozilla.xul+xml"
"application/rdf+xml"

Look at this as a progress patch ;-) I am not sure if you wanted the test first.
Comment on attachment 815627 [details] [diff] [review]
923856-initial.patch

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

This is a great start! I don't mind if you want to start with the test or not, however you feel it's best is probably best :)
Attachment #815627 - Flags: feedback+
(In reply to Thomas Andersen from comment #9)
> 
> If that sounds good, my first question is: Where are unit tests located?
> ${project}/toolkit/devtools/ ?

Unit tests for this stuff are in toolkit/devtools/webconsole/test/. You could even modify an existing test to add new assertions, or create a new test altogether (which might be a bit overkill).

These are chrome mochitests, and they can be run via the following command:
> ./mach mochitest-chrome toolkit/devtools/webconsole/test/
Added case-insensitive flag to the regex as subtypes are case-sensitive
(http://tools.ietf.org/html/rfc2045, p.12)

Maybe we should just convert aMimeType to lowercase in case some server is specifically configured?
Attachment #815627 - Attachment is obsolete: true
Attachment #816438 - Flags: feedback+
Attached patch 923856-unit-test.patch (obsolete) — Splinter Review
Hi again 
here is my first attempt to create a devtools unit test.
I'll guess you will let me know if something is wrong ;)
Attachment #816446 - Flags: review?(vporof)
I didn't see your new request, sorry! I'll take a look at this asap. As a suggestion, please ask for review using the "review" flag on a patch when you have something for me to look at.
Comment on attachment 816446 [details] [diff] [review]
923856-unit-test.patch

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

I think this is top notch, but deferring review to mihai as well to be sure if he's ok with it too.
Attachment #816446 - Flags: review?(vporof)
Attachment #816446 - Flags: review?(mihai.sucan)
Attachment #816446 - Flags: review+
Comment on attachment 816446 [details] [diff] [review]
923856-unit-test.patch

Looks good to me as well. Thank you for your patch!
Attachment #816446 - Flags: review?(mihai.sucan) → review+
WE NEED TO LAND THIS!
Keywords: checkin-needed
Attachment #816438 - Flags: feedback+ → review+
Bitrot, I'll rebase and land it myself.
Keywords: checkin-needed
I hope I do not misunderstand anything, but this is about merging the patch into the repo, right?
If there are more to this issue that needs to be done let me know.
(In reply to Thomas Andersen from comment #20)
> I hope I do not misunderstand anything, but this is about merging the patch
> into the repo, right?

Yup!

> If there are more to this issue that needs to be done let me know.

No worries, it's all good. The patch has been sitting here for a short while so it needs rebasing before landing. I'll take care of it.
Rebased and landing.
Attachment #813895 - Attachment is obsolete: true
Attachment #816438 - Attachment is obsolete: true
Attachment #816446 - Attachment is obsolete: true
Attachment #8355792 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/fac3a5389392
Whiteboard: [good first bug][mentor=vporof@mozilla.com][lang=js] → [good first bug][mentor=vporof@mozilla.com][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/fac3a5389392
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=vporof@mozilla.com][lang=js][fixed-in-fx-team] → [good first bug][mentor=vporof@mozilla.com][lang=js]
Target Milestone: --- → Firefox 29
Depends on: 961097
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.