905226 - Response tab in Network inspector shows base64 encoded response body

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect)

Description

[Jesse Stuart]

Attachment: 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.

Comment 1

[Felix Crux]

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.

Comment 2

[Felix Crux]

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.

Comment 3

[Felix Crux]

Attachment: 905226-network-response-json.patch

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!

Comment 4

[Rob Campbell [:rc] (:robcee)]

Patch looks reasonable, Felix. I'm going to defer to Victor for a full review.

Thanks for looking into this!

Comment 5

[Victor Porof [:vporof][:vp]]

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.

Comment 6

[Felix Crux]

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?

Comment 7

[Victor Porof [:vporof][:vp]]

(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.

Comment 8

[Felix Crux]

Attachment: 905226-network-response-json.patch

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!

Comment 9

[Victor Porof [:vporof][:vp]]

Comment on attachment 807565 [details] [diff] [review]
905226-network-response-json.patch

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

Nice, Like, RT, +1, r+.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/07a9df4849b7

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/07a9df4849b7