Closed Bug 1425031 Opened 4 years ago Closed 4 years ago

Firefox Quantum blocks cookies when JavaScript updates them


(Core :: Networking: Cookies, defect, P2)

57 Branch



Tracking Status
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 + wontfix
firefox59 --- fixed
firefox60 --- fixed


(Reporter: nightbreath, Assigned: jdm)



(Keywords: regression, Whiteboard: [necko-triaged])


(2 files, 2 obsolete files)

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

Steps to reproduce:

1. Open!trash/ff-test.html in Firefox Quantum 57.0.2 (fresh install, no add-ons, no profile)
2. Open browser console
3. Refresh the page a few times (up to 10 times may be required, depending on PC configuration)

Actual results:

(only in Quantum)
4. JavaScript error is thrown as if one of the cookies that should be deleted by JavaScript was not deleted.
You can see the video of the problem here:
The forum discussion takes place here:

Expected results:

(in all other browsers)
4. No JavaScript errors occur.
[Tracking Requested - why for this release]: blocks cookies

Regression window:

Suspect: Bug 1331680


Your bunch of patch seems to cause the regression. Can you look into this?
Blocks: 1331680
Ever confirmed: true
Flags: needinfo?(amchung)
Keywords: regression
I will take a look at this bug.
Flags: needinfo?(amchung)
Assignee: nobody → amchung
Priority: -- → P2
Whiteboard: necko-triaged
Tracking for now, though it's pretty late for 58 we may have to defer this.
Hi Alex,
I have tried to reproduce this problem and refreshed 30 times, but I couldn't reproduce your problem.
Would you help me to add some debug message on your js(cookie1.js to cookie4.js)?
And I need the fail message and successfully message for confirming this problem.
1. In function getCookie(name), please add the debug message which includes file name and matches.
2. In function joinAndGetChunks(name), please add the debug message which includes file name and chunks.

Flags: needinfo?(nightbreath)
I was able to reproduce the following error within a few refreshes:

SyntaxError: JSON.parse: unexpected non-whitespace character after JSON data at line 1 column 216 of the JSON data[Learn More]
Whiteboard: necko-triaged → [necko-triaged]
Hi Amy,

I have added the debug information you asked for in a separate file:!trash/ff-test-2.html

Unfortunately my modifications made the chance of the error appearing a little bit lower than it was before, but I still see it.

I can confirm that the blazing-fast mechanism of cookies processing in Firefox Quantum is a result of operations going out of sync. In the JS files I create small cookies (100 bytes each) that store one long string. Before rewriting the data I always delete previous chunks. The operation is performed abound 100 times. On some page refreshes either required chunks are missing, or already deleted chunks still exist in the cookies. Here's a video I recorded:

Chrome seems to be slower in processing cookies, but it works flawlessly: (on the last page refresh it takes a little bit more time to visualize the changes, but it does not affect the page - just a WebDeveloper toolbar lag).

Best regards,
Oleksii Stanko.
Flags: needinfo?(nightbreath)
Hi Josh,
I found this problem is timing issue.
This problem occurs on the content process too late processed the removing cookie which from parent's notification.
The testing steps as below:
1. Remove all of the cookies.
2. Set persistent cookies => persistent.0 and persistent.1, next time will create persistent.0, persistent.1 and persistent.2. and so on.
3. Create JSON from the getting cookie string and used the Regular Expression.
4. Delete persistent cookies.
5. Set session cookies => session.0 and session.1, next time will create session.0, session.1 and session.2. and so on.
6. Create JSON from the getting cookie string and used the Regular Expression.
7. Delete session cookies.

When the test case already finishes to run cookie1.js, cookie2.js starts to remove all of the cookies and content process doesn't process the removing notification.
When content process starts to remove the cookies, probably deletes the cookie which shouldn't be removed.

In my view, when the cookie's action from javascript, content process doesn't process the notifications from parent.
1. In CookieServiceChild::SetCookieStringInternal(), modify the content type of nsIChannel to "js".
2. Add new argument whose name is aJSCookie on NotifyChanged().
3. In nsCookieService::AddInternal(), confirm the content type of nsIChannel.
   If content type is "js", set aJSCookie to true when calling the the NotifyChanged().
   Otherwise, set aJSCookie to false when calling the NofifyChanged()
4. When aJSCookie is false, send a new notification "non-js-cookie-changed".
5. In ContentParent, it just has to transfer "non-js-cookie-changed" notification to CookieServiceParent.

Would you give me your suggestions?
Flags: needinfo?(josh)
Hi Amy and Josh,

More and more sites are reporting about this problem to us. Is there anything I can help with to assist in the investigation?

Best regards,
Oleksii Stanko.
If the goal is skipping notifications for cookie modifications that originate in the child process, then the solution seems like the right idea to me. However, rather than changing the channel's content-type and checking it, it would be cleaner to add a new aFromChild argument to nsCookieSerService::SetCookieStringInternal, and check `!aFromHttp && aFromChild` in NotifyChanged.

