Closed Bug 1090967 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: General, defect)

35 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 36
Tracking Status
firefox34 --- verified
firefox35 --- verified
firefox36 --- verified

People

(Reporter: cbadau, Assigned: past)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 4 obsolete files)

2.04 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
1.58 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
2.04 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
3.19 KB, patch
Felipe
: review+
Details | Diff | Splinter Review
3.15 KB, patch
Details | Diff | Splinter Review
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.
Assignee: nobody → past
Status: NEW → ASSIGNED
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.
Attachment #8515031 - Flags: review?(benjamin)
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?
Attachment #8515031 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/9065c0332c39
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
(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 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
Attachment #8515031 - Flags: approval-mozilla-aurora?
Attached patch patch for beta (obsolete) — Splinter Review
This patch applies cleanly to beta and leaves out the SetDefaultProfile call that is not present there.
Whiteboard: [NO_UPLIFT]
I discovered a hole in this patch that I am going to provide a fix for ASAP.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I missed one important detail in the previous patch: the name of the default profile, which has to be different on Aurora.
Attachment #8515984 - Flags: review?(benjamin)
Attachment #8515984 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/b5c46dda431d
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
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
Attachment #8515984 - Flags: approval-mozilla-aurora?
Comment on attachment 8515907 [details] [diff] [review]
patch for beta

Needs updating.
Attachment #8515907 - Attachment is obsolete: true
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
Camelia: what about DevEdition simultaneously with Release? 

Also for devs, are the comment 14 results all expected?
Attached patch Patch for beta (obsolete) — Splinter Review
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.
Attachment #8516831 - Flags: review?(benjamin)
(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.
(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 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
Attachment #8516831 - Flags: approval-mozilla-beta?
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?
Attachment #8516831 - Flags: review?(benjamin) → review-
Attachment #8515984 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8515031 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Attachment #8516831 - Flags: approval-mozilla-beta?
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.
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 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.
Attachment #8517590 - Flags: review?(benjamin)
Attachment #8516831 - Attachment is obsolete: true
Attached patch patch for betaSplinter Review
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.
Attachment #8518134 - Flags: review?(benjamin)
Comment on attachment 8518134 [details] [diff] [review]
patch for beta

Yep.
Attachment #8518134 - Flags: review?(benjamin) → review+
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 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
Attachment #8518134 - Flags: approval-mozilla-beta?
(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 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.
Attachment #8517590 - Flags: review?(felipc)
Attachment #8517590 - Flags: review?(benjamin)
Attachment #8517590 - Flags: review+
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.
Attachment #8517590 - Flags: review?(felipc) → review-
OK, moved the code to nsBrowserGlue.js as a separate function.
Attachment #8518905 - Flags: review?(felipc)
Attachment #8517590 - Attachment is obsolete: true
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?
Attachment #8518905 - Flags: review?(felipc) → review+
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.
Attachment #8518917 - Flags: review?(felipc)
Attachment #8518905 - Attachment is obsolete: true
(In reply to :Felipe Gomes from comment #33)
> don't you need `this.` to call this function?

Dammit, you noticed ;-)
Attachment #8518917 - Flags: review?(felipc) → review+
Rebased the last patch for gum.
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+
Attachment #8518134 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Contact: camelia.badau
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
You need to log in before you can comment on or make changes to this bug.