Closed
Bug 1372595
Opened 7 years ago
Closed 7 years ago
GetNamedCookie should return single cookie
Categories
(Testing :: geckodriver, defect)
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 | ||
Updated•7 years ago
|
Assignee: nobody → ato
Status: NEW → ASSIGNED
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
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 5•7 years ago
|
||
mozreview-review |
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+
Comment 6•7 years ago
|
||
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.
Assignee | ||
Comment 7•7 years ago
|
||
(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 8•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 9•7 years ago
|
||
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 10•7 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 11•7 years ago
|
||
(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.
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8878559 [details] Bug 1372595 - Return cookie expiry timestamp; https://reviewboard.mozilla.org/r/149884/#review155308
Attachment #8878559 -
Flags: review+
Comment 13•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
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
Comment 18•7 years ago
|
||
Backed out in https://hg.mozilla.org/integration/autoland/rev/e8c8a090be240994f29190d56b4a8713763fe544 for https://treeherder.mozilla.org/logviewer.html#?job_id=109401940&repo=autoland on Linux64 and Windows.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 22•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 29•7 years ago
|
||
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
Comment 30•7 years ago
|
||
bugherder |
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
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•