Closed Bug 828780 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Private Browsing, defect)

21 Branch
All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 21
Tracking Status
firefox19 --- unaffected
firefox20 + fixed
firefox21 + fixed

People

(Reporter: evold, Assigned: ehsan)

References

Details

Attachments

(2 files)

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);
Assignee: nobody → josh
Blocks: PBnGen
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
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.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Perhaps you're seeing a variant of bug 818224?  Can you log some other way not using the console service?
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);
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.
Assignee: josh → ehsan
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. :(
Attached patch Patch (v1)Splinter Review
Attachment #701389 - Flags: review?(gavin.sharp)
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 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 :(
Attachment #701389 - Flags: review?(gavin.sharp) → review+
It also makes me nervous to be changing this on Aurora, FWIW. I guess we don't have much other choice at this point.
We should of course watch closely for regressions... :/
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.
Attachment #701389 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/349a247b84cf
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Attachment #702673 - Flags: review?(josh)
Attachment #702673 - Flags: review?(josh) → review+
Test case: hg.mozilla.org/integration/mozilla-inbound/rev/e6ab6c9b5885
Comment on attachment 701389 [details] [diff] [review]
Patch (v1)

Will approve for Aurora, and please watch for reqressions.
Attachment #701389 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.