Response tab in Network inspector shows base64 encoded response body

RESOLVED FIXED in Firefox 27

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: jesse, Assigned: felixc)

Tracking

25 Branch
Firefox 27
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Posted image ResponseBase64.png
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20130814004004

Steps to reproduce:

1. Make a request where the response Content-Type is a vendor json type such as `application/vnd.tent.posts-feed.v0+json`.

2. Click on the request in the Network inspector.

3. Click on the Response tab.

Note that this behaves as expected when the Content-Type is `application/json`.


Actual results:

The text displayed is base64 encoded.


Expected results:

A JSON tree should have been rendered.
Component: Untriaged → Developer Tools: Netmonitor
This appears to be the offending method: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/netmonitor-view.js#1830

My proposed fix: Change the MIME type check to match on any occurrence of "json", rather than "/json". This increases the chance of false positives, but given that invalid JSON just gets displayed as usual, it should be safe.

I'll submit a patch later this evening.
Turns out it's more complicated than I originally thought. The content sent by the server appears to be getting base64 encoded somewhere earlier in the process (makes sense, since the content wasn't originally sent in base64, so I should have expected that).

I'll have to dig a bit deeper to understand how this works, but I'm still up for it if someone who has the authority to change bug assignments wants to assign this bug to me. I'd of course also appreciate any pointers regarding where to look for this functionality.
Proposed patch attached.

I set the review requestees to a) the person who (according to hg blame) last touched the affected code, and b) the person who reviewed that last change. Apologies if that was a bad choice of reviewers, in which case I'd of course appreciate knowing who it should be sent to instead. Thanks!
Attachment #799934 - Flags: review?(vporof)
Attachment #799934 - Flags: review?(rcampbell)
Assignee: nobody → felixc
Patch looks reasonable, Felix. I'm going to defer to Victor for a full review.

Thanks for looking into this!
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #799934 - Flags: review?(rcampbell)
Comment on attachment 799934 [details] [diff] [review]
905226-network-response-json.patch

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

Yup, works as advertised, thank you! The patch looks good to me as well, but please add some comments as described blow and a netmonitor test. You can clone one of the already available ones. Let me know if you're not familiar with running mochitests :)

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +1842,5 @@
>      let { mimeType, text, encoding } = aResponse.content;
>  
>      gNetwork.getString(text).then(aString => {
>        // Handle json.
> +      if (/\bjson/.test(mimeType)) {

I don't fully understand this change. It looks reasonable, but please add a comment here describing why .contains wouldn't work.

::: toolkit/devtools/webconsole/network-helper.js
@@ +420,5 @@
>        return true;
>      }
>  
> +    if (/^application\/[a-z-]+\bxml$/.test(aMimeType) ||
> +        /^application\/[a-z-]+\bjson$/.test(aMimeType)) {

Ditto.
Attachment #799934 - Flags: review?(vporof) → feedback+
Thanks for the review!

I agree the patch should come with a test or two; truth be told I originally omitted it because I was a bit baffled by the sheer variety of frameworks and methods described at https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing. From your comment, though, it sounds like Mochitest is the one to dig in to -- I'll research it and ask around on #devtools if I run into any major blockers.

The reason for using /\bjson/ rather than just .contains("json") was really just one of minimizing the change from the existing code. The original test was for "/json", so I generalized the "/" into any word boundary. Just looking for the substring "json" seems OK to me, but does in theory carry a bit of a higher risk for false positives. Likewise, in the second block of code, the original check was for /^application/[a-z]+\+xml/, so again I generalized it to any word boundary followed by "xml" or "json" (though it probably should have been just one regexp instead of two).

If the consensus is that the slightly higher chance of false positives is fine, then I'm certainly on board with just using .contains, as it's simpler and presumably faster to boot. Thoughts?
(In reply to Felix Crux from comment #6)

I think the change is fine. Just add a comment in the code describing why it's there and what it fixes.
After some stumbling with Mochitest, I've put together a second version of the patch. This one incorporates the review feedback, adding both comments and a test. Cheers!
Attachment #799934 - Attachment is obsolete: true
Attachment #807565 - Flags: review?(vporof)
Comment on attachment 807565 [details] [diff] [review]
905226-network-response-json.patch

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

Nice, Like, RT, +1, r+.
Attachment #807565 - Flags: review?(vporof) → review+
Whiteboard: [land-in-fx-team]
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
https://hg.mozilla.org/integration/fx-team/rev/07a9df4849b7
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/07a9df4849b7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
See Also: → 923856
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.