1238269 - Setting session cookie using chrome.cookies.set fails

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Michail Frolov]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.106 Safari/537.36

Steps to reproduce:

Using chrome.cookies.set to set a session cookie.

'details' parameter contains url, name, value, httpOnly=true
  url is an HTTP url with port number
'callback' parameter specifies the callback function

The following page says that setting session cookies might fail:  https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities  See cookies section.  However, there was no corresponding bug to fix this.



Actual results:

In the callback function, 'cookie' parameter is null.  chrome.extension.lastError is undefined (this is another bug.)  Session cookie is not set.



Expected results:

Session cookie should be set.  'cookie' parameter should contain the set cookie data.

Comment 1

[Kris Maglione [:kmag]]

Can you please provide a testcase extension that shows the problem?

Comment 2

[Michail Frolov]

Attachment: firefox_test-0.0.1.xpi

Test case for session cookie set failure

Comment 3

[Michail Frolov]

It looks like there is a workaround.

If I set expirationDate then the session cookie is set.

Comment 4

[Kris Maglione [:kmag]]

Hm. Yeah. Apparently we set an expiration time of 0 for session cookies, and the cookie service honors that. The cookie service itself uses INT64_MAX.

We also apparently need tests for session cookies.

Comment 5

[Michail Frolov]

(In reply to Michail Frolov from comment #3)
> It looks like there is a workaround.
> 
> If I set expirationDate then the session cookie is set.

I guess this makes the cookie permanent instead of session.  Not really a workaround.

Comment 6

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1238269 - Setting session cookie using chrome.cookies.set fails, r?kmag

Review commit: https://reviewboard.mozilla.org/r/35887/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35887/

Comment 7

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8722162 [details]
MozReview Request: Bug 1238269 - Setting session cookie using chrome.cookies.set fails, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35887/diff/1-2/

Comment 8

[Bob Silverberg [:bsilverberg]]

Note that in addition to fixing the bug and adding tests for session cookies, I also cleaned up the existing tests. 

Some of the assert messages were inconsistent in their statement, the order of the arguments for a number of the the `assertEq` were incorrect, and the expected order of arguments for the internal `assertExpected` was counter-intuitive as it does not match the standard `assertEq`. I apologize that this will add a bit of extra time to the review, but I think it's worth the effort to have tests that are consistent and accurate in their messages and use of the `assertEq` API.

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8722162 [details]
MozReview Request: Bug 1238269 - Setting session cookie using chrome.cookies.set fails, r?kmag

https://reviewboard.mozilla.org/r/35887/#review32543

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:64
(Diff revision 2)
> -    browser.test.assertEq(cookie.hostOnly, false, "not a hostOnly cookie");
> +      browser.test.assertEq(false, cookie.hostOnly, "cookie is hostOnly cookie");

*is not a host only cookie

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:68
(Diff revision 2)
> +      // create a session cookie

Please capitalize and end with a period.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:74
(Diff revision 2)
> +      browser.test.assertEq(true, cookie.session, "got session cookie");

We should remove this cookie, too, and ideally test the removal.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:76
(Diff revision 2)
> -    browser.test.notifyPass("cookies");
> +      browser.test.notifyPass("cookies");

This doesn't need its own promise handler. It can go in the last one.

Comment 10

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8722162 [details]
MozReview Request: Bug 1238269 - Setting session cookie using chrome.cookies.set fails, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35887/diff/2-3/

Comment 11

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/35887/#review32543

> *is not a host only cookie

Oops. Fixed.

Comment 12

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=176767d8c141

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/623ba7f8c57a

Comment 14

[Lina Butler [:lina]]

https://hg.mozilla.org/mozilla-central/rev/623ba7f8c57a

Comment 15

[Vasilica Tamas, QA [:vtamas]]

Is there any proper way of manually testing this bug? If yes, could you please provide some reliable steps?

Comment 16

[Bob Silverberg [:bsilverberg]]

There was a test case extension attached in comment 2, so you could load that into your instance of Firefox and then check the console log for messages. It looks like with the bug the log will show the cookie as `null`. I assume that with the bug fixed you would see some output other than `null`.

Comment 17

[Vasilica Tamas, QA [:vtamas]]

(In reply to Bob Silverberg [:bsilverberg] from comment #16)
> There was a test case extension attached in comment 2, so you could load
> that into your instance of Firefox and then check the console log for
> messages. It looks like with the bug the log will show the cookie as `null`.
> I assume that with the bug fixed you would see some output other than `null`.

Thanks Bob! You were right: http://i.imgur.com/mrcLKef.jpg

I was able to reproduce the initial issue on Firefox 46.0a1 (2016-01-25) under Windows 10 64-bit.

Verified fixed on Firefox 48.0a1 (2016-04-04) and Firefox 46.0a2 (2016-04-04) under Windows 10 64-bit, Mac OS X 10.11 and Ubuntu 12.04 32-bit.