498648 - Start private browsing while editing a message, cancel, doesn't cancel private browsing

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect, P2)

Description

[Timothy Miller]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090612 Firefox/3.0
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090612 Firefox/3.0

There seems to be a problem where starting private browsing while an edit box is open and canceling the navigate-away retains the page.  This may apply only to those like Yahoo mail with Javascript that checks for this.

Reproducible: Always

Steps to Reproduce:
1. Start composing an email in a Yahoo! email account.  Don't finish.
2. Start private browsing.
3. Either Firefox or Yahoo's javascript pops up an alert asking you if you're sure you want to navigate away from the page.
4. Click "no"
5. The alert appears again, which is odd.
6. Click "no"
7. You seem to enter private browsing, but the Yahoo tab is still open.



Expected Results:  
I expect that either this should cancel private browsing entirely until the problem is properly resolved, or that the state of the editing should be saved by Firefox so that it can be restored when private browsing is exited.

I'm not a security expert, but I'm wondering if this couldn't cause security problems.  Javascript (secretly) prevents private browsing from becoming fully private and monitors what you do.

Comment 1

[Nochum Sossonko [:Natch]]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090613 Minefield/3.6a1pre
Yikes, this is a real issue!

Comment 2

[Nochum Sossonko [:Natch]]

I think the fix would be fairly simple, add an observer for "private-browsing-cancel-vote" and don't allow pages to cancel the unload...

Comment 3

[Nochum Sossonko [:Natch]]

Basically, as a simple fix, one can check if private browsing is enabled here:

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp?mark=1104-1105#1082

as a more complex fix, you'd have to register an observer somewhere and when it's hit show the dialog if it's warranted, then return the users response to the notification.

Comment 4

[Timothy Miller]

BTW, I should have mentioned that I spoof my useragent to make certain web sites behave.  I'm actually using the Shiretoko nightly build.

Comment 5

[Bartłomiej Brzozowiec (BartZilla)]

(In reply to comment #4)
> BTW, I should have mentioned that I spoof my useragent to make certain web
> sites behave.  I'm actually using the Shiretoko nightly build.

You mean some sites misbehave if there is no "Firefox" string in UA? If this is the case, then report it (if it is not reported already): fill a bug in "Tech Evangelism" Product, add bug 334967 to "Blocks" list.

Comment 6

[Timothy Miller]

For me, it was Hotmail and Yahoo mail, which are already known problems.  Thanks.

Comment 7

[(no longer active)]

(In reply to comment #3)
> Basically, as a simple fix, one can check if private browsing is enabled here:
> 
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp?mark=1104-1105#1082
> 
> as a more complex fix, you'd have to register an observer somewhere and when
> it's hit show the dialog if it's warranted, then return the users response to
> the notification.

I think we need a combination of these approaches:

1. We can have the doc shell listen on the cancel-vote notification and display the prompt if needed.  If the user cancels the prompt, the private browsing mode switch should be canceled.

2. We should prevent the document viewer to show the prompt while the private browsing mode is being switched, because the prompt has been displayed once already.

Taking...

Comment 8

[Nochum Sossonko [:Natch]]

This can make for a pretty bad user experience, both when starting pb or leaving pb. Breaks some webapps as well that rely on returning false from onbeforeunload to warn the user of possible dataloss, so this can result in dataloss. IMHO, this is a "blocking-worthy" bug, even if it is somewhat of an edge case.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

It took me a while to understand this bug, but I think I have it.

We're saying that the web app will say "Are you sure you want to leave this page", and if the user says "no" we don't respect that decision and enter Private Browsing Mode anyway.

I agree that this needs to be fixed.

Comment 10

[Nochum Sossonko [:Natch]]

Attachment: quick 'n dirty

Prototype patch of what _can_ be done to fix this bug. The only question is whether this will effect performance (I have a feeling it will). Another option would be to register a static observer and iterate over the the current Document views calling PermitUnload on each one, but I'm not sure how one would go about doing that...

Comment 11

[Nochum Sossonko [:Natch]]

Attachment: patch

Ehsan, I hope you don't mind me stealing this from you...

After some though into this I realized this wouldn't be too difficult to do, so I decided to do it myself. Entering/Exiting pb mode now behaves the same way as closing the app when you have gmail (or any similar site with a beforeunload handler that confirms closing the window) open.

Comment 12

[Nochum Sossonko [:Natch]]

Comment on attachment 402711 [details] [diff] [review]
patch

This actually needs another notification added to the private browsing service as we can't know for sure that we're entering private browsing just because we say it's ok. New patch coming.

Comment 13

[Nochum Sossonko [:Natch]]

Attachment: patch

Wait for "private-browsing-change-granted" before setting sPrivateBrowsingChanging.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 402717 [details] [diff] [review]
patch

I might be misreading the window mediator rats-nest, but as far as I can tell this patch has a few problems:

1)  Only works for xul-based apps, not for other Gecko-based apps that might happen to have a private browsing service.

2)  Is probably not very robust in the face of content beforeunload handlers raising the window or otherwise changing window z-order, at least if I'm to believe the comments in nsIWindowMediator.idl.  For that matter, what about them just raising it by calling alert() or prompt() (which is what beforeunload handlers tend to do)?

