Netmonitor JSON.parse error on base64 encoded json responses

RESOLVED FIXED in Firefox 53

Status

defect
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: jdescottes, Assigned: schwartzmorn+bugzilla)

Tracking

53 Branch
Firefox 54
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox53 fixed, firefox54 fixed)

Details

()

Attachments

(4 attachments, 2 obsolete attachments)

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

Comment 1

2 years ago
Posted patch bug1333385.wip.patch (obsolete) — Splinter Review
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?
Flags: needinfo?(odvarko)
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
Flags: needinfo?(odvarko) → needinfo?(gasolin)
Reporter

Comment 3

2 years ago
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.
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
Flags: needinfo?(gasolin)
Reporter

Updated

2 years ago
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Reporter

Updated

2 years ago
Assignee: jdescottes → schwartzmorn+bugzilla
Reporter

Comment 5

2 years ago
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!
Assignee

Comment 6

2 years ago
Posted patch base64networkMonitor.patch (obsolete) — Splinter Review
Hello,
I think I have fixed the issue without breaking the tests.
Adrien
Attachment #8830492 - Flags: review?(jdescottes)
Reporter

Comment 7

2 years ago
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.
Reporter

Comment 8

2 years ago
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.
Attachment #8830492 - Flags: review?(jdescottes) → review+
Reporter

Comment 9

2 years ago
Try is green, except for unrelated intermittents.
Assignee

Comment 10

2 years ago
Thanks for the review! I have updated accordingly.

Adrien
Attachment #8830492 - Attachment is obsolete: true
Attachment #8830790 - Flags: review+
Reporter

Updated

2 years ago
Attachment #8829829 - Attachment is obsolete: true

Comment 12

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa181ec8aab5
Fix base64 json in netmonitor. r=jdescottes
Keywords: checkin-needed

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fa181ec8aab5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Id updated after the merge made the test fail.
Flags: needinfo?(cbook)
Attachment #8831082 - Flags: review+

Comment 15

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/07d7ecbf77e3
fix netmonitor b64 test after mc-merge;r=me a=tomcat
thanks, landed!
Flags: needinfo?(cbook)
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 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
Attachment #8830790 - Flags: approval-mozilla-aurora?
Comment on attachment 8830790 [details] [diff] [review]
base64network2.patch

Fix a devtool:netmonitor issue. Aurora53+.
Attachment #8830790 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
backed out from aurora for failures in  https://treeherder.mozilla.org/logviewer.html#?job_id=74279542&repo=mozilla-aurora
Flags: needinfo?(schwartzmorn+bugzilla)
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?
Flags: needinfo?(schwartzmorn+bugzilla) → needinfo?(cbook)
Flags: needinfo?(cbook)
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
Flags: needinfo?(jdescottes)
Attachment #8834323 - Flags: review+

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.