Empty application/json response shows a "8" in the response panel which disappear when hovered

RESOLVED FIXED in Firefox 64

Status

defect
P3
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: nchevobbe, Assigned: tanhengyeow, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 64

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

a year ago
Steps to reproduce:
1. Go to https://devtools-html-put-error.glitch.me/
2. Open the netmonitor
3. Click the "put" button
4. Click on the log in netmonitor
5. Select the response panel

Expected results:

There's nothing in the response, so maybe we should say something like "empty response" or "invalid json"

Actual results:

There's a "8" next to the "JSON" title, which when hovered disappear (See attachment)
Thanks for the report!


Some instructions/comments for anyone interested in fixing this bug:

1) This place needs attention:
https://searchfox.org/mozilla-central/rev/d03ad8843e3bf2e856126bc53b0475c595e5183b/devtools/client/netmonitor/src/components/ResponsePanel.js#91

2) The problem is with `isJSON` method that is using:

json = JSON.parse(atob(response));

in case where the following fails:

json = JSON.parse(response);

3) Apparently: `atob("OK") == 8`, and so the panel displays `8` as the JSON, which is wrong

4)`atob(response)` has been introduced in bug 1333385 to fix base64 encoded JSON responses. We need to revisit this and do it right.

Honza
Mentor: odvarko
Has STR: --- → yes
Keywords: good-first-bug
Priority: -- → P3
Hey there! Can this bug be assigned to me? This would be my first bug.
(Reporter)

Comment 3

a year ago
Hello Jason, thanks for helping us !
I assigned you the bug, so you can start working on it.

You can read http://docs.firefox-dev.tools/getting-started/ to set up your firefox repo and working environment.
Do not hesitate to ask questions (there are no dumb ones), either here or in our Slack (https://devtools-html-slack.herokuapp.com/)
Assignee: nobody → jev4zs
Great, thank you Nicolas! I will start working on this later today.
My current solution for this involves using a helper function that checks if the response string is base 64:

isBase64(response) {
  try {
    return btoa(atob(response)) == response;
  } catch(err) {
    return false;
  }
}

If the function evaluates as true, the base64 JSON will get parsed. If it evaluates as false, nothing is assigned to the json variable and the panel is not rendered. 

Is this an appropriate solution for this issue?
(In reply to Jason Valenzuela from comment #5)
> Is this an appropriate solution for this issue?
Looks good to me.

Please create a patch with the `isBase64` method and I'll review.

Also, it's better to use 'Need more information from' field (see at the bottom of this page) any time you asking something/someone otherwise you question can be easily lost in bugmail noise.

Honza
Flags: needinfo?(jev4zs)
I recently submitted the patch with the new method, but unfortunately I did not configure the commit message correctly and it could not identify you as the reviewer. The commit message was "adding isBase64 method; r?Honza".
Flags: needinfo?(jev4zs) → needinfo?(odvarko)
(Reporter)

Comment 8

a year ago
Hello Jason, you have to set the commit message like: 

```
Bug 1437435 - Adding isBase64 method; r=Honza.

More information on the fix 
```

, here it seems that you are missing the "Bug XXX - " part ?
Flags: needinfo?(odvarko)
Comment hidden (mozreview-request)

Comment 10

a year ago
mozreview-review
Comment on attachment 8955355 [details]
Bug 1437435 - adding isBase64 method; .

https://reviewboard.mozilla.org/r/224536/#review230468


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/netmonitor/src/components/ResponsePanel.js:83
(Diff revision 1)
>    }
>  
> +  isBase64(response) {
> +    try {
> +      return btoa(atob(response)) == response;
> +    } catch(err) {

Error: Expected space(s) after "catch". [eslint: keyword-spacing]

::: devtools/client/netmonitor/src/components/ResponsePanel.js:102
(Diff revision 1)
>        json = JSON.parse(response);
>      } catch (err) {
>        try {
> +        if (this.isBase64(response)) {
> -        json = JSON.parse(atob(response));
> +          json = JSON.parse(atob(response));
> +        } 

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]
Comment on attachment 8955355 [details]
Bug 1437435 - adding isBase64 method; .

https://reviewboard.mozilla.org/r/224536/#review230582

Looks great!

Just a few inline comments.
Please fix also the comments from review bot.

Thanks for working on this,
Honza

::: devtools/client/netmonitor/src/components/ResponsePanel.js:57
(Diff revision 1)
>          height: 0,
>        },
>      };
>  
>      this.updateImageDimemsions = this.updateImageDimemsions.bind(this);
> +    this.isBase64 = this.isBase64.bind(this);

I don't think it's necessary to bind the `isBase64` method

::: devtools/client/netmonitor/src/components/ResponsePanel.js:58
(Diff revision 1)
>        },
>      };
>  
>      this.updateImageDimemsions = this.updateImageDimemsions.bind(this);
> +    this.isBase64 = this.isBase64.bind(this);
>      this.isJSON = this.isJSON.bind(this);

