1900717 - Unite Update Service and Update Manager

Status: NEW

(Toolkit :: Application Update, defect)

Description

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

The update mutex prevents multiple Firefox instances from simultaneously driving Application Update. But it is really only used to guard driving updates and not other kinds of update directory modifications, like running post update processing for example. This sort of thing could cause an instance without the mutex to actually interfere with an instance with the mutex that is trying to drive update. We should more carefully guard writes to the update directory behind the update mutex.

Comment 1

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

It looks like the post update processing thing that I was worried about is actually not an issue. I believe I was tricked by this into thinking that we needed to be more careful in the rest of this function. But now that I look more carefully, I see that we immediately return if !canCheckForUpdates, here. Since that check includes a mutex check, it looks like this function actually just doesn't run at all without the mutex.

I am, however, concerned about this function. This runs on update service initialization and reads and writes to/from update directory state files. I'm not fully sure what the behavior ought to be here when we don't have the mutex. I think we should definitely read the update history (updates.xml). I'm kind of inclined to say that maybe we shouldn't even read the active update state. It's irrelevant to us until we get the mutex, and it might have changed by then. I do kind of feel like we need a mechanism that reloads that data if we didn't have the mutex on our first pass through that function. This sounds tricky to get right though. We don't really want something like this happening

let downloadingUpdate = AUS.getDownloadingUpdate();
// `downloadingUpdate` is null because we don't have the mutex
if (!downloadingUpdate) {
  checkForUpdate();
  // `checkForUpdate` successfully acquires the mutex, causing `downloadingUpdate` to be reloaded to contain an update.
  // We shouldn't really have entered this block at all.
}

Note that adding a reload mechanism doesn't introduce this problem; it very much already exists. Right now we wouldn't really do any better, we just don't reload the data at all. Meaning that we will likely eventually overwrite it, possibly abandoning an in-progress download. This could potentially cause problems like Bug 1856462.

I have a fix in mind for this, but it would be a bit involved. We could split out the bits of the interface that need and don't need the mutex. Then we could make the bits that need the mutex accessible only through those interfaces. So using it would look a bit more like:

// This is fine and doesn't need the mutex
const history = await UpdateManager.getHistory();

const ActiveUpdateManager = await UpdateManager.getActive();
if (ActiveUpdateManager) {
  // Since we know that everything is initialized and ready at this point, `downloadingUpdate`
  // could even go back to being a getter instead of asynchronous
  const downloadingUpdate = ActiveUpdateManager.downloadingUpdate;
} else {
  console.log("We probably don't have the update mutex yet")
}

Oh. Looking back at post update processing, it's not actually ideal that it also ends up checking isOtherInstanceRunning via canCheckForUpdates. We probably should still do post update processing in that case.

Comment 2

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

This is made more complicated than it needs to be by the fact that the Update Manager and Update Service are different entities that both need the update mutex. I've really never seen a good reason for it. I almost wonder if this could be a good excuse to unite them.

Comment 3

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

Attachment: WIP: Bug 1900717 - Pre: Clean up `SelfContainedPromise` usage since an equivalent standard function now exists r=nalexander!,nrishel!

Comment 4

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

I'm actually going to split this bug in pieces since I think there are segments that can and should land separately. This bug is now going to track just uniting nsIUpdateManager and nsIApplicationUpdateService.

I'll file another bug dependent on this one to track the remaining work.

To be clear, the work in this bug is necessary in order to start the remaining work. It is too confusing to try to enact my plan for how to split this class into "mutex-protected" and "not" if I have to split two classes and synchronize them.

Comment 5

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

Looking at the work necessary, I think I may also have an opportunity to make the update system more modular here. And if I have it, I think I am going to take it. This has become a bit of a spaghetti mess over the years. Teasing apart bits of the monolith into discrete components would really help this whole thing to be more reasonable to work with, I think.