Remove scheduled task(s) in uninstaller (including backgroundupdate task)
Categories
(Toolkit :: Application Update, task)
Tracking
()
Tracking | Status | |
---|---|---|
firefox90 | --- | fixed |
People
(Reporter: agashlin, Assigned: bytesized)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 5 obsolete files)
Scheduled tasks added via the task scheduler component should be removed on uninstall. Currently the uninstaller removes the scheduled task for the WDBA by asking it to uninstall itself, but that only deletes the WDBA's own task.
Possible paths:
- The only current mechanism to remove all scheduled tasks for an installation is via TaskScheduler.deleteAllTasks. This could be run from the uninstaller, though this would rely on being able to run firefox.exe in a limited capacity (likely via the background task mechanisms).
- The WDBA could be extended to remove any task matching the token. This would rely on the presence of the WDBA, and is slightly outside of its scope, but it would be the easiest change to implement.
- Write an NSIS plugin just for cleaning up scheduled tasks, similar to BitsUtils for removing BITS jobs.
- Write the whole task cleanup in NSIS. There is sample code, our usage would be even more involved as it would need to enumerate the tasks in the folder.
Comment 1•3 years ago
|
||
- The only current mechanism to remove all scheduled tasks for an installation is via TaskScheduler.deleteAllTasks. This could be run from the uninstaller, though this would rely on being able to run firefox.exe in a limited capacity (likely via the background task mechanisms).
I think this makes the most sense. I see little sense coupling any of this to the WDBA; that will just make changing both of the sides of the puzzle harder. And adding anything to NSIS seems foolish as well.
Ultimately, we need to trust firefox --backgroundtask ...
to actually work, including for tasks like these.
Comment 2•3 years ago
|
||
One awkward thing: it's possible to build without MOZ_BACKGROUNDTASKS
, but still use TaskScheduler.jsm
. I'm not particularly fussed about that, but it is something to mention in commit comments, etc.
Reporter | ||
Comment 3•3 years ago
|
||
(In reply to Nick Alexander :nalexander [he/him] from comment #1)
- The only current mechanism to remove all scheduled tasks for an installation is via TaskScheduler.deleteAllTasks. This could be run from the uninstaller, though this would rely on being able to run firefox.exe in a limited capacity (likely via the background task mechanisms).
I think this makes the most sense.
Agreed.
Ultimately, we need to trust
firefox --backgroundtask ...
to actually work, including for tasks like these.
Yeah, though I brought this up because uninstalls can happen when the install is busted in one way or another. That's what makes 4 attractive, only the uninstaller needs to be intact. If it was less of a mess to implement that'd be more reliable.
The uninstaller is removing firefox.exe anyway, so failing to remove a task shouldn't be too harmful. The worst case is a subsequent new install in the same location, which might run both the old task and a newly created one that the new instance doesn't know about, if they don't have the same name. We could protect against this by removing any tasks upon a fresh install, would a firefox --backgroundtask removescheduledtasks
work before first run? Or whatever schedules the tasks could run deleteAllTasks
before registering all the ones it intends to have running (which has the benefit of also working on macOS where there is no installer/uninstaller).
Removing the .exe is the first thing the uninstaller does, so running the background task would need to be before that.
A background task will try to run a pending update, which could get messy. We could remove the update dir earlier to prevent that.
(In reply to Nick Alexander :nalexander [he/him] from comment #2)
One awkward thing: it's possible to build without
MOZ_BACKGROUNDTASKS
, but still useTaskScheduler.jsm
. I'm not particularly fussed about that, but it is something to mention in commit comments, etc.
Yeah, and there's the same issue with MOZ_DEFAULT_BROWSER_AGENT
for 2. Doesn't seem like a blocker.
Reporter | ||
Comment 4•3 years ago
|
||
(In reply to Adam Gashlin (he/him) [:agashlin] from comment #0)
- Write the whole task cleanup in NSIS. There is sample code, our usage would be even more involved as it would need to enumerate the tasks in the folder.
I don't intend to implement this, but for completeness: That sample code was using the old Task Scheduler 1.0 interface. Anders provides an example with Task Scheduler 2.0 here (though again this doesn't handle enumeration).
Comment 5•3 years ago
|
||
The more I think about this, the more I want to restrict the backgroundupdate task scheduling to installations installed by the Windows installer. I'm worried about things like mozregression scheduling lots of OS-level tasks, etc. (I'm assuming mozregression unpackages rather than installs.)
Comment 6•3 years ago
|
||
This probably should have been part of the cleanup commit
https://hg.mozilla.org/mozilla-central/rev/673745818480.
Comment 7•3 years ago
|
||
Depends on D110783
Comment 8•3 years ago
|
||
This is best effort. For example, we can't handle the situation where
multiple OS-level users have scheduled tasks from a previous
installation.
Depends on D110784
Comment 9•3 years ago
|
||
It's easy to test the background task from the command line, and to test if we delete tests on uninstall. I hand-hacked testing removal on first startup.
Updated•3 years ago
|
Comment 10•3 years ago
|
||
It makes sense to accommodate exceptions in deleteAllTasks
: since
we're trying to clean up, do as much as possible. It's possible that
we should catch, store, and re-throw any exception rather than trying
to delete a second time, but it's not clear how that re-thrown
exception will present in the browser console, etc.
We special case the WDBA task in the task scheduler implementation
rather than having consumers accommodate it because the task is the
outlier that behaves differently from the rest of the task scheduler,
and because we don't anticipate additional outlier tasks in the
future.
Depends on D110784
Updated•3 years ago
|
Updated•3 years ago
|
Comment 11•3 years ago
|
||
(In reply to Nick Alexander :nalexander [he/him] from comment #5)
The more I think about this, the more I want to restrict the backgroundupdate task scheduling to installations installed by the Windows installer. I'm worried about things like mozregression scheduling lots of OS-level tasks, etc. (I'm assuming mozregression unpackages rather than installs.)
For what it's worth I haven't done this as part of this ticket. We'll keep thinking about this to determine if it's necessary, and file follow-up if it seems wise.
Comment 12•3 years ago
|
||
Comment on attachment 9213516 [details]
Bug 1698593 - Pre: Use EXIT_CODE in BackgroundTask_{success,failure}. r?#application-update-reviewers
Revision D110783 was moved to bug 1705281. Setting attachment 9213516 [details] to obsolete.
Comment 13•3 years ago
|
||
Comment on attachment 9215638 [details]
Bug 1698593 - Part 2: Accommodate exceptions in deleteAllTasks
; don't try to delete WDBA task. r?agashlin
Revision D111947 was moved to bug 1705281. Setting attachment 9215638 [details] to obsolete.
Comment 14•3 years ago
|
||
Comment on attachment 9213518 [details]
Bug 1698593 - Part 3: Remove any scheduled task(s) on first startup. r?agashlin!,mconley!
Revision D110785 was moved to bug 1705281. Setting attachment 9213518 [details] to obsolete.
Updated•3 years ago
|
Comment 15•3 years ago
|
||
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 16•3 years ago
|
||
:agashlin pointed out on Phabricator that the uninstaller typically runs elevated, and it is difficult to drop our privileges properly. I am quite reluctant to run Firefox with Administrator privileges. I don't see anything specific that could go wrong, but Firefox is a complicated binary with a large attack surface, so running it as an Administrator is risky.
My first choice here would be to have code specifically intended for the uninstaller to remove scheduled tasks. This would probably end up looking like option 3 or 4 from :agashlin's description for this bug. However, we are on a fairly tight timeline here, and that approach looks pretty time consuming.
The WDBA uninstallation already runs with Administrator privileges, and the binary is much simpler and thus has a much smaller attack surface. Therefore, I want to do something like option 2 from :agashlin's bug description, and have the WDBA remove these tasks.
However, I'm not super excited about coupling background update with the WDBA this way. We aren't 100% sure that we won't want to get rid of the WDBA some day, or that there won't be situations where we want to build with Background Update enabled, but the WDBA disabled. So I'm going to go ahead and file a bug now to consider decoupling these from each other at a later time.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 17•3 years ago
|
||
Comment 18•3 years ago
|
||
Pushed by ksteuber@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/47e9b99309fe Uninstall the Background Update Task from the Firefox uninstaller r=agashlin
Comment 19•3 years ago
|
||
bugherder |
Updated•3 years ago
|
Updated•3 years ago
|
Description
•