1923376 - Browser updates repeatedly instead of launching when the interactive user account has admin permissions (without a UAC)

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect, P1)

Description

[Chua Chee Wee]

Attachment: Firefox Nightly Update.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:131.0) Gecko/20100101 Firefox/131.0

Steps to reproduce:

Click on Firefox Nightly icon

Firefox Nightly 133.0.0.700
Product version 133.0a1

Actual results:

It shows "Firefox Nightly is installing your updates and will start in a few moments", then the dialog disappeared and Nightly didn't start.

Expected results:

Nightly should have started.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Toolkit::Application Update' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

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

Could you please attach update logs?

1. Navigate to about:support
2. Find the "Update Folder" entry and click "Open Folder".
3. Open the updates directory.
4. Inside, you should find some .log files: last-update.log, last-update-elevated.log, backup-update.log, backup-update-elevated.log. They may not all exist. Please attach whichever ones do exist to this bug.

Comment 3

[Chua Chee Wee]

I've reinstalled Nightly from the Installer, so I don't have any logs in there.

Comment 4

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

Okay, it sounds like all possible debugging information is probably gone then. If this happens again, you can send us the information that we need and we can reopen this bug. But there isn't much we can do here until then.

Comment 5

[Chua Chee Wee]

Ok, this happened again on the latest update.
How do I navigate to about:support if I can't even open Nightly?

Comment 6

[Chua Chee Wee]

Attachment: backup-update.log

Comment 7

[Chua Chee Wee]

Attachment: last-update.log

Comment 8

[Chua Chee Wee]

I think I found the files by looking at the latest date, and directory.
Hope these are the correct files.

Comment 9

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

Ah, yes. These are some of the correct files. I forgot about one detail though: since your updater is hanging, it will never successfully put the elevated update logs in the right place. Could you retrieve it from the the Maintenance Service directory? You should be able to find the log file at C:\Program Files (x86)\Mozilla Maintenance Service\UpdateLogs\<update_directory_name>.log.

Comment 10

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

Wait, hang on. I don't think I need it.

Comment 11

[Chua Chee Wee]

So, another issue here. Update failed, it should have rollback, so I can use the version of Nightly before the update, but it didn't rollback.
If I click the Nightly icon again, it tries to update again.

Comment 12

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

Are you sure these are the correct logs? This line sure doesn't seem likely from a Nightly installation

2024-10-08 19:24:02+0800: INSTALLATION DIRECTORY C:\Program Files\Firefox Developer Edition

They also don't really suggest any problem. They do suggest an unusual configuration

2024-10-08 19:24:02+0800: sUsingService=false
...
2024-10-08 19:24:02+0800: gIsElevated=false
...
2024-10-08 19:24:02+0800: INSTALLATION DIRECTORY C:\Program Files\Firefox Developer Edition
...
2024-10-08 19:24:02+0800: Successfully opened lock file
2024-10-08 19:24:02+0800: Going to update via this updater instance.

I'd be lying if I said I wasn't pretty curious how the heck it is updating an installation in Program Files without elevating. Suggests Firefox is either being run with privileges, which is alarming. Or Program Files is writable without elevated privileges. Which is also alarming.

Comment 13

[Chua Chee Wee]

Ok, I have no logs then, but I've attached a screenshot that shows the issue happens on local time 7 Oct 2024 8:40:02pm.

Comment 14

[Chua Chee Wee]

Attachment: 2024-10-09 09_35_30-Troubleshooting Information — Firefox Nightly.png

Comment 15

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

Oh, well I see that you at least have an update history file. That might at least give me a hint as to what's going on. That will be a file called updates.xml in the update directory. Can I see that?

Comment 16

[Chua Chee Wee]

Just logging info here for my own use for the next failure:
Update folder for Nightly:
C:\ProgramData\Mozilla-1de4eec8-1241-4177-a864-e594e8d1fb38\updates\6F193CCC56814779

Comment 17

[Chua Chee Wee]

Attachment: updates.xml

attached updates.xml

Comment 18

[Chua Chee Wee]

Attachment: 2024-10-10 00_14_43-Troubleshooting Information — Firefox Nightly.png

screenshot of latest update failures

Comment 19

[Chua Chee Wee]

Attachment: 6F193CCC56814779.log

This log is from C:\Program Files (x86)\Mozilla Maintenance Service\UpdateLogs

Comment 20

[Chua Chee Wee]

I'd be lying if I said I wasn't pretty curious how the heck it is updating an installation in Program Files without elevating. Suggests Firefox is either being run with privileges, which is alarming. Or Program Files is writable without elevated privileges. Which is also alarming.

