1112937 - [Win] Firefox can update while still running when using multiple profiles concurrently

Status: RESOLVED WONTFIX

(Toolkit :: Application Update, defect, P2)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

STR:
1. Somehow set up a FF installation in $FF that will update the next time you start it.
2. Create two empty profiles.
3. Start $FF/firefox -profile $PROF1 -no-remote
4. Go to About Nightly and wait until it downloads and applies the update.
5. Open a terminal and cd into $FF.
6. Start $FF/firefox -profile $PROF2 -no-remote

Actual result:
The update has been applied while the Firefox instance in step 3 was running. If you run |ls| in the terminal in step 5, you'll see nothing because the directory is empty. The log in $FF/updates/last-update.log will show that an update took place. If the Firefox instance in step 3 tries to run, say, plugin-container, it will run the new version and probably crash.

Expected result:
The update should not be applied while any instances are running.

I realize that it's not common for users to run the same binary with two profiles. However, it seems like some new features are coming down the pipeline where we want to use multiple profiles, so we probably want to support this.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

With content processes becoming more of a thing with e10s, I can see this biting us harder over time.

We might want to consider not doing the directory-swap thing after an update unless we're certain there's not another active Firefox process running.

Thoughts, rstrong?

Comment 2

[Robert Strong (they/them - no direct email)]

Already discussed this with bsmedberg and I've got a couple of ideas of how to address this. Will likely look into it next week.

Comment 3

[Robert Strong (they/them - no direct email)]

Not enough time / resources to take this on :(

Comment 7

[Robert Strong (they/them - no direct email)]

Quick update... I've been able to spend some time on a Windows fix for this. I have the code finished for updater.cpp and the tests. I still have to finish the UI code and hope to be able to within the next week.

Comment 8

[Robert Strong (they/them - no direct email)]

Using this bug for the Windows fix. A fix for other platforms will require a different approach.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 56 since this may land for 56 and we may need manual testing help.

Comment 10

[Honza Bambas (:mayhemer)]

This bug really drives me crazy!  BTW, I once have seen a message in Options (or somewhere) that "another instance of Firefox is taking care of updates".  It made me believe this was fixed.  But it definitely isn't.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Same for me. The major problem is that if Firefox gets updated via another profile, my always open instance of Firefox will crash in specific situations because the parent buildid doesn't match the content buildid.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

We are heading for release day next week, so I'm assuming this is a wontfix for 56. 
Is it still on the radar for 57/58?

Comment 13

[Julien Cristau [:jcristau]]

Robert, can you update on status here?

I'm going to set 57 fix-optional because this is a long standing issue and I don't think we would want to add risk by uplifting a fix to beta.

Comment 14

[Julien Cristau [:jcristau]]

(In reply to Julien Cristau [:jcristau] from comment #13)
> Robert, can you update on status here?

Comment 15

[Robert Strong (they/them - no direct email)]

Attachment: patch 1 - test renames - in progress

Comment 16

[Robert Strong (they/them - no direct email)]

Attachment: patch 2 - client and test changes - in progress

Comment 17

[Robert Strong (they/them - no direct email)]

Comment on attachment 8912869 [details] [diff] [review]
patch 2 - client and test changes - in progress

Forgot to update the service tests which is trivial so I'll resubmit

Comment 18

[Robert Strong (they/them - no direct email)]

Attachment: patch 2 - client and test changes - in progress

Fixes all but marAppInUseFailureCompleteSvc_win.js

Comment 19

[Robin Steuber (she/her) [:bytesized]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ab32457e773717f095d9bc00c556b6b56fddbf7

Comment 20

[Robin Steuber (she/her) [:bytesized]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f5c08de49ea740bf8cb719d1e11473df66ddd98

Comment 21

[Robin Steuber (she/her) [:bytesized]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbd714a234241deae9856a7eb1c162756de16f36

Comment 22

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1112937 - Rename and add tests

This patch renames/copies Windows-specific xpcshell tests that will be changing for Bug 1112937. These are to reflect differences in the update functionality. Previously, updates would be forced through (in Windows) even if the updater cannot get an exclusive lock on the binary. Now, the update will fail in that situation. After 24 hours, however, a force file will be written out to force the update regardless of whether it can lock the executable.

Review commit: https://reviewboard.mozilla.org/r/195182/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195182/

Comment 23

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1112937 - Fail updates on Windows if an exclusive lock on the binary cannot be obtained

This patch implements the changes to the updater and associated tests to fix the multiprofile update problem on Windows. The problem is that if two instances of Firefox are running with different profiles and one instance tries to apply an update, then the update will be applied despite the other running instance (which will likely manifest problems, probably when attempting to start new processess). This patch changes the update mechanism such that if it cannot get an exclusive lock on the binary to be updated, the update will fail. However, if the update continues to fail 24 hours after the first failure, a force file will be written out causing the next update attempt to update despite other instances.

Review commit: https://reviewboard.mozilla.org/r/195184/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195184/

Comment 24

[Jim Mathies [:jimm]]

wontfixing for now, bug 1366808 is the current approach.