Closed Bug 1372595 Opened 7 years ago Closed 7 years ago

GetNamedCookie should return single cookie

Categories

(Testing :: geckodriver, defect)

Version 3
defect
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: ato, Assigned: ato)

References

()

Details

Attachments

(3 files)

As reported in https://github.com/mozilla/geckodriver/issues/761, the
GetNamedCookie command should return a single cookie entry, not a list
of a single, retained cookie.

Incorrect geckodriver implementation:
https://searchfox.org/mozilla-central/rev/61054508641ee76f9c49bcf7303ef3
cfb6b410d2/testing/geckodriver/src/marionette.rs#842-846

Incorrect CookieResponse definition in the webdriver crate:
https://github.com/mozilla/webdriver-rust/blob/master/src/response.rs#L1
90-L201
Assignee: nobody → ato
Status: NEW → ASSIGNED
This patch has a dependency on
https://github.com/mozilla/webdriver-rust/pull/102 and a new webdriver
crate release.  I will not submit any try runs before that has landed.
Comment on attachment 8878558 [details]
Bug 1372595 - Let resource URL protocol be configurable;

https://reviewboard.mozilla.org/r/149882/#review154546
Attachment #8878558 - Flags: review?(james) → review+
I had a look at the fix for the expiry and it seems to introduce a regression for a session cookie:
https://reviewboard.mozilla.org/r/149884/diff/1#index_header

The former behavior was to return `null` for a session cookie, but the fix now returns the default value which is an `INT64_MAX` converted to a `DOUBLE` (9223372036854776000).

To determine if the cookie is a session cookie, the fix should evaluate whether the expiry is superior to a `Number.MAX_SAFE_INTEGER` or whether `nsICookie2::isSession` is true.
(In reply to Florent BREHERET from comment #6)
> The former behavior was to return `null` for a session cookie, but the fix
> now returns the default value which is an `INT64_MAX` converted to a
> `DOUBLE` (9223372036854776000).

The former behaviour was to _not_ return the expiry field at all.  I’m not
saying that the implementation that is in Marionette currently is correct.
I intend to fix the conformance of the command in
https://bugzilla.mozilla.org/show_bug.cgi?id=1372582.

> To determine if the cookie is a session cookie, the fix should evaluate
> whether the expiry is superior to a `Number.MAX_SAFE_INTEGER` or whether
> `nsICookie2::isSession` is true.

The expiry isn’t as far as I’m aware relevant to whether it’s a session
cookie?

I’m not sure if checking whether it’s within the permitted range of
Number.MAX_SAFE_INTEGER is relevant, or what relevance nsICookie#isSession
carries.  Can you please elaborate?
Comment on attachment 8878560 [details]
Bug 1372595 - Return single cookie for GetNamedCookie;

https://reviewboard.mozilla.org/r/149886/#review155092

::: testing/geckodriver/src/marionette.rs:845
(Diff revision 1)
> -                WebDriverResponse::Cookie(CookieResponse::new(cookies))
> +                WebDriverResponse::Cookies(CookiesResponse { value: cookies })
>              },
>              GetNamedCookie(ref name) => {
>                  let mut cookies = try!(self.process_cookies(&resp.result));
>                  cookies.retain(|x| x.name == *name);
> -                WebDriverResponse::Cookie(CookieResponse::new(cookies))
> +                let cookie = try_opt!(cookies.pop(),

Is it possible for this to return > 1 value? I expect not, but I wonder if it's worth checking that somehow.
Attachment #8878560 - Flags: review?(james) → review+
This now depends on https://bugzilla.mozilla.org/show_bug.cgi?id=1371405
because it upgrades the webdriver crate to 0.27.0. Ideally, we should
do this
Depends on: 1371405
Comment on attachment 8878559 [details]
Bug 1372595 - Return cookie expiry timestamp;

https://reviewboard.mozilla.org/r/149884/#review155192

Resetting review request for now given the still ongoing discussion on this bug.
Attachment #8878559 - Flags: review?(hskupin)
(In reply to Henrik Skupin (:whimboo) from comment #10)

> Resetting review request for now given the still ongoing discussion on
> this bug.

What ongoing discussion?  This merely depends on
https://bugzilla.mozilla.org/show_bug.cgi?id=1372582 because I don’t
want to duplicate the webdriver crate upgrade.  Ideally that should have
been a separate bug to make the dependency chain clearer.

The notes about conformance made by Florent will as I said be addressed
separately with https://bugzilla.mozilla.org/show_bug.cgi?id=1372582.
This patch only fixes the return type of GetNamedCookie.
(In reply to Andreas Tolfsen ‹:ato› from comment #11)
> The notes about conformance made by Florent will as I said be addressed
> separately with https://bugzilla.mozilla.org/show_bug.cgi?id=1372582.
> This patch only fixes the return type of GetNamedCookie.

There is still a question open, so it was misleading. Also I haven't checked the patch due to that last time, and didn't notice that those are only renaming changes. It's reviewed now.
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5cbd77708cd9
Let resource URL protocol be configurable; r=jgraham
https://hg.mozilla.org/integration/autoland/rev/65c71d2ef601
Return cookie expiry timestamp; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/6d9081d16588
Return single cookie for GetNamedCookie; r=jgraham
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 5bca39bba671 -d 85c86dc86122: rebasing 403723:5bca39bba671 "Bug 1372595 - Let resource URL protocol be configurable; r=jgraham"
rebasing 403724:ba16a9d0643b "Bug 1372595 - Return cookie expiry timestamp; r=whimboo"
merging testing/marionette/driver.js
merging testing/marionette/listener.js
warning: conflicts while merging testing/marionette/driver.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging testing/marionette/listener.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2434f1ab44a2
Let resource URL protocol be configurable; r=jgraham
https://hg.mozilla.org/integration/autoland/rev/806896fd00cf
Return cookie expiry timestamp; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/355a1b7602b3
Return single cookie for GetNamedCookie; r=jgraham
https://hg.mozilla.org/mozilla-central/rev/2434f1ab44a2
https://hg.mozilla.org/mozilla-central/rev/806896fd00cf
https://hg.mozilla.org/mozilla-central/rev/355a1b7602b3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: