828780 - "last-pb-context-exiting" doesn't fire when private window is closed

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

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

I tired the code below in Scratchpad and the event is not observed when the last private browsing window is closed as I would expect from what I read here https://developer.mozilla.org/en-US/docs/Supporting_per-window_private_browsing#Preventing_a_private_session_from_ending


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 here2");
    aSubject.QueryInterface(Components.interfaces.nsISupportsPRBool);
    // if another extension has not already canceled entering the private mode
    if (!aSubject.data) {
      if (aData == "exit") { // if we are leaving the private mode
        /* you should display some user interface here */
        aSubject.data = true; // cancel the operation
      }
   }
}}, "last-pb-context-exiting", false);

Comment 1

[Josh Matthews [:jdm]]

Fortunately, this is just a documentation error that I've now corrected. There's no aData any longer, and it worked fine for me when I got rid of that check.

Comment 2

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

I still have this issue even with the following simplified example:


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-exiting", false);

os.addObserver({observe: function (aSubject, aTopic, aData) {
    aConsoleService.logStringMessage("got here: " + aTopic);
}}, "last-pb-context-exited", false);


I do get the "last-pb-context-exited" notification though.

Comment 3

[(no longer active)]

Perhaps you're seeing a variant of bug 818224?  Can you log some other way not using the console service?

Comment 4

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

Same problem with:


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) {
    window.alert("got here: " + aTopic);
}}, "last-pb-context-exiting", false);

os.addObserver({observe: function (aSubject, aTopic, aData) {
    window.alert("got here: " + aTopic);
}}, "last-pb-context-exited", false);

Comment 5

[(no longer active)]

Ah shoot, of course.  The global service used to dispatch this, and now that the service doesn't exist anymore, no wonder this is no longer being dispatched: <http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#343>  Sorry about the confusion.

I'll write a patch.

Comment 6

[(no longer active)]

I was wrong, browser.js does dispatch the notification.  <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6199>  But the code path for closing the window by closing the last tab conveniently bypasses calling warnAboutClosingWindow, so that code (and the rest of the checks there) never gets called.

I cannot believe nobody has caught this before. :(

Comment 7

[(no longer active)]

Attachment: Patch (v1)

Comment 8

[(no longer active)]

So the existing notification mentioned in comment 6 relies on closeWindow being called with warnAboutClosingWindow as its argument, so that it can perform the checks necessary to dispatch this notification.  However turns out that the tabbrowser code calls closeWindow on a couple of occasions without using warnAboutClosingWindow, therefore bypassing that code altogether.  As a result, if you close the last PB window using the window close button which does go through the warnAboutClosingWindow code path, the notification is dispatched, and it you close it by closing the last remaining tab (such as when pressing Cmd+W), then it won't get dispatched.

In builds with the global PB service, the service was responsible for dispatching this notification as it was the central place where PB mode could be turned off, but in Firefox 20+ that code doesn't exist any more, which is why this bug has been noticed on those builds.

Comment 9

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

Comment on attachment 701389 [details] [diff] [review]
Patch (v1)

AIUI both of these closeWindow callers only occur when there are <=1 tabs left in the window, so warnAboutClosingTabs when called via warnAboutClosingWindow is a no-op, and so the only effect warnAboutClosingWindow has is notifying the observers it notifies. This code is so hard to follow :(

Comment 10

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

It also makes me nervous to be changing this on Aurora, FWIW. I guess we don't have much other choice at this point.

Comment 11

[(no longer active)]

We should of course watch closely for regressions... :/

Comment 12

[(no longer active)]

Comment on attachment 701389 [details] [diff] [review]
Patch (v1)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): No specific bug... The original code has had this bug for a long time but it was first uncovered with per-window PB.
User impact if declined: See comment 0.  Prompting to cancel private downloads when closing the last private window depends on this, so depending on whether you close the window by pressing Cmd+W, the prompt might be suppressed.  This also breaks the last-pb-context-exiting API that add-ons may rely on for similar cleanup.
Testing completed (on m-c, etc.): Locally.
Risk to taking this patch (and alternatives if risky): This can be fairly risky, but the bug that this fixes can be fairly bad (dataloss is possible, see above), so I recommend we take it on Aurora ASAP and watch for regressions.
String or UUID changes made by this patch: None.

Comment 13

[(no longer active)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/349a247b84cf

Comment 14

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

No test?

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/349a247b84cf

Comment 16

[(no longer active)]

Attachment: Test case for bug 828780

Comment 17

[(no longer active)]

Test case: hg.mozilla.org/integration/mozilla-inbound/rev/e6ab6c9b5885

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e6ab6c9b5885

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 701389 [details] [diff] [review]
Patch (v1)

Will approve for Aurora, and please watch for reqressions.

Comment 20

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e51b7b95574c
https://hg.mozilla.org/releases/mozilla-aurora/rev/53807bd73c8a