Closed Bug 1542316 Opened 5 years ago Closed 5 years ago

Firefox does not create a separate dedicated profile when launched from another install location on macOS

Categories

(Toolkit :: Startup and Profile System, defect, P1)

Product:
Toolkit
Component:
Startup and Profile System
Platform:
All
macOS
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- unaffected
firefox67 --- unaffected
firefox68 + verified

People

(Reporter: cbaica, Assigned: mozilla)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Patch
4.21 KB, patch
spohl
: review+
Details | Diff | Splinter Review

[Affected versions]:

  • Fx 68.0a1
  • Fx67.0b8

[Unaffected versions]:

  • Fx67.0b7

[Affected platforms]:

  • macOS

[Steps to reproduce]:

  1. Install Firefox in 2 different locations.
  2. Launch Firefox from one of the locations and check the about:profiles section.
  3. Launch Firefox from the second location and check the about:profiles section.

[Expected result]:

  • Each instance has a separate profile created and in use.

[Actual result]:

  • Both instances use the same profile.

[Regression range]:

  • Last good build: 28-03-2019
  • First bad build: 29-03-2019

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e31357c7759379d2279b6883cb09c91997bfaa5d&tochange=bd1e28b0143bdcff0798b0e6a4f54791c41192e8

This was the best we could narrow it down so far, due to mozregression not being too useful in this case.

Has Regression Range: --- → yes
Has STR: --- → yes
Keywords: regression

Using -no-remote makes it work, this is probably caused by bug 469990.

As described, this now matches the behavior on Windows and Linux and is therefore expected. Is there another reason why this was filed as a bug aside from the fact that the new behavior is different from past behavior?

Flags: needinfo?(cristian.baica)

