Last Comment Bug 795015 - popup content in disk cache while in private browsing
: popup content in disk cache while in private browsing
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Private Browsing (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: Firefox 18
Assigned To: :Ehsan Akhgari (out sick)
:
Mentors:
http://www.einslive.de/
Depends on:
Blocks: pbchannelfail
  Show dependency treegraph
 
Reported: 2012-09-27 10:31 PDT by Christian Ascheberg
Modified: 2012-12-07 07:53 PST (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
verified
+
verified


Attachments
Patch (v1) (3.08 KB, patch)
2012-09-27 13:52 PDT, :Ehsan Akhgari (out sick)
josh: feedback+
Details | Diff | Review
Patch (v2) (2.83 KB, patch)
2012-09-27 15:41 PDT, :Ehsan Akhgari (out sick)
no flags Details | Diff | Review
Patch (v3) (3.08 KB, patch)
2012-09-28 11:41 PDT, :Ehsan Akhgari (out sick)
dao+bmo: review+
Details | Diff | Review
Patch (v4) (6.77 KB, patch)
2012-09-28 16:45 PDT, :Ehsan Akhgari (out sick)
josh: review+
dao+bmo: feedback-
Details | Diff | Review
Patch (v5) (4.12 KB, patch)
2012-09-30 17:14 PDT, :Ehsan Akhgari (out sick)
josh: review+
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review

Description Christian Ascheberg 2012-09-27 10:31:15 PDT
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.
Comment 1 Josh Matthews [:jdm] 2012-09-27 12:13:04 PDT
For reference: bug 749187 was supposed to address this problem.
Comment 2 :Ehsan Akhgari (out sick) 2012-09-27 13:52:37 PDT
Created attachment 665634 [details] [diff] [review]
Patch (v1)

Turns out that domwindowopened does not do what it says on the can at all!
Comment 3 :Ehsan Akhgari (out sick) 2012-09-27 13:53:56 PDT
I would sleep much better during the night if we took this on all branches, to avoid a 16.0.1.
Comment 4 Dão Gottwald [:dao] 2012-09-27 14:02:16 PDT
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 5 Josh Matthews [:jdm] 2012-09-27 14:21:50 PDT
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.
Comment 6 :Ehsan Akhgari (out sick) 2012-09-27 15:36:30 PDT
(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.
Comment 7 :Ehsan Akhgari (out sick) 2012-09-27 15:41:14 PDT
Created attachment 665686 [details] [diff] [review]
Patch (v2)

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 8 Dão Gottwald [:dao] 2012-09-27 19:26:56 PDT
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?
Comment 9 Alex Keybl [:akeybl] 2012-09-28 11:12:41 PDT
This needs to land on mozilla-central and mozilla-aurora asap - our final beta build is Monday.
Comment 10 :Ehsan Akhgari (out sick) 2012-09-28 11:37:34 PDT
(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 11 :Ehsan Akhgari (out sick) 2012-09-28 11:41:01 PDT
Created attachment 666009 [details] [diff] [review]
Patch (v3)
Comment 12 :Ehsan Akhgari (out sick) 2012-09-28 11:47:59 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d927bdcfd2e4
Comment 13 :Ehsan Akhgari (out sick) 2012-09-28 11:48:28 PDT
Comment on attachment 666009 [details] [diff] [review]
Patch (v3)

Fairly minimal risk, potentially avoids 16.0.1.  ;-)
Comment 15 :Ehsan Akhgari (out sick) 2012-09-28 16:33:49 PDT
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.
Comment 16 :Ehsan Akhgari (out sick) 2012-09-28 16:45:48 PDT
Created attachment 666118 [details] [diff] [review]
Patch (v4)
Comment 17 :Ehsan Akhgari (out sick) 2012-09-28 16:46:27 PDT
https://tbpl.mozilla.org/?tree=Try&rev=2c209079f4fb
Comment 18 Mozilla RelEng Bot 2012-09-28 20:00:29 PDT
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 19 Josh Matthews [:jdm] 2012-09-28 23:36:45 PDT
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.
Comment 20 Josh Matthews [:jdm] 2012-09-29 02:06:37 PDT
Bug 795556 is tracking the presumed proper fix.
Comment 21 Dão Gottwald [:dao] 2012-09-29 03:06:40 PDT
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?
Comment 22 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-09-29 11:34:45 PDT
(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).
Comment 23 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-09-29 11:36:31 PDT
Though I don't understand comment 15, there should be no need to use the top window.
Comment 24 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-09-29 11:37:20 PDT
(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?
Comment 25 Dão Gottwald [:dao] 2012-09-29 11:49:13 PDT
(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.
Comment 26 :Ehsan Akhgari (out sick) 2012-09-29 11:55:32 PDT
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.
Comment 27 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-09-29 11:58:50 PDT
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.
Comment 28 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-09-29 12:01:19 PDT
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.
Comment 29 Alex Keybl [:akeybl] 2012-09-30 14:13:04 PDT
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.
Comment 30 :Ehsan Akhgari (out sick) 2012-09-30 17:13:39 PDT
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 31 :Ehsan Akhgari (out sick) 2012-09-30 17:14:48 PDT
Created attachment 666392 [details] [diff] [review]
Patch (v5)
Comment 32 :Ehsan Akhgari (out sick) 2012-09-30 17:15:55 PDT
https://tbpl.mozilla.org/?tree=Try&rev=cb426ec1f552
Comment 33 Josh Matthews [:jdm] 2012-09-30 18:17:57 PDT
Comment on attachment 666392 [details] [diff] [review]
Patch (v5)

Review of attachment 666392 [details] [diff] [review]:
-----------------------------------------------------------------

This makes sense to me.
Comment 34 Josh Matthews [:jdm] 2012-09-30 18:25:16 PDT
Comment on attachment 666392 [details] [diff] [review]
Patch (v5)

Let's get a docshell peer to sign off as well.
Comment 35 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-09-30 18:38:49 PDT
Comment on attachment 666392 [details] [diff] [review]
Patch (v5)

r=me, I guess.
Comment 36 :Ehsan Akhgari (out sick) 2012-09-30 18:56:27 PDT
https://hg.mozilla.org/mozilla-central/rev/4a7836f11aa7
Comment 37 Mozilla RelEng Bot 2012-09-30 19:15:30 PDT
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 38 Alex Keybl [:akeybl] 2012-10-01 10:09:50 PDT
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).
Comment 40 Simona B [:simonab](PTO) 2012-11-16 08:11:11 PST
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
Comment 41 Mihai Morar, (:MihaiMorar) 2012-12-06 01:09:29 PST
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.
Comment 42 Simona B [:simonab](PTO) 2012-12-07 07:53:54 PST
Thanks MarioMi!

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

Note You need to log in before you can comment on or make changes to this bug.