1090967 - Aurora doesn't run simultaneously with other Firefox version (on a machine with 0 profiles)

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Camelia Badau [:cbadau], Desktop QA]

Reproducible on Aurora 35.0a2 (Aurora DevEdition - latest build from the gum twig: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/gum-win32/latest/)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0

Steps to reproduce: 
1. Make sure that you are on a machine with 0 profiles. 
2. Launch Aurora DevEdition (latest build from the gum twig). -> the dev-edition-default profile is created (the "Allow Firefox Developer Edition and Firefox to run at the same time" option is checked)
3. Launch Beta/Release/Nightly. 

Expected results: The Beta/Release/Nightly is opened and run simultaneously with Aurora DevEdition (because the "Allow Firefox Developer Edition and Firefox to run at the same time" option is checked). A new profile is created ("default" profile).

Actual results: The Beta/Release/Nightly is not opened and the following message is received: "Firefox is already running, but is not responding. The old Firefox process must be closed to open a new window". No new profile is created.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Don't use the aurora-specific profile by default, if this is not aurora

This will ensure that only aurora builds will consider the aurora-specific profile as a valid default profile. Builds from other channels will just create a new default one for their own use.

This is a simple enough change that I hope we can get to beta as well, so that we only have a couple of weeks with the stable release sufferring from this bug. The patch for beta will omit the SetDefaultProfile call of course, as that was intriduced in bug 1024110, which stays in aurora.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Here is a try push:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=779c912e800c

Camelia, when the builds appear in that push, could you verify that the problem is indeed fixed?

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/9065c0332c39

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9065c0332c39

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #2)
> Camelia, when the builds appear in that push, could you verify that the
> problem is indeed fixed?

Or the next nightly I guess.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8515031 [details] [diff] [review]
Don't use the aurora-specific profile by default, if this is not aurora

