709326 - After private browsing has closed, sites viewed can still be visible in about:memory

Status: NEW

(Toolkit :: about:memory, enhancement)

Description

[Nick Whittome]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:8.0.1) Gecko/20100101 Firefox/8.0.1
Build ID: 20111120135848

Steps to reproduce:

- Opened Firefox. Waited for home page to load
- Started private browsing
- Navigated to YouTube. Selected random video and played.
- closed private browsing
- entered about:memory in the address bar

_


Actual results:

YouTube was listed in the memory config thus revealing that the site was visited using private browsing


Expected results:

This seems less than optimal. A user should be confident that no record is kept of sites visited using private browsing. However, as long as the browser is kept open, someone else could see which sites were visited

Comment 1

[Nicholas Nethercote [inactive]]

I was able to reproduce this, but the compartment disappeared when I reloaded about:memory.  So the compartment gets GC'd, but that doesn't happen immediately.

The problem isn't really with about:memory, it just presents info about all compartments in memory.  I guess when leaving private browsing a GC could be forced in an attempt to free any compartments created during private browsing?

Comment 2

[Justin Lebar (not reading bugmail)]

> I guess when leaving private browsing a GC could be forced in an attempt to free any compartments 
> created during private browsing?

But if you have any extensions which leak compartments, we're in trouble.  :-/

A heavyweight solution would be to assign a generation number to each private-browsing session, then attach that number to the URL in the JS compartment.  If we're not currently in private-browsing or the generation doesn't match, hide the URL.

(This would still leak that you had been in private browsing.  We could instead shove the whole compartment into heap-unclassified.)

Comment 4

[Justin Lebar (not reading bugmail)]

Simple bug to fix; happy to help someone with it.

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to Justin Lebar [:jlebar] from comment #4)
> Simple bug to fix; happy to help someone with it.

In case it wasn't clear, the simple fix is to force GC/CC when leaving private browsing mode.  That should cover the common cases.  Memory leaks still have the potential to expose private browsing data, but fixing that is much harder so let's try the GC/CC approach first.  That should also help fix bug 724677.

Comment 6

[u279076]

I'm going to take a shot at working on this bug -- it's my first Firefox bug so please be patient.

Comment 7

[u279076]

Attachment: WIP v1

I've been doing some investigation and I'm not sure GC will do anything here.

If I follow the steps in comment 0 and click Update on about:memory, the youtube compartment disappears. As far as I know this does not trigger a GC.

I also tried adding a Cu.forceGC() call to nsPrivateBrowsingService.js::_onAfterPrivateBrowsingModeChange with no difference.

Attaching a WIP patch so you can see my code change.

Comment 8

[u279076]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #7)
> Created attachment 605222 [details] [diff] [review]
> WIP v1

Note that it's commented out in this patch to prove the original state.

Comment 9

[Justin Lebar (not reading bugmail)]

> +        // Force GC to clear private session data from about:memory
> +        var windowsEnum = Services.wm.getEnumerator("navigator:browser");
> +        while (windowsEnum.hasMoreElements()) {

You just need to GC on one window, not all of them.

> +          var window = windowsEnum.getNext();
> +          window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                .getInterface(Ci.nsIDOMWindowUtils).garbageCollect();
> +          Cu.forceGC();
> +        }

But to actually collect all garbage windows, you need to run at least three cycle collections.  That this code doesn't do any CCs probably explains why it's not behaving as you expect.

  window.QueryInterface(Ci.nsIInterfaceRequestor)
        .getInterface(Ci.nsIDOMWindowUtils)
        .cycleCollect();

Or you can do what about:memory does, which is fire heap-minimize notifications.  See function minimizeMemoryUsage3x in toolkit/components/aboutmemory/content/aboutMemory.js.

Comment 10

[Nicholas Nethercote [inactive]]

Thanks for taking a look at this, Anthony!

> If I follow the steps in comment 0 and click Update on about:memory, the
> youtube compartment disappears. As far as I know this does not trigger a GC.

"Update" doesn't explicitly trigger a GC, but when the page is regenerated there's a decent chance that enough JS code is run that a GC will occur anyway.


> Or you can do what about:memory does, which is fire heap-minimize
> notifications.  See function minimizeMemoryUsage3x in
> toolkit/components/aboutmemory/content/aboutMemory.js.

Right.  So you can trigger heap-minimize events, each of which triggers a GC followed by a CC.  Or you can do the GC-followed-by-CC manually yourself.  It's important to return to the event loop each time around, otherwise some garbage may be missed.

https://github.com/Nephyrin/MozAreWeSlimYet/blob/master/mozmill_endurance_test/endurance.js#L110 might be instructive.  Note that it's designed to work with old versions of Firefox that didn't have some of the more recently-added GC triggers, so your version can be simpler.

Comment 11

[u279076]

Attachment: fix v1.0

Okay, thanks for the help guys. This seems to have got it. I'm using heap-minimize as suggested.

NOTE: I did some testing and iterating 3 times was not enough to clear youtube all the time. I've bumped it to 5 and it seems to work. Granted, I've not done exhaustive testing, and I've not checked the case for the memory usage spike.

Comment 12

[Justin Lebar (not reading bugmail)]

Comment on attachment 605554 [details] [diff] [review]
fix v1.0

This looks basically right; just a few small things.

