Setting session cookie using chrome.cookies.set fails

VERIFIED FIXED in Firefox 47

Status

defect
P2
normal
VERIFIED FIXED
3 years ago
11 months ago

People

(Reporter: frolovm, Assigned: bsilverberg, Mentored)

Tracking

45 Branch
mozilla47
x86_64
Windows 10
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox47 verified, firefox48 verified)

Details

(Whiteboard: [cookies][good first bug] triaged)

Attachments

(2 attachments)

Reporter

Description

3 years ago
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.
Reporter

Updated

3 years ago
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64

Updated

3 years ago
Flags: blocking-webextensions?
Priority: -- → P2
Whiteboard: [cookies] triaged
Can you please provide a testcase extension that shows the problem?
Reporter

Comment 2

3 years ago
Test case for session cookie set failure
Reporter

Comment 3

3 years ago
It looks like there is a workaround.

If I set expirationDate then the session cookie is set.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-webextensions? → blocking-webextensions+
Mentor: kmaglione+bmo
Whiteboard: [cookies] triaged → [cookies][good first bug] triaged
Reporter

Comment 5

3 years ago
(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.
Reporter

Updated

3 years ago
Attachment #8715097 - Attachment mime type: application/zip → application/x-xpinstall

Updated

3 years ago
Assignee: nobody → bob.silverberg
Assignee

Updated

3 years ago
Iteration: --- → 47.3 - Mar 7
Assignee

Comment 7

3 years ago
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/
Assignee

Comment 8

3 years ago
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 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.
Attachment #8722162 - Flags: review?(kmaglione+bmo) → review+
Assignee

Comment 10

3 years ago
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/
Assignee

Comment 11

3 years ago
https://reviewboard.mozilla.org/r/35887/#review32543

> *is not a host only cookie

Oops. Fixed.
Assignee

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/623ba7f8c57a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Is there any proper way of manually testing this bug? If yes, could you please provide some reliable steps?
Flags: needinfo?(kmaglione+bmo)
Assignee

Comment 16

3 years ago
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`.
Flags: needinfo?(kmaglione+bmo)
(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.
Status: RESOLVED → VERIFIED

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.