We also doesn't have to bind 'isJSON'

::: devtools/client/netmonitor/src/components/ResponsePanel.js:80
(Diff revision 1)
>          height: target.naturalHeight,
>        },
>      });
>    }
>  
> +  isBase64(response) {

Please add a small comment in front of this new method describing its purpose.
Attachment #8955355 - Flags: review?(odvarko)
Comment on attachment 8955472 [details]
Bug 1437435 - adding comments to isBase64 method, cleaning up whitespace and unnecessary code. .

https://reviewboard.mozilla.org/r/224646/#review230650

Looks great and works well!

Just one inline nit comment related to comment syntax.

Also, the comment #0 says that:

> There's nothing in the response, so maybe we should say something like "empty response" or "invalid json

I tested an empty response for JSON and the UI currently shows an error within the Sidebar:
"SyntaxError: JSON.parse: unexpected end of data at line 1 column 1 of the JSON data"

I think that an empty JSON reponse (any response in fact) doesn't have to necessarily result in an error.
It would be better to:

* Don't show the error message
* Render someting like "Empty response" (italic/gray?) instead of the response text.


It's ok for me if you prefer filing a follow up bug for this.

Thanks for the help!
Honza

::: devtools/client/netmonitor/src/components/ResponsePanel.js:80
(Diff revision 1)
>      });
>    }
> -
> +  // Takes the response passed to the isJSON method by decoding and encoding
> +  // it to base 64. The final string is compared with the original response,
> +  // and returns true if the strings are the same. If the responses are not
> +  // the same, or an error is thrown, the method will return false.

Can you please use, the following structure for comments located in front of the method?

/**
 *
 */
 
So, it's consistent with the rest of the DT code base.

Also for the `isJSON` method.
Attachment #8955472 - Flags: review?(odvarko)
Hello Honza, 

Thank you, I will fix that right away! And yes, I would prefer a follow up bug to implement something for an empty response. Would it be possible to assign me to that bug?
Flags: needinfo?(odvarko)

Comment 16

a year ago
mozreview-review
Comment on attachment 8955355 [details]
Bug 1437435 - adding isBase64 method; .

https://reviewboard.mozilla.org/r/224536/#review230668


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/netmonitor/src/components/ResponsePanel.js:83
(Diff revision 1)
>    }
>  
> +  isBase64(response) {
> +    try {
> +      return btoa(atob(response)) == response;
> +    } catch(err) {

Error: Expected space(s) after "catch". [eslint: keyword-spacing]

::: devtools/client/netmonitor/src/components/ResponsePanel.js:102
(Diff revision 1)
>        json = JSON.parse(response);
>      } catch (err) {
>        try {
> +        if (this.isBase64(response)) {
> -        json = JSON.parse(atob(response));
> +          json = JSON.parse(atob(response));
> +        } 

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]
Comment on attachment 8955491 [details]
Bug 1437435 - editing multi-line comments for isBase64, isJSON method, and other parts of the component.

https://reviewboard.mozilla.org/r/224656/#review230898

Excellent! It's all nicely coming along.

Remaining things:
1) One small inline comment to solve
2) Comments from review bot needs to be resolved (comment #16)
3) It isn't good practice to create a new patch for every change. Please merge all patches into one.

Thanks!
Honza

::: devtools/client/netmonitor/src/components/ResponsePanel.js:120
(Diff revision 1)
> -      // Extract the actual json substring in case this might be a "JSONP".
> -      // This regex basically parses a function call and captures the
> -      // function name and arguments in two separate groups.
> +      /**
> +       * the actual json substring in case this might be a "JSONP".
> +       * This regex basically parses a function call and captures the
> +       * function name and arguments in two separate groups.
> +       */
>        let jsonpRegex = /^\s*([\w$]+)\s*\(\s*([^]*)\s*\)\s*;?\s*$/;

