495495 - Entering Private Browsing mode prohibits keyboard focus on URL location bar

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Aaron Train [:aaronmt]]

Attempted to browse my college's student information and registration web site today. This system is on a secure server and requires each student to sign in with proper credentials. Upon signing in to the system, the system spawns a new browser window with available options for a student to select. Upon signing in to the system I entered private browsing mode and was surprised to see that I was from then on forth unable to set keyboard focus in the URL location bar. 

Exiting private browsing mode continues to prohibit access from focusing the URL location bar.

Clicking the URL location bar, CMD-L, File -> Open Location all do not work.

Using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090529 Shiretoko/3.5pre as well as latest trunk 

Steps to reproduce (

1. Go to https://siris.senecac.on.ca
2. Sign in
3. In the new window, enter PB mode

Expected results: Can set focus on the URL location bar
Actual results: Can not set focus on the URL location bar

Unfortunately, to test this I need help from other students - will create a screencast too

Comment 1

[(no longer active)]

Do you see anything on the error console?

I'm trying to understand what the significance of logging in to the siris site is.  Is it that it opens a new window?  If you close its new window, does the same problem happen again?

Comment 2

[John Ford [:jhford] CET/CEST Berlin Time]

i don't have a URL bar in the popup at all.  I am on 3.5B4

Ehsan, if you enter a wrong password there is still a popup

Comment 3

[(no longer active)]

Hmm, so when you switch in to the private browsing mode, the window without the url bar remains open and the original window is closed?

Comment 4

[Aaron Train [:aaronmt]]

There is nothing in the error console. I am uploading a screencast.

Comment 5

[Aaron Train [:aaronmt]]

Attachment: SWF Screencast

Comment 6

[(no longer active)]

From the screencast, I think what happens is comment 3.  Am I right?

Comment 7

[Aaron Train [:aaronmt]]

It certainly feel's like it's something related to opening a new window. If I
close the window (with the student options), I have no access to them and am
required to log in again. As for testing that, I signed in (window opened),
entered PB mode on the login page and witnessed no issues.

Comment 8

[(no longer active)]

OK, I reproduced it on Windows.  The URL bar is disabled on the popped up window.  Actually it's a lot more obvious on Windows since the URL bar would be gray.

I think this should block, because if a user ends up in this state, they are pretty much unable to do anything, let alone close down Firefox and re-open it.

Comment 9

[Aaron Train [:aaronmt]]

(In reply to comment #3)
> Hmm, so when you switch in to the private browsing mode, the window without the
> url bar remains open and the original window is closed?

When I switch into PB mode on my shiretoko nightly, the window does have a url bar - and is the focused window on entry into PB. This window I can not type in. When I exit PB mode, my browser is returned to a state where I can not type or set focus in the url bar at all - thus requires me to shutdown the browser.

Comment 10

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

Connor tells me that this is due to an interaction with Weave? Renominate if someone without Weave can reproduce ...

Comment 11

[Aaron Train [:aaronmt]]

Profile I tested on has no weave addon nor any addon installed.

Comment 12

[Aaron Train [:aaronmt]]

Tested on XP. Back in the primary window after exiting PB mode I opened up the error console.

Error: this.windowToFocus.content is null
Source File: file:///C:/Documents%20and%20Settings/Aaron/Desktop/firefox/components/nsSessionStore.js
Line: 2331

Related?

Comment 13

[(no longer active)]

(In reply to comment #10)
> Connor tells me that this is due to an interaction with Weave? Renominate if
> someone without Weave can reproduce ...

Yeah, I reproduced this on a new profile.  It has nothing to do with Weave.

Comment 14

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

Talked with Connor on IRC, and broke it down this way:

If you focus a popup window and then enter Private Browsing Mode, the URL in that popup window will not be editable/usable. The URL bar in new windows, however, will be fine. Further, upon exiting PB, the URL bar in the popup will continue to not be usable.

While this sucks, and warrants a relnote, it won't block. Would take a safe, tested patch, maybe, though likely only as a ride-along for a future RC.

Also marking wanted1.9.1.x+

Comment 15

[Dietrich Ayala (:dietrich)]

mconnor said removing the readonly attribute from aWindow.gURLBar might fix it. ehsan, do you know what disabled the urlbar in the first place?

Comment 16

[Simon Bünzli]

Related to bug 454047?

Comment 17

[Aaron Train [:aaronmt]]

(In reply to comment #16)
> Related to bug 454047?

That bug has a great simple test in the posted URL. 

http://popuptest.com/goodpopups.html

1. Click Good PopUp #3
2. Enter PB mode in the new window
3. Verify that you cant type in the URL location bar
4. Exit PB mode in the new window
5. Returned to primary browser window
6. Verify that you cant set focus to the URL location bar and are prohibited from typing.

You have to restart the browser

Comment 18

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

<gavin> mconnor: I guess we should just re-run http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1080 from setBrowserState for now?
<gavin> that'd be pretty safe, though not pretty, I think
<gavin> and perhaps we should audit the session store code for other such gotchas - I don't see any other relevant chromehidden referecnes in browser.js
<mconnor> gavin: you mean reverse, right? :)
<gavin> mconnor: well, we want to add it if we're turning the window into a popup, don't we?
<gavin> but also remove it when we're not, yeah
<mconnor> gavin: is there code where we turn a window into a popup?
<gavin> mconnor: I'm assuming http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#2269 is used both ways
<gavin> that seems like the place we'd want to put it
<mconnor> gavin: mmm, possibly
<zpao> yea, that's used both ways. we'd have to store that in the window state though (probably in _updateWindowState http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#1577)
<zpao> or rather _updateWindowFeatures
<gavin> why?
<gavin> can't it be based on isPopup?
<zpao> well, is the urlbar readonly in all popups?
<zpao> if so, yea that should be fine. but if not, then we should probably store the readonly setting  if we want the window to be in the same state after restoration.
<gavin> good point

Comment 19

[(no longer active)]

Attachment: Patch (v1)

This patch fixes the session store API to save and restore the read-only and autocomplete states for the URL bar.

The patch contains two test cases: one for testing to make sure setBrowserState can correctly set the status of the URL bar, and one for testing the original problem as reported in comment 0.  Of course ideally for the second test case we would open a popup window and enter PB mode from that window, but browser chrome tests couldn't handle that, unfortunately.

Requesting review from Simon on the sessionstore part, and from mconnor on the privatebrowsing test case.

Comment 20

[Simon Bünzli]

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

>     if (aWinData.isPopup)
>       this._windows[aWindow.__SSi].isPopup = true;
>     else
>       delete this._windows[aWindow.__SSi].isPopup;
>-    
>+
>+    if (aWindow.gURLBar) {
>+      if ("urlbar_readonly" in aWinData)
>+        aWindow.gURLBar.readOnly = aWinData.urlbar_readonly;
>+      else
>+        aWindow.gURLBar.readOnly = false;
>+      aWindow.gURLBar.setAttribute("enablehistory", aWinData.urlbar_enablehistory || "true");
>+    }

BrowserStartup disables the address bar, when the toolbars are hidden. This is equivalent to a window having set isPopup inside nsSessionStore. Instead of saving additional attributes, just set both readOnly and "enablehistory" depending on whether isPopup was set right above.

Comment 21

[(no longer active)]

Attachment: Patch (v2)

(In reply to comment #20)
> (From update of attachment 380708 [details] [diff] [review])
> >     if (aWinData.isPopup)
> >       this._windows[aWindow.__SSi].isPopup = true;
> >     else
> >       delete this._windows[aWindow.__SSi].isPopup;
> >-    
> >+
> >+    if (aWindow.gURLBar) {
> >+      if ("urlbar_readonly" in aWinData)
> >+        aWindow.gURLBar.readOnly = aWinData.urlbar_readonly;
> >+      else
> >+        aWindow.gURLBar.readOnly = false;
> >+      aWindow.gURLBar.setAttribute("enablehistory", aWinData.urlbar_enablehistory || "true");
> >+    }
> 
> BrowserStartup disables the address bar, when the toolbars are hidden. This is
> equivalent to a window having set isPopup inside nsSessionStore. Instead of
> saving additional attributes, just set both readOnly and "enablehistory"
> depending on whether isPopup was set right above.

You're right, that way the code would be a lot cleaner.  Unfortunately with this change, testing the privatebrowsing part is no longer possible since session store only writes isPopup when a new window is loaded, which brings us to the same limitation I mentioned in comment 19.  So, a litmus test would also be necessary here.

Comment 22

[(no longer active)]

Try server builds available at: <https://build.mozilla.org/tryserver-builds/[e-mail]-pbfrompopup/>.

The unit test passed on all platforms.

Comment 23

[Simon Bünzli]

Comment on attachment 380717 [details] [diff] [review]
Patch (v2)

>+              ss.setWindowState(win, state, true);
>+              is(win.gURLBar.readOnly, expected.readOnly,
>+                "URL bar read-only state should be restored correctly");
>+              is(win.gURLBar.getAttribute("enablehistory"), expected.enablehistory,
>+                "URL bar autocomplete state should be restored correctly");

Just to be thorough, please verify that these conditions aren't true before setting the window state.

(In reply to comment #21)
> Unfortunately with this change, testing the privatebrowsing part
> is no longer possible since session store only writes isPopup
> when a new window is loaded

... or when a new window state is set. You should thus be able to set the window used for testing to be in a popup state, then change into Private Browsing mode (letting the current window state be overwritten), ensure that the location bar is usable, change back and verify that the window is a popup again, before re-setting the window to the state it was in at the beginning of the test.

Comment 24

[(no longer active)]

Attachment: Patch (v3)

(In reply to comment #23)
> (From update of attachment 380717 [details] [diff] [review])
> >+              ss.setWindowState(win, state, true);
> >+              is(win.gURLBar.readOnly, expected.readOnly,
> >+                "URL bar read-only state should be restored correctly");
> >+              is(win.gURLBar.getAttribute("enablehistory"), expected.enablehistory,
> >+                "URL bar autocomplete state should be restored correctly");
> 
> Just to be thorough, please verify that these conditions aren't true before
> setting the window state.

Done.

> (In reply to comment #21)
> > Unfortunately with this change, testing the privatebrowsing part
> > is no longer possible since session store only writes isPopup
> > when a new window is loaded
> 
> ... or when a new window state is set. You should thus be able to set the
> window used for testing to be in a popup state, then change into Private
> Browsing mode (letting the current window state be overwritten), ensure that
> the location bar is usable, change back and verify that the window is a popup
> again, before re-setting the window to the state it was in at the beginning of
> the test.

Thanks for the tip; don't know why I didn't think of it myself.  So, here goes the privatebrowsing test once again!

Requesting review from mconnor on the privatebrowsing part, carrying over Simon's r+.

Comment 25

[Simon Bünzli]

Comment on attachment 380744 [details] [diff] [review]
Patch (v3)

>+  function pretendToBeAPopup(whatToPretend) {
>+    window.toolbar.visible = !whatToPretend;
>+    gURLBar.readOnly = whatToPretend;
>+    gURLBar.setAttribute("enablehistory", whatToPretend ? "false" : "true");
>+    ss.setWindowState(window, "{}", true);
>+  }

Won't setWindowState(window, "{}", true) return the window into a non-popup state (since |isPopup| isn't set in the new state)? Below, you only test that the window isn't a popup at certain points, however you never verify that it has ever been one previously. Please also do so.

Also: You should only need the setWindowState line, if you include the following in your window state: |"isPopup": true, "hidden": "toolbar"|.

Comment 26

[Simon Bünzli]

BTW: If you do another iteration, I'd also have a nit for you:

>+              is(win.gURLBar.readOnly, expected.readOnly,
>+                "URL bar read-only state should be restored correctly");

The quote should be aligned with |win| and not the opening parenthesis (i.e. please correctly align parameters).

Comment 27

[Mike Connor [:mconnor]]

Comment on attachment 380744 [details] [diff] [review]
Patch (v3)

r=me, only looked at the PB bits though

Comment 28

[(no longer active)]

(In reply to comment #25)
> (From update of attachment 380744 [details] [diff] [review])
> >+  function pretendToBeAPopup(whatToPretend) {
> >+    window.toolbar.visible = !whatToPretend;
> >+    gURLBar.readOnly = whatToPretend;
> >+    gURLBar.setAttribute("enablehistory", whatToPretend ? "false" : "true");
> >+    ss.setWindowState(window, "{}", true);
> >+  }
> 
> Won't setWindowState(window, "{}", true) return the window into a non-popup
> state (since |isPopup| isn't set in the new state)?

I only used this setWindowState call to trigger nsSessionStore.onLoad.  That method checks toolbar.visible on the window and sets isPopup accordingly.

> Below, you only test that
> the window isn't a popup at certain points, however you never verify that it
> has ever been one previously. Please also do so.

Sure, will do.

> Also: You should only need the setWindowState line, if you include the
> following in your window state: |"isPopup": true, "hidden": "toolbar"|.

But that won't make the location bar read-only, will it?

Comment 29

[Aaron Train [:aaronmt]]

Comment on attachment 380480 [details]
SWF Screencast

http://popuptest.com/goodpopups.html

Comment 30

[Simon Bünzli]

(In reply to comment #28)
> I only used this setWindowState call to trigger nsSessionStore.onLoad.

setWindowState only triggers onLoad, if SessionStore has never before seen the window you've passed in as first argument. This isn't the case in your test.

> But that won't make the location bar read-only, will it?

The code you're adding to nsSessionStore.js should make sure that it does (restoreWindowFeatures is called from restoreWindow which is called from setWindowState - and you update the address bar's readOnly state inside restoreWindowFeatures depending on isPopup).

Comment 31

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/171da87539a4

Comment 32

[(no longer active)]

I forgot to mention that I incorporated Simon's comments upon landing.

Comment 33

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

I've asked bz to back this out:
 - turned a test orange (he's got more details)
 - landed without pre-approval (you need that to land on mozilla-central, still)

Comment 34

[Boris Zbarsky [:bzbarsky]]

Backed out:  http://hg.mozilla.org/mozilla-central/rev/6a07f00fbfb2

This checkin looks like it caused this test failure on Linux:

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/shell/test/browser_420786.js | Timed out

It's been orange for three cycles now.  Ehsan starred the tree saying this is bug 483382, but that bug is about a different test causing this test to fail.  That different test was then disabled, and that bug is about reenabling said different test.  That is, this test is NOT random on m-c right now (and that bug has none of the "random orange" markers on it, fwiw).  So either this patch or the test in question is wrong, but they seem unable to coexist in the tree...

Comment 35

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

Comment on attachment 380744 [details] [diff] [review]
Patch (v3)

Don't think this'll make 3.5, let's try for 3.5.1

Comment 36

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

Is this still wanted for 3.5.x? Ehsan, can you figure out why this caused the tree to go orange and get a fix landed for trunk?

Comment 37

[(no longer active)]

browser_420786.js fails for me all the time.  I filed bug 506452 about this.  Once I figure that failure out, I can investigate the issue with this test.

Comment 38

[(no longer active)]

Attachment: Linux test failure fixed

This new patch fixes the browser_420786.js test failure on Linux using the technique suggested in bug 483382.  I am putting the full patch up for Simon's review, but the only changed part (besides the unbitrotting) is:

diff --git a/browser/components/sessionstore/test/browser/browser_495495.js b/browser/components/sessionstore/test/browser/browser_495495.js
--- a/browser/components/sessionstore/test/browser/browser_495495.js
+++ b/browser/components/sessionstore/test/browser/browser_495495.js
@@ -78,7 +78,9 @@ function test() {
           }
 
           testState(state1, {readOnly: false, enablehistory: "true"}, function() {
-            testState(state2, {readOnly: true, enablehistory: "false"}, finish);
+            testState(state2, {readOnly: true, enablehistory: "false"}, function() {
+              executeSoon(finish);
+            });
           });
         });
       }, false);

Comment 39

[Simon Bünzli]

Comment on attachment 390663 [details] [diff] [review]
Linux test failure fixed

Sure. One unrelated nit: you're indenting with spaces instead of tabs in the first Makefile.in.

Comment 40

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/9b8f46d71e4e

Comment 41

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

Comment on attachment 390663 [details] [diff] [review]
Linux test failure fixed

Approved for 1.9.1.2. a=ss for release-drivers

Please land on mozilla-1.9.1 then use the ".2-fixed" option of the "status1.9.1" flag to indicate this has landed.

Comment 42

[(no longer active)]

Landed on 1.9.1.2: <http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8cbe12f3a208>

Comment 43

[Aaron Train [:aaronmt]]

Verified FIXED on 1.9.1.2

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5

Comment 44

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

Aaron: You did not verify this on trunk, so the status should not be VERIFIED. You did verify this on branch, so the keyword should be verified1.9.1.

Comment 45

[Dão Gottwald [:dao]]

The keyword for this would be verified1.9.1.2 traditionally. The status1.9.1 flag thingy lacks support for that.

Comment 46

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

See also: http://groups.google.com/group/mozilla.dev.planning/browse_frm/thread/0f439307e9db1f6b#