I was doing some development work, so I ran all editions of Firefox (stable/Nightly/Developer) using an account that has UAC disabled.

Comment 21

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

Alright, I think I have tracked down the source of the problem. It actually has to do with exactly this:

I ran all editions of Firefox (stable/Nightly/Developer) using an account that has UAC disabled.

Usually, the updater runs unelevated initially and then, if necessary, it starts another instance of the updater with elevated privileges to operate on the installed files. The elevated updater saves its output (status, logs, etc) to another privileged directory. Then, when it exits, the unelevated updater examines its output to complete the final stages of the update.

But when we made this change, we changed how the updater detects if it is the first update process or the second one. We made the assumption that if the process has administrator privileges, it must be the second process run. This means that, in your case, the updater immediately thinks it is running for the second time. This causes it not to (a) on launch, change the status to show that the update is in-progress, (b) write its final status back to the original status file, or (c) start Firefox again after it exits. Normally the first updater process would do all of those things. Since we never change the status file, every time Firefox launches, it thinks it is seeing that update for the first time. And combined with missing behavior (c), this means that Firefox never launches successfully.

The audience for this is probably small because it requires that your account be set up in an unusual way. But the effect is to completely break Firefox in a way that requires either technical knowledge or reinstallation to recover from, and the trigger is updating completely normally. So this is a serious issue. I will try to get a patch up for this ASAP.

Chua Chee Wee, thank you for reporting this.

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

:nrishel, since you are the author of the regressor, bug 1917536, could you take a look?

For more information, please visit BugBot documentation.

Comment 23

[Chua Chee Wee]

But when we made this change, we changed how the updater detects if it is the first update process or the second one. We made the assumption that if the process has administrator privileges, it must be the second process run. This means that, in your case, the updater immediately thinks it is running for the second time. This causes it not to (a) on launch, change the status to show that the update is in-progress, (b) write its final status back to the original status file, or (c) start Firefox again after it exits. Normally the first updater process would do all of those things. Since we never change the status file, every time Firefox launches, it thinks it is seeing that update for the first time. And combined with missing behavior (c), this means that Firefox never launches successfully.

Thanks for explaining this and letting me understand Firefox better, and it was interesting to read the code referred above, and I see you have code to detect whether the process is launched under an admin account with UAC disabled.

I looked at the error log in the other ticket, 1922920 and it appears to contain the same content as mine!

Could you help me understand why the change referred above, was made?

Thank you!

Comment 24

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