nit: sorry if I wasn't clear, but comments within methods should use just `//`.
Attachment #8955491 - Flags: review?(odvarko)
(In reply to Jason Valenzuela from comment #14)
> Thank you, I will fix that right away! And yes, I would prefer a follow up
> bug to implement something for an empty response. Would it be possible to
> assign me to that bug?

Bug 1443089 reported to cover this and assigned to you.
Honza
Flags: needinfo?(odvarko)

Updated

11 months ago
Product: Firefox → DevTools
Assignee: jev4zs → tanhengyeow
Status: NEW → ASSIGNED
Assignee: tanhengyeow → E0032242
@Heng: Can you please mark all the obsolete attachments as obsolete?

Go to details and, edit details and check "Obsolete".

Thanks!
Honza
Flags: needinfo?(E0032242)
(Assignee)

Updated

7 months ago
Attachment #8955355 - Attachment is obsolete: true
Flags: needinfo?(E0032242)
(Assignee)

Updated

7 months ago
Attachment #8955472 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Attachment #8955491 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Attachment #9009922 - Attachment is obsolete: true
Attachment #9009922 - Flags: review?(odvarko)
Thanks!

I commented on the phabricator review request. Please ask me for review as soon as the comment is resolved.

Good job here!
Honza
(Assignee)

Comment 23

7 months ago
:Honza

It has been resolved. Please review it :)
@Heng: this change also needs a test. See my comment in Phabricator.

Also, please always need-info me (see the checkbox at the bottom of this page) if you have any questions, so it ends-up in my qeueue. Thanks!

Honza
Flags: needinfo?(E0032242)
(Assignee)

Comment 25

7 months ago
:Honza

Hi Honza, I'm looking through the test file to have a better understanding of what's going on under the hood. Are there any good ways for me to step through/debug the mochitest file? When I executed the test, it ran too quickly for me to observe the actions visually :P 

Here are the things I tried:
1. I'm aware that `JSON_BASIC_URL + "?name=null"` refers to the constant name specified in `head.js` and the query parameter respectively. I'm currently using Visual Studio Code and tried to go to its relevant `.html` file, "html_json-basic.html" > `Open in Live Server` > included `?name=null` as the query parameter but couldn’t reproduce the test because the page keeps refreshing. 

2. I tried to use the browser toolbox's debugger but the JS file in the test folder wasn't loaded in the resource, thus I couldn't step through it this way. Also, the page kept refreshing so even if the resource did show, I think it would be hard to step through it this way.

3. I found this resource, https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest#Debugging_individual_tests, but it didn't seem to help with this.

Appreciate any help to head in the right direction :D
Flags: needinfo?(odvarko)
(Assignee)

Comment 26

7 months ago
:Honza

Some updates on my experiments. I inserted something like `await timeout(2147483647);` in the `add_task` function of the test and a helper method like:

```
function timeout(ms) {
  return new Promise(resolve => setTimeout(resolve, ms));
}
```

Coupled with `./mach mochitest --jsdebugger /path/to/testfile`, I was able to “freeze” the test and find out how it looks visually in the browser and also the debugger that was attached. 

Are there ways we can debug the variables and values in the test? The test file isn’t loaded in the debugger so I can’t step through it. For example, the test is doing something like:

`const tabpanel = document.querySelector("#response-panel");`

So I’m assuming we can interact with the elements in the devtools panel somehow.

Not sure if there is a better way to debug the test file so appreciate other inputs :)
Yes, using `--jsdebugger` is the right way.

You should also put `debugger;` keyword at the beginning of the test to halt the debugger, step and inspect variables.

You can also call `waitForTime(ms)`, which does the same as your `timeout`, but it's built in.

You can also call dump("Hello from test!\n"); displayed in the system console you are using to run the test.

Honza
Flags: needinfo?(odvarko)
(Assignee)

Comment 28

7 months ago
:Honza

Thanks for the tips! It did help a ton :D

I've updated the test as shown in Phabricator, please review.

Sorry for the slight delay, I was spending some time to understand the whole test workflow and playing around with the elements presented in the network panel through the toolbox :) Also spent some time to set up a local test server to return custom responses based on the user's request. I feel all this work/experiments would be worthwhile it as it will ease some workload/learning curve for future bugs :D
Flags: needinfo?(E0032242) → needinfo?(odvarko)
Thanks for the update!

One more tip. When you running a test you can do:

