Josh, found this after reading your blog post "Private Browsing and You". I am using the latest nightly build, i.e. "18.0a1 (2012-09-27)". STR: 1. switch to private browsing mode 1. go to http://www.einslive.de/ (for example) 2. click "Webradio" on the top of the page 3. popup opens 4. check about:cache?device=disk What I see are entries like http://www.einslive.de/sendungen/player/style.css and more.

For reference: bug 749187 was supposed to address this problem.

Turns out that domwindowopened does not do what it says on the can at all!

I would sleep much better during the night if we took this on all branches, to avoid a 16.0.1.

Comment on attachment 665634 [details] [diff] [review] Patch (v1) >+ aWindow.addEventListener("load", PBS__onWindowLoad(function() { >+ aWindow.removeEventListener("load", arguments.callee); >+ }), false); The function returned by PBS__onWindowLoad and arguments.callee aren't the same.

Comment on attachment 665634 [details] [diff] [review] Patch (v1) Do we know why the test added in the other bug didn't catch this? This will almost certainly need backporting as well. I also do not feel qualified to review this.

(In reply to Josh Matthews [:jdm] from comment #5) > Do we know why the test added in the other bug didn't catch this? Not really. But clearly relying on domwindowopened is a mistake.

Thanks for the catch, Dao. I changes the patch to rely on whether or not a valid event parameter is being passed in, which is both easier and actually correct.

Comment on attachment 665686 [details] [diff] [review] Patch (v2) >+ case "chrome-document-global-created": >+ function PBS__onWindowLoad(aEvent) { >+ if (aEvent) { // If being called from an event handler, aEvent will be non-null >+ aWindow.removeEventListener("load", arguments.callee); >+ } > if (aWindow.document > .documentElement > .getAttribute("windowtype") == "navigator:browser") { > self._setPerWindowPBFlag(aWindow, self._inPrivateBrowsing); > } >- }, false); >+ } how about: let onLoad = function PBS__onWindowLoad(aEvent) { ... }.bind(this); and then use onLoad instead of arguments.callee (which is deprecated) and this instead of self. >+ let doc = aWindow.document; doc is used only once. Either just use aWindow.document there, or use doc instead of aWindow.document in PBS__onWindowLoad. I'd also prefer if you declared the variables before the function using them. >+ if (doc.readyState == "complete") { >+ PBS__onWindowLoad(null); >+ } else { >+ aWindow.addEventListener("load", PBS__onWindowLoad, false); >+ } I'm not sure why this switch is needed. Can you explain and document this?

This needs to land on mozilla-central and mozilla-aurora asap - our final beta build is Monday.

(In reply to Dão Gottwald [:dao] from comment #8) > >+ if (doc.readyState == "complete") { > >+ PBS__onWindowLoad(null); > >+ } else { > >+ aWindow.addEventListener("load", PBS__onWindowLoad, false); > >+ } > > I'm not sure why this switch is needed. Can you explain and document this? This should not be really needed, but I was already surprised by the domwindowopened semantics once. This code is mainly intended to cover all of the grounds...

Comment on attachment 666009 [details] [diff] [review] Patch (v3) Fairly minimal risk, potentially avoids 16.0.1. ;-)

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/9d1eb0a8b5cd test failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=15640267&tree=Mozilla-Inbound

So the test failure boiled down to the fact that we should use window.top to make sure that we're dealing with the topmost window. I'm also hitting a fatal assertion with regard to loading the favicons, and I'm trying to debug that right now.

Try run for 2c209079f4fb is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=2c209079f4fb Results (out of 13 total builds): success: 13 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-2c209079f4fb

Comment on attachment 666118 [details] [diff] [review] Patch (v4) Review of attachment 666118 [details] [diff] [review]: ----------------------------------------------------------------- This will do for the short term. I'll look into doing this properly in the platform.

Bug 795556 is tracking the presumed proper fix.

Comment on attachment 666118 [details] [diff] [review] Patch (v4) I don't understand this code. Why would you call _setPerWindowPBFlag for the top window when a sub window gets created?

(In reply to Dão Gottwald [:dao] from comment #21) > I don't understand this code. Why would you call _setPerWindowPBFlag for the > top window when a sub window gets created? _setPerWindowPBFlag sets the property on the tree owner (which then recursively sets it for all child docshells, AIUI), so that part doesn't really matter, though it's a little inefficient (bug 795556 should fix that).

Though I don't understand comment 15, there should be no need to use the top window.

(In reply to Ehsan Akhgari [:ehsan] from comment #6) > (In reply to Josh Matthews [:jdm] from comment #5) > > Do we know why the test added in the other bug didn't catch this? > > Not really. But clearly relying on domwindowopened is a mistake. This also bothers me. How do we know we're fixing things if we don't understand why they were broken?

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #22) > (In reply to Dão Gottwald [:dao] from comment #21) > > I don't understand this code. Why would you call _setPerWindowPBFlag for the > > top window when a sub window gets created? > > _setPerWindowPBFlag sets the property on the tree owner (which then > recursively sets it for all child docshells, AIUI), so that part doesn't > really matter, though it's a little inefficient (bug 795556 should fix that). As I understand it we should just do nothing if we get chrome-document-global-created for a non-top chrome window. If calling _setPerWindowPBFlag in that case is actually necessary to avoid the failure which the first patch got backed out for, then I don't understand what's going on here.

Gavin is right in comment 22 about why we need to set this flag on the root window. We need to make sure that the flag propagates through all of the docshells, and also ensure that it is always accessible from the root docshells, because that's where many other places in the code check for the existence of that flag. (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #24) > (In reply to Ehsan Akhgari [:ehsan] from comment #6) > > (In reply to Josh Matthews [:jdm] from comment #5) > > > Do we know why the test added in the other bug didn't catch this? > > > > Not really. But clearly relying on domwindowopened is a mistake. > > This also bothers me. How do we know we're fixing things if we don't > understand why they were broken? So as far as I can tell, domwindowopened is dispatched form nsWindowWatcher::AddWindow, which is called by a number of different places in the codebase. chrome-document-global-created, however, is only call from nsGlobalWindow::SetNewDocument, so it is more reliable in the sense that it's not likely for us to find places in the codebase where we have forgotten to trigger domwindowopened. Ultimately this is not the ideal fix, we would really want to do what Josh is working on in bug 795556 in the long term. For now I am rather confident that this fix is sufficient, if not ideal.

Oh, I didn't notice that we were dealing with chrome windows here. We need to make sure that e.g. the sidebar's chrome window (web-panels.xul) has PB mode set appropriately, right? I don't see how that could be related to the test failure, though.

Ehsan: can you explain the cause of the test failure for not using .top? The test doesn't do anything with child chrome windows, so I don't see how that can matter.

It probably goes without saying, but we're going to build with our final Beta tomorrow. it would make us much more comfortable if this change already landed on m-c today with a test fix.

OK, I tracked down where the about:blank document is coming from, and this is getting exceedingly ridiculous. This code <http://hg.mozilla.org/mozilla-central/annotate/490b19dde476/dom/base/nsGlobalWindow.cpp#l2137>, according to the comment there, is supposed to make sure that DispatchDOMWindowCreated is only called when we receive the final chrome document, not with the temporary about:blank document. But the code there does something entirely different, and I'm not sure why that is. Henri and Smaug have pointed this out in the last comments in bug 608669 but there has been no reply there. But the gist of the matter is that I no longer believe that chrome-document-global-created can be trusted either. Which puts us in an extremely sucky situation. :( This leaves us with only the nuke option of setting the initial value of the per-docshell flag based on the state of the global PB service. I have a patch which does that. Needless to say, this needs to get backed out with bug 795556. My proposal is for us to take this patch on central and backport it to branches, and then after a few days back it out and land the real fix in bug 795556 on central (and perhaps aurora.) Josh, please let me know if you agree with this plan.

Comment on attachment 666392 [details] [diff] [review] Patch (v5) Review of attachment 666392 [details] [diff] [review]: ----------------------------------------------------------------- This makes sense to me.

Comment on attachment 666392 [details] [diff] [review] Patch (v5) Let's get a docshell peer to sign off as well.

Comment on attachment 666392 [details] [diff] [review] Patch (v5) r=me, I guess.

Try run for cb426ec1f552 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=cb426ec1f552 Results (out of 13 total builds): success: 13 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-cb426ec1f552

Comment on attachment 666392 [details] [diff] [review] Patch (v5) [Triage Comment] Considered minimal risk, and we want to prevent this PB data leakage. Approving for our final beta (and Aurora).

https://hg.mozilla.org/releases/mozilla-aurora/rev/e6e33658ac7f https://hg.mozilla.org/releases/mozilla-beta/rev/cd5d575ea7a4

Verified on Firefox 17 beta 6 (using the steps from the description) that there are no popup content in disk cache while in private browsing. Verified on Windows 7 64-bit: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0

Was not able to reproduce this issue using FF18.b2: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 (20121128060531). about:cache is complete clean while navigating in private browsing and after private browsing was stopped too.

Thanks MarioMi! Based on Comment 41 setting tracking flag for Firefox 18 to Verified.