Closed Bug 1462886 Opened 2 years ago Closed 2 years ago

Size related filter flag break the netmonitor when using invalid values

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox-esr52 unaffected, firefox-esr60 fixed, firefox60 wontfix, firefox61 verified, firefox62 verified)

VERIFIED FIXED
Firefox 62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- fixed
firefox60 --- wontfix
firefox61 --- verified
firefox62 --- verified

People

(Reporter: ntim, Assigned: abhinav.koppula, Mentored)

References

Details

(Keywords: good-first-bug, regression, Whiteboard: good-first-bug)

Attachments

(1 file)

STR:
- Type transferred-larger-than:1kb

(this is invalid because the syntax accepts 1k, but not 1kb).

AR:
- The netmonitor becomes blank.
Note: this only reproduces when there are requests in the monitor.

Also happens with other size related flags: size, larger-than and transferred.
Summary: transferred-larger-than flag breaks the netmonitor when using invalid values → Size related filter flag break the netmonitor when using invalid values
Thanks for the report!
I can confirm, this bug is reproducible on my machine.

The problem is with `isFlagFilterMatch` function. The `value` argument passed into this function can be null and there should be a condition checking that case:

```js
if (!value) {
  return false;
}
```

https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/devtools/client/netmonitor/src/utils/filter-text-utils.js#108

We also need a test case for this to avoid future regressions.

No need for extra new test, we can extend the existing one::
https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/devtools/client/netmonitor/test/browser_net_filter-flags.js#301

Honza
Mentor: odvarko
Has STR: --- → yes
Keywords: good-first-bug
Priority: -- → P2
Whiteboard: good-first-bug
Hi guys i managed to reproduce this issue on Windows 10 , here are the results from mozregression :

INFO: Last good revision: 0c39c734b41929d2de8ed1f090b51bca95fefb9e
INFO: First bad revision: 3fb1f0afdc6decd6d53876304c0806cc6b7d39d0
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0c39c734b41929d2de8ed1f090b51bca95fefb9e&tochange=3fb1f0afdc6decd6d53876304c0806cc6b7d39d0
Thanks!
Because the whole tool becomes blank, unusable and cannot be brought back by anything else than restarting the tool, I would say this is a P1, not a P2. And it seems to be a regression affecting all of the channels right now. So it might be good if someone could jump on it sooner rather than later.
Honza: I see you marked it as a good-first-bug, that's great, but please keep an eye on this. If no one steps up in the coming weeks, can you please find someone to fix this in 62?
Flags: needinfo?(odvarko)
Hi Honza,
I have created a review-request with the fix for the above issue. 

I have added `value==null` check instead of `!value` since existing tests get broken on using `!value` for the filter - `size:0` as false is returned even for 0 value 

Please let me know if anything else needs to be done.
Thanks Abhinav! (I am reviewing the patch now)

Assigning to Abhinav and increasing priority.

Honza
Assignee: nobody → abhinav.koppula
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)
Priority: P2 → P1
Comment on attachment 8980427 [details]
Bug 1462886 - Fix for filter flag breaking the netmonitor when using invalid values.

https://reviewboard.mozilla.org/r/246598/#review253242

Looks great to me, thanks for fixing this!

R+ assuming try is green

Push to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab472e6b28c9d491d0e4f50044ff72c8b80b7dec

Honza
Attachment #8980427 - Flags: review?(odvarko) → review+
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b19a39acb939
Fix for filter flag breaking the netmonitor when using invalid values.r=Honza
Comment on attachment 8980427 [details]
Bug 1462886 - Fix for filter flag breaking the netmonitor when using invalid values.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1416824
[User impact if declined]: see comment 0
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: see comment 0
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: low
[Why is the change risky/not risky?]: small change covered by tests
[String changes made/needed]: n/a
Attachment #8980427 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/b19a39acb939
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Blocks: 1416824
Flags: qe-verify+
Comment on attachment 8980427 [details]
Bug 1462886 - Fix for filter flag breaking the netmonitor when using invalid values.

Avoid breaking the NetMonitor with invalid values. Thanks for adding an automated test for this. Approved for 61.0b10.
Attachment #8980427 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I managed to reproduce the initial issue on 62.0a1 (2018-05-18). I also can confirm that 61.0b10 build1 (20180530184300)and 62.0a1 (2018-05-31) are verified fixed using Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.12.6.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Comment on attachment 8980427 [details]
Bug 1462886 - Fix for filter flag breaking the netmonitor when using invalid values.

This grafts cleanly to ESR60 and includes an automated test. Approving for ESR 60.1.
Attachment #8980427 - Flags: approval-mozilla-esr60+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.