netmonitor response panel is empty if a value is null

VERIFIED FIXED in Firefox 53

Status

defect
P2
normal
VERIFIED FIXED
3 years ago
Last year

People

(Reporter: jdescottes, Assigned: kausam2015, Mentored)

Tracking

({good-first-bug})

53 Branch
Firefox 54

Firefox Tracking Flags

(firefox53 fixed, firefox54 verified)

Details

Attachments

(3 attachments, 1 obsolete attachment)

STRs:
- open devtools, open netmonitor
- go to https://testpilot.firefox.com/
- in the requests list, click on experiments.json
- open the Response tab 

AR: Response tab is empty
ER: Response tab should show the json preview

This json contains some values that are "null" which crashes the properties-view component.

A more isolated test case is at http://juliandescottes.github.io/devtools-demos/netmonitor/big-json.html . The json is just

> { "results": [{ "nullstring": null }] }

The issue comes from http://searchfox.org/mozilla-central/rev/ed04f7a44925dace34f2d2e15ef9c0f2809d70b0/devtools/client/netmonitor/shared/components/properties-view.js#114

> typeof member.value === "object" && member.value.value

When member is null the first check passes and the second one crashes. Guarding with member.value && member.value.value should be good enough here.

Adding a new mochitest could be nice though! I think it should be straightforward to add one similar to devtools/client/netmonitor/test/browser_net_json-*
Has STR: --- → yes
Priority: -- → P2
I would like to work on this bug.
Thanks Cosm! Assigned to you.

Some links to get started with devtools development:
- https://developer.mozilla.org/en-US/docs/Tools/Contributing

I gave a few hints for the fix in the summary. Let me know if you have any issue reproducing the bug or implementing the fix.

If you breeze through the fix, and want to try adding a test, try to add a new one, similar to devtools/client/netmonitor/test/browser_net_json-long.js

To run a test you can use the command `./mach test {name}`, for instance:
> ./mach test devtools/client/netmonitor/test/browser_net_json-long.js

When adding a new test, you must reference it in the browser.ini file located in the same directory as your test.
Assignee: nobody → kausam2015
Status: NEW → ASSIGNED
STRs screen recording
Attachment #8831097 - Flags: review?(jdescottes)
Attachment #8831097 - Attachment is patch: true
Comment on attachment 8831097 [details] [diff] [review]
proposed patch

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

Thanks for the patch Cosm, looks good!

I have submitted your changeset on our continuous integration platform. This will run our test suite to make sure this change doesn't introduce obvious regressions.
The results will be displayed on https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf46409342393d1f1c6605434c5919d116791722.

If the tests are ok ("try is green") we will mark your patch as checkin-needed and a sheriff will pick it up and land it on an integration branch. In the meantime I will try to add a new mochitest to make sure we don't regress your fix in the future.
Attachment #8831097 - Flags: review?(jdescottes) → review+
Posted patch bug1333383.test.patch (obsolete) — Splinter Review
Honza: here's a mochitest to go together with the patch from Cosm. Try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=0da022f4e7327cd69c957f9732d6bcde384d76a9
Attachment #8831237 - Flags: review?(odvarko)
Attachment #8831237 - Attachment is obsolete: true
Attachment #8831237 - Flags: review?(odvarko)
Attachment #8831417 - Flags: review?(odvarko)
Duplicate of this bug: 1336026
Comment on attachment 8831417 [details] [diff] [review]
bug1333383.test.patch

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

Looks good, thanks for working on this!

R+

Honza

::: devtools/client/netmonitor/test/html_json-basic.html
@@ +11,5 @@
> +    <title>Network Monitor test page</title>
> +  </head>
> +
> +  <body>
> +    <p>JSON resquest test page</p>

typo: resquest -> request
Attachment #8831417 - Flags: review?(odvarko) → review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7305ca850fb3
fix netmonitor response tab for json responses with null values;r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/713e1811a14f
add test for netmonitor rendering JSON response with null;r=honza
https://hg.mozilla.org/mozilla-central/rev/7305ca850fb3
https://hg.mozilla.org/mozilla-central/rev/713e1811a14f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Duplicate of this bug: 1335723
Comment on attachment 8831097 [details] [diff] [review]
proposed patch

Approval Request Comment
[Feature/Bug causing the regression]:

[User impact if declined]: the Response tab in Netmonitor is empty when viewing JSON responses containing the value 'null'. Several duplicates have been recently filed for this so it looks like the issue is pretty frequently spotted

[Is this code covered by automated tests?]: Tests have been added in a separate patch (attachment 8831417 [details] [diff] [review]). They can't be easily ported to aurora.

[Has the fix been verified in Nightly?]: It's been in nightly for a few days and other bugs have been confirmed as solved as a duplicate of this one.

[Needs manual test from QE? If yes, steps to reproduce]: 
- open devtools, open netmonitor
- go to https://testpilot.firefox.com/
- in the netmonitor requests list, click on experiments.json
- open the netmonitor response tab 

[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Just guarding against a null value
[String changes made/needed]: N/A
Attachment #8831097 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 1337421
Hi Rares,
could you help find someone to verify if this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(rares.bologa)
Flags: needinfo?(rares.bologa) → needinfo?(roxana.leitan)
Verified as fixed using the latest Nightly 54.0a1 (2017-02-07) on Windows 10 x64, Ubuntu 16.04 and Mac OS X 10.11.
Status: RESOLVED → VERIFIED
Flags: needinfo?(roxana.leitan)
Comment on attachment 8831097 [details] [diff] [review]
proposed patch

Fix the issue in netmonitor response panel if a value is null and was verified. Aurora53+.
Attachment #8831097 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I verified that Aurora (FirefoxDeveloperEdition-53.0a2, 2017-02-10) works again.
I verified this bug
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.