Closed
Bug 1425031
Opened 7 years ago
Closed 7 years ago
Firefox Quantum blocks cookies when JavaScript updates them
Categories
(Core :: Networking: Cookies, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla60
People
(Reporter: nightbreath, Assigned: jdm)
References
Details
(Keywords: regression, Whiteboard: [necko-triaged])
Attachments
(2 files, 2 obsolete files)
38.45 KB,
image/png
|
Details | |
19.67 KB,
patch
|
jdm
:
review+
RyanVM
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•7 years 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
status-firefox57:
--- → affected
status-firefox58:
--- → affected
status-firefox59:
--- → affected
status-firefox-esr52:
--- → unaffected
tracking-firefox58:
--- → ?
Ever confirmed: true
Flags: needinfo?(amchung)
Keywords: regression
Updated•7 years ago
|
Assignee: nobody → amchung
Priority: -- → P2
Whiteboard: necko-triaged
Comment 3•7 years ago
|
||
Tracking for now, though it's pretty late for 58 we may have to defer this.
Comment 4•7 years ago
|
||
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•7 years 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•7 years ago
|
Whiteboard: necko-triaged → [necko-triaged]
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)
Comment 7•7 years ago
|
||
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)
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•7 years 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•7 years 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)
Comment 11•7 years ago
|
||
Hi Josh, Thanks for your suggestions, I will create a mochitest for this bug.
Flags: needinfo?(amchung)
Comment 12•7 years ago
|
||
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)
Comment 13•7 years ago
|
||
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•7 years ago
|
Attachment #8942315 -
Flags: feedback?(josh) → feedback+
Assignee | ||
Comment 14•7 years 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•7 years ago
|
||
Hey guys. We're being chased by our clients on that bug. Is there a plan for the fix?
Assignee | ||
Comment 16•7 years 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•7 years ago
|
Assignee: amchung → josh
Assignee | ||
Comment 17•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a64ce266ba5dcfe54d5d7d04b78cac100083b05
Comment 18•7 years ago
|
||
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
Flags: needinfo?(josh)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(josh)
Assignee | ||
Comment 19•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d754137eff674bcc5f9ed34615f42150cf24e5d
Assignee | ||
Updated•7 years ago
|
Attachment #8942311 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8942315 -
Attachment is obsolete: true
Assignee | ||
Comment 20•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
Attachment #8946531 -
Flags: review+
Comment 21•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a448cd50f3fd
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 23•7 years ago
|
||
Please request Beta approval on this when you get a chance. It grafts cleanly as-landed.
Assignee | ||
Comment 24•7 years 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)
Reporter | ||
Comment 26•7 years 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•7 years 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?
Comment 28•7 years ago
|
||
We'll likely accept this for 59b8 once it gets verified on Nightly.
Reporter | ||
Comment 29•7 years 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 30•7 years ago
|
||
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+
Comment 31•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/e87e706def11
Comment 32•7 years ago
|
||
(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-
Depends on: 1450199
You need to log in
before you can comment on or make changes to this bug.
Description
•