1696276 - When processing updates in a background task, ensure that no UAC prompt is displayed and that the backgroundtask is restarted

Status: RESOLVED FIXED

(Toolkit :: Application Update, task)

Description

[Nick Alexander :nalexander [he/him]]

After Bug 1695797, firefox --backgroundtask ... will not process updates when another instance of Firefox is running. This ticket seeks to improves this experience yet further.

1. We never want to display UI or raise a UAC prompt from a background task. We should add a flag to the updater invocation in ProcessUpdates when we're in background task mode to ensure we do neither.

   I'm hopeful that the updater can just leave any pending update in place when it would show UI or a UAC prompt without major modifications. But it's possible this work may require adding new exit codes or error status codes to the updater.

   It's also possible that we should add such a flag to other updater invocations when in background mode as well, since it's not clear to me if any other invocations would show UI or a UAC prompt. I think not, since they're all intended for use within a regular headed Firefox session.

2. If we do have an update to process, we want to ensure that we don't launch a regular headed Firefox session, but instead we relaunch the background task (with any additional arguments). This is managed by these gRestartArg* parameters. This may already Just Work.

For both of these things, arranging the test environment is probably the hardest part. I'm hopeful that some of the existing AUS tests can be modified for these use cases.

Comment 1

[Nick Alexander :nalexander [he/him]]

agashlin: can you dig into this? My guess is that verifying 2. is probably straightforward:

1. arrange a pending update;
2. start firefox --backgroundtask success;
3. see what gets launched and how the relevant arguments are constructed.

It might even be that a test for this almost exists in tree, because we have an affordance for dumping relaunched arguments with --test-process-updates which should basically work with a background task. (Except, of course, that after Bug 1695797 we'll need to have the host xpcshell test call resetLock(...) so that it doesn't exclude the background task from processing updates!)

I'm less sure exactly where 1. may lead. Investigation appreciated!

Comment 2

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

The restart seems to work, though in the CheckArg call we need to pass CheckArgFlag::None to prevent it from removing the flag before it can get into gRestartArgv. If that gets removed, and it just runs firefox from an initial firefox --backgroundtask success, Firefox startups up normally, but in a weird headless state (no windows).

As for setting "no UI mode" (no progress UI, no UAC), it'd be good for whatever method is used to be passed through the maintenance service to the elevated updater[1].

A few possibilities:

• Add another arg to the updater. I'm not enthusiastic given how unwieldy that command line already is, and the maintsvc needs to parse it, too.
• Check that if callback app is given a backgroundtask switch, that should be a reliable clue. Though it feels awkward peeking into the callback portion of the command line. Is the callback ever not the main app? I'm favoring this one.

Any other ideas? I was thinking we could pass an environment variable, but that would never make it to the service.

---

Comment 3

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

(footnote meant for comment 2)

[1] On Windows the SYSTEM updater tries to show the progress UI, though it seems like it doesn't succeed (probably because there's all these restrictions and even then it's still just on session 0.)

When staging or replacing the UI isn't shown, except if it's a replace request and the service takes too long, or when elevated on OSX.

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to Adam Gashlin (he/him) [:agashlin] from comment #2)

The restart seems to work, though in the CheckArg call we need to pass CheckArgFlag::None to prevent it from removing the flag before it can get into gRestartArgv. If that gets removed, and it just runs firefox from an initial firefox --backgroundtask success, Firefox startups up normally, but in a weird headless state (no windows).

Yes, this weird headless state would probably be because the launched Firefox inherits MOZ_HEADLESS: see a similar ticket for devtools.

As for setting "no UI mode" (no progress UI, no UAC), it'd be good for whatever method is used to be passed through the maintenance service to the elevated updater[1].

A few possibilities:

• Add another arg to the updater. I'm not enthusiastic given how unwieldy that command line already is, and the maintsvc needs to parse it, too.

As per a Zoom discussion, bumping the MMS is challenging because the command line parsing there needs to be robust across many versions, etc.

• Check that if callback app is given a backgroundtask switch, that should be a reliable clue. Though it feels awkward peeking into the callback portion of the command line. Is the callback ever not the main app? I'm favoring this one.

Also per our discussion, this feels like the most sensible approach. It localizes changes to the updater, reducing churn and risk around the MMS.

The last open question is whether we need to signal an error or we can just leave everything in the pending state. Presumably pending is fine?

Comment 5

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

(In reply to Nick Alexander :nalexander [he/him] from comment #4)

The last open question is whether we need to signal an error or we can just leave everything in the pending state. Presumably pending is fine?

Yeah, I think leaving it pending should work.

---

I think we should also quit in the background if we're not able to lock the callback exe. We rename the exe and proceed since bug 715746 so we're not stuck unable to update, but it would be good to be extra cautious here when the user can't debug something that happened in the background. Bug 1695797 tries not to update when a background task is launched unless it is the only instance, but multi-instance locks are not failsafe, so I'd prefer this extra bit of caution. It's a little bit of a stretch for this bug so I can open another one, but it'd be using the same bit of state so it seemed close enough.

:nalexander does this sound appropriate?

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Adam Gashlin (he/him) [:agashlin] from comment #5)

(In reply to Nick Alexander :nalexander [he/him] from comment #4)

The last open question is whether we need to signal an error or we can just leave everything in the pending state. Presumably pending is fine?

Yeah, I think leaving it pending should work.

\o/

I think we should also quit in the background if we're not able to lock the callback exe. We rename the exe and proceed since bug 715746 so we're not stuck unable to update, but it would be good to be extra cautious here when the user can't debug something that happened in the background. Bug 1695797 tries not to update when a background task is launched unless it is the only instance, but multi-instance locks are not failsafe, so I'd prefer this extra bit of caution. It's a little bit of a stretch for this bug so I can open another one, but it'd be using the same bit of state so it seemed close enough.

:nalexander does this sound appropriate?

Yes, and a separate ticket sounds good. Thanks for raising and addressing these edge cases!

Comment 7

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

(In reply to Adam Gashlin (he/him) [:agashlin] from comment #5)

(In reply to Nick Alexander :nalexander [he/him] from comment #4)

The last open question is whether we need to signal an error or we can just leave everything in the pending state. Presumably pending is fine?

Yeah, I think leaving it pending should work.

Pending won't work, we'll get into an update loop if we try to run the callback to let the background task run[1]. I added a new error status for this, so if UAC is needed we won't be able to do an update until the update service resets the status. We should be able to clear it and reset to pending once we're in the background task, but I don't want to keep blocking this work on all the details of that.

[1] We don't actually get into an infinite loop, for complicated reasons: The updater starts Firefox, which starts another updater. Because there are two updaters, the second one decides to use the secure output paths because it can't remove the "elevated lock file" (this is an unfortunate name, it is actually held by the unelevated updater, if it can't be locked that's how the elevated updater knows it's elevated). So when it gets to setting status to "applying", it fails to write the secure status file, and exits without trying to run the callback.

Comment 8

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Bug 1696276 - Don't display UAC prompt or progress UI from a background task. r?nalexander!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3087bdaae72c
Don't display UAC prompt or progress UI from a background task. r=application-update-reviewers,nalexander

Comment 10

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/3087bdaae72c