> +    function runSoon(f) {
> +      let tm = Cc["@mozilla.org/thread-manager;1"]
> +              .getService(Ci.nsIThreadManager);
> +      tm.mainThread.dispatch({ run: f }, Ci.nsIThread.DISPATCH_NORMAL);
> +    }

I think you should be able to use setTimeout(sendHeapMinNotificationsInner, 0) for this.

I'm also kind of concerned about sending five heap-min notifications back to back like this.  A GC/CC can take 100ms or longer, so five of them could be a half-second hang.

Could you instead send the five notifications with some delay (say, 1s) between each?  setTimeout should make that easy.

(btw, I'm not a browser peer, so we'll have to loop someone else in for the final review.)

Comment 13

[u279076]

Attachment: wip v2

Running into a couple of different issues trying to use setTimeout:

> if (++i < 5)
>   runSoon();
This works, but as you said, there are risks for a hang scenario..

> if (++i < 5)
>  window.setTimeout(sendHeapMinNotificationsInner, 1000);
This does not resolve the bug, youtube still appears

> if (++i < 5)
>  window.setTimeout(sendHeapMinNotificationsInner, 0);
This appears to trigger bug 713203 (network-memory-cache of -0.00MB)

Any advice on how I might proceed with this?

Comment 14

[u279076]

I've retested my original theory of 5 simultaneous "heap-minimize" events and this does not work 100%. I'm still able to see youtube in about:memory in some instances. In particular:

1) Load about:memory
2) Start private browsing
3) Go to youtube and load a video
4) Exit private browsing
5) Repeat from step 2 until youtube appears in private browsing

I'm not sure it's enough to simply force trigger these events.

Comment 15

[u279076]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #14)
> 5) Repeat from step 2 until youtube appears in private browsing
Sorry, this should read "...until youtube appears in about:memory"

Comment 16

[Justin Lebar (not reading bugmail)]

> > if (++i < 5)
> >  window.setTimeout(sendHeapMinNotificationsInner, 1000);
> This does not resolve the bug, youtube still appears

Even after you wait 5 seconds?  Or do you think you're just hitting the non-determinism described in comment 14?

> > if (++i < 5)
> >  window.setTimeout(sendHeapMinNotificationsInner, 0);
> This appears to trigger bug 713203 (network-memory-cache of -0.00MB)

Could you post that in bug 713203?  We've been looking for STR in there.  :)

> I've retested my original theory of 5 simultaneous "heap-minimize" events and this does 
> not work 100%. I'm still able to see youtube in about:memory in some instances. In 
> particular:

It could be that even 5 GC/CCs is not sufficient; maybe we simply need to send more heap-minimize notifications.  We may not be able to solve this bug 100% without doing an excessive number of GC/CCs, but we may be able to get most of the way there...

> > 5) Repeat from step 2 until youtube appears in private browsing
> Sorry, this should read "...until youtube appears in about:memory"

One other thing to check: When you finally see youtube in about:memory, does it go away after a certain amount of time (refreshing about:memory).  Does it go away faster if you click the "minimize memory usage" button at the bottom of about:memory?

Comment 17

[u279076]

