Bugzilla

# Proposed plan of attack

The stated goal of this bug to make UpdateManager asynchronous.  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:
```c++
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();
}
```

# 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:
```c++
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();
}
```