Closed Bug 1425031 Opened 3 years ago Closed 2 years ago
Firefox Quantum blocks cookies when Java
Script updates them
[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?
Hi, I will take a look at this bug. Thanks!
Assignee: nobody → amchung
Priority: -- → P2
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!
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
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.
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?
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.
Hi Josh, Thanks for your suggestions, I will create a mochitest for this bug.
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)
Attachment #8942315 - Flags: feedback?(josh) → feedback+
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. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0595f80ce029f648e5fafc8a031d5ebc530042cc
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
Attachment #8942311 - Attachment is obsolete: true
Attachment #8942315 - Attachment is obsolete: true
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/a448cd50f3fd Don't broadcast to content processes cookie updates that initiated in content processes. r=jdm
Please request Beta approval on this when you get a chance. It grafts cleanly as-landed.
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)
Sigh, didn't mean to redirect.
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
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.
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.
You need to log in before you can comment on or make changes to this bug.