(In reply to Chua Chee Wee from comment #23)

Could you help me understand why the change referred above, was made?

I'm afraid that since Bug 1917536 is still marked as secure, I can't really get into it.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

I've pushed a backout of bug 1917536 to the Beta branch for tomorrow's 132.0b6 build while we continue to work through this regression.

Comment 27

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

Attachment: Bug 1923376 - Keep track of argument indices with variables r=nrishel!,nalexander!

We need to add an argument to this executable, but it is made more difficult than it needs to be by the fact that we refer to arguments by number all over this code. Which, to be honest, also just makes the code harder to understand than necessary. This patch simply adds swaps out raw index values for variables in order to make this easier to understand.

There should only be one actual functionality change in this patch. This particular invocation of LaunchCallbackApp inexplicably passes the path to the callback binary as the first argument instead of the path to the callback's working directory. Presumably this is a bug.

Comment 28

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

Attachment: Bug 1923376 - Add an argument to the updater to convey which updater instance it is r=nrishel!,nalexander!

Note that this patch does not implement the proper usage of the argument. It reads the argument but then does nothing with it.

Comment 29

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

Attachment: Bug 1923376 - Use argument value to determine which updater instance is running rather than elevation r=nrishel!,nalexander!

Comment 30

[Ryan VanderMeulen [:RyanVM]]

After discussing with Robin, we've backed out the regressing change ahead of the next round of Nightly builds due to start shortly.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

We won't be attempting to re-land the patch that caused this regression this cycle. Updating the tracking flags accordingly.

Comment 32

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

Attachment: Bug 1923376 - Add MMS support for new updater args r=nrishel!,nalexander!

Comment 33

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

Attachment: Bug 1923376 - Fix "force service fallback" update test feature r=nrishel!,nalexander!

This patch basically reimplements this functionality (removed in Bug 1917536), but using the mechanism introduced by Bug 1923376 to convey which invocation the updater should run as.

Comment 34

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

I'm not really sure why you closed this bug. I am still working on a patch. The bug will be automatically closed when it merges.

Comment 35

[Donal Meehan [:dmeehan]]

Setting Fx133 to disabled since Bug 1917536 was backed out before Fx133 went to beta

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4dcbb138e052
Keep track of argument indices with variables r=nalexander,nrishel
https://hg.mozilla.org/integration/autoland/rev/20183df7b002
Add an argument to the updater to convey which updater instance it is r=nalexander,nrishel
https://hg.mozilla.org/integration/autoland/rev/0372e17b9b6d
Add MMS support for new updater args r=nrishel,nalexander
https://hg.mozilla.org/integration/autoland/rev/98cb02543c02
Use argument value to determine which updater instance is running rather than elevation r=nalexander,nrishel
https://hg.mozilla.org/integration/autoland/rev/1b17564efbdb
Fix "force service fallback" update test feature r=nrishel,nalexander

Comment 37

[agoloman]

Backed out for causing build bustages @updater.cpp.

• Backout link
• Push with failures
• Failure Log

Comment 38

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

I think I addressed the problem. Re-merging.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/258949ba2ec4
Keep track of argument indices with variables r=nalexander,nrishel
https://hg.mozilla.org/integration/autoland/rev/5b26561eae74
Add an argument to the updater to convey which updater instance it is r=nalexander,nrishel
https://hg.mozilla.org/integration/autoland/rev/7d71532633b3
Add MMS support for new updater args r=nrishel,nalexander
https://hg.mozilla.org/integration/autoland/rev/f133a856cc4a
Use argument value to determine which updater instance is running rather than elevation r=nalexander,nrishel
https://hg.mozilla.org/integration/autoland/rev/f538ea57a5f9
Fix "force service fallback" update test feature r=nrishel,nalexander

Comment 40

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/258949ba2ec4
https://hg.mozilla.org/mozilla-central/rev/5b26561eae74
https://hg.mozilla.org/mozilla-central/rev/7d71532633b3
https://hg.mozilla.org/mozilla-central/rev/f133a856cc4a
https://hg.mozilla.org/mozilla-central/rev/f538ea57a5f9

Comment 41

[Norisz Fay [:noriszfay]]

Backed out alongside Bug 1917536 for causing bustage @ updater.cpp in mozilla-central

Backout link

Push with failures

Failure log

Comment 42

[Norisz Fay [:noriszfay]]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/7dc4f9a38ea9

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/950417640ee4
Keep track of argument indices with variables r=nalexander,nrishel
https://hg.mozilla.org/integration/autoland/rev/a01206f5887c
Add an argument to the updater to convey which updater instance it is r=nalexander,nrishel
https://hg.mozilla.org/integration/autoland/rev/4e87e80d48fd
Add MMS support for new updater args r=nrishel,nalexander
https://hg.mozilla.org/integration/autoland/rev/4c7c00c2a9eb
Use argument value to determine which updater instance is running rather than elevation r=nalexander,nrishel
https://hg.mozilla.org/integration/autoland/rev/e8abed69355a
Fix "force service fallback" update test feature r=nrishel,nalexander

Comment 44

[amarc]

https://hg.mozilla.org/mozilla-central/rev/950417640ee4
https://hg.mozilla.org/mozilla-central/rev/a01206f5887c
https://hg.mozilla.org/mozilla-central/rev/4e87e80d48fd
https://hg.mozilla.org/mozilla-central/rev/4c7c00c2a9eb
https://hg.mozilla.org/mozilla-central/rev/e8abed69355a

Comment 45

[Ryan VanderMeulen [:RyanVM]]

We should get extra QA smoke testing around Nightly updates for this and bug 1917536.

Comment 46

[Bogdan Maris, Desktop Test Engineering [Away till 13th July]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #45)

We should get extra QA smoke testing around Nightly updates for this and bug 1917536.

We are done with the testing from bug 1917536 where we also covered a few scenarios with admin user and no UAC and we did not find any issues updating to latest Beta version.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

Backed out for 135.0rc1 while the investigation of bug 1937806 continues.
https://hg.mozilla.org/releases/mozilla-release/rev/983c92bf7d784b40684da7409a2f72d69a14eec0

Comment 48

[Pascal Chevrel (relman team) -> :pascalc]

Backed out from 136 beta for causing bug 1945654 preventing shipping beta 1.

Comment 49

[Donal Meehan [:dmeehan]]

When uplifting this, Bug 1946084 and Bug 1945654 will be required on ESR128/ESR115

Comment 50

[Nick Rishel [:nrishel]]

Attachment: Bug 1923376 - Keep track of argument indices with variables

We need to add an argument to this executable, but it is made more difficult than it needs to be by the fact that we refer to arguments by number all over this code. Which, to be honest, also just makes the code harder to understand than necessary. This patch simply swaps out raw index values for variables in order to make this easier to understand.

There should only be one actual functionality change in this patch. This particular invocation of LaunchCallbackApp inexplicably passes the path to the callback binary as the first argument instead of the path to the callback's working directory. Presumably this is a bug.

Original Revision: https://phabricator.services.mozilla.com/D225420

Comment 51

[Nick Rishel [:nrishel]]

Attachment: Bug 1923376 - Add an argument to the updater to convey which updater instance it is

Note that this patch does not implement the proper usage of the argument. It reads the argument but then does nothing with it.

Original Revision: https://phabricator.services.mozilla.com/D225421

Comment 52

[Nick Rishel [:nrishel]]

Attachment: Bug 1923376 - Add MMS support for new updater args

Original Revision: https://phabricator.services.mozilla.com/D225742

Comment 53

[Nick Rishel [:nrishel]]

Attachment: Bug 1923376 - Use argument value to determine which updater instance is running rather than elevation

Original Revision: https://phabricator.services.mozilla.com/D225422

Comment 54

[Nick Rishel [:nrishel]]

Attachment: Bug 1923376 - Fix "force service fallback" update test feature

This patch basically reimplements this functionality (removed in Bug 1917536), but using the mechanism introduced by Bug 1923376 to convey which invocation the updater should run as.

Original Revision: https://phabricator.services.mozilla.com/D225743

Comment 55

[Nick Rishel [:nrishel]]

Attachment: Bug 1923376 - Keep track of argument indices with variables

We need to add an argument to this executable, but it is made more difficult than it needs to be by the fact that we refer to arguments by number all over this code. Which, to be honest, also just makes the code harder to understand than necessary. This patch simply swaps out raw index values for variables in order to make this easier to understand.

There should only be one actual functionality change in this patch. This particular invocation of LaunchCallbackApp inexplicably passes the path to the callback binary as the first argument instead of the path to the callback's working directory. Presumably this is a bug.

Original Revision: https://phabricator.services.mozilla.com/D225420

Comment 56

[Nick Rishel [:nrishel]]

Attachment: Bug 1923376 - Add an argument to the updater to convey which updater instance it is

Note that this patch does not implement the proper usage of the argument. It reads the argument but then does nothing with it.

Original Revision: https://phabricator.services.mozilla.com/D225421

Comment 57

[Nick Rishel [:nrishel]]

Attachment: Bug 1923376 - Add MMS support for new updater args

Original Revision: https://phabricator.services.mozilla.com/D225742

Comment 58

[Nick Rishel [:nrishel]]

Attachment: Bug 1923376 - Use argument value to determine which updater instance is running rather than elevation r=nrishel!,nalexander!

Original Revision: https://phabricator.services.mozilla.com/D225422

Comment 59

[Nick Rishel [:nrishel]]

Attachment: Bug 1923376 - Fix "force service fallback" update test feature

This patch basically reimplements this functionality (removed in Bug 1917536), but using the mechanism introduced by Bug 1923376 to convey which invocation the updater should run as.

Original Revision: https://phabricator.services.mozilla.com/D225743

Comment 60

[Pascal Chevrel (relman team) -> :pascalc]

This bug is backed out on 137 beta, thanks.

Comment 61

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/0c57a047bf22
https://hg.mozilla.org/releases/mozilla-esr128/rev/7fc4ca2e979d
https://hg.mozilla.org/releases/mozilla-esr128/rev/8eec7c9bac05
https://hg.mozilla.org/releases/mozilla-esr128/rev/e4b27a1ee733
https://hg.mozilla.org/releases/mozilla-esr128/rev/9bf15fee3367

Comment 62

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/19e9e7a746b2
https://hg.mozilla.org/releases/mozilla-esr115/rev/a651d518d232
https://hg.mozilla.org/releases/mozilla-esr115/rev/b906f2b567bb
https://hg.mozilla.org/releases/mozilla-esr115/rev/63446c44877e
https://hg.mozilla.org/releases/mozilla-esr115/rev/1dcad182ec2f

Comment 63

[Bogdan Maris, Desktop Test Engineering [Away till 13th July]]

After discussing on slack with :nrishel there will be no manual QA needed here due to complications, we'll rely on automated tests. Dropping qa-verify+