829360 - "last-pb-context-exited" doesn't fire when private window is closed

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

Basically same issue as bug 828780 but for the "last-pb-context-exited" notification this time.

Comment 1

[Josh Matthews [:jdm]]

Ehsan, I have strong suspicions this is caused by the changes from bug 815847 (ie. always assume a private window is open) not taking into account the changes from bug 827188 (ie. lazy hidden window).

Comment 2

[Josh Matthews [:jdm]]

Bug 827188 hasn't been uplifted yet, so this shouldn't be affecting large audience yet.

Comment 3

[Josh Matthews [:jdm]]

Oh wait, I should have read the patch in bug 827188 before writing comment 1.

Comment 4

[(no longer active)]

(In reply to comment #1)
> Ehsan, I have strong suspicions this is caused by the changes from bug 815847
> (ie. always assume a private window is open) not taking into account the
> changes from bug 827188 (ie. lazy hidden window).

I'm not sure what you mean here.  Is this not a dupe of bug 829383?

Comment 5

[Erik Vold [:erikvold] (please needinfo? me)]

Ok this appears to be working on today's Nightly, maybe I did something wrong?

but the following code does work for me now:

var os = Components.classes["@mozilla.org/observer-service;1"]
                   .getService(Components.interfaces.nsIObserverService);
                   
var aConsoleService = Components.classes["@mozilla.org/consoleservice;1"].
     getService(Components.interfaces.nsIConsoleService);
os.addObserver({observe: function (aSubject, aTopic, aData) {
    aConsoleService.logStringMessage("got here: " + aTopic);
}}, "last-pb-context-exited", false);

Comment 6

[Josh Matthews [:jdm]]

Whereas it doesn't for me.

Comment 7

[(no longer active)]

OK Josh, so I'll leave this to you.  :-)

Comment 8

[Josh Matthews [:jdm]]

Grumble grumble, I was opening the error console from the private window, so it inherited the parent's privacy status.

Comment 9

[Josh Matthews [:jdm]]

Attachment: Ensure error console doesn't inherit privacy status of opener.

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Josh Matthews [:jdm] from comment #9)
> Created attachment 702204 [details] [diff] [review]
> Ensure error console doesn't inherit privacy status of opener.

No test?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 702204 [details] [diff] [review]
Ensure error console doesn't inherit privacy status of opener.

I don't understand how this patch relates to the bug summary. Ehsan said on IRC that it has to do with private web console windows messing up the count of docshells or something, but it's not clear to me why that would be the case.

Comment 12

[(no longer active)]

(In reply to comment #11)
> Comment on attachment 702204 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=702204
> Ensure error console doesn't inherit privacy status of opener.
> 
> I don't understand how this patch relates to the bug summary. Ehsan said on IRC
> that it has to do with private web console windows messing up the count of
> docshells or something, but it's not clear to me why that would be the case.

We count the number of private docshells, and from the point of view of that code, both the error console and a browser window count as one docshell each.

So if you close the last private browser window, and still have the private error console window open, the active private docshell count would be at 0 and the last-pb-context-exited will not fire unless the error console window is closed, which is not expected.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Ehsan Akhgari from comment #12)
> So if you close the last private browser window, and still have the private
> error console window open, the active private docshell count would be at 0
> and the last-pb-context-exited will not fire unless the error console window
> is closed, which is not expected.

So how is this fix sufficient? There are many other sub-windows that can be opened from a private window (e.g. prefs, page info, places manager, arbitrary extension windows, etc.). Seems like we need to fix the counting code to only count the windows it cares about.

Comment 14

[(no longer active)]

Hmm, that's a good point.

Josh, perhaps we should just explicitly count browser.xul's in browser.js?  Moving this outside of core sort of worries me, but we already have that situation with last-pb-context-exiting...

Comment 15

[Josh Matthews [:jdm]]

I would be more happy with trying to avoid propagating privacy flags to new windows that aren't navigator:browser types, but I don't know if that information is available where we want it.

Comment 16

[(no longer active)]

(In reply to comment #15)
> I would be more happy with trying to avoid propagating privacy flags to new
> windows that aren't navigator:browser types, but I don't know if that
> information is available where we want it.

It is not.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

I thought having privacy flags on windows opened from private windows was a feature (that makes it easier for these other windows to support per-window PB). Why not just change this counting code (do you have a link to it?) to only count the windows it cares about?

Comment 18

[(no longer active)]

(In reply to comment #17)
> I thought having privacy flags on windows opened from private windows was a
> feature (that makes it easier for these other windows to support per-window
> PB). Why not just change this counting code (do you have a link to it?) to only
> count the windows it cares about?

The code is here: <http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#709> (see also the sister Decrease function).

We don't have a constructed DOM at that point, since that happens way early in the docshell creation process, so we can't know anything about what the window is used for.

I'm sort of torn between the ideal world in which all XUL windows will be marked as private and a perhaps more real world where we would need to have a blacklist of some windows which cannot be marked as private... :(

Comment 19

[Josh Matthews [:jdm]]

One thing to note about the current patch is that I investigated several places we open windows, and many do not supply a parent window, thus sidestepping the problem (such as preferences and scratchpad).

Comment 20

[(no longer active)]

Comment on attachment 702204 [details] [diff] [review]
Ensure error console doesn't inherit privacy status of opener.

I thought about this some more, and I think this is the lesser of all of the evil options that we have.  Re-asking review form both Gavin and Justin in case Gavin doesn't see this in time before leaving.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 702204 [details] [diff] [review]
Ensure error console doesn't inherit privacy status of opener.

The specific problem of the notification not firing when you close all windows except for an Error Console window that was opened from a private browsing window hardly seems like a critical issue. So I don't see a need to rush an incomplete, hacky fix.

We should find ways to let that docshell counting code only count the windows it wants to count (easy enough to add a flag to docshell that browser windows could set, for example), or move the counting code somewhere else closer to the application logic.

Comment 22

[(no longer active)]

Josh, can you please give this suggestion a shot?  Thanks!

Comment 23

[Josh Matthews [:jdm]]

The problem with moving the counting code into application logic is that it becomes much easier for some random extension to just close a window and avoid it. I'm going to look at using bug 829383 to make some window types not be counted.

Comment 24

[Josh Matthews [:jdm]]

Attachment: Hack

This works, but it's a hack. Next step is to find out whether we actually have a way of observing all new chrome window creations such that I can access the windowtype attribute in the frontend.

Comment 25

[Josh Matthews [:jdm]]

Attachment: Keep non-browser windows from extending the lifetime of the private session.

This moves it into the frontend instead, which is much nicer. Ehsan, what are your concerns with chrome-document-global-created again? It seems to work fine for me.

Comment 26

[Josh Matthews [:jdm]]

Comment on attachment 704863 [details] [diff] [review]
Keep non-browser windows from extending the lifetime of the private session.

>+    if (type === "navigator:browser" != -1) {

Ignore the != -1; I've fixed it locally and everything still works.

Comment 27

[(no longer active)]

(In reply to Josh Matthews [:jdm] from comment #25)
> Created attachment 704863 [details] [diff] [review]
> Keep non-browser windows from extending the lifetime of the private session.
> 
> This moves it into the frontend instead, which is much nicer. Ehsan, what
> are your concerns with chrome-document-global-created again? It seems to
> work fine for me.

Bug 795961.

Comment 28

[(no longer active)]

Comment on attachment 704863 [details] [diff] [review]
Keep non-browser windows from extending the lifetime of the private session.

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

This won't cover all edge cases, I'm afraid.  chrome-document-global-created should really be called chrome-document-global-created...-or-maybe-not.  :(

Comment 29

[(no longer active)]

There is yet another approach that we can take.  We can make XUL elements actually know about the windowtype attribute set on root elements in XUL documents, and communicate to the docshell when needed.  This is I think perhaps the only way in which add-ons cannot destroy this house of cards.  We already have similar code for the drawintitlebar and title attributes set on root XUL elements in nsXULElement.cpp.  Josh, can you please see if you can rewrite this patch using that idea?

(Sorry about the back-and-forth here.)

Comment 30

[Josh Matthews [:jdm]]

Comment 29 sounds like my "Hack" patch that I attached yesterday.

Comment 31

[Josh Matthews [:jdm]]

Attachment: Keep non-browser windows from extending the lifetime of the private session.

Boris, how do you feel about this patch? I plan to file a follow up for when we're able to sort out the chrome-document-global-created weirdness at a later date.

Comment 32

[Josh Matthews [:jdm]]

Comment on attachment 705659 [details] [diff] [review]
Keep non-browser windows from extending the lifetime of the private session.

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

::: content/xul/content/src/nsXULElement.cpp
@@ +662,5 @@
> +        else if (attr == nsGkAtoms::windowtype) {
> +            nsAutoString value;
> +            GetAttr(kNameSpaceID_None, attr, value);
> +            if (!value.Equals(NS_LITERAL_STRING("navigator:browser")) &&
> +                !value.Equals(NS_LITERAL_STRING("navigator:view-source"))) {

Pretend this view-source one doesn't exist. I don't think anything but navigator:browser should actually prolong a private session.

Comment 33

[(no longer active)]

(In reply to comment #32)
> ::: content/xul/content/src/nsXULElement.cpp
> @@ +662,5 @@
> > +        else if (attr == nsGkAtoms::windowtype) {
> > +            nsAutoString value;
> > +            GetAttr(kNameSpaceID_None, attr, value);
> > +            if (!value.Equals(NS_LITERAL_STRING("navigator:browser")) &&
> > +                !value.Equals(NS_LITERAL_STRING("navigator:view-source"))) {
> 
> Pretend this view-source one doesn't exist. I don't think anything but
> navigator:browser should actually prolong a private session.

Yeah that's fair.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 705659 [details] [diff] [review]
Keep non-browser windows from extending the lifetime of the private session.

I don't see why it makes sense to put this in AddListenerFor... Shouldn't it go in things like AfterSetAttr or whatnot?  And shouldn't we be testing for a root element somewhere?

Comment 35

[Josh Matthews [:jdm]]

After discussion with Boris, I'll be adding special checks to nsXULElement::Create and nsXULElement::AfterSetAttr.

Comment 36

[Josh Matthews [:jdm]]

Attachment: Keep non-browser windows from extending the lifetime of the private session.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 708523 [details] [diff] [review]
Keep non-browser windows from extending the lifetime of the private session.

Probably better to use AttrValueIs.

But more importantly, shouldn't we be calling MaybeUpdatePrivateLifetime on root elements?  Ideally only in root documents or something?

Comment 38

[Josh Matthews [:jdm]]

Attachment: Keep non-browser windows from extending the lifetime of the private session.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 709924 [details] [diff] [review]
Keep non-browser windows from extending the lifetime of the private session.

>+++ b/content/xul/content/src/nsXULElement.cpp
>+    nsIDocument* document = GetCurrentDoc();
>+    if (!document) {
>+        document = mNodeInfo->GetDocument();
>+    }

Just OwnerDoc() for that whole thing if that's what you mean, please?

Note that OwnerDoc() should never be null.

r=me with that fixed.

Comment 40

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12cbe5187dc9

Comment 41

[Josh Matthews [:jdm]]

Comment on attachment 709924 [details] [diff] [review]
Keep non-browser windows from extending the lifetime of the private session.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 829383
User impact if declined: Potential for private sessions that outlive user expectations.
Testing completed (on m-c, etc.): m-c, automated test, manual testing.
Risk to taking this patch (and alternatives if risky): Extremely small.
String or UUID changes made by this patch: None.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/12cbe5187dc9

Comment 43

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2f144f6fcd55