1462886 - Size related filter flag break the netmonitor when using invalid values

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Tim Nguyen :ntim]

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

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

AR:
- The netmonitor becomes blank.

Comment 1

[Tim Nguyen :ntim]

Note: this only reproduces when there are requests in the monitor.

Also happens with other size related flags: size, larger-than and transferred.

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 3

[Rares Doghi, Desktop QA]

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

Comment 4

[Tim Nguyen :ntim]

Thanks!

Comment 5

[Patrick Brosset <:pbro>]

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?

Comment 6

[Abhinav Koppula]

Attachment: Bug 1462886 - Fix for filter flag breaking the netmonitor when using invalid values.

Review commit: https://reviewboard.mozilla.org/r/246598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246598/

Comment 7

[Abhinav Koppula]

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.

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks Abhinav! (I am reviewing the patch now)

Assigning to Abhinav and increasing priority.

Honza

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b19a39acb939
Fix for filter flag breaking the netmonitor when using invalid values.r=Honza

Comment 11

[Tim Nguyen :ntim]

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

Comment 12

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/b19a39acb939

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/267ac0403602

Comment 15

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/d7e42c12b4a7