That is bug 1517718

I think I want to take this one on, with fixes for bug 717877 coming as a ride-along. That bug wants XHR, but I'm thinking of using the Fetch API instead.

That said, the getter for UpdateManager.prototype._updates will make this harder than one might expect. I don't think a getter function can be async. On the flip side, we have _updatesDirty to tell us when it's time to refresh it.

This is going to get ugly. The question is how ugly do we want this to be. I see a couple different ways to do this, and I want to consult with someone before I go too much further.

The problem is the nsIUpdateManager interface is not just written for synchronous operations, it's used synchronously in many places on several different operations, especially tests.

Options:

1. Add a Promise waitForPending() method to nsIUpdateManager, which will return either a resolved Promise or a pending one, depending on if work is in progress. Then callers to the UpdateManager must await um.waitForPending() before calling the synchronous API's.
2. Convert every member of nsIUpdateManager to be a method returning a Promise.

The good news is there's only one C++ caller directly into the update manager that I found, and that one is ultimately just triggering an observer service notification (nsUpdateDriver.cpp, refreshUpdateStatus()). That one we can probably get away with not returning a Promise.

The bad news is I haven't looked yet at whether I would have to convert other parts of UpdateService.jsm to be Promise-based as well.

needinfo: which way should I go on this? Is there a better option that I'm missing?

Note to self: comm-central will be impacted too, and just as badly.

Proposed plan of attack for UpdateManager

This started as something that looked easy: remove the last consumer of DOMParser.parseFromStream() in Firefox code. (Bug 717877)

To do this, UM__loadXMLFileIntoArray has to be made asynchronous. Unfortunately, it's used in the constructor
for the UpdateManager service to define the initial active update. Worse is that getUpdateAt and getUpdate also call
UM__loadXMLFileIntoArray via the .updates getter, at least once.

My first foray into this involved converting most methods of nsIUpdateManager into methods that return Promises. However,
that very quickly spirals out of control, as the update manager's properties are referenced very heavily throughout browser,
toolkit, and in a few places in comm-central. Simply put, what should be a small change rapidly explodes into a massive
undertaking that would drive both me and reviewers nuts. There's no small way to do this.

My second foray involved defining UpdateManager.initialized, a Promise which UpdateManager will resolve when both _updates
and _activeUpdate have been initially defined. That also started getting complicated.

My third attempt involves defining a nsIUpdateManagerInit service as a singleton, with one readonly attribute, a Promise for
the initialized UpdateManager. This is an order of magnitude less than the second attempt above (one entry point versus seven),
but it still gets ugly.

In all of these cases, we have to make the call stacks asynchronous as well. This is a problem of logistics, so I suggest the following sequence:

Convert the call stacks to asynchronous functions.

• No changes to the functions, just finding them and marking them async.
• End on an event listener or event handler.
• End when we hit an async caller.
• Probable changes to .idl files down the stack.
  • nsIBrowserHandler.defaultArgs -> nsBrowserContentHandler.getArgs() -> let update = UpdateManager.activeUpdate;
  • Unknown yet how many more there are.
• Special case for toolkit/xre/nsUpdateDriver.cpp, nsUpdateProcessor::UpdateDone() needs a MozPromise in case something goes wrong

Introduce UpdateManagerInit as a replacement service.

