945829 - Metrofox hangs on startup when trying to show the profile chooser dialog

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: General, defect, P2)

Description

[Kartikaya Gupta (email:[e-mail])]

Previously when working on Metro I had set the option to always show the profilemanager profile chooser dialog on startup. On Metrofox that usually did nothing and would just start Metrofox directly, while starting in desktop mode it would show the dialog.

Recently (sometime in the last ~10 days) Metrofox has started to hang under these conditions. Attaching Visual studio shows that it is trying to show the modal dialog for the profile manager.

Comment 1

[Jim Mathies [:jimm]]

There are a few cases here that can launch this dialog - parameters on shortcuts and we also store a flag in hte profile ini file - 

http://mxr.mozilla.org/mozilla-central/ident?i=ShowProfileManager

http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#2297
http://mxr.mozilla.org/mozilla-central/source/toolkit/profile/nsToolkitProfileService.cpp#906

Comment 2

[Brian R. Bondy [:bbondy]]

I think we should just ignore the profile manager flag on Metro for v1

Comment 3

[Brian R. Bondy [:bbondy]]

p=1

Comment 4

[Kartikaya Gupta (email:[e-mail])]

In case anybody else runs into this, I worked around it by commenting out the line at http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp?rev=bd024cf6af34#2297

Comment 6

[Brian R. Bondy [:bbondy]]

p=1

Comment 7

[Brian R. Bondy [:bbondy]]

Attachment: rev1

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 8345354 [details] [diff] [review]
rev1

XRE_GetWindowsEnvironment() is ifdef'd into XP_WIN builds only which is going to make this messy. Maybe add a helper function in nsAppRunner with the ifdef's GetWindowsEnvironment call to simplify?

Comment 9

[Brian R. Bondy [:bbondy]]

Talked about making the environment type call cross platform but bsmedberg x'ed the idea.
I think having a helper on a helper is a bit ugly, so just going to add in the ifdefs to each call.
I think w can propose and find a better way in a diff bug that won't hold this up if we want to.
This will just uses our previous convention for this kind of thing.

Comment 10

[Brian R. Bondy [:bbondy]]

Actually we can go with your idea and get past my gripe about helper on helper just by making it called ShouldShowProfileManager() and not something like IsMetro. Maybe you meant that originally :)
In any case I'll do that so we only have one ifdef

Comment 11

[Brian R. Bondy [:bbondy]]

Attachment: rev2

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 8346066 [details] [diff] [review]
rev2

Review of attachment 8346066 [details] [diff] [review]:
-----------------------------------------------------------------

What happen when we drop out of this, do we still get a default profile loaded?

::: toolkit/xre/nsAppRunner.cpp
@@ +584,5 @@
> +{
> +#if defined(XP_WIN)
> +  return XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Desktop;
> +#else
> +  return false;

return true?

Comment 13

[Brian R. Bondy [:bbondy]]

(In reply to Jim Mathies [:jimm] from comment #12)
> Comment on attachment 8346066 [details] [diff] [review]
> rev2
> 
> Review of attachment 8346066 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What happen when we drop out of this, do we still get a default profile
> loaded?
> 
> ::: toolkit/xre/nsAppRunner.cpp
> @@ +584,5 @@
> > +{
> > +#if defined(XP_WIN)
> > +  return XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Desktop;
> > +#else
> > +  return false;
> 
> return true?

Yep oops, fixed.  Try would have caught it but thanks for catching it early :D


> What happen when we drop out of this, do we still get a default profile
> loaded?

From my testing we get the default profile

Comment 14

[Brian R. Bondy [:bbondy]]

Attachment: rev3

Carrying forward r+

Comment 15

[Brian R. Bondy [:bbondy]]

I'll be out a lot today (on PTO) and inbound is closed now, this passes try tests, marking checkin-needed in case someone gets to it before me.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/00a28ea962f2

Please make sure your patch includes a commit message next time. Thanks :)

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00a28ea962f2

Comment 18

[Brian R. Bondy [:bbondy]]

Comment on attachment 8346100 [details] [diff] [review]
rev3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 924860
User impact if declined: Always show profile manager would hang Metro Firefox on startup.
Testing completed (on m-c, etc.): It's been on m-c for a couple days.
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none

Comment 19

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cac87bf8f6ec

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Target milestone reflects what was on m-c when the bug landed. Status-firefoxXX flags track uplifts to other branches/trains.

Comment 21

[u279076]

Kartikaya, can you please confirm this is working for you now with Firefox 28 and 29?

Comment 22

[Kartikaya Gupta (email:[e-mail])]

I tried a few days ago after this bug landed on 29, and it would show me the profile chooser dialog and then start the non-metro interface of Firefox. I could switch to the metro interface from the menu. So I would consider this bug fixed, yes.

Comment 23

[u279076]

Thanks for your help Kartikaya.