I still don't understand the description of the problem, however. Can you create an automated test that demonstrates the problem?
Flags: needinfo?(josh)
For clarity, my question about the automated test is directed at Amy, who is familiar with our automated testing harness as well as the problem.
Flags: needinfo?(amchung)
Hi Josh,
Thanks for your suggestions, I will create a mochitest for this bug.
Flags: needinfo?(amchung)
Hi Josh,
I have created a mochitest, and the testing steps as below:
1. Create a new cookie whose name is cookie0.
2. Delete the cookie0.
3. Create cookie0 again.
4. Wait the notification from observer.
5. Due to content process doesn't process any notification when cookie is set/removed from js, cookie0 doesn't be deleted when test case get the "deleted" notification.
   Base on above reason, I design the test case to confirm cookie0 exists in content process when the last notification "added" is coming.

Would you give me the suggestions on my test case?
Attachment #8942311 - Flags: feedback?(josh)
Hi Josh,
I have modified the nsCookieService and CookieServiceChild.
1. Created new notification "non-js-cookie-changed".
2. Created new argument aFromChild on SetCookieStringInternal(), SetCookieInternal(), AddInternal() and NotifyChanged().
3. If aFromHttp && !aFromChild, send non-js-cookie-changed from observer.
4. When ContentParent get the non-js-cookie-changed, CookieServiceParent send the ipc msg which is the cookie operation to CookieServiceChild.

Would you give me the suggestions on this patch?
Attachment #8942315 - Flags: feedback?(josh)
Attachment #8942315 - Flags: feedback?(josh) → feedback+
Comment on attachment 8942311 [details] [diff] [review]
test case -- confirm cookie service doesn't send notification to cookie service child when the cookie is set/removed from js.

Review of attachment 8942311 [details] [diff] [review]:

::: netwerk/test/mochitests/test_1425031.html
@@ +1,4 @@
> +<html>
> +<!--
> +

Old bug number.

@@ +8,5 @@
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +</head>
> +<body>
> +<a target="_blank" href="">Mozilla Bug 1331680</a>

Old bug number.

@@ +34,5 @@
> +      is(op, "added", "Confirm the cookie operation is added.");
> +      is(document.cookie, cookieString, "An update for the cookie named " + document.cookie + " was observed.");
> +      break;
> +    case 2:
> +      is(op, "deleted", "Confirm the cookie operation is deleted.");

We should verify that document.cookie still matches cookieString here.

@@ +48,5 @@
> +}
> +
> +/* Test document.cookie
> + * 1. Set a cookie and confirm the cookies which are processed from observer.
> + * 2. Set a cookie and get cookie.

This comment doesn't describe this test.

@@ +59,5 @@
> +  document.cookie = "cookie0=; expires=Thu, 01-Jan-1970 00:00:01 GMT;";
> +}
> +
> +function testSetCookie() {
> +  var cookieNum = 1;

This is unused.

@@ +60,5 @@
> +}
> +
> +function testSetCookie() {
> +  var cookieNum = 1;
> +  setCookie();

I don't think it makes sense to have separate setCookie and removeCookies functions. The test will be clearer if we inline the code in testSetCookie instead.
Attachment #8942311 - Flags: feedback?(josh) → feedback+
Hey guys. We're being chased by our clients on that bug. Is there a plan for the fix?
Thanks for the reminder. I'm taking this bug over, since Amy isn't working on Firefox any more.
Assignee: amchung → josh
Flags: needinfo?(josh)
Attachment #8942311 - Attachment is obsolete: true
Attachment #8942315 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #8946531 - Flags: review+
Pushed by
Don't broadcast to content processes cookie updates that initiated in content processes. r=jdm
Keywords: checkin-needed
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Please request Beta approval on this when you get a chance. It grafts cleanly as-landed.
Flags: needinfo?(josh)
Flags: in-testsuite+
Alex, could you verify that the problem has been fixed if you use a nightly build from
Flags: needinfo?(josh) → needinfo?(nightbreath)
Sigh, didn't mean to redirect.
Flags: needinfo?(josh)
Hi Josh,

The bug is not reproducible during our smoke checks. Please give us a day or two to go through the use cases we faced previously, so that we 100% sure it is fixed.

Best regards,
Oleksii Stanko.
Comment on attachment 8946531 [details] [diff] [review]
Don't broadcast to content processes cookie updates that initiated in content processes

Approval Request Comment
[Feature/Bug causing the regression]: 1331680
[User impact if declined]: Unexpected website behaviours that are challenging to track down
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Despite this patch changing the behaviour of how cookies modifications are processed by the content process, any unexpected interactions would be overwritten by the next page load. This reduces the window of time in which an unexpected behaviour change can be exposed to web content.
[String changes made/needed]: None
Flags: needinfo?(josh)
Attachment #8946531 - Flags: approval-mozilla-beta?
We'll likely accept this for 59b8 once it gets verified on Nightly.
Hi All,

The problem is fixed in the Nightly build. Please proceed with merging the required changes to the Beta version.

Thank you all for the work on the issue! All our teams are looking forward at getting the fix into their browsers :)

Best regards,
Oleksii Stanko.
Flags: needinfo?(nightbreath)
Comment on attachment 8946531 [details] [diff] [review]
Don't broadcast to content processes cookie updates that initiated in content processes

Thanks for the confirmation, Alex! Approved for 59b8.
Attachment #8946531 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Josh Matthews [:jdm] from comment #27)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No
(In reply to Alex from comment #29) 
> The problem is fixed in the Nightly build. Please proceed with merging the
> required changes to the Beta version.

Based on the fact that this fix has confirmation on Nightly and Josh has automated tests covering it, we are setting qe-verify- for manual testing needs.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.