1941788 - The update mutex prevents interactive updates when multiple instances are running

Status: NEW

(Toolkit :: Application Update, defect)

Description

[Yannis Juglaret [:yannis]]

Attachment: update-mutex.png

The update mutex is supposed to prevent multiple running instances of Firefox from doing conflicting updates: only one of them will acquire the update mutex and proceed with the update. However, the current logic prevents all running instances from proceeding with an update, including the one that acquires the update mutex: as long as there is another instance running we won't be able to proceed.

STR:

• Start Firefox.
• Navigate to about:profiles and launch a different profile in a new browser.
• In both instances, use the hamburger menu to go to Help, About Firefox.

Observed:

• Both instances show "Firefox is being updated by another instance".

Expected:

• One instance checks for updates. The other shows "Firefox is being updated by another instance".

Attachment shows the behavior on Windows in 128esr branch, to emphasize that this is not a regression from bug 1900726 but a preexisting issue.

I believe that this behavior was introduced by bug 1553982 which added the logic below:

  get isOtherInstanceHandlingUpdates() {
    return !hasUpdateMutex() || isOtherInstanceRunning();
  }

Assuming that the intention was that, if we're the only instance, we want to proceed with updates even if we failed to acquire the update mutex, then this should rather be:

  get isOtherInstanceHandlingUpdates() {
    return !hasUpdateMutex() && isOtherInstanceRunning();
  }

Fixing this means that we need to decide how we want to proceed regarding the case where we don't manage to acquire the update mutex but there is no other instance is running. We could decide to show a specific error message in this case, because we would consider it dangerous to start applying an update if we don't own the update mutex, in case a new instance would appear while we are applying the update.

However, since bug 1900726 and the new implementation of the update mutex, it is unlikely that we will succeed to apply an update anyway if we are unable to acquire the update mutex, which only requires us to write to the update directory. So it could be reasonable to not consider this an error, and just try to proceed with the update (and likely fail early). This would restore the previous behavior that users from bug 1940481 expect. What do you think, :bytesized?

Comment 1

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

(In reply to Yannis Juglaret [:yannis] from comment #0)

The update mutex is supposed to prevent multiple running instances of Firefox from doing conflicting updates: only one of them will acquire the update mutex and proceed with the update. However, the current logic prevents all running instances from proceeding with an update, including the one that acquires the update mutex: as long as there is another instance running we won't be able to proceed.

From the rest of your description, I suspect you mean "the current logic prevents all running instances from proceeding with a manual update". Is that right? Because it doesn't seem like you ever talk about timer-based updates.

Comment 2

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

To expand slightly, it has long been the case that Firefox will not really let you do any part of a manual update with other instances running. We just can't run through the whole update process like that. When we get an update downloaded and normally would show the "Restart to Update" button, we sort of cannot do that because a restart with other profiles running won't actually cause an update until the MSIL timer expires.

We certainly could change the interface to choose between the "Restart to Update" button and some sort of "Your update has been downloaded but can't be installed right now due to other instances" message. But it doesn't currently have such logic so it basically just locks out the whole UI. I'm not against changing this, but tbh I would kind of rather the effort go into fixing this problem properly. Which is a long road but should ultimately lead to substantial improvements in multiple areas.

Comment 3

[Yannis Juglaret [:yannis]]

(In reply to Robin Steuber (she/her) [:bytesized] from comment #1)

From the rest of your description, I suspect you mean "the current logic prevents all running instances from proceeding with a manual update". Is that right? Because it doesn't seem like you ever talk about timer-based updates.

Yes, I'm reporting what felt like a bug to me given my understanding of the role of the update mutex and the name isOtherInstanceHandlingUpdates. I reproduced that with manual updates indeed. But I'm mostly ignorant here so this bug could be invalid. So -- timer-based updates don't go through AppUpdater.check?

Comment 4

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

AppUpdater is only used by the update UIs and background update. Both are currently intended to be locked out when another instance is running.

I'm not going to close this bug though. Although this behavior currently does match the design, it's also not great and it is technically within our team's power to improve it. Once we have implemented Versioned Installation Directories on Windows (Bug 1891600) and macOS (still in design), we will be able to get rid of the isOtherInstanceRunning check here. And then what I'd really like to do is for Firefox to intentionally drop the mutex whenever it doesn't need it (we currently never drop it - whoever gets it holds it until exit). Ideally we should only hold the mutex while an update is downloading and staging. We could maybe even drop it once the download starts, if we are using BITS (or the macOS equivalent). This would allow the user to open the update interface in any copy of Firefox and actually use it so long as another instance wasn't actively pushing the update process forward at that moment.

But the prerequisites here are very high. So this is a long way off.

Comment 5

[Yannis Juglaret [:yannis]]

Removing the blocking relation since in the end the solution will be more complicated than comment 0 suggested. We might want to fix bug 1940481 prior to the current one for severity reasons.