• readonly attribute Promise updateManager;
• Replace all first references to UpdateManager with await UpdateManagerInit.updateManager;
• UpdateManagerInit: get updateManager() { return UpdateManager.initialize(); }
• UpdateManager.initialize():
  • if (this._initializedPromise) return this._initializedPromise;
  • this._initializedPromise = Promise.all for:
    • async UpdateManager.initialActiveUpdate() (UpdateManager's current constructor)
    • async UpdateManager.initialUpdates()
  • return this._initializedPromise;

Start converting functions in UpdateManager to asynchronous.

• Replace observe method and nsIObserver references for "um-reload-update-data" (only used in tests)
• Bug 717877, remove DOMParser::parseFromStream from UM__loadXMLFileIntoArray (code attached to that bug)
• Use OS.File, fetch API's as appropriate to replace synchronous file input/output.

Ultimately, all of this work will be necessary in one form or another to complete just the UpdateManager portions of UpdateService.jsm.

I'm looking for general comments on this plan before I start filing bugs to do this work. It's just far too much to take in one or even three Phabricator patches. I'm willing to do the work.

Snippet for nsUpdateDriver.cpp:

void nsUpdateProcessor::UpdateDone() {
  MOZ_ASSERT(NS_IsMainThread(), "not main thread");

  nsCOMPtr<nsIUpdateManagerInit> um =
      do_GetService("@mozilla.org/updates/update-manager-init;1");
  if (um) {
    RefPtr<mozilla::dom::Promise> p;
    um->RefreshUpdateStatus(getter_AddRefs(p));
    MOZ_DIAGNOSTIC_ASSERT(
      p,
      "UpdateManagerInit.refreshUpdateStatus() didn't return a promise"
    );

#ifdef DEBUG
    MozPromise<bool, nsresult, false>::FromDomPromise(p)
      ->Then(
        GetMainThreadSerialEventTarget(), __func__,
        [](bool) {
          // do nothing
        },
        [](nsresult) {
          MOZ_ASSERT_UNREACHABLE("UpdateManagerInit threw in refreshUpdateStatus");
        }
      );
#endif

  }

  ShutdownWatcherThread();
}

Yes, I know I didn't mention the rest of nsUpdateService.jsm, and for that I apologize. I've been excessively focused on the update manager.

I will make the UpdateManager work a new bug which blocks this and do the previous comment's plan there, if module owners sign off.

(In reply to Alex Vincent [:WeirdAl] from comment #7)

My third attempt involves defining a nsIUpdateManagerInit service as a singleton, with one readonly attribute, a Promise for
the initialized UpdateManager.

I think this is a solid plan.

Convert the call stacks to asynchronous functions.

• No changes to the functions, just finding them and marking them async.
• End on an event listener or event handler.
• End when we hit an async caller.
• Probable changes to .idl files down the stack.
  • nsIBrowserHandler.defaultArgs -> nsBrowserContentHandler.getArgs() -> let update = UpdateManager.activeUpdate;
  • Unknown yet how many more there are.

This sounds ok, in theory. I'm a little worried that, like your first attempt to convert everything to Promises, this may spiral out of control quickly. But I don't really think there is a way around that. This is fundamentally a wide-reaching change.

• Special case for toolkit/xre/nsUpdateDriver.cpp, nsUpdateProcessor::UpdateDone() needs a MozPromise in case something goes wrong

I'm not entirely sure I understand what the special case is. Could you elaborate?
(Wait, I might have ultimately figured out what the special case is, I mention it at the very bottom of this comment)

Could you please put the changes to nsUpdateDriver.cpp in their own patch? I haven't really used Promises in C++ before, so I'm going to want someone that is familiar with that to review those changes. It will make it easier for them if the parts they need to look at are in their own patch.

Introduce UpdateManagerInit as a replacement service.

• readonly attribute Promise updateManager;
• Replace all first references to UpdateManager with await UpdateManagerInit.updateManager;
• UpdateManagerInit: get updateManager() { return UpdateManager.initialize(); }
• UpdateManager.initialize():
  • if (this._initializedPromise) return this._initializedPromise;
  • this._initializedPromise = Promise.all for:
    • async UpdateManager.initialActiveUpdate() (UpdateManager's current constructor)
    • async UpdateManager.initialUpdates()
  • return this._initializedPromise;

I would rather this be done slightly differently. If possible, I'd really like to avoid any objects being created in a partially constructed state. Thus, I don't really want to have something like UpdateManager.initialize(). I would rather have something more like this:

get updateManager() {
    if (!this._initializedPromise) {
        this._initializedPromise = new Promise(async resolve => {
            let activeUpdate = await this.getActiveUpdate();
            let manager = new UpdateManager(activeUpdate);
            manager.QueryInterface(Ci.nsIUpdateManager);
            resolve(manager);
        });
    }
    return this._initializedPromise;
}

The functionality in _loadXMLFileIntoArray will need to be moved out of UpdateManager, but that should be fine.

I'm guessing that UpdateManager.initialUpdates() is meant to be the existing get _updates()? I don't really understand why that's being included here. I believe that, ideally, we want to lazy load that, the way we are now. And since we are converting the class methods to be asynchronous, it should be simple enough to preserve the lazy loading behavior, I think.

Oh, I took another look at this, and I'm guessing that this was planned this way to allow for synchronous getters like updateCount. Ideally, I'd rather the synchronous getters just be converted to asynchronous getter functions. I took a quick look around though, and there are definitely a number of synchronous functions only accessing getters, so those could potentially be a problem. If they turn out to be problematic, I think it's probably reasonable to load both XMLs when constructing UpdateManager. I do remember that there is a test that I have broken before that checks for unexpected files being loaded early in Firefox startup. I don't know if this change would cause a failure there. I'll spend some time looking into it for you and I'll report back on what I find.

Start converting functions in UpdateManager to asynchronous.

• Replace observe method and nsIObserver references for "um-reload-update-data" (only used in tests)
• Bug 717877, remove DOMParser::parseFromStream from UM__loadXMLFileIntoArray (code attached to that bug)
• Use OS.File, fetch API's as appropriate to replace synchronous file input/output.

I took a look at the code you posted in Bug 717877. I'm always wary about changing out mechanisms in the updater, but I can't see any problems here. I asked for some input from someone that I think is more knowledgeable about the mechanisms we would be replacing/using, just to see if they know of any potential problems we might encounter here.

Snippet for nsUpdateDriver.cpp:

void nsUpdateProcessor::UpdateDone() {
  MOZ_ASSERT(NS_IsMainThread(), "not main thread");

  nsCOMPtr<nsIUpdateManagerInit> um =
      do_GetService("@mozilla.org/updates/update-manager-init;1");
  if (um) {
    RefPtr<mozilla::dom::Promise> p;
    um->RefreshUpdateStatus(getter_AddRefs(p));
    MOZ_DIAGNOSTIC_ASSERT(
      p,
      "UpdateManagerInit.refreshUpdateStatus() didn't return a promise"
    );

I'm a little confused about what's going on here. Do you plan to make a nsIUpdateManagerInit::RefreshUpdateStatus function like:

refreshUpdateStatus() {
    return new Promise(async resolve => {
        let um = await this.updateManager;
        um.refreshUpdateStatus();
    });
}

Maybe this is the special case that I was confused about that you mentioned above?

Thanks for your work on this! Let me know if you have any more questions.

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

Thanks for your work on this! Let me know if you have any more questions.

You and I are largely in agreement on everything, particularly the special case.

I will admit I'm not comfortable (yet) about pulling activeUpdate out of the UpdateManager, but that is a detail in the overall plan I proposed, and can probably be done in another bug under the hierarchy of bugs I am about to file for this work. That said, there's a fair bit of overlap between the singleton activeUpdate processing and the other updates processing, and I'm not comfortable with that either. That might need refactoring too, which would make it easier to justify what you suggested.

My big concern with the suggested activeUpdate change is really about the tests. I do not understand the overall update service as I should in order to undertake this work, but with the amount of testing in place to catch dumb mistakes before review, that's less of a concern. Moving the active update out of the UpdateManager code is something I'd like someone else to take care of, please.

I may change my mind on that as I delve in deeper.

As for the wide-ranging conversion to async, I have decided I'm going to attempt writing a static analysis tool to assist with generating the changesets, subject of course to multiple human reviews. It turns out the static analysis team doesn't yet have good tooling for this, so why not start with me, hmm?

Thanks for your sign-off on the plan, and I hope to keep in touch as this evolves.

I ran some tests over the weekend to check whether loading both XMLs at UpdateManager construction would cause any test failures, and they look fine to me. So that might be a viable option, if you end up wanting to go that route.

:bytesized, I think you might've partly solved my nsUpdateProcessor::UpdateDone() problem in bug 1674277. I'd still need to introduce the initializing service. I'm starting to think about picking this up again.

Can we talk about converting the UpdateManager, UpdateService and friends to JavaScript classes, preferably with private class fields and/or static fields for singletons? I've really fallen in love with them...

I'm not really sure how much would need to change to convert them to JS classes. I would guess not much? I would certainly be happy to have that change.

I would also be happy to use private class fields.

I'm not as sure about using static fields. It doesn't really seem like we would gain anything from it, and I'm concerned that it could make stubbing things out for testing more difficult.