1354507 - Add cookie-related filter options for network requests

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement)

Description

[Sebastian Zartner [:sebo]]

The filter field within the Network Monitor should have support for cookie-related filter options.

The Chrome DevTools have these filters:
set-cookie-domain
set-cookie-name
set-cookie-value

Sebastian

Comment 1

[Vangelis Katsikaros]

Attachment: Bug 1354507 - Add cookie-related filter options for network requests.

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

Comment 2

[Vangelis Katsikaros]

Attachment: set-session-x.png

Comment 3

[Vangelis Katsikaros]

Comment on attachment 8858354 [details]
Bug 1354507 - Add cookie-related filter options for network requests.

https://reviewboard.mozilla.org/r/130306/#review133002

::: devtools/client/netmonitor/src/utils/filter-text-utils.js:165
(Diff revision 1)
>          match = !item.status;
>        }
>        break;
> +    case "set-cookie-domain":
> +      if (typeof item.responseCookies === "object"
> +        && item.responseCookies.constructor === Array) {

I am not entirely sure on the best way to check if item.responseCookies is an array or not.

I need to check this because item.responseCookies can be either:
```
item.responseCookies [
{ name: "foo", ...},
{ ... },
]
```
or
```
item.responseCookies {
  from: "foo"
  cookies: []
}
```

Comment 4

[Tim Nguyen :ntim]

(In reply to Vangelis Katsikaros from comment #3)
> Comment on attachment 8858354 [details]
> Bug 1354507 - Add cookie-related filter options for network requests.
> 
> https://reviewboard.mozilla.org/r/130306/#review133002
> 
> ::: devtools/client/netmonitor/src/utils/filter-text-utils.js:165
> (Diff revision 1)
> >          match = !item.status;
> >        }
> >        break;
> > +    case "set-cookie-domain":
> > +      if (typeof item.responseCookies === "object"
> > +        && item.responseCookies.constructor === Array) {
> 
> I am not entirely sure on the best way to check if item.responseCookies is
> an array or not.

does `if (item.responseCookies instanceof Array) {` work ?

Comment 5

[Vangelis Katsikaros]

Comment on attachment 8858354 [details]
Bug 1354507 - Add cookie-related filter options for network requests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130306/diff/1-2/

Comment 6

[Vangelis Katsikaros]

(In reply to Tim Nguyen :ntim from comment #4)
> (In reply to Vangelis Katsikaros from comment #3)
> > Comment on attachment 8858354 [details]
> > Bug 1354507 - Add cookie-related filter options for network requests.
> > 
> > https://reviewboard.mozilla.org/r/130306/#review133002
> > 
> > ::: devtools/client/netmonitor/src/utils/filter-text-utils.js:165
> > (Diff revision 1)
> > >          match = !item.status;
> > >        }
> > >        break;
> > > +    case "set-cookie-domain":
> > > +      if (typeof item.responseCookies === "object"
> > > +        && item.responseCookies.constructor === Array) {
> > 
> > I am not entirely sure on the best way to check if item.responseCookies is
> > an array or not.
> 
> does `if (item.responseCookies instanceof Array) {` work ?

Seems to be working fine, thanks!

Comment 7

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

Comment on attachment 8858354 [details]
Bug 1354507 - Add cookie-related filter options for network requests.

https://reviewboard.mozilla.org/r/130306/#review133740

Thanks for working on this!

Please, see my inline comment.

@Ricky: I don't understand why `item.requestCookies` contain the array with cookies but response cookies are stored in `item.responseCookies.cookies`.

I am also seeing this code:
http://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/devtools/client/netmonitor/src/netmonitor-controller.js#507

.. is it some back compatibility support?

How response cookies array should be properly accessed?

Honza

::: devtools/client/netmonitor/src/utils/filter-text-utils.js:164
(Diff revision 2)
>        } else if (value === "running") {
>          match = !item.status;
>        }
>        break;
> +    case "set-cookie-domain":
> +      if (item.responseCookies instanceof Array) {

`item.responseCookies instanceof Array` is always false for me since cookies are stored in: `item.responseCookies.cookies instanceof Array`

Comment 8

[Tim Nguyen :ntim]

I guess you meant to needinfo Ricky in comment 7.

Comment 9

[Ricky Chien [:rickychien] (inactive)]

Pass ball to Fred since he is the person who finished cookies panel.

Comment 10

[[e-mail]]

Based on test case, the backend provide request/response cookies in either `requestCookies/responseCookies` or `requestCookies.cookies/responseCookies.cookies`, so I normalize that in netmonitor-controller. In component phase cookies should be accessible as an array via `request.requestCookies`

Comment 11

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

(In reply to Fred Lin [:gasolin] from comment #10)
> Based on test case, the backend provide request/response cookies in either
> `requestCookies/responseCookies` or
> `requestCookies.cookies/responseCookies.cookies`, so I normalize that in
> netmonitor-controller. In component phase cookies should be accessible as an
> array via `request.requestCookies`

I am setting a breakpoint in `isFlagFilterMatch` (filter-text-utils.js) when debugging the "set-cookie-name" filter. I can see that response cookies are available in `item.responseCookies.cookies` and request cookies in `item.requestCookies`. It looks like it isn't properly normalized at this point.

I suspect that `getFilterFn` selector caches old state that isn't normalized yet. Is that possible?

@Ntim, @Fred: any tips?

Honza

Comment 12

[Vangelis Katsikaros]

Attachment: 1354507.demo.patch

I apologize for my confusion, perhaps it's my debugging (console.log) code that is wrong or skewing the output. I see that `responseCookies` either returns an Array, with the cookies, or an Object with keys `from` and `cookies`. This is why I first do `item.responseCookies instanceof Array` and then iterate on the Array.

PS I'll attach a screenshot of the "browser console" too.

Comment 13

[Vangelis Katsikaros]

Attachment: 1354507.demo.png

Comment 14

[Tim Nguyen :ntim]

I wonder if removing this line: https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/src/netmonitor-controller.js#432

fixes the inconsistency. Maybe that line could override the normalized cookie data with the non-normalized one.

Comment 15

[Tim Nguyen :ntim]

I don't think we should block on this however, we've been shipping work arounds around this issue (notably for the cookies panel), so I think it would be safe to fix this in its own bug.

Comment 16

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

Attachment: cookies-filter.png

(In reply to Tim Nguyen :ntim from comment #15)
> I don't think we should block on this however, we've been shipping work
> arounds around this issue (notably for the cookies panel), so I think it
> would be safe to fix this in its own bug.
Sounds ok but, what the workaround should be?

Note that, `set-cookie-name:NID` filter doesn't work for me ATM (see the attached screenshot).

Honza

Comment 17

[Tim Nguyen :ntim]

(In reply to Jan Honza Odvarko [:Honza] from comment #16)
> Created attachment 8859935 [details]
> cookies-filter.png
> 
> (In reply to Tim Nguyen :ntim from comment #15)
> > I don't think we should block on this however, we've been shipping work
> > arounds around this issue (notably for the cookies panel), so I think it
> > would be safe to fix this in its own bug.
> Sounds ok but, what the workaround should be?
https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/src/components/cookies-panel.js#41

Comment 18

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

(In reply to Tim Nguyen :ntim from comment #17)
> (In reply to Jan Honza Odvarko [:Honza] from comment #16)
> > Created attachment 8859935 [details]
> > cookies-filter.png
> > 
> > (In reply to Tim Nguyen :ntim from comment #15)
> > > I don't think we should block on this however, we've been shipping work
> > > arounds around this issue (notably for the cookies panel), so I think it
> > > would be safe to fix this in its own bug.
> > Sounds ok but, what the workaround should be?
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/
> src/components/cookies-panel.js#41
Ah, yeah, well ok :-)

@Vangelis: so, there you go...

Honza

Comment 19

[[e-mail]]

(In reply to Tim Nguyen :ntim from comment #14)
> I wonder if removing this line:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/
> src/netmonitor-controller.js#432
> 
> fixes the inconsistency. Maybe that line could override the normalized
> cookie data with the non-normalized one.

It will be removed in bug 1356957

Comment 20

[Vangelis Katsikaros]

Comment on attachment 8858354 [details]
Bug 1354507 - Add cookie-related filter options for network requests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130306/diff/2-3/

Comment 21

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

Comment on attachment 8858354 [details]
Bug 1354507 - Add cookie-related filter options for network requests.

https://reviewboard.mozilla.org/r/130306/#review135716

This test case doesn't work for me:

1) Load google.cm
2) type "set-cookie-name:NID" (no quotes) into the filter field
3) Check out the Net panel content, it's empty but it shoiuld not -> BUG.

When doing the same in Chrome there are some requests displayed after aplying the same filter.

Honza

Comment 22

[Vangelis Katsikaros]

Attachment: google.cm.png

Honza, I couldn't replicate the problem after clearing site data in both browsers, ie I see the same results :( If a cookie is already stored then neither browser shows a request with filter `set-cookie-name:NID`

PS 1, the "set-cookies-*" filters check only cookies in the response not in the request.

PS 2, maybe I add a test for this feature.

Comment 23

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

(In reply to Vangelis Katsikaros from comment #22)
> PS 1, the "set-cookies-*" filters check only cookies in the response not in
> the request.
Ok, I see. In that case it work as expected (and inline with what Chrome does)

> PS 2, maybe I add a test for this feature.
Yes this would be great (might be separate patch)

Honza

Comment 24

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

Comment on attachment 8858354 [details]
Bug 1354507 - Add cookie-related filter options for network requests.

https://reviewboard.mozilla.org/r/130306/#review136040

Comment 25

[Tim Nguyen :ntim]

Comment on attachment 8858354 [details]
Bug 1354507 - Add cookie-related filter options for network requests.

https://reviewboard.mozilla.org/r/130306/#review136042

::: devtools/client/netmonitor/src/utils/filter-text-utils.js:185
(Diff revision 3)
> +        let i = responseCookies.findIndex(c => c.hasOwnProperty("domain"));
> +        let domain = i > -1 ? responseCookies[i].domain : item.urlDetails.host;

Will this work when there are multiple cookies that have a domain field, when the match will be in the second cookie or so?

Comment 26

[Vangelis Katsikaros]

Comment on attachment 8858354 [details]
Bug 1354507 - Add cookie-related filter options for network requests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130306/diff/3-4/

Comment 27

[Vangelis Katsikaros]

Comment on attachment 8858354 [details]
Bug 1354507 - Add cookie-related filter options for network requests.

https://reviewboard.mozilla.org/r/130306/#review136042

> Will this work when there are multiple cookies that have a domain field, when the match will be in the second cookie or so?

Good spot, I think it won't. It should be now fixed.

This also means I should add some testing around this, I created https://bugzilla.mozilla.org/show_bug.cgi?id=1360473

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e7af0c48115
Add cookie-related filter options for network requests. r=Honza

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9e7af0c48115

Comment 30

[Sebastian Zartner [:sebo]]

Like in bug 1354508, I've added the related description of the filter to https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Filtering_by_properties and added the related bug number to https://developer.mozilla.org/en-US/Firefox/Releases/55#Developer_Tools.

Vangelis, can you please have a look at the documentation and let me know whether it's ok?

Sebastian

Comment 31

[Vangelis Katsikaros]

Thanks Sebastian, it looks fine.

Comment 32

[Sebastian Zartner [:sebo]]

Perfect! Thank you for the fast review and implementing this feature.

Sebastian