I'm assuming you did in fact want to fire beforeunload handlers on various chrome windows and such.  If not, then nsIWindowWatcher::getWindowEnumerator might be a better fit, maybe.  Worth checking exactly what set of windows it returns in Firefox.

Comment 15

[Nochum Sossonko [:Natch]]

(In reply to comment #14)
> 1)  Only works for xul-based apps, not for other Gecko-based apps that might
> happen to have a private browsing service.

I'm not sure I understand this, what else should I be looking out for? Will this be fixed if I switch to nsIWindowWatcher?

> 2)  Is probably not very robust in the face of content beforeunload handlers
> raising the window or otherwise changing window z-order, at least if I'm to
> believe the comments in nsIWindowMediator.idl.  For that matter, what about
> them just raising it by calling alert() or prompt() (which is what beforeunload
> handlers tend to do)?

Fair enough, it _seems_ that way from the comment, although in my initial testing (using the url provided which guarantees the window will be raised) it didn't happen. This can happen though if something else does it to the window, but it seems that if the current window that is being enumerated is raised it doesn't change the order.

> I'm assuming you did in fact want to fire beforeunload handlers on various
> chrome windows and such.  If not, then nsIWindowWatcher::getWindowEnumerator
> might be a better fit, maybe.  Worth checking exactly what set of windows it
> returns in Firefox.

I'll look into using nsIWindowWatcher instead, it seems better. I'm not sure about the chrome stuff though, I don't necessarily want to fire onbeforeunload on them since the private browsing service doesn't necessarily close all chrome windows (currently I think it only closes browser windows and page info windows, and maybe view source windows). Besides, I think chrome windows tend to use onclose anyhow, and they won't be as broken. This patch should probably focus on content windows as they are the ones that can have this broken-ness.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> I'm not sure I understand this

nsIWindowMediator only enumerates nsIXULWindows, basically.  Something like Camino, say, doesn't have any.  I believe using nsIWindowWatcher will help with this problem.

> it seems that if the current window that is being enumerated is raised it
> doesn't change the order.

Sure, but content script can cause other windows to be raised (e.g. by calling alert on some other window it has lying about).

> This patch should probably focus on content windows

I agree.

Comment 17

[Nochum Sossonko [:Natch]]

Attachment: patch

So basically, nsIWindowWatcher::GetWindowEnumerator and nsIWindowMediator::GetZOrderDOMWindowEnumerator return the same exact windows in my tests (I tested with a bunch of different windows, page info windows and DOMi window). It returns the top level xul window of all of these, which is sub-optimal. I switched to just GetEnumerator and specified "navigator:browser" to get only browser windows (which is what the private browsing service closes).

This solution is sub-optimal, as IMHO the pb service should deal with this itself, bailing on the pb change if it can't close the window. I'm not asking for review on this patch as I'm looking into the latter solution, will post a patch once I have that figured out.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Gah.  I thought we had a sane way to enumerate DOMWindows...  jst, do we just not have one?

The attached patch still has the problem of putting very XUL-app-specific code in a part of the browser where imo it just doesn't belond.  I'm not convinced that the pb service would be any better, unless it's not meant to be used with non-xul-apps.

Comment 19

[Nochum Sossonko [:Natch]]

Attachment: Alt. patch

Boris, what do you think of this API? I haven't hooked it up to the pb service yet, but the idea would be that the pb service can check the windows first, then tell the content viewer not to check PermitUnload afterward.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Why is that better than having the content viewer explicitly do the beforeunload checks and then stop entry into private browsing mode?  Though this does address an issue with the first patch, which is that it would have run beforeunload handlers twice...

Which is actually an interesting problem: web content that expects to get beforeunload might not expect to get it multiple times if it doesn't prevent it.  But I guess we have this issue already with a window that has a bunch of tabs in it if you try to close the whole window, huh?

Comment 21

[Nochum Sossonko [:Natch]]

