754220 - Add support for getting and removing cookies from the page

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Comment 1

[David Burns :automatedtester]

currently Selenium python tests that try access cookies on the page are told that the method does not exist. 

WebDriverException: Message: {'status': 404, 'value': {'message': '/session/13/cookie not found}}

Comment 2

[David Burns :automatedtester]

Corresponding bug on Selenium-Proxy at https://github.com/AutomatedTester/Selenium-Proxy/issues/9

Comment 4

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

As Zac mentioned on the duped bug 810254 we would also need methods to remove individual and all cookies. That request should be taken care of on this bug too.

Comment 5

[David Burns :automatedtester]

Push to try https://tbpl.mozilla.org/?tree=Try&rev=3349232d93fc

Comment 6

[David Burns :automatedtester]

Attachment: Adding APIs to manipulate cookies

will add review details when try results are in

Comment 7

[David Burns :automatedtester]

Attachment: Adding APIs to manipulate cookies

Comment 8

[David Burns :automatedtester]

pushed to try https://tbpl.mozilla.org/?tree=Try&rev=73d27cc37de3

Comment 9

[David Burns :automatedtester]

ignore previous comment, was meant for another bug

Comment 10

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 683606 [details] [diff] [review]
Adding APIs to manipulate cookies

Review of attachment 683606 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/client/marionette/tests/unit/test_cookies.py
@@ +1,4 @@
> +import calendar
> +import time
> +import random
> +#from selenium._test.selenium.webmarionette.common import utils

can you remove this line?

@@ +32,5 @@
> +        cookies = self.marionette.get_cookies()
> +        self.assertEquals(0, len(cookies))
> +
> +    def testDeleteAllCookie(self):
> +        self.marionette.add_cookie(self.COOKIE_A)

You should probably add a verification step here, to make sure the cookie was in fact added.

@@ +37,5 @@
> +        self.marionette.delete_all_cookies()
> +        self.assertFalse(self.marionette.get_cookies())
> +
> +    def testDeleteCookie(self):
> +        self.marionette.add_cookie(self.COOKIE_A)

Same here

@@ +41,5 @@
> +        self.marionette.add_cookie(self.COOKIE_A)
> +        self.marionette.delete_cookie("foo")
> +        self.assertFalse(self.marionette.get_cookies())
> +
> +    def testShouldGetCookieByName(self): 

there's some extra whitespace at the end here

::: testing/marionette/marionette-listener.js
@@ +53,5 @@
>  let asyncTestTimeoutId;
>  //timer for doc changes
>  let checkTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>  
> +

extra whitespace

@@ +798,5 @@
> +  cookie = msg.json.cookie;
> +
> +  if (!cookie.expiry) {
> +    var date = new Date();
> +    date.setYear(2030);

Even though it's still arbitrary, can we just get this year + 20 years or so? I'd rather not set it to a date that may one day have to change.

@@ +816,5 @@
> +  }
> +
> +  // The cookie's domain may include a port. Which is bad. Remove it
> +  // We'll catch ip6 addresses by mistake. Since no-one uses those
> +  // this will be okay for now.

Can we file a bug for catching ip6 addresses by mistake, and mention it here? It'll be good to keep track of this, so when the time comes, we can fix it appropriately.

Comment 11

[David Burns :automatedtester]

landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/15ce50d1d609

Comment 12

[Andrew Halberstadt [:ahal]]

Had to back out since all the new cookie tests were failing in B2G:
https://tbpl.mozilla.org/php/getParsedLog.php?id=17281480&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/db7d2a9d2ce3

Comment 13

[Mozilla RelEng Bot]

Try run for 70af827ac9c6 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=70af827ac9c6
Results (out of 20 total builds):
    success: 18
    warnings: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-70af827ac9c6

Comment 14

[Jonathan Griffin (:jgriffin)]

I looked at this patch and don't know why it's failing on B2G.  I'd suggest adding some debugging statements to it and running it on B2G to see where it's failing.

Comment 15

[David Burns :automatedtester]

Pushed to try with B2G cookie tests off. https://tbpl.mozilla.org/?tree=Try&rev=abdff5001a08

Comment 16

[David Burns :automatedtester]

desktop try again https://tbpl.mozilla.org/?tree=Try&rev=acd6402477e1

Comment 17

[David Burns :automatedtester]

green on B2G

Comment 18

[Mozilla RelEng Bot]

Try run for acd6402477e1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=acd6402477e1
Results (out of 11 total builds):
    success: 10
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-acd6402477e1

Comment 19

[David Burns :automatedtester]

Attachment: made b2g disabled

Disabled B2G tests due to bug 815616

Comment 20

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 685736 [details] [diff] [review]
made b2g disabled

Review of attachment 685736 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I missed this review request email. Looks good to me, but it looks like Bug 815616 is now resolved, so can you remove the unit-tests.ini changes?

Comment 21

[David Burns :automatedtester]

landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/f2eca3ea531f

Comment 22

[David Burns :automatedtester]

Attachment: updated patch that was landed

Comment 23

[Ed Morley [:emorley]]

Backed out for:
{
07:54:12     INFO -  ERROR: testAddCookie (test_cookies.CookieTest)
07:54:12     INFO -  ----------------------------------------------------------------------
07:54:12    ERROR -  Traceback (most recent call last):
07:54:12     INFO -    File "/builds/slave/talos-slave/test/build/tests/marionette/tests/testing/marionette/client/marionette/tests/unit/test_cookies.py", line 26, in testAddCookie
07:54:12     INFO -      cookie_returned = str(self.marionette.execute_script("return document.cookie"))
07:54:12     INFO -    File "/builds/slave/talos-slave/test/build/tests/marionette/tests/testing/marionette/client/marionette/tests/unit/test_cookies.py", line 26, in testAddCookie
07:54:12     INFO -      cookie_returned = str(self.marionette.execute_script("return document.cookie"))
07:54:12     INFO -    File "/tools/python27/lib/python2.7/bdb.py", line 48, in trace_dispatch
07:54:12     INFO -      return self.dispatch_line(frame)
07:54:12     INFO -    File "/tools/python27/lib/python2.7/bdb.py", line 67, in dispatch_line
07:54:12     INFO -      if self.quitting: raise BdbQuit
07:54:12     INFO -  TEST-UNEXPECTED-FAIL | test_cookies.py CookieTest.testAddCookie | BdbQuit
}
eg https://tbpl.mozilla.org/php/getParsedLog.php?id=17672602&tree=Mozilla-Inbound#error2

https://hg.mozilla.org/integration/mozilla-inbound/rev/987a92aa6fca

Comment 24

[David Burns :automatedtester]

sorry, left a pdb statement from debugging bitrotting.

relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/54ec5996fb9b

Comment 25

[David Burns :automatedtester]

Attachment: unbitrotted patch

carrying mdas' review through

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/54ec5996fb9b

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d8f2ec987216
https://hg.mozilla.org/releases/mozilla-beta/rev/3e03cd357b9f

Comment 28

[Jonathan Griffin (:jgriffin)]

The beta commit had some spurious changes; a merge problem, maybe?  This commit fixes that:

https://hg.mozilla.org/releases/mozilla-beta/rev/99207d3a7041

Comment 29

[Mozilla RelEng Bot]

Try run for 70af827ac9c6 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=70af827ac9c6
Results (out of 21 total builds):
    success: 18
    warnings: 2
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-70af827ac9c6

Comment 30

[Mozilla RelEng Bot]

Try run for 3349232d93fc is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3349232d93fc
Results (out of 13 total builds):
    success: 11
    warnings: 1
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-3349232d93fc

Comment 31

[Mozilla RelEng Bot]

Try run for acd6402477e1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=acd6402477e1
Results (out of 12 total builds):
    success: 10
    warnings: 1
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-acd6402477e1