Add cookie-related filter options for network requests

RESOLVED FIXED in Firefox 55

Status

enhancement
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: sebo, Assigned: vkatsikaros)

Tracking

(Blocks 1 bug, {dev-doc-complete})

55 Branch
Firefox 55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(6 attachments)

Reporter

Description

2 years ago
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
Reporter

Updated

2 years ago
See Also: → 1354508
Reporter

Updated

2 years ago
Blocks: netmonitor-filtering
No longer blocks: 1041895
Assignee

Updated

2 years ago
Assignee: nobody → vkatsikaros
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Assignee

Comment 2

2 years ago
Posted image set-session-x.png
Assignee

Comment 3

2 years ago
mozreview-review
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

2 years ago
(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 hidden (mozreview-request)
Assignee

Comment 6

2 years ago
(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 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`
Attachment #8858354 - Flags: review?(odvarko) → review-

Comment 8

2 years ago
I guess you meant to needinfo Ricky in comment 7.
Flags: needinfo?(rchien)
Pass ball to Fred since he is the person who finished cookies panel.
Flags: needinfo?(rchien) → needinfo?(gasolin)
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`
Flags: needinfo?(gasolin)
(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
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(gasolin)
Assignee

Comment 12

2 years ago
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.
Flags: needinfo?(gasolin)
Assignee

Comment 13

2 years ago
Posted image 1354507.demo.png

Comment 14

2 years ago
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.
Flags: needinfo?(ntim.bugs)

Comment 15

2 years ago
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.
Posted image 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

2 years ago
(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
(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
(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 hidden (mozreview-request)
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
Attachment #8858354 - Flags: review?(odvarko) → review-
Assignee

Comment 22

2 years ago
Posted image 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.
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(odvarko)
Comment on attachment 8858354 [details]
Bug 1354507 - Add cookie-related filter options for network requests.

https://reviewboard.mozilla.org/r/130306/#review136040
Attachment #8858354 - Flags: review- → review+

Comment 25

2 years ago
mozreview-review
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 hidden (mozreview-request)
Assignee

Updated

2 years ago
Blocks: 1360473
Assignee

Comment 27

2 years ago
mozreview-review-reply
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

2 years ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9e7af0c48115
Add cookie-related filter options for network requests. r=Honza

Comment 29

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9e7af0c48115
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

2 years ago
Keywords: dev-doc-needed
Reporter

Comment 30

2 years ago
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
Flags: needinfo?(vkatsikaros)
Assignee

Comment 31

2 years ago
Thanks Sebastian, it looks fine.
Flags: needinfo?(vkatsikaros)
Reporter

Comment 32

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

Sebastian

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.