(In reply to comment #20)
I though it would be better to have an api to do this instead of adding a static observer to Gecko code. Also, it's less complication (no need to add 3 observers) and can be used by pb mode even before the "private-browsing-cancel-vote" so as not to even show the "Start Private Browsing" prompt if the user doesn't want to close the window.

Also, since the pb code explicitly closes only xul windows (it uses session store which uses the same api as the one we're using to search for windows[1]), I think it makes more sense in the pb code.

If you'd rather have the static observer, I'm ok with that too.

1: http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#2571 is the function used by setBrowserState here: http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#877

Comment 22

[Nochum Sossonko [:Natch]]

Attachment: irc discussion with bz

Going to look into this now...

Comment 23

[Nochum Sossonko [:Natch]]

In order to implement bz's idea, this will require changes to session store, tabbrowser, private browsing, and nsGlobalWindow as follows:

session store: add an optional boolean param to setBrowserState that uses the new method on nsIDOMChromeWindow which skips beforeunload checks. This boolean will also force all tabs to be close and new ones opened for the new state (either this or we can add a method to docShell that allows loading a new uri without check for beforeunload).

tabbrowser: removeTab gets another optional boolean param which dictates whether or not permitUnload checks will be made before closing the tab.

nsGlobalWindow: enhanced with a new method on nsIDOMChromeWindow that allows closing a window without asking for permission.

I've cc'ed some of the people involved in these parts of the code, and I'd like to hear their opinions on this, I'll have a proto patch available in a bit so you can see what it's like.

Comment 24

[Nochum Sossonko [:Natch]]

Attachment: proto patch

Oh, and of course, the changes to private browsing: check permitUnload on all windows, then set the browser state using the new boolean param to force closing. The tab open/remove thing is unnecessary as session store hands back a clean window with the new param.

Comment 25

[(no longer active)]

(In reply to comment #24)
> Created an attachment (id=404302) [details]
> proto patch
> 
> Oh, and of course, the changes to private browsing: check permitUnload on all
> windows, then set the browser state using the new boolean param to force
> closing. The tab open/remove thing is unnecessary as session store hands back a
> clean window with the new param.

If I'm reading the patch correctly, with the second param of setBrowserState set to true, session store no longer reuses any already open tabs.  Is this desired/necessary?  Also, does it guarantee the safety of removing the first hunk in nsPrivateBrowsingService.js (i.e., have you tested bug 476463)?

I have a number of review comments on the PB part, but I'll hold them off until you're ready to submit a version of the patch for review (with automated tests, of course :) )

Comment 26

[Nochum Sossonko [:Natch]]

Attachment: new approach

After much discussion with mconnor and bz trying to figure out all the different options, I think this patch should address everyone's concerns. This ensures permitUnload is called at least once, doesn't add any browser-specific code in platform and allows pb to bail if any tab prevents unloading.

Comment 27

[Boris Zbarsky [:bzbarsky]]

That patch looks wrong.  Once mCallerIsClosingWindow becomes true it can never become false, but it becoming true doesn't guarantee the window will in fact be closed.

Comment 28

[Boris Zbarsky [:bzbarsky]]

And note that this is likely to be a problem with any patch that wants to store state in the document viewer....

Comment 29

[Nochum Sossonko [:Natch]]

(In reply to comment #27)
Since calling permitUnload is the responsibility of the caller, this should be safe. permitUnload isn't guaranteeing that _it's_ closing the window, it's just taking a flag that tells it that the _caller_ is closing the window. Past that, it's up to the caller to ensure the windows are actually closed.

If you think it's absolutely necessary to be able to restore mCallerIsClosingWindow, I can add another method that does just that (not allowing you to pass in a value, only allowing you to restore its original value). In any event, this route seems to be the best for all parties...

Comment 30

[Boris Zbarsky [:bzbarsky]]

Maybe I should make this clearer.  Say I have two tabs with unload handlers.  The first has a web-based editor in it.  I edit some stuff in the editor, then go into private browsing mode.  I get a prompt from the editor, and I say "ok, go ahead and unload".  I then get a prompt from my other tab and say "no, don't unload!".  We don't enter private browsing and don't close the window.  We set the member on both viewers to true.

If I now go and do some more editing in the editor and then close it, I get no prompt to save my stuff; it just closes.  That seems like somewhat unexpected user behavior.

Similar effects if something returns false from _canEnterPrivateBrowsingMode() even if there's only one tab with beforeunload.

Comment 31

[Boris Zbarsky [:bzbarsky]]

If the concern with my proposed nsIDOMChromeWindow API was that it's easy to screw up with it, I'd argue that this API is even easier to screw up.  In fact, this one is impossible to use correctly, as far as I can tell.

Comment 32

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 3.1

New patch addresses the issue in comment 30. The reason this would be preferred is that even with the nsIDOMChromeWindow API added there would have to be other "API" changes and major changes in session store. I'd rather do this, it seems safer and more directly addresses this issue.

Also, this doesn't leave much room for abuse, the caller is required to call permitUnload at least once. Granted, they can do it at a totally inappropriate time, but if we're worrying about extensions (and what not) doing bad stuff, there's a much longer and more worrisome list of things to look at.

Comment 33

[Boris Zbarsky [:bzbarsky]]

> I'd rather do this, it seems safer and more directly addresses this issue.

I guess it seems to address the issue more indirectly to me, at the cost of core API changes that affect all Gecko consumers...

In any case, two obvious comments:

1)  Need to change the iid.
2)  If mCallerIsClosingWindow, do you really want to recurse into the kids?

Comment 34

[Nochum Sossonko [:Natch]]

(In reply to comment #33)
> In any case, two obvious comments:
> 
> 1)  Need to change the iid.

Will do.
> 2)  If mCallerIsClosingWindow, do you really want to recurse into the kids?