(In reply to Stephen A Pohl [:spohl] from comment #2)

As described, this now matches the behavior on Windows and Linux and is therefore expected. Is there another reason why this was filed as a bug aside from the fact that the new behavior is different from past behavior?

Windows and Linux do not need -no-remote in order to launch a second instance.

I may have misunderstood the bug description. When the second instance launches, is this actually a separate Firefox process, running the executable in the second location? Or does this merely focus and launch a new window in the first Firefox process?

(In reply to Stephen A Pohl [:spohl] from comment #4)

I may have misunderstood the bug description. When the second instance launches, is this actually a separate Firefox process, running the executable in the second location? Or does this merely focus and launch a new window in the first Firefox process?

We can't run two instances with the same profile so I was assuming the latter, and this is what I see locally.

(In reply to Dave Townsend [:mossop] (he/him) from comment #5)

We can't run two instances with the same profile so I was assuming the latter, and this is what I see locally.

You are correct. As mentioned in the 'Actual result' part, the instances running with the same profile. On windows an linux this issue is not occurring, meaning that when I open Firefox from 2 different locations (just by double clicking) 2 different profiles are created and in use.

Flags: needinfo?(cristian.baica)

Yuri, is this something that you could work on? This boils down to the fact that if an instance is launched from a different path, it will be running in a separate profile and should therefore be allowed to launch as a separate instance.

Flags: needinfo?(mozilla)

Cristian, I just spotted that you mentioned that this reproduces in 67.0b8, but bug 469990 hasn't landed on 67 that I can see. Can you confirm that this reproduces there?

Regressed by: 469990
No longer regressed by: 1539839

Hello Dave!

I tested the both the builds with the code from bug 469990 and without it(https://hg.mozilla.org/mozilla-central/rev/b1374f2163b6), I could not reproduce this issue.

When I tested the builds with/without the code from bug 1539839 (https://hg.mozilla.org/mozilla-central/rev/46b9df4a18e8), I noticed that builds without the code could not reproduce this issue, however builds with the code can reproduce this issue.

This leads me to believe that the regressor is indeed bug 1539839.

Thank you!

Regressed by: 1539839
No longer regressed by: 469990

Thanks for isolating the issue to the correct bug!

Flags: needinfo?(mozilla)

(In reply to Cristian Comorasu [:ccomorasu], Release Desktop QA from comment #9)

This leads me to believe that the regressor is indeed bug 1539839.

Odd. Bug 1539839 should only affect builds run with MOZ_PROFILER_STARTUP set in the environment. I made a build locally with bug 1539839 backed out and could still reproduce this issue. Bug 1539839 is also not in 67 so if this actually is reproducible on 67 then something weird is going on.

So I did some tests locally and got different results to Cristian. I'm still of the opinion that this is caused by bug 469990.

First I tested two installs of Firefox 67.0b8 and could not reproduce it in any way. Given that the bugs we're discussing are not on 67 that makes sense so I think we can say this isn't happening on 67 at this point.

I made a local build of the changeset that bug 1539839 landed in and the previous changeset (in both cases plus a backout of bug 1528963 as that was busting the build at this point). I installed both builds twice. Running those gave me the following results (pre means the build from before bug 1539839 and post means the build with bug 1539839):

Run pre first and then run second install of pre: Second instance created a new profile for use.
Run pre first and then run post: Second instance created a new profile for use.
Run post first and then run pre: Second instance created a new profile for use.
Run post first and then run second install of post: Second instance created a new profile for use.

This makes sense to me since bug 1539839 should not affect builds run normally.

Then I made similar builds for the changeset where bug 469990 landed and the previous changeset. Running those gave me these results:

Run pre first and then run second install of pre: Second instance created a new profile for use.
Run pre first and then run post: Nothing seemed to happen, no new Firefox window opened, no new profile created.
Run post first and then run pre: Second instance created a new profile for use.
Run post first and then run second install of post: A new window opens in the existing instance.

Then I made builds of current inbound and inbound plus backouts of bug 469990 and bug 1540821 Running those gave me these results:

Run inbound first and then run a second install of inbound: A new window opens in the existing instance.
Run inbound first and then run after backout: Second instance created a new profile for use.
Run after backout and then run inbound: Nothing seemed to happen, no new Firefox window opened, no new profile created.
Run after backout and then a second install of after backout: Second instance created a new profile for use.

status-firefox67: affected → unaffected
Attached patch thefix3.patch (obsolete) — — Splinter Review

(In reply to Stephen A Pohl [:spohl] from comment #7)

Yuri, is this something that you could work on? This boils down to the fact that if an instance is launched from a different path, it will be running in a separate profile and should therefore be allowed to launch as a separate instance.

Attached is the patch that takes into account paths of the processes: the no-remote logic is applied only if the bundle paths of the caller and the callee match.

Flags: needinfo?(spohl.mozilla.bugs)
Comment on attachment 9056719 [details] [diff] [review]
thefix3.patch

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

Thank you for jumping on this so quickly!

I had to revise this patch to make it apply cleanly. There seems to be an issue with your .mozconfig such that it doesn't use relative paths, but absolute paths referencing code files on your hard drive. If you intend to submit more code (and I highly encourage you to!) you may want to look into the new way of submitting code to our new review tool, Phabricator. The new process is documented here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Submitting_the_patch

::: /Users/mike/mozillasrc/nsNativeAppSupportCocoa.mm.original3
@@ +219,5 @@
> +                                   [[NSBundle mainBundle] bundleIdentifier]];
> +    NSString* thisAppBundlePath = [[NSBundle mainBundle] bundlePath];
> +    for (NSRunningApplication* app in appsWithMatchingId) {
> +      if([thisAppBundlePath isEqual:[[app bundleURL] path]]) {
> +        runningInstancesFoundCount++;

We can simply set `runningInstanceFound` here and `break` out of the loop, avoiding the need for the extra `runningInstancesFoundCount` local variable. I'm going to make this change and include it in the patch that I'm going to land for you.
Attachment #9056719 - Flags: review+

Thanks Stephen.
Note, that there must be exactly at least two running instances at the same path and same bundle id to set runningInstanceFound to YES. One of them will be an already existing process, the other one is the current process.

(In reply to Yuri from comment #15)

Thanks Stephen.
Note, that there must be exactly at least two running instances at the same path and same bundle id to set runningInstanceFound to YES. One of them will be an already existing process, the other one is the current process.

Oof, you're right! Thanks for catching this. Will update the patch accordingly.

Flags: needinfo?(spohl.mozilla.bugs)

And actually now that the paths must match, we don't have to check for bundle ids matches (this was added initially to prevent Thunderbird/Firefox interference). But having this check in the patch doesn't hurt either - in fact, in unlikely case the requirements change back to not expect the paths to match we will have this already working (just one extra check will have to be removed).

[Tracking Requested - why for this release]:

This breaks our dedicated profiles story.

tracking-firefox68: --- → ?
Priority: -- → P1
Regressed by: 469990
Attached patch Patch — — Splinter Review

I changed this to check for the bundle path AND that the current NSRunningApplication is different, which is essentially saying that there need to be at least two running instances. Yuri, can you take a look and confirm that this addresses your comment 15 and that we're good to go here?

Attachment #9056719 - Attachment is obsolete: true
Attachment #9059540 - Flags: review+
Assignee: nobody → mozilla

Hi Stephen, Sorry for late reply.

That should be fine, provided [NSRunningApplication hash] (and, consequently, isEqual:) behaves as we would expect it to (I didn't find in documentation what is actually being hashed) - are you sure this is the case?

Otherwise isEqual simply compares object pointers, and this is not what we want (we can't expect the framework to give us the same object at the same address even if two NSRunningApplications actually represent the same app).

Flags: needinfo?(spohl.mozilla.bugs)

(In reply to Yuri from comment #20)

Hi Stephen, Sorry for late reply.

That should be fine, provided [NSRunningApplication hash] (and, consequently, isEqual:) behaves as we would expect it to (I didn't find in documentation what is actually being hashed) - are you sure this is the case?

Otherwise isEqual simply compares object pointers, and this is not what we want (we can't expect the framework to give us the same object at the same address even if two NSRunningApplications actually represent the same app).

Usually, I would have gone ahead and compared pids. The reason why I picked isEqual: instead of pids is because Apple's documentation discourages the direct comparison of pids and recommends the use of isEqual:. See https://developer.apple.com/documentation/appkit/nsrunningapplication/1526998-processidentifier?language=objc

Does this address your concerns?

Flags: needinfo?(spohl.mozilla.bugs) → needinfo?(mozilla)

(In reply to Stephen A Pohl [:spohl] from comment #21)

Usually, I would have gone ahead and compared pids. The reason why I picked isEqual: instead of pids is because Apple's documentation discourages the direct comparison of pids and recommends the use of isEqual:. See https://developer.apple.com/documentation/appkit/nsrunningapplication/1526998-processidentifier?language=objc

Does this address your concerns?

Yes. Thanks for the link!

Flags: needinfo?(mozilla)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cddd0efaf16b0e57c04e0a69e2b90735353cad3
Bug 1542316: Allow new instances to be launched if the path to the application bundle is different than currently running instances on macOS. r=spohl

Setting n-i to get independent confirmation that this is indeed fixed. The fix should be in the next Nightly.

Flags: needinfo?(dtownsend)
Flags: needinfo?(cristian.baica)

https://hg.mozilla.org/mozilla-central/rev/9cddd0efaf16

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox68: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Sorry for the late response.
The issue is verified fixed on the latest Fx68.0a1 using macOS 10.13. Firefox starts with different profiles when launched from different locations.

Status: RESOLVED → VERIFIED
status-firefox68: fixed → verified
Flags: needinfo?(cristian.baica)
Flags: needinfo?(dtownsend)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: