481687 - [Mac] Firefox Menu Listing does not show Addons, Downloads, Error Console Popup Windows in PB mode

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P1)

Description

[Marcia Knous [:marcia]]

Attachment: Screenshot of missing windows in File Menu

Seen while running  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3) Gecko/20090304 Firefox/3.1b3

STR:
1. New Profile, enter PB mode.
2. Open a few Windows including the Error Console, etc.
3. Observe Screenshot

Comment 1

[Marcia Knous [:marcia]]

The missing Windows come up when I mouse over the blank area. In this case they were the Addons Manager and the Error Console.

Comment 2

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

Same here with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3) Gecko/20090304 Firefox/3.1b3 ID:20090304214853

Comment 3

[Marcia Knous [:marcia]]

I checked 10.4 Tiger and can easily reproduce the issue, but only in PB mode.

Comment 4

[(no longer active)]

Does Error Console have to be open every time in order to reproduce this bug?  Do you see anything reported on the error console?

Comment 5

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

Ehsan, all of those listed windows have normally a window title. But switching into the Private Browsing mode the title is cleared. That's why no name is shown for them in the window menu.

Comment 6

[(no longer active)]

Hmm.  So, is anything reported to the error console?

Comment 7

[(no longer active)]

Also, does the same thing happen if those windows are already open when entering the PB mode?

Comment 8

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

No, nothing is shown in the Error Console. And it doesn't make a difference if the windows are open before or not. The window title gets removed either way.

Comment 9

[(no longer active)]

I'm not sure right now about the reason.  We don't do anything special to the title of those windows, and I don't have the hardware to test it.  mconnor, can you take a look, please?

Comment 10

[Mike Connor [:mconnor]]

Argh, this'll be the code in onEnterPrivateBrowsing, since it will run on these windows too.

Blocks final, will be trivial to fix, taking.

Comment 11

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

--> P1, as this bug will require the wider feedback of a beta release or is of sufficient complexity that we should be looking at it sooner, not later.

Comment 12

[Mike Connor [:mconnor]]

Attachment: only change the title for the main browser window

Really straightforward and safe.

Comment 13

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

Won't that patch cause the menu item to be disabled for non-browser windows when entering private browsing in the non-autostart case?

In other words, I think the window.location check should be inside the current if(!this._privateBrowsingAutoStarted), or the else should be split out into it's own if (this._privateBrowsingAutoStarted).

Comment 14

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

Comment on attachment 366686 [details] [diff] [review]
only change the title for the main browser window

per comment 13

Comment 15

[Mike Connor [:mconnor]]

Attachment: whee

changed the logic around in onEnterPrivateBrowsing, pretty straightforward.

The onExitPrivateBrowsing bits you'll ask about, basically I don't think it matters if it was autostarted or not, we can do all of those things.  The number of times this will be called while in autostart mode will be minimal, so the extra check isn't a net win.

Comment 16

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

Comment on attachment 366733 [details] [diff] [review]
whee

Hrm, is it even possible to exit private browsing when it was autoStarted, given that we disable the command?

Comment 17

[(no longer active)]

(In reply to comment #16)
> (From update of attachment 366733 [details] [diff] [review])
> Hrm, is it even possible to exit private browsing when it was autoStarted,
> given that we disable the command?

Yes.  This is disabled at the UI level, however it can happen in two cases:

1. When the user exits the application (the private browsing service is always exited before quitting the application).

2. If an extension uses the API directly to do that.

Comment 18

[Mike Connor [:mconnor]]

Of course, if and when we have UI for this, we might directly exit PB without restarting.  Future proof ftw. :)

Comment 19

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

This was checked-in as http://hg.mozilla.org/mozilla-central/rev/e27fe0b767ea

Comment 20

[Mike Connor [:mconnor]]

and was backed out, though it'll reland today, since the boxes went green before the backout landed.

Comment 21

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

Was this relanded?

Comment 22

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/e0fc5ea2f05e

Comment 23

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

Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090405 Minefield/3.6a1pre ID:20090405031025

Can this be tested automatically?

Comment 24

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5320f28035fe

Comment 25

[(no longer active)]

(In reply to comment #23)
> Can this be tested automatically?

Yes, should be easy.  I'd write one, but I don't have a Mac to test it against.  mconnor, are you up for writing the test?  If not, I can give it a try and use the try server to debug it, although it may probably take a week or so. ;-)

Comment 26

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090407 Shiretoko/3.5b4pre