(In reply to Justin Lebar [:jlebar] from comment #16)

Sorry, I've not gotten back to this sooner. I'll try to set aside some time this coming week to respond thoughtfully.

Comment 18

[u279076]

(In reply to Justin Lebar [:jlebar] from comment #16)
> > > if (++i < 5)
> > >  window.setTimeout(sendHeapMinNotificationsInner, 1000);
> > This does not resolve the bug, youtube still appears
> 
> Even after you wait 5 seconds?  Or do you think you're just hitting the
> non-determinism described in comment 14?

Sorry it's taken so long to get back to you on this. I think waiting 5 seconds defeats the purpose of this bug does it not? The solution we are trying to achieve is for no Private Browsing data to leak through to the non-private session (ie. youtube should not be visible from the get-go). Also, without my patch, simply refreshing about:memory after a while clears it up. I'm not sure what I've done here accomplishes anything, it seems more like I'm trying to hack a bandaid together instead of solving the problem.

Maybe if I can get some tips to debug what's actually happening to let the data leak through then we might come to a real solution?

Comment 19

[u279076]

Follow up to comment 18...

I'm all for hacking something together to solve this in the interim but I'd much rather spend a bit more energy to find the *right* solution if one can be found. I'd prefer my first mozilla-central contribution to be less of the former and more of the latter.

Comment 20

[Justin Lebar (not reading bugmail)]

> I think waiting 5 seconds defeats the purpose of this bug does it not?

Perhaps, but what I wanted to know is, is 5 enough GCs?  If we schedule the GCs so they happen over 5 seconds and we don't wait for 5 seconds before checking about:memory, then we're not really testing what we wrote...

> Maybe if I can get some tips to debug what's actually happening to let the data leak through then we 
> might come to a real solution?

It's no mystery what's going on here.  When you switch into and out of PB, it's the same Firefox process, so it can't immediately forget all objects from PB mode.  about:memory is reporting all window objects in existence.  Window objects don't get cleaned up until they're GC'ed.  It takes multiple GCs to clean up all windows.  And the GC doesn't happen except occasionally.

If you break any link in the chain above, you solve the problem.  The easiest thing to change here is probably the final element, the frequency of GCs.  But that's hacky, I agree, and maybe you've shown it to be too fragile.

An alternative way of fixing this bug would be to instruct about:memory to hide URLs from private browsing windows (cutting the second link of the chain).

That's doable, but it's more complicated.  We need a way to determine whether a window is "tainted" by private browsing.  The way I'd do it is to add a new method to nsGlobalWindow which queries the window's docshell, if it has one, to determine if it's in private browsing mode.  And when a window is detached from its docshell (nsGlobalWindow::SetDocShell(NULL)), it remembers its docshell's private browsing mode.  That way, it can return the right thing even after it no longer has a docshell.

I think we'd have to invent a new interface for this is-tainted property to hang off, too.

Anyway, this is getting a lot more complicated.  This is totally doable, but it's not simple, either -- I don't know how deep you want to get into this stuff.

Comment 21

[u279076]

(In reply to Justin Lebar [:jlebar] from comment #20)
> Anyway, this is getting a lot more complicated.  This is totally doable, but
> it's not simple, either -- I don't know how deep you want to get into this
> stuff.

I'd be willing to try providing I have some guidance along the way; at the very least it would be a good learning experience for me. Of course, this will mean a fix will take longer to land than if an *experienced* developer took over. I completely understand if a timely fix is more imporant.

Comment 22

[Justin Lebar (not reading bugmail)]

Boris, does comment 20 sound reasonable to you?  We may not need to add a new nsGlobalWindow interface after all -- the code which generates the window memory reports has a raw nsGlobalWindow*.

> Of course, this will mean a fix will take longer to land than if an *experienced* developer took 
> over. I completely understand if a timely fix is more imporant.

I'm certainly not in a hurry.

Are you comfortable programming in C++?

Comment 23

[u279076]

It's been a few years since I touched C++ code. It's like riding a bike though; just hop back on the seat and it'll come back to me, right? :)

Comment 24

[Justin Lebar (not reading bugmail)]

Heh, okay!  Here are some pointers:

dom/base/nsWindowMemoryReporter.cpp::AppendWindowURI is where the URL is put into the string which eventually appears in about:memory.  Our goal is, for windows tainted by PB, not to append the URL there (maybe we append "(private)" or something).

To determine whether a window's URL is private, you'll need to add a public method to dom/base/nsGlobalWindow.h.

Critical for this method is the notion of a window's "docshell".  A docshell is a container for a window, and if the window is currently displayed in a tab, its mDocShell field is non-null.  But windows can be alive and have mDocShell == NULL; they're just not in a tab.  mDocShell gets set in nsGlobalWindow::SetDocShell.

Private browsing mode is set on a per-docshell basis (bug 722840).  So in this method you're adding to nsGlobalWindow, you can check if we have a docshell.  If so, retrieve its PB flag.  If not, well, we have to remember whether our docshell *was* in PB when SetDocShell(NULL) was last called.

You can get the private browsing flag out of a docshell by doing something like this:

  nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(mDocShell);
  if (loadContext) {
    bool isPrivateBrowsing = false;
    loadContext->GetUsePrivateBrowsing(&isPrivateBrowsing);
  }

The QueryInterfacce call gives us a different view of the same object -- in this case, it lets us call the nsILoadContext methods, one of which is GetUsePrivateBrowsing.

Let me know if/when you get stuck.  :)

Comment 25

[Josh Matthews [:jdm]]

With regards to nsILoadContext, you can also do |loadContext->UsePrivateBrowsing()| and get a boolean value. It's more pleasant to read and write, in my experience.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment 20 seems reasonable, yes.

Comment 27

[u279076]

It's been over a month and I still have not found time to work on this bug. I'm going to remove my name from it for now to let someone with more time to step in. I just don't see myself having any time to even look at this, let alone attempt a fix, in the near future.

Removing my name from the assignee field will make this available for someone else to attempt. I will re-attempt a fix if I find the time and this is still open.

Sorry I couldn't be more useful, and thanks for the assistance and patience.

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: wip v3

I *think* this is what comment 24 wanted, but I haven't touched C++ since an introductory programming class in junior high.

I'm willing to work through it, though.

Comment 29

[Josh Matthews [:jdm]]

Comment on attachment 641377 [details] [diff] [review]
wip v3

These changes won't build. You need to define a method on nsGlobalWindow that returns the private browsing boolean, and call that (via ->) in AppendWindowURI.

Comment 30

[Justin Lebar (not reading bugmail)]

In other words, this isn't a function:
 
+  // Call this to determine if this window is in private browsing mode
+  nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(mDocShell);
+  if (loadContext) {
+    bool isPrivateBrowsing = false;
+    loadContext->GetUsePrivateBrowsing(&isPrivateBrowsing);
+  }

Comment 31

[Nicholas Nethercote [inactive]]

Comment on attachment 641377 [details] [diff] [review]
wip v3

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

This approach will work for windows, which is good.  But it does nothing for JS compartments.  We need a way to mark a JS compartment as having been created by PB.  This flag could be put in the CompartmentPrivate.

::: dom/base/nsWindowMemoryReporter.cpp
@@ +85,5 @@
>  
> +  // Windows from private browsing can be reported in about:memory
> +  // which can be a leak in privacy.
> +  if(aWindow.isPrivateBrowsing) {
> +    aStr += NS_LITERAL_CSTRING("[private]");

I was thinking about this... it reveals the fact that you were using PB.  I think it would be better to simply ignore windows remaining from PB.

Comment 32

[Justin Lebar (not reading bugmail)]

> I was thinking about this... it reveals the fact that you were using PB.  I think it would be better 
> to simply ignore windows remaining from PB.

Tune in next week for "Private browsing causes a HUGE heap unclassified leak!".

I'm not sure what to do about this.

Comment 33

[Andrew McCreight [:mccr8]]

I don't think it is enough of a problem to worry about.

Comment 34

[Nigel]

Occurs within Firefox 16 - resolved by running a minimise Memory - so why not add that function as part of the switch back from Private Browsing.

This is a fault - and is similar to the bug that meant 15.01 had to be released where Firefox was remembering private browsing.

Comment 35

[Justin Lebar (not reading bugmail)]

> Occurs within Firefox 16 - resolved by running a minimise Memory - so why not add that function as 
> part of the switch back from Private Browsing.

This was an approach that we considered and tried.  There a few problems:

* Minimize memory usage is slow, which would make coming back from PB slow.
* Minimize memory usage does not always clear all windows.  If Firefox or and extension leaks memory, minimize memory won't clear the window.

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Unassigning from myself because I probably won't get to it anytime soon. Did this change at all with the per-window pb stuff?

Comment 38

[(no longer active)]

What's the status of this bug?  I had never seen it until now, and it's sort of embarrassing that this fell off our radar. :(

Comment 39

[Josh Matthews [:jdm]]

I was under the impression that it wasn't a serious concern. It was deprioritized when it became clear that the potential solution in comment 20 was more involved than original anticipated.

Comment 40

[(no longer active)]

(In reply to Josh Matthews [:jdm] from comment #39)
> I was under the impression that it wasn't a serious concern. It was
> deprioritized when it became clear that the potential solution in comment 20
> was more involved than original anticipated.

That is very sad to hear.  This can pretty much defeat the purpose of things such as sessionstore forgetting about closed private windows, etc.

Do you have cycles to work on this?

Comment 41

[Josh Matthews [:jdm]]

Not until my b2g work is wrapped up, and definitely not next week.

Comment 42

[Guilherme Lima]

Is it a problem that when you have a private window and a 'non-private'-window, if you open about:memory in the non-private, you see the memory and pages from the private one?

Comment 43

[Justin Lebar (not reading bugmail)]

(In reply to Guilherme Lima from comment #42)
> Is it a problem that when you have a private window and a
> 'non-private'-window, if you open about:memory in the non-private, you see
> the memory and pages from the private one?

What you describe does happen, but it's not this bug.

This bug is about the fact that after you close all private windows, URLs that you had open in those windows show up in about:memory in the non-private window until they're GC'ed.  If we have a memory leak, those URLs could stick around until you close the browser.

Comment 44

[(no longer active)]

(In reply to comment #43)
> (In reply to Guilherme Lima from comment #42)
> > Is it a problem that when you have a private window and a
> > 'non-private'-window, if you open about:memory in the non-private, you see
> > the memory and pages from the private one?
> 
> What you describe does happen, but it's not this bug.
> 
> This bug is about the fact that after you close all private windows, URLs that
> you had open in those windows show up in about:memory in the non-private window
> until they're GC'ed.  If we have a memory leak, those URLs could stick around
> until you close the browser.

Yes.  More specifically, if you have the non-private window open already, you can just look at those windows directly, and you don't need to go through hoops such as about:memory.

Comment 45

[Mike Hoye [:mhoye]]

Ehsan, this bug is looking for a mentor. Would you be willing?

Comment 46

[(no longer active)]

Sure!

Comment 47

[harshil goel]

I would like to work on it.
Thanks

Comment 48

[Nicholas Nethercote [inactive]]

I just looked at this a bit. It looks like only top windows have docshells. For non-top windows I guess we can use GetTop() to check if its top window is private. But detached non-top windows (exactly the ones that this bug is about) don't have a top window, and so don't have a docshell, and so there's no easy way to determine if they are private, as far as I can tell. Unless, when the docshell dies, we record the docshell's private status in every window in the relevant window tree.

Comment 49

[Boris Zbarsky [:bzbarsky]]

> It looks like only top windows have docshells

All windows have docshells while they're actually being rendered.  The docshell pointer is nulled out when the window is torn down (e.g. by being closed or by having the iframe element hosting it be removed from the DOM if it's not a toplevel window)

> For non-top windows I guess we can use GetTop() to check if its top window is private

I would expect GetTop() to not return anything useful after the docshell has gone away, since it goes up the docshell tree to get to the toplevel window....

> But detached non-top windows (exactly the ones that this bug is about) don't have a top
> window

Yep.

> and so don't have a docshell

Well, the causality is reversed.  ;)

> Unless, when the docshell dies, we record the docshell's private status in every window
> in the relevant window tree

More precisely, record the private status in the window before nulling out mDocShell in nsGlobalWindow::DetachFromDocShell.  In fact, we should change DetachFromDocShell to take the private browsing state as a boolean arg, since we might be in the docshell destructor by the time we get there, so using mDocShell for anything is chancy.

The other option is to have nsDocShell::SetPrivateBrowsing eagerly propagate the state to the window so it always has the right boolean and doesn't depend on mDocShell for it at all.

Comment 50

[Nicholas Nethercote [inactive]]

Attachment: (DRAFT)

> The other option is to have nsDocShell::SetPrivateBrowsing eagerly propagate
> the state to the window so it always has the right boolean and doesn't depend
> on mDocShell for it at all.

I tried this. It's not working very well... none of the non-top windows are
getting the new PB flag set, viz:

> │  ├──24,544,088 B (34.22%) -- top(<private-6>::https://www.youtube.com/, id=6)
> │  │  ├──20,325,856 B (28.34%) -- active
> │  │  │  ├──17,359,328 B (24.20%) ++ window(<public-8>::https://www.youtube.com/)
> │  │  │  ├───1,208,024 B (01.68%) ++ window(<public-16>::https://ad.doubleclick.net/N4061/adi/com.ythome/_default;sz=970x250;tile=1;ssl=1;dc_yt=1;kbsg=HPAU150106;kga=-1;kgg=-1;klg=en;kmyd=ad_creative_1;ytexp=931354,924622,900245,939985,949006,9406027;ord=1543271358475838?)
> │  │  │  ├─────602,592 B (00.84%) ++ window(<public-24>::https://content.googleapis.com/static/proxy.html?jsh=m%3B%2F_%2Fscs%2Fabc-static%2F_%2Fjs%2Fk%3Dgapi.gapi.en.mDMl48C7FR8.O%2Fm%3D__features__%2Frt%3Dj%2Fd%3D1%2Frs%3DAItRSTOveVmyqj7FQViAq5Q-ZAhF4tZ22g#parent=https%3A%2F%2Fwww.youtube.com&rpctoken=386973124)
> │  │  │  ├─────564,488 B (00.79%) ++ window(<public-23>::https://accounts.google.com/o/oauth2/postmessageRelay?parent=https%3A%2F%2Fwww.youtube.com#rpctoken=608379378&forcesecure=1)
> │  │  │  ├─────383,168 B (00.53%) ++ window(<public-18>::https://ad.doubleclick.net/N4061/adi/com.ythome/_default;sz=970x250;tile=1;ssl=1;dc_yt=1;kbsg=HPAU150106;kga=-1;kgg=-1;klg=en;kmyd=ad_creative_1;ytexp=931354,924622,900245,939985,949006,9406027;ord=1543271358475838?)
> │  │  │  ├──────96,032 B (00.13%) ++ window(<public-14>::javascript:"")
> │  │  │  ├──────94,336 B (00.13%) ++ window(<public-15>::https://ad.doubleclick.net/N6762/adi/mkt.ythome_1x1/;sz=1x1;tile=3;ssl=1;dc_yt=1;kga=-1;kgg=-1;klg=en;kmyd=ad_creative_3;ytexp=931354,924622,900245,939985,949006,9406027;ord=9135457893871682?)
> │  │  │  ├───────4,912 B (00.01%) ++ window(<public-20>::about:blank)
> │  │  │  ├───────4,912 B (00.01%) ++ window(<public-22>::about:blank)
> │  │  │  ├───────1,152 B (00.00%) ++ window(<private-6>::https://www.youtube.com/)/dom
> │  │  │  ├───────1,152 B (00.00%) ++ window(<public-11>::https://ad.doubleclick.net/N6762/adi/mkt.ythome_1x1/;sz=1x1;tile=3;ssl=1;dc_yt=1;kga=-1;kgg=-1;klg=en;kmyd=ad_creative_3;ytexp=931354,924622,900245,939985,949006,9406027;ord=9135457893871682?)/dom
> │  │  │  ├───────1,152 B (00.00%) ++ window(<public-13>::javascript:"")/dom
> │  │  │  ├───────1,152 B (00.00%) ++ window(<public-17>::https://ad.doubleclick.net/N4061/adi/com.ythome/_default;sz=970x250;tile=1;ssl=1;dc_yt=1;kbsg=HPAU150106;kga=-1;kgg=-1;klg=en;kmyd=ad_creative_1;ytexp=931354,924622,900245,939985,949006,9406027;ord=1543271358475838?)/dom
> │  │  │  ├───────1,152 B (00.00%) ++ window(<public-19>::https://accounts.google.com/o/oauth2/postmessageRelay?parent=https%3A%2F%2Fwww.youtube.com#rpctoken=608379378&forcesecure=1)/dom
> │  │  │  ├───────1,152 B (00.00%) ++ window(<public-21>::https://content.googleapis.com/static/proxy.html?jsh=m%3B%2F_%2Fscs%2Fabc-static%2F_%2Fjs%2Fk%3Dgapi.gapi.en.mDMl48C7FR8.O%2Fm%3D__features__%2Frt%3Dj%2Fd%3D1%2Frs%3DAItRSTOveVmyqj7FQViAq5Q-ZAhF4tZ22g#parent=https%3A%2F%2Fwww.youtube.com&rpctoken=386973124)/dom
> │  │  │  └───────1,152 B (00.00%) ++ window(<public-9>::https://ad.doubleclick.net/N4061/adi/com.ythome/_default;sz=970x250;tile=1;ssl=1;dc_yt=1;kbsg=HPAU150106;kga=-1;kgg=-1;klg=en;kmyd=ad_creative_1;ytexp=931354,924622,900245,939985,949006,9406027;ord=1543271358475838?)/dom

Maybe the code I've added to nsDocShell::SetPrivateBrowsing() isn't quite
right? Alternatively, are we certain that nsDocShell::SetPrivateBrowsing()
is called for all the non-top windows?

Comment 51

[(no longer active)]

Don't you also need to do something on nsGlobalWindow::SetDocShell?

Comment 52

[Nicholas Nethercote [inactive]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #51)
> Don't you also need to do something on nsGlobalWindow::SetDocShell?

Yes! Thank you, that helps:

> ├──23,202,808 B (29.63%) -- top(<private-6>::https://www.youtube.com/, id=6)
> │  ├──19,468,616 B (24.86%) -- active
> │  │  ├──17,622,024 B (22.50%) ++ window(<public-8>::https://www.youtube.com/)
> │  │  ├─────797,304 B (01.02%) ++ window(<public-25>::https://content.googleapis.com/static/proxy.html?jsh=m%3B%2F_%2Fscs%2Fabc-static%2F_%2Fjs%2Fk%3Dgapi.gapi.en.mDMl48C7FR8.O%2Fm%3D__features__%2Frt%3Dj%2Fd%3D1%2Frs%3DAItRSTOveVmyqj7FQViAq5Q-ZAhF4tZ22g#parent=https%3A%2F%2Fwww.youtube.com&rpctoken=668978182)
> │  │  ├─────657,800 B (00.84%) ++ window(<public-24>::https://accounts.google.com/o/oauth2/postmessageRelay?parent=https%3A%2F%2Fwww.youtube.com#rpctoken=678285121&forcesecure=1)
> │  │  ├──────97,360 B (00.12%) ++ window(<public-19>::about:blank)
> │  │  ├──────97,216 B (00.12%) ++ window(<public-14>::javascript:"")
> │  │  ├──────95,976 B (00.12%) ++ window(<public-21>::about:blank)
> │  │  ├──────95,176 B (00.12%) ++ window(<public-15>::https://ad.doubleclick.net/N6762/adi/mkt.ythome_1x1/;sz=1x1;tile=3;ssl=1;dc_yt=1;kga=-1;kgg=-1;klg=en;kmyd=ad_creative_3;ytexp=942628,940940,955005,938682,913444,916631,933226,9405612;ord=1711107010256295?)
> │  │  ├───────1,152 B (00.00%) ++ window(<private-10>::https://ad.doubleclick.net/N6762/adi/mkt.ythome_1x1/;sz=1x1;tile=3;ssl=1;dc_yt=1;kga=-1;kgg=-1;klg=en;kmyd=ad_creative_3;ytexp=942628,940940,955005,938682,913444,916631,933226,9405612;ord=1711107010256295?)/dom
> │  │  ├───────1,152 B (00.00%) ++ window(<private-13>::javascript:"")/dom
> │  │  ├───────1,152 B (00.00%) ++ window(<private-18>::https://accounts.google.com/o/oauth2/postmessageRelay?parent=https%3A%2F%2Fwww.youtube.com#rpctoken=678285121&forcesecure=1)/dom
> │  │  ├───────1,152 B (00.00%) ++ window(<private-20>::https://content.googleapis.com/static/proxy.html?jsh=m%3B%2F_%2Fscs%2Fabc-static%2F_%2Fjs%2Fk%3Dgapi.gapi.en.mDMl48C7FR8.O%2Fm%3D__features__%2Frt%3Dj%2Fd%3D1%2Frs%3DAItRSTOveVmyqj7FQViAq5Q-ZAhF4tZ22g#parent=https%3A%2F%2Fwww.youtube.com&rpctoken=668978182)/dom
> │  │  └───────1,152 B (00.00%) ++ window(<private-6>::https://www.youtube.com/)/dom

So it's now 7 public and 5 private; it should be all 12 private. I guess there
must be more places that need modifying?

Comment 53

[(no longer active)]

Sorry I read the patch too quickly.  I think what nsGlobalWindow::UsePrivateBrowsing should do is to look to see if we have a docshell and read the flag off of it, otherwise use the cached value.  Can you please try that?

Comment 54

[Nicholas Nethercote [inactive]]

Attachment: (DRAFT)

Comment 55

[Nicholas Nethercote [inactive]]

Here's the current status:

- windows with docshells are showing up as privately, correctly
- windows without docshells are showing up as public, incorrectly
  - these windows never have SetDocShell() called on them

But look at this:

> ├──28,036,736 B (34.09%) -- top(<private-with-docshell-6>::https://www.youtube.com/watch?v=a1Y73sPHKxw, id=6)
> │  ├──23,163,096 B (28.17%) -- active
> │  │  ├──14,624,272 B (17.78%) ++ window(<public-no-docshell-8>::https://www.youtube.com/watch?v=a1Y73sPHKxw)
> │  │  ├───7,370,400 B (08.96%) ++ window(<public-no-docshell-19>::https://plus.googleapis.com/u/0/_/widget/render/comments?usegapi=1&first_party_property=YOUTUBE&href=https%3A%2F%2Fwww.youtube.com%2Fwatch%3Fv%3Da1Y73sPHKxw&owner_id=r1QbZ_vXF-lm8p2kJvrkww&query=http%3A%2F%2Fwww.youtube.com%2Fwatch%3Fv%3Da1Y73sPHKxw&stream_id=UCr1QbZ_vXF-lm8p2kJvrkww&substream_id=a1Y73sPHKxw&view_type=FILTERED&width=612&youtube_video_acl=PUBLIC&hl=en_US&origin=https%3A%2F%2Fwww.youtube.com&search=%3Fv%3Da1Y73sPHKxw&hash=&gsrc=1p&jsh=m%3B%2F_%2Fscs%2Fabc-static%2F_%2Fjs%2Fk%3Dgapi.gapi.en.mDMl48C7FR8.O%2Fm%3D__features__%2Frt%3Dj%2Fd%3D1%2Frs%3DAItRSTOveVmyqj7FQViAq5Q-ZAhF4tZ22g#_methods=onPlusOne%2C_ready%2C_close%2C_open%2C_resizeMe%2C_renderstart%2Concircled%2Cdrefresh%2Cerefresh%2Confirsttimeplusonepromo%2Conthumbsup%2Contimestampclicked%2Conshareboxopen%2Conready%2Conupgradeaccount%2Conallcommentsclicked&id=I0_1420589615777&parent=https%3A%2F%2Fwww.youtube.com&pfname=&rpctoken=44150050)
> │  │  ├─────832,136 B (01.01%) ++ window(<public-no-docshell-12>::https://www.youtube.com/watch?v=a1Y73sPHKxw)
> │  │  ├─────233,312 B (00.28%) ++ window(<public-no-docshell-14>::https://googleads.g.doubleclick.net/pagead/html/r20141209/r20141212/zrt_lookup.html)
> │  │  ├──────97,216 B (00.12%) ++ window(<public-no-docshell-10>::javascript:"")
> │  │  ├───────1,152 B (00.00%) ++ window(<private-with-docshell-11>::https://www.youtube.com/watch?v=a1Y73sPHKxw)/dom
> │  │  ├───────1,152 B (00.00%) ++ window(<private-with-docshell-13>::https://googleads.g.doubleclick.net/pagead/html/r20141209/r20141212/zrt_lookup.html)/dom
> │  │  ├───────1,152 B (00.00%) ++ window(<private-with-docshell-17>::https://plus.googleapis.com/u/0/_/widget/render/comments?usegapi=1&first_party_property=YOUTUBE&href=https%3A%2F%2Fwww.youtube.com%2Fwatch%3Fv%3Da1Y73sPHKxw&owner_id=r1QbZ_vXF-lm8p2kJvrkww&query=http%3A%2F%2Fwww.youtube.com%2Fwatch%3Fv%3Da1Y73sPHKxw&stream_id=UCr1QbZ_vXF-lm8p2kJvrkww&substream_id=a1Y73sPHKxw&view_type=FILTERED&width=612&youtube_video_acl=PUBLIC&hl=en_US&origin=https%3A%2F%2Fwww.youtube.com&search=%3Fv%3Da1Y73sPHKxw&hash=&gsrc=1p&jsh=m%3B%2F_%2Fscs%2Fabc-static%2F_%2Fjs%2Fk%3Dgapi.gapi.en.mDMl48C7FR8.O%2Fm%3D__features__%2Frt%3Dj%2Fd%3D1%2Frs%3DAItRSTOveVmyqj7FQViAq5Q-ZAhF4tZ22g#_methods=onPlusOne%2C_ready%2C_close%2C_open%2C_resizeMe%2C_renderstart%2Concircled%2Cdrefresh%2Cerefresh%2Confirsttimeplusonepromo%2Conthumbsup%2Contimestampclicked%2Conshareboxopen%2Conready%2Conupgradeaccount%2Conallcommentsclicked&id=I0_1420589615777&parent=https%3A%2F%2Fwww.youtube.com&pfname=&rpctoken=44150050)/dom
> │  │  ├───────1,152 B (00.00%) ++ window(<private-with-docshell-6>::https://www.youtube.com/watch?v=a1Y73sPHKxw)/dom
> │  │  └───────1,152 B (00.00%) ++ window(<private-with-docshell-9>::javascript:"")/dom

Every public window has a corresponding private window, and the private ones are tiny. So this looks like an inner vs. outer window issue. It looks like SetDocShell() can only be called on outer windows. So if the window is an inner window we need to call UsePrivateBrowsing() on its outer window.

Comment 56

[Nicholas Nethercote [inactive]]

BTW, Ehsan: SetDocShell(nullptr) isn't allowed, judging by the assertion at the top of SetDocShell().

Comment 57

[Nicholas Nethercote [inactive]]

> So if the window is an inner window we need to call UsePrivateBrowsing() on its outer window.

This fixes it! Hooray.

Ehsan, you asked me to change nsGlobalWindow::UsePrivateBrowsing() to look at the docshell if one is present and otherwise fall back to using the PB flag on the nsGlobalWindow itself. But I don't think this is necessary -- we should be ok so long as we update the nsGlobalWindow flag when (a) its docshell is set, and (b) its docshell's PB flag is updated. Testing so far confirms this hypothesis.

So, progress. There's still more work to do, e.g. JS compartments are still being reported, so I need to propagate the PB flag into the JS reporters.

Comment 58

[Boris Zbarsky [:bzbarsky]]

> I tried this. It's not working very well... 

This patch assumed SetPrivateBrowsing happens after mScriptGlobal has been created.  If the order is reversed, it would not work.  I think we should do what this patch did, plus propagating the state when mScriptGlobal is actually created.

> It looks like SetDocShell() can only be called on outer windows.

Correct.

I'm somewhat in favor of pushing this state instead of pulling, still, which we could still do; the outer window would just set it on all its inners.

Comment 59

[Nicholas Nethercote [inactive]]

Hmm, the simplest (in one sense) and most reliable way to fix this would be to put the PB window in its own process...

Comment 60

[(no longer active)]

(In reply to Nicholas Nethercote [:njn] from comment #59)
> Hmm, the simplest (in one sense) and most reliable way to fix this would be
> to put the PB window in its own process...

We can't do that for a host of reasons.

Comment 61

[aishwarya naidu]

Hey, i am aishwarya. I am new to the concept of filing a bug. This is the first time i will be working on this and a little guidance would be great!.

Comment 62

[Nicholas Nethercote [inactive]]

(In reply to aishwarya naidu from comment #61)
> Hey, i am aishwarya. I am new to the concept of filing a bug. This is the
> first time i will be working on this and a little guidance would be great!.

Hi there. Are you interested in working on this bug? It's a tricky one, so if you are looking to contribute to Firefox it's probably not the best place to start.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction has lots of good information for new contributors.

Comment 64

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 65

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 66

[sagudev]

Is this still occurring. I cannot replicate it.

Comment 67

[Nigel]

I was able to produce the issue easily with Firefox that does not save webpages or other data between sessions

Steps
1 - Open Firefox and go into about:memory
2- Run Verbose Memory report
3-Search for website names in Memory that you plan on visiting in the private window - I used bbc.com and techspot.com and searched for bbc and techspot - you will not find it in the memory
4-Open private browsing window - and open two tabs say for bbc.com and techspot.com and load them
5-Close the private window
6-Go into the about memory tab and re-run the verbose memory report
7-Search for bbc and techspot and you will see information in the memory report as Firefox has not cleared the private browsing memory

Comment 68

[Nigel]

Tested with Firefox 75 64 bit on Windows

After 9 years Firefox still does not clear up its memory properly after closing private browsing window(s)

Comment 69

[Andreas]

And here too. 76.0 64bit Linux.
This kind of data should not even be retrievable WHEN 'private' browsing. Let alone after. Shouldn't a private window obtain its memory through a dedicated allocator so that the regular memory report doesn't know about it even?

It is very worrying that this problem is still here and a 100 times more worrying that not daily it is discussed which design flaw makes this problem so hard to solve.

You have given people a false sense of security and safety for more than long enough now; please get this right.

Comment 70

[Mike Hoye [:mhoye]]

Hi, everyone -

We aren't going to be able to tackle this problem until after the Project Fission work - https://wiki.mozilla.org/Project_Fission - has landed. The architecture that underpins this problem - the "design flaw makes this problem so hard to solve" that Andreas mentions - is going to improve dramatically as a result, but for the moment it's changing too much for work on this bug to be viable.

Once that work is done and Fission lands for good we aren't quite going to get this bugfix for "free" - or whatever the right word for "as a byproduct of a bunch of other hard work" is - but we'll be in a position where it's structurally much, much simpler to address.

Thank you for bearing with us while we get this shipped.

Comment 71

[Andrew McCreight [:mccr8]]

Under normal circumstances, the private browsing windows will go away in less than a minute, and then stop showing up in about:memory.

Comment 72

[Nigel]

(In reply to Mike Hoye [:mhoye] from comment #70)

Hi, everyone -

We aren't going to be able to tackle this problem until after the Project Fission work - https://wiki.mozilla.org/Project_Fission - has landed. The architecture that underpins this problem - the "design flaw makes this problem so hard to solve" that Andreas mentions - is going to improve dramatically as a result, but for the moment it's changing too much for work on this bug to be viable.

Once that work is done and Fission lands for good we aren't quite going to get this bugfix for "free" - or whatever the right word for "as a byproduct of a bunch of other hard work" is - but we'll be in a position where it's structurally much, much simpler to address.

Thank you for bearing with us while we get this shipped.

I note the goals of Project Fission, and hope that the new work will not as a byproduct break Firefox containers functionality

Comment 73

[Andreas]

Andrew, that was exactly the kind of thing to say to let all alarm bells go off: I’d call this '(partly) denying that there is a problem'. It belongs to the problem category that is a 100 times more worrying.

Thank you Mike Hoye, it's reassuring to hear that the problem is quite localized and we're moving in the right direction.

Comment 74

[jack]

I'm kinda irked about this. I keep a large number of tabs open, and rely upon them being there when I re-open the browser.
Earlier, when the browser was not running, I opened a private window, surfed a few sites, then closed it.
When I opened the regular browser later, I expected to see my regular tabs, but no, there's the tabs from the private session I ran earlier. To make matters worse, I cannot re-open my regular tabs from the "History/Recently Closed Windows" button.

Comment 75

[Andrew McCreight [:mccr8]]

Content processes should have a shorter lifetime now that we have process separation, but this will still happen. I don't think our private browsing has any kind of explicit goal of hiding all data once the private windows are closed, but the browser hasn't been exited, but obviously it would be nicer if it was like that.