It's possible to restoreCloseWindow on one of the kids, which will still be skipped if the parent is closed. Does this warrant recursing? I think so, I'll go with whatever you'd prefer though. Private Browsing doesn't need the recursing.

About this approach, I'm doing this to avoid API changes to session store as well as tabbrowser (or docShell), also this approach is much less invasive for this bug. It seems to also make sense to me since it _is_ the caller's responsibility to call permitUnload, it should also be the caller's option to indicate that other callers should be skipped (kind of like telling the content viewer that I'll be handling calling permitUnload for this view). Since there are a lot of callers, this allows the caller to "nip it in the bud". It also has the added advantage in that it ensures permitUnload will be called at least once for this window (and can only be skipped if the user was ok with closing the window).

Comment 35

[Boris Zbarsky [:bzbarsky]]

> Does this warrant recursing?

I don't think so.  I think it should be made very clear (via API documentation) that calling restoreCloseWindow on a document viewer you did not yourself call permitUnload(true) on is wrong.

Can I see a patch without recursion on mCallerIsClosingWindow (which will have the salutary effect of being a lot more readable, if you do it with an early return)?

Comment 36

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 3.2

Removed recursion when mCallerIsClosingWindow. Also added some API documentation, with a note about when it is appropriate to call resetCloseWindow.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 405359 [details] [diff] [review]
patch ver. 3.2

I guess the document viewer parts look ok, but I'd like jst to look over them too.  And you need someone to review the browser/ changes too.

Comment 38

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 3.3

Small update to the pb code to handle cases where transitioning in/out of pb mode throws an error and the switch is aborted. In that case restoreCloseWindow must be called on the windows we've called permitUnload(true) on.

ehsan, r? for the pb stuff.

jst, this already has r+ from bz, he wanted another from you for the document viewer stuff.

Comment 39

[Johnny Stenback (:jst)]

Comment on attachment 405888 [details] [diff] [review]
patch ver. 3.3

Looks good to me, r=jst

Comment 40

[Nochum Sossonko [:Natch]]

Comment on attachment 405888 [details] [diff] [review]
patch ver. 3.3

Ok, there's a problem with this approach, specifically not recursing through all the child docShells when mCallerIsClosingWindow is true, in that if a new load is started in a child docShell resetting mCallerIsClosingWindow in PrepareToStartLoad doesn't help since the parent has mCallerIsClosingWindow set to true already.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Why is that a problem?  What behavior do you expect in that case?

Comment 42

[Nochum Sossonko [:Natch]]

Well, in this particular case (for pb) once permitUnload(true) call succeeds that same window will never receive another prompt, since the window itself isn't closed, only child docShells are created and/or changed (as in tabs added/removed or tab contents modified). What I'm thinking of now is to simply walk up the chain of parents in PrepareToStartLoad and call ResetCloseWindow on all the parents. That is, if you'd prefer that to just recursing in PermitUnload.

Comment 43

[Boris Zbarsky [:bzbarsky]]

> once permitUnload(true) call succeeds that same window will never receive
> another prompt

Isn't that the desired behavior?

Comment 44

[Nochum Sossonko [:Natch]]

Yes, it is. The problem is really with the pb code, calling permitUnload(true) on the root nsIXULWindow's docShell while it has no intention of actually closing it. I'll have to patch the front-end of things for this.

