1922395 - Turn the Post Update Processing function into the Update Service initialization function

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect, P1)

Description

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

_postUpdateProcessing (here) already does a bit more than just what is necessary post-update. It makes sure we are in a good state, restarts in-progress downloads, and more. In fact, a good description of what this does is that it initializes the update service. There is really only one initialization state AFAIK that it doesn't handle very well: starting up with an update pending installation.

The historical reasons for this are that post update processing only runs right at startup and valid pending updates are always installed at startup. These reasons, however, no longer really hold true. Since Bug 1875502, post update processing can be run at times other than startup. And background tasks do not necessarily install updates at startup. Though the background tasks risk is somewhat mitigated by the fact that most background tasks shouldn't be running update.

But this becomes even more important with Bug 1907127 since that introduces a mechanism for intentionally starting Firefox up without installing a pending update.

All of which is to say that I'm going to rename _postUpdateProcessing and add proper support for running it when an update is pending.

---

Semi-relatedly, when I was reevaluating _postUpdateProcessing's fitness for being an initialization routine just now, I realized there is a potentially sort of nasty bug in that function. In many cases it may call cleanupActiveUpdates or cleanupDownloadingUpdate, but we never look for and attempt to cancel the BITS job that might be associated with the downloading update. I wonder if that could be the cause of Bug 1856462. I guess I'll try to fix that while I'm here.

Comment 1

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

Attachment: Bug 1922395 - Turn Post Update Processing into initialization function r=nrishel!,nalexander!

This not only renames the post update processing function. It also adds support for initializing with a pending update and ensures that BITS jobs are cancelled before cleaning them up.

Comment 2

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

Attachment: Bug 1922395 - Refactor BITS request creation into its own function r=nrishel!,nalexander!

This will allow us to call into this functionality more easily to reconnect to and cancel an existing job during cleanup or testing.

This also adds support for connecting to a BITS job without attaching a listener, since cleanup and tests will want to be able to do that.

Comment 3

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

Attachment: Bug 1922395 - Always try to cancel any associated BITS job before cleaning up a downloading update r=nrishel!,nalexander!

Comment 4

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

Attachment: Bug 1922395 - Add tests to verify update init fixes r=nrishel!,nalexander!

We test that, on init, we only clean up invalid pending updates. We also test that BITS jobs are cleaned up when we clean up the corresponding downloading update during initialization.

Comment 5

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

Attachment: Bug 1922395 - Correctly identify downloading active update r=nrishel!,nalexander!

Adding testing for Bug 1922395 incidentally revealed that we over-trust update.status when determining which update is which.

Comment 6

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

Attachment: Bug 1922395 - Fix tests broken by preserving pending updates at startup r=nrishel!,nalexander!

These tests need to be fixed for some slightly complicated reasons which I will attempt to explain below.

invalidArg* tests

These tests were added to address an exploit (Bug 1342742) that was achieved by passing invalid values to the updater. The test patch (https://hg.mozilla.org/mozilla-central/rev/5b52d7c9212c) added all these invalid value tests that were mostly quite similar to each other.

But these tests are a bit odd. In many cases, we are testing a fairly contrived situation in order to be able to test the maintenance service. So the changes made here to these tests don't represent a change in tested behavior, just a change of how the contrived situation works.

Three of the invalid arg tests, however, (invalidArgPatchDirPath*) create and test for a different previously impossible situation that is now possible. They simulate the attack case by running the updater in an invalid way (as the attacker would) and then simulating a Firefox launch by reloading the update service and update manager to test how Firefox reacts to the attack.

In the other invalid arg tests, invoking the updater binary (with the exploit fixed) maliciously like this results in an error and it makes sense for Firefox to clean that up at startup, as they do when not using the service. But, in these 3 cases, the attacker didn't give the updater enough information to communicate the error back to the browser (since it would be communicated via files in the patch directory and a bad patch directory was provided).

In this case, running the initialization of the update system doesn't properly reproduce what would actually happen after an attack like this. The browser was left in a state with a potentially valid pending update in a pending state. At startup, we would have attempted to install it, invoking the updater again with the correct arguments and ultimaitely resulting in either a success or a failure state.

Now that it is possible to not apply updates at startup, it actually is reasonable to initialize the update service with an update pending, as this test simulates. And, in this case, we want the update to be preserved, not cleaned up. So these tests are being changed to reflect that expectation.

InUseStageFailure tests

These tests legitamately confuse me and I think are wrong in kind of a similar way to the previous situation. Compare these tests, especially marAppInUseStageFailureComplete_win, to marAppInUseSuccessComplete. It looks to me like they all are setup the same way, by artificially locking up a file. Then marAppInUseSuccessComplete installs that update normally, which works fine. And the other tests stage first and then fail on the replace request and the update gets cleaned up.

Why would that be desirable behavior that we would test for? If we are perfectly capable of installing this update, why have a test to ensure that we cannot? Seems odd. We know that, in this case, the update binary exits with a "pending" status, which is pretty unusual. I believe that that must be happening here. That code has a comment that, I believe, helpfully explains the weirdness going on here and why this test is so odd. It says:

// When attempting to replace the application, we should fall back
// to non-staged updates in case of a failure.  We do this by
// setting the status to pending, exiting the updater, and
// launching the callback application.  The callback application's
// startup path will see the pending status, and will start the
// updater application again in order to apply the update without
// staging.

And, yeah, that's basically exactly what's happening in these tests. Right up to the part where the callback application invokes the updater again, which the testing callback application does not do. Instead, the test simulates a startup by re-initializing the update system. I think that this test was, oddly, testing behavior that would never actually happen - what would happen if things happened as the above comment described, but the callback application didn't re-invoke the updater. Now, it is actually possible for this to happen because the application is being changed such that it actually is possible to startup with updates pending rather than unconditionally applying them in that case. But I think that when this test code was written, it was testing for a failure mechanism that was effectively impossible.

But, even though that's what the test was testing for, it was never the behavior we actually wanted. We can install updates in this case. We should not clean up an update that might still be successfully installed. So these tests are being adjusted to test for the behavior actually desired.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14a04ceafba4
Refactor BITS request creation into its own function r=nrishel,nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/c10e23cb1377
Turn Post Update Processing into initialization function r=nrishel,nalexander
https://hg.mozilla.org/integration/autoland/rev/abf0928161d6
Always try to cancel any associated BITS job before cleaning up a downloading update r=nrishel,nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/170b465d0f64
Add tests to verify update init fixes r=nrishel,nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/3332e79d10cd
Correctly identify downloading active update r=nrishel,nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/ef1edab6a75b
Fix tests broken by preserving pending updates at startup r=nrishel,nalexander,application-update-reviewers

Comment 8

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/14a04ceafba4
https://hg.mozilla.org/mozilla-central/rev/c10e23cb1377
https://hg.mozilla.org/mozilla-central/rev/abf0928161d6
https://hg.mozilla.org/mozilla-central/rev/170b465d0f64
https://hg.mozilla.org/mozilla-central/rev/3332e79d10cd
https://hg.mozilla.org/mozilla-central/rev/ef1edab6a75b