1605362 - `Network.getCookies()` has to also return cookies for all the sub frames

Status: RESOLVED FIXED

(Remote Protocol :: CDP, task, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The basic implementation for Network.getCookies() will not care about sub frames. Main reason is our upcoming Fission work, and that the current Page.getFrameTree implemenation is not handling childFrames. I filed bug 1605359 for proper support.

Comment 1

[nafees87n]

Can I get more context on this?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sure. So this bug is about the following method:
https://searchfox.org/mozilla-central/rev/525ff01397317b4b24b1a2cc51fe4511ec49ac8b/remote/cdp/domains/parent/Network.jsm#186-217

A description of the CDP API is here:
https://chromedevtools.github.io/devtools-protocol/tot/Network/#method-getCookies

Right now we create an array of just a single url, which is the one of the current target (line 188). But as the CDP documentation states if no url argument is passed (we don't support that yet) the cookies of the current page and all of its sub frames have to be returned.

As such what's missing here for now is that we would not only use the URL from the top-level browsing context, but from browsing context of the full tree. Internally it could look like:

    const urls = this.session.target.browsingContext
      .getAllBrowsingContextsInSubtree()
      .map(bc => bc.currentURI.spec);

This change alone should not change any behavior of current browser chrome tests that live in /remote/cdp/test/browser/network/. You can run these with ./mach test /remote/cdp/test/browser/network/browser_getCookies.js. If something fails use the --setpref="remote.log.level=Trace" command line option to see all the protocol logging output.

If it's all fine a new test should be added to browser_getCookies.js that loads a URL with a frame included, which itself also sets a cookie. Feel free to even do it three levels deep.

At the very end also the Puppeteer unit tests have to be run.

Feel free to ask more questions if something is still not clear and needs further information. You can also always drop a line in Elements. Thanks!

Comment 3

[nafees87n]

Need some help
I couldn't understand how to write tests for this. Also, how can I load URL with a frame?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, so here some more information around the required browser chrome tests:

1. The test for sub frames should end-up in /remote/cdp/test/browser/network/browser_getCookies.js
2. See other tests under /remote/cdp/test/browser/network/ in how they utilize frames via the file doc_frameset.html
3. For your test create a new HTML file like doc_get_cookies_frameset.html, and add a sub page that sets cookies via JS like `document.cookie = "foo=bar". Ideally do the same in the frameset page so we also get a cookie set at the top-level. Important is to use different cookie name/value pairs here.
4. Then your test has to load the frameset, and calls the Network.getCookies() command
5. Checks have to be performed that both the cookies with the correct data are returned.

Comment 5

[nafees87n]

Attachment: Bug 1605362 - Network.getCookies() returns all cookies of current page and all of its sub frames as well.

Comment 6

[nafees87n]

Attachment: Bug 1605362 - WIP: Added tests for Network.getCookies(). r=whimboo

Added test to /remote/cdp/test/browser/network/browser_getCookies.js
This test checks for cookies from the page as well as its subpage
Currently the tests dont give the desired result: WIP

Depends on D122382

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59898aa8c795
Network.getCookies() returns all cookies of current page and all of its sub frames as well. r=webdriver-reviewers,jdescottes

Comment 8

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/59898aa8c795

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

nafees87n, thanks a lot for your contribution! It's great to see that this feature has finally been landed. In case you have interest for a follow-up contribution, maybe bug 1605354 would be a good choice for you.

Comment 10

[nafees87n]

@whimboo, Thank You.
Yes, I could take up bug 1605354

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Great. Thanks a lot. I've CC'ed you over there.