Firefox Quantum blocks cookies when JavaScript updates them

RESOLVED FIXED in Firefox 59

Status

()

defect
P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: nightbreath, Assigned: jdm)

Tracking

({regression})

57 Branch
mozilla60
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 wontfix, firefox58+ wontfix, firefox59 fixed, firefox60 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

a year ago
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 http://alexeystanko.com/!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: https://www.screencast.com/t/qgvUFqVRn
The forum discussion takes place here: https://support.mozilla.org/en-US/questions/1193683


Expected results:

(in all other browsers)
4. No JavaScript errors occur.

Comment 1

a year ago
[Tracking Requested - why for this release]: blocks cookies

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5742919ec43f834cc061a96d87c767af1a1f7f75&tochange=32083f24a1bb2c33050b4c972783f066432194eb

Suspect: Bug 1331680


@:Amy

Your bunch of patch seems to cause the regression. Can you look into this?
Blocks: 1331680
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(amchung)
Keywords: regression
Hi,
I will take a look at this bug.
Thanks!
Flags: needinfo?(amchung)

Updated

a year ago
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.

Thanks!
Flags: needinfo?(nightbreath)
Assignee

Comment 5

a year ago
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]
cookie1.js:80:18

Updated

a year ago
Whiteboard: necko-triaged → [necko-triaged]
Reporter

Comment 6

a year ago
Hi Amy,

I have added the debug information you asked for in a separate file: http://alexeystanko.com/!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: https://www.screencast.com/t/NhphBEYy8Jdp

Chrome seems to be slower in processing cookies, but it works flawlessly: https://www.screencast.com/t/O0zZ5WXIatlX (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.

[Solution]
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?
Thanks!
Flags: needinfo?(josh)
Reporter

Comment 8

a year ago
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.
Assignee

Comment 9

a year ago
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)
Assignee

Comment 10

a year ago
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?
Thanks!
Attachment #8942315 - Flags: feedback?(josh)
Assignee

Updated

a year ago
Attachment #8942315 - Flags: feedback?(josh) → feedback+
Assignee

Comment 14

a year ago
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 @@
> +<!DOCTYPE HTML>
> +<html>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=1331680

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="https://bugzilla.mozilla.org/show_bug.cgi?id=1331680">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+
Reporter

Comment 15

a year ago
Hey guys. We're being chased by our clients on that bug. Is there a plan for the fix?
Assignee

Comment 16

a year ago
Thanks for the reminder. I'm taking this bug over, since Amy isn't working on Firefox any more.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0595f80ce029f648e5fafc8a031d5ebc530042cc
Assignee

Updated

a year ago
Assignee: amchung → josh
Assignee

Updated

a year ago
Flags: needinfo?(josh)
Assignee

Updated

a year ago
Attachment #8942311 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8942315 - Attachment is obsolete: true
Assignee

Updated

a year ago
Keywords: checkin-needed
Assignee

Updated

a year ago
Attachment #8946531 - Flags: review+

Comment 21

a year ago
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a448cd50f3fd
Don't broadcast to content processes cookie updates that initiated in content processes. r=jdm
Keywords: checkin-needed

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a448cd50f3fd
Status: NEW → RESOLVED
Last Resolved: a year 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+
Assignee

Comment 24

a year ago
Alex, could you verify that the problem has been fixed if you use a nightly build from https://www.mozilla.org/en-US/firefox/channel/desktop/?
Flags: needinfo?(josh) → needinfo?(nightbreath)
Assignee

Comment 25

a year ago
Sigh, didn't mean to redirect.
Flags: needinfo?(josh)
Reporter

Comment 26

a year ago
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.
Assignee

Comment 27

a year ago
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.
Reporter

Comment 29

a year ago
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.