Closed Bug 1333383 Opened 6 years ago Closed 6 years ago

netmonitor response panel is empty if a value is null

Categories

(DevTools :: Netmonitor, defect, P2)

Product:
DevTools
Component:
Netmonitor
Version:
53 Branch
Type:
defect

Tracking

(firefox53 fixed, firefox54 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox53 --- fixed
firefox54 --- verified

People

(Reporter: jdescottes, Assigned: kausam2015, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(3 files, 1 obsolete file)

netmonitor-null-empty.gif
918.05 KB, image/gif
Details
proposed patch
989 bytes, patch
jdescottes
: review+
gchang
: approval-mozilla-aurora+
Details | Diff | Splinter Review
bug1333383.test.patch
9.27 KB, patch
Honza
: review+
Details | Diff | Splinter Review 
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
Attached patch proposed patch — — Splinter Review 

        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+

    
    

    
  

      
      
      
      

        Attached 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)

    
    

    
  
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: 6 years ago

          status-firefox54:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54

    
    

    
  
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?

    
    

    
  
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

          status-firefox54:
          fixed → 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.