From flod's test challenge report 1) Cleared all history, verified that no cookies were stored, closed the Info dialog. 2) Opened www.corriere.it, load a ton of ads and cookies. 3) Forget last 5 minutes. Result: history is gone, 4 cookies remain. Explaining the procedure since I've seen suggestions that cookies could exists prior to the 5 minutes timeframe, or people were keeping the Info dialog open.

I can reproduce with www.corriere.it 4 cookies remain under corrieri.it in the cookies manager.

Hmm. If I enable cookie NSPR logging (plus a few dump()s in sanitizer.js), I see a number of cookies being set *after* the sanitizer has closed windows and cleared cookies. Adding in some Necko logging, it looks like we're making network connections when that's happening. (It's a little hard to tell exactly what's happening because I'm not familiar with the logging output.) I'd guess that tracking scripts on the page are running when it's closed (XHRs?), and those parts finish async after we think we've closed the window. :( We should verify that, but I'm not sure how to address it if that's what is happening. Either we need to somehow wait for content to finish before clearing, or we need to poison lingering context so it can't persist data. Both seem difficult.

The browser console does show 2 network requests, although oddly neither is to the site (www.corriere.it) for the remaining cookie... After triggering Forget, I see a "GET http://metrics.rcsmetrics.it/b/ss/rcsglobal/1/H.25.2/s49713314309640" and "GET https://bam.nr-data.net/jserrors/1/3b7afaa056" Some poking with lldb shows that the actual cookie setting seems to be done by a |pagehide| listener, setting document.cookie. (Full stack attached.)

I've seen that if you run Forget (5 minutes) right away in the new window, things get cleaned up. Would running the cookie cleanup portion of Forget after the original window has been closed do the trick, if possible?

Ok, I think the change from bug 874502 is biting us here. (I previously dealt with some of that fallout in bug 917797, which is why some of this seemed familiar.) Windows are closed async from chrome, so we need to explicitly wait for that to be complete. I think that means https://developer.mozilla.org/en-US/docs/Observer_Notifications#Windows Hopefully we can just wait for the expected number of xul-window-destroyed notifications, and not need to track the individual tabs being closed.

...Based on some quick logging, I do see xul-window-destroyed being fired after the cookie setting happens. So using that should hopefully work. Although it's a little surprising that all the inner-window-destroyed notifications happen after xul-window-destroyed, we should check with our document-lifetime wizards to see if there's anything that can manage to run between them.

Taking per mail with dolske.

nightly iteration backlog work? I hope you're planning to fix on 33 soon. It is a show stopper for 33.1, which must be signed off in 7 days.

Messy WIP, but fixes the bug. Loading www.corriere.it and using the Forget button will now correctly clear cookies after the async window closing is finished, and no cookies are shown in the cookie manager. I'm a little confounded by a new failure in the canClear() code for formdata, though. |let transactionMgr = searchBar.textbox.editor.transactionManager;| is now throwing because |editor| is null. I just commented this code out for testing, but I don't understand what's happening. Need to either figure it out or bandaid with a null-check. Needs a test too.

(In reply to Justin Dolske [:Dolske] from comment #12) > |let transactionMgr = searchBar.textbox.editor.transactionManager;| is now throwing because > |editor| is null. Ok, so this is another race. It's only working right now because it runs while the old window is still open. By waiting to sanitize until after the old window is well and truly dead, this code breaks. Could fix it to null-check, but we'd want to have a test for this condition (no open window) which might be tricky. Probably easier to explicitly wait for the new window to exist before sanitizing.

Updated to use the browser-delayed-startup-finished notification instead of a load event. Mostly just because bug 1079222 wants to use that too, but I suppose it's better to wait until then. And it's a bit cleaner to use 2 observers instead of 1 observer and 1 event listener. Manually tested on Windows as well, as I wanted to make sure the state in comment 13 (old windows closed, new window not yet fully open) was correctly handled (i.e., closing the last window on Windows normally quits the app, so I wanted to make sure it correctly handles there being a window in the process of opening). Also realized we can't test this bug in an automated and realistic fashion -- just like bug 1069300 itself -- because our test frameworks expect to be running from an open window. So manual testing will have to do. Finally, I'd thought about converting onWindowsCleaned to be a Promise instead of a callback, but... Trying that is kind of making my head hurt instead of making it clearer what's going on (at least to me). Plus the current code already works, so I'm inclined to go with what I have to get it landed ASAP.

Comment on attachment 8511733 [details] [diff] [review] Patch v.2 Gijs/MattN/Jared -- whoever can get to this first, please steal. :)

Comment on attachment 8511733 [details] [diff] [review] Patch v.2 Review of attachment 8511733 [details] [diff] [review]: ----------------------------------------------------------------- First pass: ::: browser/base/content/sanitize.js @@ +75,5 @@ > let item = this.items.openWindows; > + > + function onWindowsCleaned() { > + try { > + var clearedPromise = this.sanitize(itemsToClear); var! @@ +79,5 @@ > + var clearedPromise = this.sanitize(itemsToClear); > + clearedPromise.then(deferred.resolve, deferred.reject); > + } catch(e) { > + Cu.reportError("Sanitizer threw after closing windows: " + e); > + deferred.reject(); Give a rejection reason to help with debugging. @@ +83,5 @@ > + deferred.reject(); > + } > + } > + > + var ok = item.asyncClear(onWindowsCleaned.bind(this)) var => let here too. Also missing a semicolon. @@ +90,1 @@ > deferred.reject(); Rejection reason here too @@ +457,5 @@ > for (let win of aWindowList) { > win.getInterface(Ci.nsIDocShell).contentViewer.resetCloseWindow(); > } > }, > + asyncClear: function(aCallback) This rename seems like it could cause problems in the future as a "clear" method seems to be part of the undocumented interface for each item. Two ideas: * add a |clear| method which reports an error when called and refers to asyncClear. * keep this as |clear| but warning if |aCallback| is missing. @@ +499,5 @@ > let features = "chrome,all,dialog=no," + this.privateStateForNewWindow; > let newWindow = existingWindow.openDialog("chrome://browser/content/", "_blank", > features, defaultArgs); > > + // Window creation and destruction is asyncronous. We need to wait Typo: asynchronous

Comment on attachment 8511733 [details] [diff] [review] Patch v.2 Review of attachment 8511733 [details] [diff] [review]: ----------------------------------------------------------------- I'm not that familiar with xul-window-destroyed but I trust that you're testing makes sense. ::: browser/base/content/sanitize.js @@ +504,5 @@ > + // until all existing windows are fully closed, and the new window is > + // fully open, before continuing. Otherwise the rest of the sanitizer > + // could run too early (and miss new cookies being set when a page > + // closes) and/or run too late (and not have a fully-formed window yet > + // in existance). See bug 1088137. typo: existence @@ +517,5 @@ > + if (numWindowsClosing == 0) > + aCallback(); > + } > + > + var numWindowsClosing = windowList.length; s/var/let/

Nits fixed. I had used asychClose() instead of close() just to make sure it didn't get caught by something that might ignorantly call it, but having it hard-fail seems fine too.

Also I continually misspell "async". :|

Comment on attachment 8511782 [details] [diff] [review] Patch v.3 [Triage Comment] Needed for correct operation of privacy button launching on an accelerated schedule.

remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/3ebeb2d211e1

remote: https://hg.mozilla.org/releases/mozilla-beta/rev/4c8d686c690b

(In reply to Justin Dolske [:Dolske] from comment #21) > https://hg.mozilla.org/integration/fx-team/rev/61cbd994f800 NB: this comment: https://hg.mozilla.org/integration/fx-team/rev/61cbd994f800#l1.5 is now wrong. Can you fix it with a DONTBUILD push? :-)

Ugh, bugzilla.

verified this is fixed on the alder-nighty build for 20141028 with cookies from both www.corriere.it and nytimes.com

Verified on Windows 7 64bit, Ubuntu 14.04 32bit and Mac OS X 10.8.5 using Firefox 33.1, latest Aurora and latest Nightly that cookies are deleted properly except with google.com PREF cookie in some cases (which I understand it`s a safe browsing cookie, see bug 1008706). Based on my testing and testing done by Tracy in comment 29, marking flags accordingly.

I was able to reproduce this bug on Nightly 36.0a1 (2014-10-23) using Windows 7 x64. Verified fixed on Windows 7 x64, Mac OSX 10.9.5 and Ubuntu 14.04 x32 using Beta 34.0b7 (20141106201515). Only google.com PREF cookie is displayed in Cookies Window.

(In reply to :Gijs Kruitbosch from comment #27) > NB: this comment: > https://hg.mozilla.org/integration/fx-team/rev/61cbd994f800#l1.5 > is now wrong. Can you fix it with a DONTBUILD push? :-) It seems accurate to me.