1440275 - Properly store cookies in internal Net panel data structure

Status: NEW

(DevTools :: Netmonitor, task, P3)

Description

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

This is a follow up for bug 1434333

Cookies should be stored in:

file.requestCookies.cookies
file.responseCookies.cookies

where `cookies` is an array.

But sometimes the `cookies` field is missing and the array of cookies is directly `file.responseCookies`
which is wrong.

You can see:
https://searchfox.org/mozilla-central/rev/9a8d3f8191b15cdca89a7ce044c7bea2dd0462dc/devtools/client/netmonitor/src/components/CookiesPanel.js#89

requestCookies = requestCookies.cookies || requestCookies;
responseCookies = responseCookies.cookies || responseCookies;

This place shows that the `cookies` array is accessed either from `requestCookies.cookies` or `requestCookies`

Of course `requestCookies.cookies` should be used at all times.

We need to identify the place where cookies are stored in `requestCookies` (resp `responseCookies`) and fix it.

This also has an impact on HARBuilder modified here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1434333#c15
(see end of the comment)

Honza

Comment 1

[Omar]

Dear Jan, This is my first time at bugzilla and I'd like to work on this bug. Do you have any recommendations to where I can start looking for the wrong addition??
Thanks

Comment 2

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

(In reply to Omar from comment #1)
> Dear Jan, This is my first time at bugzilla and I'd like to work on this
> bug. Do you have any recommendations to where I can start looking for the
> wrong addition??
Hi,
great, bug assigned to you!

See comment #0 and also:

1) FirefoxDataProvider
This object is responsible for fetching data related to a HTTP request from the remote backend (can be a page running in different process or on mobile device)

See this comment:
https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/devtools/client/netmonitor/src/connector/firefox-data-provider.js#187

... and this one:
https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/devtools/client/netmonitor/src/connector/firefox-data-provider.js#209


2) RequestListColumnCookies
This component represents the column you can see in the Net monitor panel column

This place also needs to be fixed:
https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/devtools/client/netmonitor/src/components/RequestListColumnCookies.js#35

See also render() method

3) RequestListColumnSetCookies
Again, column component

https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/devtools/client/netmonitor/src/components/RequestListColumnSetCookies.js#35

See also render() method

4) isFlagFilteredMath()
helper function
https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/devtools/client/netmonitor/src/utils/filter-text-utils.js#113


5) setCookies
helper function
https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/devtools/client/netmonitor/src/utils/sort-predicates.js#131

6) cookies
helper function
https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/devtools/client/netmonitor/src/utils/sort-predicates.js#141

---

All these places needs to be fixed.

It looks like the problem is related how fetched cookies (from the backend) are initially stored in the FirefoxDataProvider. This should be investigated first.

Honza

Comment 3

[Aanchal Agarwal]

Hi, I'm interested in working on this. Can I give it a try?

Comment 4

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

Assigned to you!
Honza

Comment 5

[Aanchal Agarwal]

Attachment: StoreCookies.patch

Tried to properly store cookies.

Can U please guide me on how to check that cookies are stored in requestCookies.cookies(resp responseCookies.cookies)? Thanks!

Comment 6

[Shivam Singhal [ :championshuttler ]]

Hi I would like to work on it , can I take this up?

Comment 7

[Hengyeow (:tanhengyeow)]

Hi Shivam

Thanks for your interest! I'm in the midst of helping to resolve this bug as it might seem more involved than expected. There are a few other good bugs to work on :)

Comment 8

[Hengyeow (:tanhengyeow)]

It looks like the problem is related to how fetched cookies (from the backend) are initially stored in the FirefoxDataProvider. This should be investigated first.

Hi Honza

So I did some investigation on how fetched cookies are initially stored in FirefoxDataProvider. Here are my findings so far.

1. Panels like CookiesPanel, RequestListColumnCookies and RequestListColumnSetCookies would call the requestData method in FirefoxDataProvider to get the request/response cookies.

2. The internal helper _requestData method is called to fetch HTTP details from the backend.

3. The initial response is stored here. This function is responsible for fetching the initial response. An example function that it triggers is here.

4. From then on, this response variable returned would be used throughout the network request. For example, the response is passed into the callback method like onRequestCookies to process.

Judging from step 3, this problem looks like it escalates out of the scope of the netmonitor. Would like your input on this.

Comment 9

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

I don't have enough cycles to work on this, so removing the current assignee and hoping that we can get back to this soon.

Honza

Comment 10

[Angelina]

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #9)

Hello< can I work on this?

Comment 11

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

Bomsy, I am unsure whether this is a good first bug (and also whether we still have the original problem)

Comment 12

[Hubert Boma Manilla (:bomsy)]

Bomsy, I am unsure whether this is a good first bug (and also whether we still have the original problem)

Having a quick look,
I think this is nice to cleanup and not have one consistent way for cookies

Here are a couple of places this might be set wrong
https://searchfox.org/mozilla-central/rev/46ff2252568db36e811109fa4026c8e3c12e9ee1/devtools/client/netmonitor/src/har/har-importer.js#78-79
https://searchfox.org/mozilla-central/rev/46ff2252568db36e811109fa4026c8e3c12e9ee1/devtools/client/netmonitor/src/connector/firefox-data-provider.js#234,259

This seems delicate, and is more involved and I'm not sure how much test coverage we have on this to make sure we do not regress any thing.

Might be more of a good mentored bug.
Some one with a bit of knowledge of the codebase could take a stab at it.

Comment 13

[Angelina]

Attachment: Bug 1440275 Properly store cookies in internal net panel data structure r?honza

Depends on D128670

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 15

[Abhinav]

Hi Jan

I would like to take-up this issue. Could you please assign it to me?

Comment 16

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

Let me first ask Bomsy, whether this bug is still valid. We've made a lot of changes in the code base to make it Fission compatible and I am not sure whether this report still makes sense.

Bomsy, WDYT?

Comment 17

[Shushanik]

Description:
Please provide an explanation of the feature or change. Include a description of the user scenario in which it would be used and how the user would complete the task(s).
Screenshots and visual UI specs are welcome, but please include sufficient accompanying explanation so that blind members of the accessibility team are able to understand the feature/change.

How do we test this?
If there is an implementation to test, please provide instructions for testing it; e.g. setting preferences, other preparation, how to trigger the UI, etc.

When will this ship?
Tracking bug/issue:
Design documents (e.g. Product Requirements Document, UI spec):
Engineering lead:
Product manager:

The accessibility team has developed the Mozilla Accessibility Release Guidelines which outline what is needed to make user interfaces accessible:
https://wiki.mozilla.org/Accessibility/Guidelines
Please describe the accessibility guidelines you considered and what steps you've taken to address them:

Describe any areas of concern to which you want the accessibility team to give special attention:

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Affects Firefox for Android]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:

Comment 18

[Hubert Boma Manilla (:bomsy)]

Hi Abhinav,
Sorry for the late reply. This involves a little bit more investigation and there's already a patch provided that need's review.
i think we can find you some other issue for you to look at.

Thanks

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.