Closed
Bug 498648
Opened 16 years ago
Closed 15 years ago
Start private browsing while editing a message, cancel, doesn't cancel private browsing
Categories
(Firefox :: Private Browsing, defect, P2)
Firefox
Private Browsing
Tracking
()
VERIFIED
FIXED
Firefox 3.7a1
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta2-fixed |
status1.9.1 | --- | .8-fixed |
People
(Reporter: theosib, Assigned: Natch)
References
()
Details
(6 keywords)
Attachments
(4 files, 18 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Natch
:
review+
dveditz
:
approval1.9.1.8+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•16 years ago
|
||
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!
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-firefox3.6?
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Assignee | ||
Comment 2•16 years ago
|
||
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...
Flags: wanted1.9.1.x?
Assignee | ||
Comment 3•16 years ago
|
||
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.
Reporter | ||
Updated•16 years ago
|
Summary: Start private browsing while exiting a message, cancel, doesn't cancel private browsing → Start private browsing while editing a message, cancel, doesn't cancel private browsing
Reporter | ||
Comment 4•16 years ago
|
||
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•16 years ago
|
||
(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.
Reporter | ||
Comment 6•16 years ago
|
||
For me, it was Hotmail and Yahoo mail, which are already known problems. Thanks.
Comment 7•15 years ago
|
||
(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...
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Updated•15 years ago
|
status1.9.1:
--- → ?
Flags: wanted1.9.1.x?
Assignee | ||
Comment 8•15 years ago
|
||
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•15 years ago
|
||
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.
Flags: blocking-firefox3.6? → blocking-firefox3.6+
Updated•15 years ago
|
Priority: -- → P2
Assignee | ||
Comment 10•15 years ago
|
||
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...
Assignee | ||
Comment 11•15 years ago
|
||
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.
Assignee: ehsan.akhgari → highmind63
Attachment #402542 -
Attachment is obsolete: true
Attachment #402711 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•15 years ago
|
Attachment #402711 -
Attachment is obsolete: true
Attachment #402711 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 12•15 years ago
|
||
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.
Assignee | ||
Comment 13•15 years ago
|
||
Wait for "private-browsing-change-granted" before setting sPrivateBrowsingChanging.
Attachment #402717 -
Flags: review?(bzbarsky)
![]() |
||
Comment 14•15 years ago
|
||
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.
Attachment #402717 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 15•15 years ago
|
||
(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•15 years ago
|
||
> 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.
Assignee | ||
Comment 17•15 years ago
|
||
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.
Attachment #402717 -
Attachment is obsolete: true
![]() |
||
Comment 18•15 years ago
|
||
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.
Assignee | ||
Comment 19•15 years ago
|
||
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.
Attachment #402900 -
Attachment is obsolete: true
![]() |
||
Comment 20•15 years ago
|
||
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?
Assignee | ||
Comment 21•15 years ago
|
||
(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
Assignee | ||
Comment 22•15 years ago
|
||
Going to look into this now...
Attachment #402914 -
Attachment is obsolete: true
Assignee | ||
Comment 23•15 years ago
|
||
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.
Assignee | ||
Comment 24•15 years ago
|
||
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•15 years ago
|
||
(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 :) )
Assignee | ||
Comment 26•15 years ago
|
||
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.
Attachment #404302 -
Attachment is obsolete: true
Attachment #404859 -
Flags: review?(bzbarsky)
![]() |
||
Comment 27•15 years ago
|
||
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•15 years ago
|
||
And note that this is likely to be a problem with any patch that wants to store state in the document viewer....
Assignee | ||
Comment 29•15 years ago
|
||
(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•15 years ago
|
||
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•15 years ago
|
||
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.
Assignee | ||
Comment 32•15 years ago
|
||
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.
Attachment #404859 -
Attachment is obsolete: true
Attachment #404996 -
Flags: review?(bzbarsky)
Attachment #404859 -
Flags: review?(bzbarsky)
![]() |
||
Comment 33•15 years ago
|
||
> 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?
Assignee | ||
Comment 34•15 years ago
|
||
(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•15 years ago
|
||
> 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)?
Assignee | ||
Comment 36•15 years ago
|
||
Removed recursion when mCallerIsClosingWindow. Also added some API documentation, with a note about when it is appropriate to call resetCloseWindow.
Attachment #404996 -
Attachment is obsolete: true
Attachment #405359 -
Flags: review?(bzbarsky)
Attachment #404996 -
Flags: review?(bzbarsky)
![]() |
||
Updated•15 years ago
|
Attachment #405359 -
Flags: review?(jst)
Attachment #405359 -
Flags: review?(bzbarsky)
Attachment #405359 -
Flags: review+
![]() |
||
Comment 37•15 years ago
|
||
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.
Assignee | ||
Comment 38•15 years ago
|
||
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.
Attachment #405359 -
Attachment is obsolete: true
Attachment #405888 -
Flags: review?(jst)
Attachment #405888 -
Flags: review?(ehsan.akhgari)
Attachment #405359 -
Flags: review?(jst)
Comment 39•15 years ago
|
||
Comment on attachment 405888 [details] [diff] [review] patch ver. 3.3 Looks good to me, r=jst
Attachment #405888 -
Flags: review?(jst) → review+
Assignee | ||
Comment 40•15 years ago
|
||
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.
Attachment #405888 -
Attachment is obsolete: true
Attachment #405888 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 42•15 years ago
|
||
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•15 years ago
|
||
> once permitUnload(true) call succeeds that same window will never receive
> another prompt
Isn't that the desired behavior?
Assignee | ||
Comment 44•15 years ago
|
||
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•15 years ago
|
||
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.
Assignee | ||
Comment 46•15 years ago
|
||
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.
Attachment #405939 -
Flags: review?(ehsan.akhgari)
Comment 47•15 years ago
|
||
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.
Assignee | ||
Comment 48•15 years ago
|
||
Dao, is the new name ok? Also changed the last window cleaning to be a bit more consistent.
Attachment #405939 -
Attachment is obsolete: true
Attachment #406119 -
Flags: review?(ehsan.akhgari)
Attachment #405939 -
Flags: review?(ehsan.akhgari)
Comment 50•15 years ago
|
||
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.
Attachment #406119 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 51•15 years ago
|
||
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).
Attachment #406119 -
Attachment is obsolete: true
Attachment #406923 -
Flags: review?(ehsan.akhgari)
Comment 52•15 years ago
|
||
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?
Assignee | ||
Comment 53•15 years ago
|
||
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.
Assignee | ||
Comment 55•15 years ago
|
||
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•15 years ago
|
||
This works for me: domwindow.getInterface(Ci.nsIWebNavigation) .QueryInterface(Ci.nsIDocShellTreeItem) .treeOwner .QueryInterface(Ci.nsIInterfaceRequestor) .getInterface(Ci.nsIXULWindow) .docShell
Assignee | ||
Comment 58•15 years ago
|
||
Attachment #406923 -
Attachment is obsolete: true
Attachment #406944 -
Flags: review?(ehsan.akhgari)
Attachment #406923 -
Flags: review?(ehsan.akhgari)
Comment 59•15 years ago
|
||
Comment on attachment 406944 [details] [diff] [review] patch ver. 3.7 Thanks for your work on this! r=me on the privatebrowsing parts.
Attachment #406944 -
Flags: review?(ehsan.akhgari) → review+
Comment 62•15 years ago
|
||
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...)
Flags: in-litmus?
Updated•15 years ago
|
Target Milestone: --- → Firefox 3.7a1
Comment 63•15 years ago
|
||
(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.
Flags: in-testsuite?
Comment 64•15 years ago
|
||
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?
Assignee | ||
Comment 65•15 years ago
|
||
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...
Assignee | ||
Comment 67•15 years ago
|
||
Well, unless Ehsan knows of a way to do this. I guess we should wait for his input...
Comment 68•15 years ago
|
||
Do you still have this unfinished test laying around? It would help when you could attach it to this bug meanwhile.
Comment 69•15 years ago
|
||
(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.
Assignee | ||
Comment 70•15 years ago
|
||
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•15 years ago
|
||
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.
Assignee | ||
Comment 73•15 years ago
|
||
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.
Attachment #407707 -
Attachment is obsolete: true
Attachment #408075 -
Attachment is obsolete: true
Attachment #408275 -
Flags: review?
Assignee | ||
Updated•15 years ago
|
Attachment #408275 -
Flags: review? → review?(ehsan)
Assignee | ||
Comment 74•15 years ago
|
||
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.
blocking1.9.1: --- → ?
Comment 75•15 years ago
|
||
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.
Attachment #408275 -
Flags: review?(ehsan) → review-
Comment 76•15 years ago
|
||
Comment on attachment 408275 [details] [diff] [review] test and pb fix Should have been r+...
Attachment #408275 -
Flags: review- → review+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: [c-n: test to m-c, patch and test on 1.9.2]
Comment 78•15 years ago
|
||
(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.
Keywords: checkin-needed
Whiteboard: [c-n: test to m-c, patch and test on 1.9.2]
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: [c-n: test to m-c, patch and test on 1.9.2]
Updated•15 years ago
|
blocking1.9.1: ? → ---
Comment 79•15 years ago
|
||
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
Assignee | ||
Comment 80•15 years ago
|
||
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?
Assignee | ||
Comment 81•15 years ago
|
||
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.
Assignee | ||
Comment 82•15 years ago
|
||
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.
Attachment #408463 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: [c-n: test to m-c, patch and test on 1.9.2]
Assignee | ||
Comment 83•15 years ago
|
||
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.
Keywords: checkin-needed
Whiteboard: [c-n: test to m-c, patch and test on 1.9.2] [requires bug 525300 first]
Comment 84•15 years ago
|
||
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>.
Updated•15 years ago
|
Attachment #409172 -
Flags: review+
Comment 85•15 years ago
|
||
Test landed as http://hg.mozilla.org/mozilla-central/rev/d3dc1d0d6359.
Keywords: checkin-needed
Whiteboard: [c-n: test to m-c, patch and test on 1.9.2] [requires bug 525300 first] → [c-n: patch and test on 1.9.2] [requires bug 525300 first]
Assignee | ||
Updated•15 years ago
|
Flags: in-testsuite? → in-testsuite+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 86•15 years ago
|
||
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!
status1.9.2:
--- → final-fixed
Flags: in-litmus?
Keywords: checkin-needed
Whiteboard: [c-n: patch and test on 1.9.2] [requires bug 525300 first]
Assignee | ||
Comment 87•15 years ago
|
||
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.
Attachment #409729 -
Flags: approval1.9.1.5?
Comment 88•15 years ago
|
||
Comment on attachment 409729 [details] [diff] [review] 1.9.1 patch Not for 1.9.1.5.
Attachment #409729 -
Flags: approval1.9.1.5? → approval1.9.1.6?
Assignee | ||
Comment 89•15 years ago
|
||
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•15 years ago
|
||
(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•15 years ago
|
||
(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. :)
Updated•15 years ago
|
Attachment #409729 -
Flags: approval1.9.1.6?
Comment 92•15 years ago
|
||
Comment on attachment 409729 [details] [diff] [review] 1.9.1 patch Requesting review on behalf of Natch.
Attachment #409729 -
Flags: review?(bzbarsky)
Comment 93•15 years ago
|
||
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
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2
Updated•15 years ago
|
Whiteboard: [needs r=bz]
![]() |
||
Comment 95•15 years ago
|
||
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?
Assignee | ||
Comment 96•15 years ago
|
||
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.
Assignee | ||
Comment 97•15 years ago
|
||
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•15 years ago
|
||
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.
Attachment #409729 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 99•15 years ago
|
||
Carrying forward bz's review. This is ready to land, pending approval.
Attachment #409729 -
Attachment is obsolete: true
Attachment #418788 -
Flags: review+
Attachment #418788 -
Flags: approval1.9.1.8?
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs r=bz]
Comment 100•15 years ago
|
||
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.
Attachment #418788 -
Flags: approval1.9.1.8? → approval1.9.1.8+
Comment 101•15 years ago
|
||
Landed on 1.9.1: <http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6e60470b04ab>
Comment 102•15 years ago
|
||
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
Keywords: verified1.9.1
Comment 103•15 years ago
|
||
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•15 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•