1425031 - Firefox Quantum blocks cookies when JavaScript updates them

Status: RESOLVED FIXED

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

Description

[Alex]

Attachment: Quantum_cookies_bugs.png

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

[Alice0775 White]

[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?

Comment 2

[Amy Chung [:Amy]]

Hi,
I will take a look at this bug.
Thanks!

Comment 3

[Julien Cristau [:jcristau] (back April 22)]

Tracking for now, though it's pretty late for 58 we may have to defer this.

Comment 4

[Amy Chung [:Amy]]

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!

Comment 5

[Josh Matthews [:jdm]]

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

Comment 6

[Alex]

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.

Comment 7

[Amy Chung [:Amy]]

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!

Comment 8

[Alex]

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.

Comment 9

[Josh Matthews [:jdm]]

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?

Comment 10

[Josh Matthews [:jdm]]

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.

Comment 11

[Amy Chung [:Amy]]

Hi Josh,
Thanks for your suggestions, I will create a mochitest for this bug.

Comment 12

[Amy Chung [:Amy]]

Attachment: test case -- confirm cookie service doesn't send notification to cookie service child when the cookie is set/removed from js.

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?

Comment 13

[Amy Chung [:Amy]]

Attachment: impelemetation -- Created a new notification to avoid content process repeats the cookie operation.

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!

Comment 14

[Josh Matthews [:jdm]]

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.

Comment 15

[Alex]

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

Comment 16

[Josh Matthews [:jdm]]

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

Comment 17

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a64ce266ba5dcfe54d5d7d04b78cac100083b05

Comment 18

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 1a64ce266ba5 (bug 1425031) for mochitest failures on test_1425031.html


Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1a64ce266ba5dcfe54d5d7d04b78cac100083b05&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=159170717&repo=mozilla-inbound&lineNumber=18443

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/8c52ad85c722619dc6aa7e0ad83ab72e6215fe4e

Comment 19

[Josh Matthews [:jdm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d754137eff674bcc5f9ed34615f42150cf24e5d

Comment 20

[Josh Matthews [:jdm]]

Attachment: Don't broadcast to content processes cookie updates that initiated in content processes

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a448cd50f3fd
Don't broadcast to content processes cookie updates that initiated in content processes. r=jdm

Comment 22

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/a448cd50f3fd

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance. It grafts cleanly as-landed.

Comment 24

[Josh Matthews [:jdm]]

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/?

Comment 25

[Josh Matthews [:jdm]]

Sigh, didn't mean to redirect.

Comment 26

[Alex]

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 27

[Josh Matthews [:jdm]]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

We'll likely accept this for 59b8 once it gets verified on Nightly.

Comment 29

[Alex]

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.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 31

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e87e706def11

Comment 32

[Andrei Vaida [:avaida]]

(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.