Approval Request Comment
[Feature/regressing bug #]: bug 1024110
[User impact if declined]: on a new machine, installing Aurora first and then another Firefox version will cause the latter to not create or use an appropriate profile. This patch is not strictly necessary for Aurora, but it is necessary everywhere else, so we should uplift it in the interest of consistency.
[Describe test coverage new/current, TBPL]: m-c, gum
[Risks and why]: tiny patch, which additionally does not have any visible impact on Aurora, as the code will not get compiled there
[String/UUID change made/needed]: none

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: patch for beta

This patch applies cleanly to beta and leaves out the SetDefaultProfile call that is not present there.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

I discovered a hole in this patch that I am going to provide a fix for ASAP.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: part 2 - Don't use "default" as the name of the default profile on Aurora

I missed one important detail in the previous patch: the name of the default profile, which has to be different on Aurora.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b5c46dda431d

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8515984 [details] [diff] [review]
part 2 - Don't use "default" as the name of the default profile on Aurora

Approval Request Comment
[Feature/regressing bug #]: bug 1024110
[User impact if declined]: on a new machine, installing Aurora first will cause it to create default profile without the appropriate name
[Describe test coverage new/current, TBPL]: m-c, gum
[Risks and why]: tiny patch, which only changes the existing behavior on Aurora
[String/UUID change made/needed]: none

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8515907 [details] [diff] [review]
patch for beta

Needs updating.

Comment 14

[Camelia Badau [:cbadau], Desktop QA]

I verified the bug with latest gum build (2014-11-04: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/gum-win32/1415065657/) and I have the following: 
(I tested on a machine with 0 profiles)
- I run DevEdition simultaneously with latest Nightly (buildID: 20141104030202) - it works ok
- I run DevEdition simultaneously with Firefox 34 Beta 6 (buildID: 20141103144234) - it doesn't work, the "Firefox is already running, but is not responding. The old Firefox process must be closed to open a new window" message is displayed
- I run DevEdition simultaneously with latest Aurora (buildID: 20141104004002) - it doesn't work, the "Firefox is already running, but is not responding. The old Firefox process must be closed to open a new window" message is displayed

Comment 15

[Tracy Walker [:tracy]]

Camelia: what about DevEdition simultaneously with Release? 

Also for devs, are the comment 14 results all expected?

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch for beta

This is what is necessary for beta to work in the scenario described in comment 0. This patch includes the two other patches in this bug, as well as a small part of the patch from bug 1024110 (the introduction of the profile service's defaultProfile).

All of these changes have already been reviewed by bsmedberg in this bug and bug 1024110, but I'm asking again, mostly to make sure he is aware of it. From a complexity standpoint this is not a complicated patch and all of its bits have already been tested in m-c and gum, although not this precise amalgamation on top of the beta codebase.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to [:tracy] Tracy Walker - QA Mentor from comment #15)
> Camelia: what about DevEdition simultaneously with Release? 

This fix will not make Release work in this scenario without a hotfix, which I presume is out of the question. Our hope is that by fixing beta, there will only be 2 weeks of stable Firefox that has to be installed before Aurora (8 weeks if we only uplift to beta).

> Also for devs, are the comment 14 results all expected?

Yes, because the fix has not been uplifted to aurora and beta yet.

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Panos Astithas [:past] (overloaded, please needinfo) from comment #17)
> (8 weeks if we only uplift to beta).

Sorry, that should have been "8 weeks if we only uplift to Aurora".

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8516831 [details] [diff] [review]
Patch for beta

Approval Request Comment
[Feature/regressing bug #]: bug 1024110 changes the way the default profile is being selected, which doesn't interact well with current beta
[User impact if declined]: installing Aurora first on a new machine and then Fx 34, will make Fx 34 not use the right profile, but the one tailored for Aurora
[Describe test coverage new/current, TBPL]: m-c, gum, locally
[Risks and why]: there is certainly some risk, but all of the changes have been present in other branches with no ill effects so far. I think some focused QA would be warranted
[String/UUID change made/needed]: none

Comment 20

[Benjamin Smedberg]

Comment on attachment 8516831 [details] [diff] [review]
Patch for beta

This at least requires a UUID rev of nsIToolkitProfileService, but we probably can't do that on beta. Given what I understand about this, though, I don't think we need to make that change on beta?

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #20)
> This at least requires a UUID rev of nsIToolkitProfileService, but we
> probably can't do that on beta. Given what I understand about this, though,
> I don't think we need to make that change on beta?

I didn't know we couldn't use this on beta, but the change would help minimize the time between shipping bug 1024110 and having deployed releases with this fix. If this gets only up to 35, there will be 2+6 weeks where stable Firefox cannot be installed after Aurora in a system, without getting an Aurora profile.

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Create a default profile for other channels, if Aurora is the first channel installed on a system

This is an alternative approach on avoiding the issue that only needs to reach Aurora. It makes Aurora create a second profile (marked as default) when it detects that it is running on a system with no other profiles created. It's a hack that can be removed once Firefox releases earlier than 35 are too rare for this situation to be a concern.

I need to do some more testing before requesting review, but in my limited testing so far it appears to work. As it is now, it will have a negative impact on browser startup, so I'd like to find a way to avoid that.

Comment 23

[Benjamin Smedberg]

The other option for beta is just to add some simple code that detects the single-profile-named-devedition case and create a separate default. That wouldn't require any IDL changes at all.

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8517590 [details] [diff] [review]
Create a default profile for other channels, if Aurora is the first channel installed on a system

This patch appears to work great in my testing. The impact in terms of wasted disk space is insignificant, because we only create an empty directory and add a few lines to profiles.ini. More importantly though, it covers the case of users installing Aurora first on a new system and then ESR, or a severely outdated version of Firefox.

I decided to leave the code in _delayedStartup because (a) it's only compiled in Aurora, and (b) it's only 2 lines of code in the common case and I doubt it would be much better to just register an observer for browser-delayed-startup-finished.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #23)
> The other option for beta is just to add some simple code that detects the
> single-profile-named-devedition case and create a separate default. That
> wouldn't require any IDL changes at all.

I will also try that next.

Comment 25

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: patch for beta

OK, I'm dumb. The first beta-specific patch was almost there, but instead of using SetDefaultProfile as we do after bug 1024110 I should just use SetSelectedProfile, which performed the same task in older versions. No need to bring in the IDL changes or anything.

Comment 26

[Benjamin Smedberg]

Comment on attachment 8518134 [details] [diff] [review]
patch for beta

Yep.

Comment 27

[Benjamin Smedberg]

Comment on attachment 8517590 [details] [diff] [review]
Create a default profile for other channels, if Aurora is the first channel installed on a system

Do you still want this patch?

Comment 28

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8518134 [details] [diff] [review]
patch for beta

Approval Request Comment
[Feature/regressing bug #]: bug 1024110 changes the way the default profile is being selected, which doesn't interact well with current beta
[User impact if declined]: installing Aurora first on a new machine and then Fx 34, will make Fx 34 not use the right profile, but the one tailored for Aurora
[Describe test coverage new/current, TBPL]: m-c, gum, locally
[Risks and why]: there is certainly some risk, but all of the changes have been present in other branches with no ill effects so far. I think some focused QA would be warranted
[String/UUID change made/needed]: none

Comment 29

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #27)
> Do you still want this patch?

I think we do, for the users that install Aurora first and a very old release next (ESR or not).

Comment 30

[Benjamin Smedberg]

Comment on attachment 8517590 [details] [diff] [review]
Create a default profile for other channels, if Aurora is the first channel installed on a system

The code is ok: I'm not the right person to decide whether delayedStartup is the right place to put it or not.

Comment 31

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8517590 [details] [diff] [review]
Create a default profile for other channels, if Aurora is the first channel installed on a system

delayedStartup runs once per window. A better place would be in nsBrowserGlue.js onWindowsRestored, which will run once after all windows have finished loading.

Also it would be better to define this as a separate function with a proper name instead of adding the code directly inside that function.

Comment 32

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Create a default profile for other channels, if Aurora is the first channel installed on a system v2

OK, moved the code to nsBrowserGlue.js as a separate function.

Comment 33

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8518905 [details] [diff] [review]
Create a default profile for other channels, if Aurora is the first channel installed on a system v2

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

::: browser/components/nsBrowserGlue.js
@@ +922,5 @@
>        }
>      }
>  
> +#ifdef MOZ_DEV_EDITION
> +  _createExtraDefaultProfile();

don't you need `this.` to call this function?

Comment 34

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Create a default profile for other channels, if Aurora is the first channel installed on a system v3

Moved the call before other tasks that could block it by waiting for user input. I've made the mistake of opening the browser and closing it after the default browser prompt appeared, expecting the extra profile to be created. This should provide less opportunity for missing the profile creation step.

Comment 35

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to :Felipe Gomes from comment #33)
> don't you need `this.` to call this function?

Dammit, you noticed ;-)

Comment 36

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/43cad6287583

Comment 37

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Two profiles patch for Gum

Rebased the last patch for gum.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/43cad6287583

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/24bf9ff8e2bd
https://hg.mozilla.org/releases/mozilla-aurora/rev/333f986e84ee
https://hg.mozilla.org/releases/mozilla-aurora/rev/a2a2e615349b

Comment 40

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8518134 [details] [diff] [review]
patch for beta

We don't want to break any channel with the new changes to Aurora/dev browser. Let's take this in beta8, which will also prompt users about the dev edition browser. Beta+

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/46829698a2b9

Comment 42

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5: 
- I run latest Firefox Developer Edition 35.0a2 (buildID: 20141116004002) simultaneously with latest Nightly 36.0a1(buildID: 20141116030212) - it works ok
- I run latest Firefox Developer Edition 35.0a2 (buildID: 20141116004002) simultaneously with Firefox 34 Beta 9 (buildID: 20141114133026) - it works ok