./mach test devtools/client/netmonitor/test/my-test.js --verify --headless

--verify runs the test many times if different modes to make sure it doesn't fail (good to catch intermittents)
--headless runs without the browser window, so you can continue using your machine and don't worry about e.g. focus

Honza
Flags: needinfo?(odvarko)
(Assignee)

Comment 30

7 months ago
Awesome! Thanks for the tip :D

I've fixed the typo as shown in Phabricator :)
Flags: needinfo?(odvarko)
Review done, waiting for try results.

Honza
Flags: needinfo?(odvarko)
(Assignee)

Comment 32

7 months ago
Try results can be seen here: https://treeherder.mozilla.org/testview.html?repo=try&revision=c91220f2226a3096ee887ce33e0af858c8e3469d

Some tests that are unrelated to the change failed though. Is that an issue?
Flags: needinfo?(odvarko)
QA Contact: odvarko
Assignee: E0032242 → nobody
Status: ASSIGNED → NEW
Assignee: nobody → E0032242
Status: NEW → RESOLVED
Last Resolved: 7 months ago
QA Contact: odvarko
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
(In reply to Heng Yeow (:tanhengyeow) from comment #32)
> Try results can be seen here:
> https://treeherder.mozilla.org/testview.
> html?repo=try&revision=c91220f2226a3096ee887ce33e0af858c8e3469d
> 
> Some tests that are unrelated to the change failed though. Is that an issue?

The following test is failing for me locally when the patch is applied, so it seems to be related:
devtools/client/netmonitor/test/browser_net_json-malformed.js

I couldn't reproduce failure of this test, might be intermittent failure:
devtools/client/webconsole/test/mochitest/browser_jsterm_await_helper_dollar_underscore.js

Honza
Flags: needinfo?(odvarko) → needinfo?(E0032242)
(Assignee)

Comment 34

7 months ago
:Honza

The updated code didn't account for initializing an error message when the response is base64-encoded (the if condition triggered and skipped the initialization).

Thus the browser_net_json-malformed.js test is always failing as the response error header won't be shown (not intended).

I've updated the code as shown in Phabricator to account for this. Both tests pass for me locally now, kindly review :)
Flags: needinfo?(E0032242) → needinfo?(odvarko)
I am seeing: devtools/client/netmonitor/test/browser_net_json-empty.js failing.
Is that related?
Honza
Flags: needinfo?(E0032242)
(Assignee)

Comment 37

7 months ago
:Honza

Yes it is related. Apologies, should have checked for any regressions for the original new test after changing the logic.

I have updated the code as shown in Phabricator:
1. The method isJSON is supposed to check for JSON response in the first place. Thus, instead of writing an "OK" response, I have modified the test file to return an empty JSON response (e.g. "{}"). 
2. I Have also double checked that these 3 tests in context all passed in my local build.

Kindly review :)
Flags: needinfo?(E0032242) → needinfo?(odvarko)
(Assignee)

Updated

7 months ago
Attachment #9009983 - Flags: checkin+
Comment on attachment 9009983 [details]
Bug 1437435 - Added isBase64, isEmpty method, cleaned up typos and format of comments. r=Honza

When landing a patch we can either

1) Use checkin-needed keyword (for attached patches) or
2) Land using Lando in Phabricator

Honza
Attachment #9009983 - Flags: checkin+
(Assignee)

Comment 40

7 months ago
Hi :Honza

Seems like I don't have enough privileges to land patches in Lando. Quoting from the Lando user guide, "You must have the required SCM permissions to land to the destination repo (e.g. scm_level_3 for mozilla-central)."

Also, under what conditions would the checkin-needed keyword appear? I tinkered around and the checkin flag is the closest I found.
Flags: needinfo?(odvarko)

Comment 41

7 months ago
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c9d9dd203994
Added isBase64, isEmpty method, cleaned up typos and format of comments. r=Honza
(In reply to Heng Yeow (:tanhengyeow) from comment #40)
> Seems like I don't have enough privileges to land patches in Lando.
Landed

> Also, under what conditions would the checkin-needed keyword appear? I
> tinkered around and the checkin flag is the closest I found.
Some contributors attach their patches directly here in bugzilla
(not using Phabricator). In such case we can use checkin-needed
to make sure that one of the sheriffs land it.

Honza
Flags: needinfo?(odvarko)

Comment 43

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c9d9dd203994
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in before you can comment on or make changes to this bug.