1333385 - Netmonitor JSON.parse error on base64 encoded json responses

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect)

Description

[Julian Descottes [:jdescottes]]

I don't have the exact STRs here. The bug was reported by a user who is getting base64 encoded JSON responses from a server. The response tab fails to display the preview while Chrome manages to show it.

But I have alternate STRs which I think trigger the same issue:

- open devtools, open netmonitor
- go to http://bgrins.github.io/devtools-demos/misc/file.json
- in the request list select file.json
- open the response tab

AR: "Syntax error: JSON.parse [...]" error message displayed and the response tab contains the base64 encoded version of the JSON object
ER: We should be able to view the json here (considering it's being displayed correctly by the main json viewer, and that Chrome also supports it).

This might be related to Bug 1319749.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: bug1333385.wip.patch

Here is a quick patch on response-panel.js which fixes the original issue of the user as well as the STRs in mentioned in the summary. 

We simply fallback to use atob if the first JSON.parse failed. 

Honza: do you think the base64 decoding should be handled there or earlier?

Comment 2

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

The patch looks good but, I can't reproduce the issue on my machine using STR from comment #0 (the JSON is properly rendered for me).

@Fred, can you please try to repro this on your machine?

Honza

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: netmonitor-json-parse.gif

Thanks for the info Honza! I can repro on Windows, OSX and Ubuntu, so I'm attaching a GIF in case the STRs are confusing.

Comment 4

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

OK, I was finally able to reproduce the issue and I can confirm that the patch fixes it. Also, the way how the isJSON() method is modified looks good to me.

Honza

Comment 5

[Julian Descottes [:jdescottes]]

Thanks for taking this Adrien!

Here are some guidelines to contribute on devtools: https://developer.mozilla.org/en-US/docs/Tools/Contributing

You can use the patch attached here as a starting point. I sent it to try (which is our continuous integration platform) and it makes the following test fail: devtools/client/netmonitor/test/browser_net_json-malformed.js
(This is probably because we no longer throw the same error because of my patch)

The first step will be to setup your development environment, and import the patch attached here.
Let me know if you have any issue getting started!

Comment 6

[Adrien Enault]

Attachment: base64networkMonitor.patch

Hello,
I think I have fixed the issue without breaking the tests.
Adrien

Comment 7

[Julian Descottes [:jdescottes]]

Thanks! pushed your changeset to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba1c20cb4fbb6669f1d2996c581ae370d9783cab

This will run the devtools tests on a various set of platforms, to check that your patch didn't introduce any regression.

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8830492 [details] [diff] [review]
base64networkMonitor.patch

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

Looks good to me! 

R+ with my comments addressed, and a green try.
(since you were already granted an r+ here, you can just set the review flag to "+" yourself when uploading a new version of the patch)

::: devtools/client/netmonitor/test/sjs_content-type-test-server.sjs
@@ +137,5 @@
>          response.finish();
>          break;
>        }
> +      case "json-b64": {
> +        let str = "{ \"greeting\": \"Hello long string JSON!\" },";

Remove this unused variable.

@@ +141,5 @@
> +        let str = "{ \"greeting\": \"Hello long string JSON!\" },";
> +        response.setStatusLine(request.httpVersion, status, "OK");
> +        response.setHeader("Content-Type", "text/json; charset=utf-8", false);
> +        setCacheHeaders();
> +        response.write("eyJncmVldGluZ3MiOiAidGhpcyBpcyBhIGJhc2UgNjQgc3RyaW5nIn0=");

As we will update this file for my previous comment, let's use btoa() of the stringified json to make the test easier to understand.

Comment 9

[Julian Descottes [:jdescottes]]

Try is green, except for unrelated intermittents.

Comment 10

[Adrien Enault]

Attachment: base64network2.patch

Thanks for the review! I have updated accordingly.

Adrien

Comment 11

[Julian Descottes [:jdescottes]]

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=dabb30119ca0d3b6a2789b28634ea95c07bb075f

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa181ec8aab5
Fix base64 json in netmonitor. r=jdescottes

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fa181ec8aab5

Comment 14

[Julian Descottes [:jdescottes]]

Attachment: bug1333385.test.patch

Id updated after the merge made the test fail.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/07d7ecbf77e3
fix netmonitor b64 test after mc-merge;r=me a=tomcat

Comment 16

[Carsten Book [:Tomcat]]

thanks, landed!

Comment 17

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 54.0a1 (2017-01-24) (64-bit) on Windows 7 , 64 Bit !

This bug's fix is verified with latest Nightly!

Build    ID : 20170201030207
User Agent  : Mozilla/5.0(Windows NT 6.1; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0

[bugday-20170201]

Comment 18

[Julian Descottes [:jdescottes]]

Comment on attachment 8830790 [details] [diff] [review]
base64network2.patch

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: Response Tab of devtools netmonitor fails to preview JSON responses if they are base64 encoded
[Is this code covered by automated tests?]: yes, a mochitest has been added in this patch : browser_net_json-b64.js
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes. Steps to reproduce:
- open devtools, open netmonitor
- go to http://bgrins.github.io/devtools-demos/misc/file.json
- in the request list select file.json
- open the response tab 
(see comment 3 if unclear)
[List of other uplifts needed for the feature/fix]: the other patch in this bug has to be uplifted too https://bug1333385.bmoattachments.org/attachment.cgi?id=8831082
[Is the change risky?]: No
[Why is the change risky/not risky?]: Simple javascript change in devtools only code. We are just adding an additional fallback in an exising try catch statement.
[String changes made/needed]: N/A

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8830790 [details] [diff] [review]
base64network2.patch

Fix a devtool:netmonitor issue. Aurora53+.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7bae7aaeaead
https://hg.mozilla.org/releases/mozilla-aurora/rev/b87d0d68c15a

Comment 21

[Carsten Book [:Tomcat]]

backed out from aurora for failures in  https://treeherder.mozilla.org/logviewer.html#?job_id=74279542&repo=mozilla-aurora

Comment 22

[Julian Descottes [:jdescottes]]

Tomcat: my bad, only attachment 8830790 [details] [diff] [review] should have been uplifted here. 
The second patch was making the test compatible with changes that have not been uplifted (from Bug 1333364)
What should we do here? Request another uplift?

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4928b37a5650

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Backed out for browser_net_json-b64.js failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=74966211&repo=mozilla-aurora

https://hg.mozilla.org/releases/mozilla-aurora/rev/fcc32735c01d504767a0dee94c6bc706b6a23b5e

Comment 25

[Julian Descottes [:jdescottes]]

Attachment: patch for aurora

Sorry about that, the markup was still a bit different from what we had on Nightly.

I fixed the test for aurora and here's an associated try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=58d0901d4be9890ab4ed69e89e6101cb43023878

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cefad960ed80