Comment 45

[Dão Gottwald [:dao]]

Comment on attachment 405888 [details] [diff] [review]
patch ver. 3.3

>+    while(windowsEnum.hasMoreElements()) {

while (

>+      if (!contentViewer.permitUnload(true))
>+        throw Cr.NS_ERROR_ABORT;

Why don't you return false?

>+      continue;

You're already at the end of the loop.

Comment 46

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 3.4

This just resets the permitUnload on the last window since pb is the last to touch it (and still needs to skip permitUnload until it's done).

The reason I'm throwing is so that I can handle restoring the windows all in one place, if pb code actually throws (i.e. one of the observers, etc.), this needs to be reset anyways.

Comment 47

[Dão Gottwald [:dao]]

You should name the method differently and get rid of the return value then. For a method that's named "_canCloseBrowserWindows", finding a window that cannot be closed would be an expected result rather than an exception.

Comment 48

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 3.5

Dao, is the new name ok?

Also changed the last window cleaning to be a bit more consistent.

Comment 49

[Dão Gottwald [:dao]]

Looks better to me. There's still a stale "return true;".

Comment 50

[(no longer active)]

Comment on attachment 406119 [details] [diff] [review]
patch ver. 3.5

>diff --git a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
>--- a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
>+++ b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
>@@ -119,16 +119,18 @@ PrivateBrowsingService.prototype = {
>   _alreadyChangingMode: false,
> 
>   // Whether the private browsing mode has been started automatically (ie. always-on)
>   _autoStarted: false,
> 
>   // List of view source window URIs for restoring later
>   _viewSrcURLs: [],
> 
>+  _closedWindows: [],

This name is misleading, try something like |_windowsToClose|.  Also, please add a comment line above the variable explianing why you are using it.

>+  _ensureCanCloseWindows: function PBS__ensureCanCloseWindows() {
>+    let windowMediator = Cc["@mozilla.org/appshell/window-mediator;1"].
>+                         getService(Ci.nsIWindowMediator);
>+    let windowsEnum = windowMediator.getXULWindowEnumerator("navigator:browser");
>+
>+    while (windowsEnum.hasMoreElements()) {
>+      this._closedWindows.push(windowsEnum.getNext().QueryInterface(Ci.nsIXULWindow));
>+      let contentViewer =
>+        this._closedWindows[this._closedWindows.length - 1].docShell.contentViewer;
>+      if (!contentViewer.permitUnload(true))
>+        throw Cr.NS_ERROR_ABORT;
>+    }

I find this block kind of odd.  I'd prefer storing the XUL window object in a local variable, and push it onto the array when permitUnload returns true.  Something like:

  while (windowsEnum.hasMoreElements()) {
    let win = windowsEnum.getNext().QueryInterface(Ci.nsIXULWindow);
    if (win.docShell.contentViewer.permitUnload(true))
      this._windowsToClose.push(win);
    throw Cr.NS_ERROR_ABORT;
  }

>+    return true;

Please get rid of the return statement here.  This method doesn't need to return anything.

>+  },

>@@ -417,19 +437,28 @@ PrivateBrowsingService.prototype = {
>-      Cu.reportError("Exception thrown while processing the " +
>-        "private browsing mode change request: " + ex.toString());
>+      for (let i = 0; i < this._closedWindows.length - 1; i++)
>+        this._closedWindows[i].docShell.contentViewer.resetCloseWindow();
>+      if (ex != Cr.NS_ERROR_ABORT)
>+        Cu.reportError("Exception thrown while processing the " +
>+          "private browsing mode change request: " + ex.toString());

Please add a comment here explaining what this piece of code does.

Comment 51

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 3.6

Update to all the comments, plus an issue of my own that I realized (hence the new function for getting the xul window of the browser).

getMostRecentWindow doesn't match (at least not always) the last window that's returned from getEnumerator (or getXULEnumerator). Therefore, doing the job in the finally clause doesn't really work, we have to get the actual xul window of the current browser fresh from the service (getMostRecentWindow is supposedly the same window as getZOrderDOMWindowEnumerator("navigator:browser", true) and either way there's only one window at the time).

If you want I can NS_ASSERT that there's only one window (although IMO NS_ASSERT shouldn't be used at all since it has an effect on release builds).

Comment 52

[Dão Gottwald [:dao]]

Comment on attachment 406923 [details] [diff] [review]
patch ver. 3.6

>           // just in case the only remaining window after setBrowserState is different.
>           // it probably shouldn't be with the current sessionstore impl, but we shouldn't
>           // rely on behaviour the API doesn't guarantee
>           let browser = this._getBrowserWindow().gBrowser;
> 
>           // this ensures a clean slate from which to transition into or out of
>           // private browsing
>           browser.addTab();
>+          browser.getBrowserForTab(browser.tabContainer.firstChild).stop();
>           browser.removeTab(browser.tabContainer.firstChild);
>+          let xulWindow = this._getXULBrowserWindow();
>+          if (xulWindow)
>+            xulWindow.docShell.contentViewer.resetCloseWindow();

I don't understand your previous comment. How can this._getBrowserWindow() and this._getXULBrowserWindow() be different if there's only one window?

Comment 53

[Nochum Sossonko [:Natch]]

They're not _really_ different windows, one is an nsIDOMWindow while the other is an nsIXULWindow, depends what method you use to get the window. nsIXULWindow exposes the docShell which exposes the contenViewer, nsIDOMWindow doesn't (it's not part of the dom, really).

My comment was reflecting the previous approach of just using the last window in this._closedWindows, it's incorrect.

Comment 54

[Dão Gottwald [:dao]]

Why don't you use QI on the _getBrowserWindow return value?

Comment 55

[Nochum Sossonko [:Natch]]

nsIDOMWindows don't QI to nsIXULWindows. At least I don't know how to get one (an nsIXULWindow) from an nsIDOMWindow, and I'm pretty sure it isn't possible. it's really a different entity, nsIDOMWindow is the dom, while nsIXULWindow is more of a "native" window representation.

Comment 56

[Dão Gottwald [:dao]]

This works for me:

domwindow.getInterface(Ci.nsIWebNavigation)
         .QueryInterface(Ci.nsIDocShellTreeItem)
         .treeOwner
         .QueryInterface(Ci.nsIInterfaceRequestor)
         .getInterface(Ci.nsIXULWindow)
         .docShell

Comment 57

[Nochum Sossonko [:Natch]]

Cool, I'll repost just using that for the last window. Thanks.

Comment 58

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 3.7

Comment 59

[(no longer active)]

Comment on attachment 406944 [details] [diff] [review]
patch ver. 3.7

Thanks for your work on this!  r=me on the privatebrowsing parts.

Comment 60

[Nochum Sossonko [:Natch]]

Thanks all.

Comment 61

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/68490d9daf23

Comment 62

[(no longer active)]

Henrik, do you think we can get a Mozmill or Litmus test for this?  (Not sure if I need to toggle in-testsuite for Mozmill tests or not...)

Comment 63

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #62)
> Henrik, do you think we can get a Mozmill or Litmus test for this?  (Not sure
> if I need to toggle in-testsuite for Mozmill tests or not...)

Why can't we have browser chrome test here? We have a testcase which cannot be used for? When I have to create a Mozmill test we should first add this test to Litmus. We should consider first what is necessary.

Comment 64

[(no longer active)]

Actually, I think that we can have a browser chrome test here.  Here is how the test should work:

1. It should open a new tab with the URL in the URL field of this bug.
2. It should start private browsing mode.
   a. It should expect a dialog asking if it is OK to leave the current page.
   b. It should click OK on this dialog.
   c. It should verify that no other such dialogs get opened.
   d. It should verify that the private browsing mode actually starts.
3. It should quit the private browsing mode.
4. It should start private browsing mode again.
   a. It should expect the same dialog.
   b. It should click Cancel this time.
   c. It should verify that no other such dialogs get opened.
   d. It should verify that the private browsing mode does not get started.

Natch, would you mind writing the test?

Comment 65

[Nochum Sossonko [:Natch]]

I tried writing tests when I put the patch together, I just wasn't able to dismiss the dialog. Scripts are stopped because it's modal, and there is no "domwindowopened" notification for the modal dialog...

Comment 66

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Nochum, so we would definitely need a Mozmill test for this regression?

Comment 67

[Nochum Sossonko [:Natch]]

Well, unless Ehsan knows of a way to do this. I guess we should wait for his input...

Comment 68

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Do you still have this unfinished test laying around? It would help when you could attach it to this bug meanwhile.

Comment 69

[(no longer active)]

(In reply to comment #67)
> Well, unless Ehsan knows of a way to do this. I guess we should wait for his
> input...

I know of another trick which can come handy here.  You can basically replace the prompt service on your own object which implements nsIPromptService, so that no actual dialogs get opened, only methods on your own object would be called.

See this test <http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/unit/test_bug299716.js> for an example where exactly this trick is used for the prompt service, and also <http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/unit/test_privatebrowsing_downloadLastDir.js> for a sample where this trick is used for the file picker component.

Let me know if this helps you.

Comment 70

[Nochum Sossonko [:Natch]]

Attachment: test - not working

This is what I had going before, with the addition of the promptService override. It isn't working for me, my confirm method isn't being called. It seems to me that nothing is being called because it isn't generating confirm modal dialogs either, so I'm not really sure how to proceed. Help would be appreciated.

Comment 71

[(no longer active)]

Attachment: test - half working

This test passes when run standalone, but fails strangely when run with the other PB tests.  Natch said on IRC that he can take a look at it, so I'm attaching it here for him to pick it up.

Comment 72

[Nochum Sossonko [:Natch]]

I'm gonna look into this over the weekend...

Comment 73

[Nochum Sossonko [:Natch]]

Attachment: test and pb fix

So the failure was because I completely forgot about the keep_current_session pref. We have to bail on the permitUnload calls since we're not planning on closing any windows. All tests pass with this fix.

Comment 74

[Nochum Sossonko [:Natch]]

This is a glaring bug for users that happen to transition private browsing modes on 1.9.1. I haven't yet tried this particular patch on 1.9.1, but I'll post one if it is deemed worthy.

Comment 75

[(no longer active)]

Comment on attachment 408275 [details] [diff] [review]
test and pb fix

>diff --git a/browser/components/privatebrowsing/test/browser/Makefile.in b/browser/components/privatebrowsing/test/browser/Makefile.in
>-		browser_console_clear.js \
>+    browser_console_clear.js \
>+    browser_privatebrowsing_beforeunload.js \

Nit: use tabs please (yes, that's half my fault!).

r=me with that change.

Comment 76

[(no longer active)]

Comment on attachment 408275 [details] [diff] [review]
test and pb fix

Should have been r+...

Comment 77

[Nochum Sossonko [:Natch]]

Attachment: fix tabs

Comment 78

[(no longer active)]

(In reply to comment #74)
> This is a glaring bug for users that happen to transition private browsing
> modes on 1.9.1. I haven't yet tried this particular patch on 1.9.1, but I'll
> post one if it is deemed worthy.

Honestly, I'm convinced that this bug is serious, but given the eminent release of 3.6 (hopefully), I'm wonder if this is worth taking for 1.9.1.  Also, the fact that this patch includes interface changes doesn't help much.

I'll leave the decision to the drivers, though.

Comment 79

[Dão Gottwald [:dao]]

The test is broken, causes browser_bug400731.js and others to fail and/or time out.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1256828689.1256831410.1045.gz

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1256827917.1256830004.16566.gz

Comment 80

[Nochum Sossonko [:Natch]]

I don't even see how this is _possible_ but I'm looking into it. Were there errors on WINNT too, or is this my lucky day?

Comment 81

[Nochum Sossonko [:Natch]]

Oh, obviously the reason is because my assumption about nsIComponentRegistrar were absolutely false. If we unregister our factory, "@mozilla.org/embedcomp/prompt-service;1" ceases to exist, it doesn't fall back to the old prompt service. Unfortunately, I'm not sure how to do this correctly, so for the time being I think the test should be left out. I'll post a patch with just the fix for this issue. See the logs, which contain e.g. [JavaScript Error: "Components.classes['@mozilla.org/embedcomp/prompt-service;1'] is undefined" {file: "chrome://passwordmgr/content/passwordManager.js" line: 149}], and other strange js errors. I'll throw the new patch to try once I have it ready.

Comment 82

[Nochum Sossonko [:Natch]]

Attachment: new test

Test remains the same, except for the fix in bug 525300, which now allows us to use an observer instead.

Sending this to try, will post results.

Comment 83

[Nochum Sossonko [:Natch]]

Try results:

Linux: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1256850394.1256859872.5378.gz&fulltext=1 (success)

Mac: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1256850389.1256860224.9537.gz&fulltext=1 (success)

WINNT: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1256850388.1256860457.11879.gz&fulltext=1 (success, modulo two unrelated hangs)

The Windows stuff is weird, those tests have nothing to do with the code that's being changed, so I'm marking them off as "random" since they aren't test failures (or even test time outs, it seems) but application hangs.

Comment 84

[(no longer active)]

I think that bug 525300 is a much better solution, but for the record, this test could just restore the original factory once it's done like <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_bug356571.js#79>.

Comment 85

[(no longer active)]

Test landed as http://hg.mozilla.org/mozilla-central/rev/d3dc1d0d6359.

Comment 86

[(no longer active)]

Landed both chunks on 1.9.2 as <http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7e11e26a8155>.  Thanks a lot for working on this, Natch!

Comment 87

[Nochum Sossonko [:Natch]]

Attachment: 1.9.1 patch

All-inclusive patch including the patch for bug 525300 (needed for the test that landed in this bug) and the patches in this bug. Also added the _MOZILLA_191_BRANCH to the new nsIContentViewer iface.

The interface additions does complicate things somewhat, so I'll leave up to the drivers to decide if it's worth it to take this change on that branch.

Comment 88

[Samuel Sidler (old account; do not CC)]

Comment on attachment 409729 [details] [diff] [review]
1.9.1 patch

Not for 1.9.1.5.

Comment 89

[Nochum Sossonko [:Natch]]

The new 1.9.1 patch probably needs a quick re-review by bz, but I'd rather not bother him if ultimately it won't be accepted. Is there any way I can find that out?

Comment 90

[(no longer active)]

(In reply to comment #89)
> The new 1.9.1 patch probably needs a quick re-review by bz, but I'd rather not
> bother him if ultimately it won't be accepted. Is there any way I can find that
> out?

You probably need to ping one of the drivers on IRC and ask them if they consider such a change acceptable for the stable branch.

Comment 91

[Samuel Sidler (old account; do not CC)]

(In reply to comment #89)
> The new 1.9.1 patch probably needs a quick re-review by bz, but I'd rather not
> bother him if ultimately it won't be accepted. Is there any way I can find that
> out?

We'll accept this patch after it gets review from Boris and gets sufficient baking. Please nominate for 1.9.1.7 then. We'd like to take it early in the cycle of that release. Thanks for checking. :)

Comment 92

[(no longer active)]

Comment on attachment 409729 [details] [diff] [review]
1.9.1 patch

Requesting review on behalf of Natch.

Comment 93

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified on trunk and 1.9.2 with builds on OS X and Windows like:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091108 Minefield/3.7a1pre ID:20091108030811

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b2pre) Gecko/20091108 Namoroka/3.6b2pre ID:20091108033832

Comment 94

[Nochum Sossonko [:Natch]]

bz: review ping?

Comment 95

[Boris Zbarsky [:bzbarsky]]

Er, right.  So how does this 1.9.1 patch differ from what I already reviewed, and which parts of it do you want me to re-review?

Comment 96

[Nochum Sossonko [:Natch]]

Just the DocumentViewerImpl parts, the difference is that we can't change interfaces on 1.9.1 so I had to create another one, then I had to change the BinaryName because of namespace issues as well as make the new iface not inherit from the old iface, because of naming issues.

Comment 97

[Nochum Sossonko [:Natch]]

bz: is comment 96 enough? If it's not something you really need to review, I can just request approval, just wanted to be sure.

Comment 98

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 409729 [details] [diff] [review]
1.9.1 patch

As it happens, I actually read this through carefully earlier today....

>+++ b/docshell/base/nsIContentViewer.idl
>+  [binaryname(MozPermitUnload)] boolean permitUnload([optional] in boolean aCallerClosesWindow);

Hmm.  So what actually guarantees that this is callable from JS?  It seems like
it'd end up depending on the order in which QIs happen or something (hence be fragile).

I would prefer just using a different name for this method.

With that changed, r=bzbarsky.

Comment 99

[Nochum Sossonko [:Natch]]

Attachment: 1.9.1 patch - comment addressed

Carrying forward bz's review. This is ready to land, pending approval.

Comment 100

[Daniel Veditz [:dveditz]]

Comment on attachment 418788 [details] [diff] [review]
1.9.1 patch - comment addressed

Approved for 1.9.1.8 if you can land over the weekend, a=dveditz for release-drivers

Next week we're going to be removing approvals for unlanded non-blockers.

Comment 101

[(no longer active)]

Landed on 1.9.1: <http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6e60470b04ab>

Comment 102

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on 1.9.1 with builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.8pre) Gecko/20100125 Shiretoko/3.5.8pre ID:20100125030736

Comment 103

[Daniel Holbert [:dholbert]]

This appears to have made the Firefox 3.5 tinderboxen perma-orange, with timeouts & failures in privatebrowsing tests, ever since it landed -- see bug 543071.

This needs a backout or an orangeness fix ASAP.

Comment 104

[(no longer active)]

(In reply to comment #103)
> This appears to have made the Firefox 3.5 tinderboxen perma-orange, with
> timeouts & failures in privatebrowsing tests, ever since it landed -- see bug
> 543071.
> 
> This needs a backout or an orangeness fix ASAP.

Fix landed as http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6f4f1c26308a.