1720993 - Background update disables launcher process

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[mozbugz]

After enabling the background update feature and running the task from task scheduler, Firefox reports in about:support that "Launcher Process Disabled due to failure".

If I enable the launcher process by flipping the pref off and on again, it gets enabled like it should, but gets disabled again after running the background update task.

Comment 1

[Nick Alexander :nalexander [he/him]]

Thanks for the report! We'll definitely investigate. This is almost certainly connected to https://bugzilla.mozilla.org/show_bug.cgi?id=1689481. I wonder if we're truly disabling the launcher process, or if the state is simply reported incorrectly.

bytesized: NI to you to dig into this next week.

Comment 2

[Robin Steuber (they/them) [:bytesized]]

I'm looking into it. Unfortunately, I can reproduce this outside of a debugger, but not (yet) in one. So this might take some time to figure out.

Comment 3

[Robin Steuber (they/them) [:bytesized]]

Ah, I think I have pinned down the problem. The problem is essentially that ERROR_ACCESS_DENIED is being returned here. Apparently we do not have whatever permissions are required to query information about the process that launches scheduled tasks.

I've talked to some coworkers that say that they do not seem to be experiencing this problem. I'm not really sure why. Maybe it has something to do with the Task Scheduler internals? Hard to say. We do properly handle things if the parent process has already exited, so maybe that's what's going on?

As to fixing this, it seems me that we should assume that we will have the necessary permissions to query launcher process. So if we get an Access Denied error, we should probably assume that the parent process is not the same binary and so IsSameBinaryAsParentProcess should return false.

But we also want to address the issue of users that have had the launcher process improperly turned off because of this. We could just turn it back on again, but that is not ideal for situations where it is turned off for a good reason. AFAICT we can't tell why we turned it off, but I believe that we could check the timestamp on the registry entry that disabled it. I think that if that timestamp is dated after the Background Update rollout started, we should just go ahead and turn the launcher process back on.

I'll try to have a patch up for this as soon as possible.

Comment 4

[Robin Steuber (they/them) [:bytesized]]

(In reply to Kirk Steuber (he/him) [:bytesized] from comment #3)

But we also want to address the issue of users that have had the launcher process improperly turned off because of this.

Oh, apparently this part is unnecessary because of this. I've been told that we intentionally turn this back on after every update in the hope that the update fixes the problem that caused the launcher process failure. I didn't know about that. That makes this patch much simpler.

Comment 5

[Robin Steuber (they/them) [:bytesized]]

Attachment: Bug 1720993 - Assume that a parent process that we don't have access to is not the same binary r=aklotz

This patch doesn't cover turning the launcher process back on, because that already happens automatically during the PostUpdate process.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f05832afe8f1
Assume that a parent process that we don't have access to is not the same binary r=aklotz,tkikuchi

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f05832afe8f1

Comment 8

[Robin Steuber (they/them) [:bytesized]]

Comment on attachment 9232441 [details]
Bug 1720993 - Assume that a parent process that we don't have access to is not the same binary r=aklotz

Beta/Release Uplift Approval Request

• User impact if declined: A large number of Windows users have had their launcher process disabled due to this bug. Until it is patched, the launcher process will most likely continue to be unusable for them. This patch should address the issue and also prevent it from impacting more users.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a very small patch that just changes the error handling for one error code that a system call potentially returns.
• String changes made/needed:

Comment 9

[Pascal Chevrel:pascalc (PTO until April 26)]

(In reply to Kirk Steuber (he/him) [:bytesized] from comment #8)

Kirk, this bug has no priority/severity set, it's not marked as a regression and there was no tracking flag requesting tracking for the 91 release. It's not clear to me how important it is to have it in 91 and what the actual end user impact of having the launcher process deactivated is. Does it mean that Firefox is no longer usable? Do you have an idea of how many people are impacted?

Even though the patch is small, it touches code at startup time, it has no automated test, there is no request for manual validation from our QA team and it just landed on nightly. For these reasons I am inclined not to take it in our final beta build before RC on Monday so as to not risk unknown regressions it may cause, but I will wait on your additional feedback on the issue as I don't fully understand the impact of not taking it before taking a decision (in this bug or slack/matrix if you prefer). Thanks!

Comment 10

[Robin Steuber (they/them) [:bytesized]]

(In reply to Pascal Chevrel:pascalc from comment #9)

It's not clear to me how important it is to have it in 91 and what the actual end user impact of having the launcher process deactivated is. Does it mean that Firefox is no longer usable?

No, it does not mean Firefox is not usable. The launcher process' main purpose, as I understand it, is to attempt to prevent unwanted DLL injections. We get a lot of programs (especially antivirus programs) attempting to inject code into Firefox. This injected code is sometimes buggy, or relies on Firefox internals that are not necessarily stable, causing crashes. People typically are not aware that their antivirus is doing this, so they blame the crashes on us.

Do you have an idea of how many people are impacted?

It seems like quite a few people have been impacted. This feature has not been completely deployed on Release yet, but here are some results showing launcher process failures on Beta. For comparison, this shows the deployment of the Background Update feature to Beta. Note that it is difficult to compare numbers exactly here because AFAICT multiple launcher process failures can be sent per user and the deployment graph only represents 1% of the population. But the timelines do seem to match closely and the number of launcher process failures seems quite high.

Comment 11

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/68ec3b4d7a25