607482 - When in Private Mode, opening a new window causes FF button to flash orange and then turn purple

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[imradyurrad]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101026 Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101026 Firefox/4.0b8pre

The button's orange for about a quarter of a second before it turns purple.

Reproducible: Always

Comment 1

[(no longer active)]

This happens because the privatebrowsingmode attribute is set from delayedStartup.  The easy solution would be to init it inside BrowserStartup.  What do you think, Dao?

Comment 2

[Dão Gottwald [:dao]]

Yeah. I think there are also a few tests that could stop messing with delayedStartup.

Comment 3

[(no longer active)]

(In reply to comment #2)
> Yeah. I think there are also a few tests that could stop messing with
> delayedStartup.

Hmm, do you mean that we should just call gPrivateBrowsingUI.init() from BrowserStartup instead of delayedStartup?  I'm worried that it might have some perf impact...

Comment 4

[Dão Gottwald [:dao]]

The code directly in browser.js looks cheap enough. Is getting the service potentially expensive?

Comment 5

[(no longer active)]

(In reply to comment #4)
> The code directly in browser.js looks cheap enough. Is getting the service
> potentially expensive?

It shouldn't be.  In fact, the service is already initialized when we show a window, so we just grab a reference to the existing object (which involves some XPConnect stuff, obviously).  Let's try it.

Comment 6

[(no longer active)]

Attachment: Patch (v1)

Comment 7

[Dão Gottwald [:dao]]

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

>@@ -1248,16 +1248,19 @@ function BrowserStartup() {
> 
>   allTabs.readPref();
> 
>   TabsOnTop.syncCommand();
> 
>   BookmarksMenuButton.init();
> 
>   setTimeout(delayedStartup, 0, isLoadingBlank, mustLoadSidebar);
>+
>+  // initialize the private browsing UI
>+  gPrivateBrowsingUI.init();
> }

setTimeout should remain at the very end of that function.

I don't really see the point of browser_privatebrowsing_initonwindowload.js. It's very much tied to the current implementation without actually verifying that the button doesn't flash orange. Tests aren't for free but need to be maintained from time to time (as you know very well :-)). I wouldn't add this one.

Comment 8

[(no longer active)]

Attachment: For check-in

(In reply to comment #7)
> Comment on attachment 486707 [details] [diff] [review]
> Patch (v1)
> 
> >@@ -1248,16 +1248,19 @@ function BrowserStartup() {
> > 
> >   allTabs.readPref();
> > 
> >   TabsOnTop.syncCommand();
> > 
> >   BookmarksMenuButton.init();
> > 
> >   setTimeout(delayedStartup, 0, isLoadingBlank, mustLoadSidebar);
> >+
> >+  // initialize the private browsing UI
> >+  gPrivateBrowsingUI.init();
> > }
> 
> setTimeout should remain at the very end of that function.

Oops, right.  Fixed that.

> I don't really see the point of browser_privatebrowsing_initonwindowload.js.
> It's very much tied to the current implementation without actually verifying
> that the button doesn't flash orange. Tests aren't for free but need to be
> maintained from time to time (as you know very well :-)). I wouldn't add this
> one.

Fair enough!  I removed the test.

Comment 9

[imradyurrad]

Can someone set this to blocking final?

Comment 10

[Dietrich Ayala (:dietrich)]

I don't think this blocks. Nice polish for a non-default case, so will a+ the patch instead.

Comment 11

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/119ed9cbd3c6

Comment 12

[(no longer active)]

Backed out because I suspect that this is causing a crash in the browser-chrome suite:

<http://hg.mozilla.org/mozilla-central/rev/59cc39e88e75>

The crash logs:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1288906850.1288908627.9851.gz&fulltext=1
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1288905988.1288908421.8895.gz&fulltext=1
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1288907394.1288908638.9885.gz&fulltext=1

Comment 13

[(no longer active)]

Attachment: Part 2: Adjust the test so that the code doesn't need to unset the attribute asynchronously

This patch fixes the test which required the removeAttribute call to be made asynchronously.  With this, the crash in bug 609793 doesn't happen, so we can land this patch while someone comes up with a solution to bug 609793.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 490446 [details] [diff] [review]
Part 2: Adjust the test so that the code doesn't need to unset the attribute asynchronously

Looks like this would be more robust if the observer was added in onEnterPrivateBrowsing and onExitPrivateBrowsing and always removed itself (not just for the second pass).

Comment 15

[(no longer active)]

Attachment: Part 2: Adjust the test so that the code doesn't need to unset the attribute asynchronously

(In reply to comment #14)
> Comment on attachment 490446 [details] [diff] [review]
> Part 2: Adjust the test so that the code doesn't need to unset the attribute
> asynchronously
> 
> Looks like this would be more robust if the observer was added in
> onEnterPrivateBrowsing and onExitPrivateBrowsing and always removed itself (not
> just for the second pass).

Fair enough.

Comment 16

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/364409371144
http://hg.mozilla.org/mozilla-central/rev/4fea98061fae

Comment 17

[(no longer active)]

I had to back out because of oranges on m-c (which I had not been getting on the try server).

Comment 18

[(no longer active)]

Relanded again:

http://hg.mozilla.org/mozilla-central/rev/8515f6bd4306
http://hg.mozilla.org/mozilla-central/rev/23262313e08b

Comment 19

[(no longer active)]

Seems like this patch stuck this time!  :-)