801610 - Several occurrences of main thread I/O in WebApps.jsm

Status: RESOLVED WONTFIX

(Core Graveyard :: DOM: Apps, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

See eg http://hg.mozilla.org/mozilla-central/annotate/942ed5747b63/dom/apps/src/Webapps.jsm#l421

We should get rid of these occurrences.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Also: http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/WebappsInstaller.jsm

Comment 3

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #2)
> http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/
> WebappsInstaller.jsm

Ignore this, I had misunderstood how asyncCopy works.

Comment 4

[Marco Castelluccio [:marco]]

Attachment: use_osfile_webappsjsm

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #0)
> See eg
> http://hg.mozilla.org/mozilla-central/annotate/942ed5747b63/dom/apps/src/
> Webapps.jsm#l421
> 
> We should get rid of these occurrences.

Hmm, is this actually main-thread IO? My understanding is that the stream passed by asyncFetch to the callback is an in-memory stream of the data read from the underlying file stream off-main-thread.

So while I think that patch is useful (OS.File is a better overall API for this), I think that particular patch isn't getting rid of any main-thread IO.

Comment 6

[Marco Castelluccio [:marco]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #5)
> So while I think that patch is useful (OS.File is a better overall API for
> this), I think that particular patch isn't getting rid of any main-thread IO.

Yes, I agree. Main-thread IO here is mostly due to exists() calls. Some of them I think aren't worth fixing, others instead happen at startup and should be fixed (in loadCurrentRegistry and in _readManifests).
I'll post an updated patch.

Comment 7

[Marco Castelluccio [:marco]]

Attachment: use_osfile_webappsjsm

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Ah, indeed, my bad: this particular line number wasn't doing any main thread I/O.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 779449 [details] [diff] [review]
use_osfile_webappsjsm

Review of attachment 779449 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but there are a few errors in Task.jsm  / OS.File usage.
Also, could you take the opportunity to get rid of all main thread I/O calls to FileUtils.getDir? These are the calls that pass |true| as third argument to that function.

::: dom/apps/src/Webapps.jsm
@@ +784,5 @@
> +                       aFile.path + " " + ex + "\n" + ex.stack);
> +
> +        if (aCallback) {
> +          aCallback(null);
> +        }

If callback raises an exception, it will be called twice. That was already the case before your patch, but could you take the opportunity to fix that?

@@ +795,1 @@
>          aCallback(null);

Since you're in a Task, just put this in a try/catch block, this will be easier to read.

@@ +968,5 @@
>      });
>    },
>  
>    _getAppDir: function(aId) {
>      return FileUtils.getDir(DIRECTORY_NAME, ["webapps", aId], true, true);

This is a very bad method:
- it does main thread I/O to check the existence of directories and create them if necessary ;
- at every call site (except one?), it is used only to actually remove the directory.

Could you please fix it?

@@ +980,5 @@
> +    promise.then(function onSuccess() {
> +      if (aCallback) {
> +        aCallback();
> +      }
> +    }, Components.utils.reportError);

According to my experiments, you'll need to bind Components.utils.reportError if you intend to pass it like this.

Also, you may use |Cu| instead of |Components.utils|.

@@ +1155,5 @@
> +              downloadSize: app.downloadSize
> +            }, isUpdate, function(aId, aManifest) {
> +              // Success! Keep the zip in of TmpD, we'll move it out when
> +              // applyDownload() will be called.
> +              let tmpDir = FileUtils.getDir("TmpD", ["webapps", aId], true, true);

FileUtils.getDir does main thread I/O. Can you get rid of it?

@@ +1184,5 @@
> +              });
> +            });
> +        }).bind(this));
> +      }
> +    }.bind(this), Cu.reportError);

Here, too, Cu.reportError should be bound.

@@ +1202,2 @@
>        // Move the application.zip and manifest.webapp files out of TmpD
>        let tmpDir = FileUtils.getDir("TmpD", ["webapps", id], true, true);

As above, main thread I/O.

@@ +1205,5 @@
>        manFile.append("manifest.webapp");
>        let appFile = tmpDir.clone();
>        appFile.append("application.zip");
>  
>        let dir = FileUtils.getDir(DIRECTORY_NAME, ["webapps", id], true, true);

As above, main thread I/O.

@@ +1214,5 @@
> +
> +
> +      Task.spawn(function(){
> +        OS.File.move(appFile.path, appFileTo.path);
> +        OS.File.move(manFile.path, manFileTo.path);

You should |yield|, otherwise errors won't ever be caught.

@@ +1223,5 @@
> +        let stagedTo = dir.clone();
> +        stagedTo.append("update.webapp");
> +
> +        try {
> +          OS.File.move(staged.path, stagedTo.path);

|yield|, here, too.

@@ +1228,5 @@
> +        } catch (ex) {
> +          // If we are applying after a restarted download, we have no
> +          // staged update manifest.
> +          if (!(ex instanceof OS.File.Error && ex.becauseNoSuchFile)) {
> +            throw ex;

} catch (ex if ex instanceof OS.File.Error && ex.becauseNoSuchFile) {

@@ +1234,4 @@
>          }
>  
> +        try {
> +          OS.File.remove(tmpDir.path);

OS.File.remove won't remove a directory.

yield OS.File.removeEmptyDir(tmpDir.path, { ignoreAbsent: true} );
 (and you can get rid of the try/catch)

@@ +1293,2 @@
>          }).bind(this));
> +      }.bind(this)).then(null, Cu.reportError);

As above.

@@ +1923,5 @@
> +    Task.spawn(function() {
> +      let packageId = aData.app.packageId;
> +      if (packageId) {
> +        let dir = FileUtils.getDir("TmpD", ["webapps", packageId],
> +                                   true, true);

As above, main thread I/O – which is crazy, because we create the directory only to remove it immediately.

@@ +1925,5 @@
> +      if (packageId) {
> +        let dir = FileUtils.getDir("TmpD", ["webapps", packageId],
> +                                   true, true);
> +        try {
> +          OS.File.remove(dir.path);

As above, missing yield and use removeDir.

@@ +2098,5 @@
> +        zipFileTo.append("application.zip");
> +
> +        Task.spawn(function() {
> +          OS.File.move(zipFile.path, zipFileTo.path);
> +          let tmpDir = FileUtils.getDir("TmpD", ["webapps", aId], true, true);

As above, crazy main thread I/O.

@@ +2101,5 @@
> +          OS.File.move(zipFile.path, zipFileTo.path);
> +          let tmpDir = FileUtils.getDir("TmpD", ["webapps", aId], true, true);
> +
> +          try {
> +            OS.File.remove(tmpDir.path);

As above, wrong function + missing yield.

@@ +2135,5 @@
> +            if (aInstallSuccessCallback) {
> +              aInstallSuccessCallback(aManifest);
> +            }
> +          }).bind(this));
> +        }.bind(this), Cu.reportError);

As above, please bind the method.

@@ +2203,5 @@
>  
>      // the manifest file used to be named manifest.json, so fallback on this.
>      let baseDir = this.webapps[id].basePath == this.getCoreAppsBasePath()
>                      ? "coreAppsDir" : DIRECTORY_NAME;
>      let file = FileUtils.getFile(baseDir, ["webapps", id, "manifest.webapp"], true);

Could you take the opportunity to not use FileUtils.getFile here? No performance reason, just to avoid mixing (too much) nsIFile/FileUtils and OS.File wherever possible.

@@ +2206,5 @@
>                      ? "coreAppsDir" : DIRECTORY_NAME;
>      let file = FileUtils.getFile(baseDir, ["webapps", id, "manifest.webapp"], true);
> +
> +    Task.spawn(function() {
> +      let exists = OS.File.exists(file.path);

Missing yield.

@@ +2211,5 @@
> +      if (!exists) {
> +        file = FileUtils.getFile(baseDir, ["webapps", id, "update.webapp"], true);
> +      }
> +
> +      exists = OS.File.exists(file.path);

Missing yield.

@@ +2224,5 @@
> +        } else {
> +          this._readManifests(aData, aFinalCallback, index + 1);
> +        }
> +      }).bind(this));
> +    }.bind(this));

Could you add some error reporting to the task?

@@ +2465,5 @@
> +                OS.File.remove(file.path).then(null, function(ex) {
> +                  if (!(ex instanceof OS.File.Error && ex.becauseNoSuchFile)) {
> +                    throw ex;
> +                  }
> +                });

Just |OS.File.remove| should do the trick.

@@ +3043,5 @@
> +          let manifestURL = this.webapps[record.id].manifestURL;
> +          delete this.webapps[record.id];
> +          let dir = this._getAppDir(record.id);
> +          try {
> +            OS.File.remove(dir.path);

As above.

@@ +3064,4 @@
>          }
>        }
> +      this._saveApps(aCallback);
> +    }.bind(this)).then(null, Cu.reportError);

As above.

Comment 10

[Marco Castelluccio [:marco]]

Attachment: Part 1

Missing from this patch are:

1) Use OS.Path in some cases, for consistency.
> Note that coreAppsDir is hardcoded to "/system/b2g".
> (see http://dxr.mozilla.org/mozilla-central/source/b2g/components/DirectoryProvider.js#l73)
> And WebappRegD is defined as WebappRT.config.registryDir.
> (see http://dxr.mozilla.org/mozilla-central/source/webapprt/DirectoryProvider.js#l9)
> For WebappRegD, I would use WebappRT.config.registryDir. On the other hand, for coreAppsDir, I would define a lazy getter that calls FileUtils.getDir() (without any directory creation) to resolve that name.

2) Use OS.File to remove directories (there isn't yet an OS.File alternative to the nsIFile recursive remove function).

3) There are other places, much less important, where there's main thread IO (for example in installPreinstalledApps and installSystemApps, but they are only executed on first run and upgrades and only on b2g).

I think we can start to land this in the meantime (when it's r+ed of course :D) and leave the bug open to fix the other things. Note that this doesn't apply cleanly to trunk, because I've other patches in my queue that modify the same file (I hope to land them soon).

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to Marco Castelluccio [:marco] from comment #10)
> (there isn't yet an OS.File alternative
> to the nsIFile recursive remove function).

We should ensure there is an OS.File bug on file if there isn't already.

> 3) There are other places, much less important, where there's main thread IO
> (for example in installPreinstalledApps and installSystemApps, but they are
> only executed on first run and upgrades and only on b2g).
> 
> I think we can start to land this in the meantime (when it's r+ed of course
> :D) and leave the bug open to fix the other things.

Seems reasonable, but we should move the remaining work to a followup bug - "leave bug open for further work" (i.e. tracking multiple separate landings in the same bug) just leads to confusing bug histories, and bugs are cheap :)

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 781079 [details] [diff] [review]
Part 1

Review of attachment 781079 [details] [diff] [review]:
-----------------------------------------------------------------

Nice progress, but we can clean up some more.

::: dom/apps/src/Webapps.jsm
@@ +747,5 @@
>        ppmm = null;
>      }
>    },
>  
>    _loadJSONAsync: function(aFile, aCallback) {

You should turn aFile into a string (call it aPath). This will play more nicely with OS.File and avoid a number of conversions from/to nsIFile.

@@ +766,5 @@
> +        Cu.reportError("DOMApplicationRegistry: Could not read from " +
> +                       aFile.path + " : " + ex + "\n" + ex.stack);
> +
> +        if (aCallback) {
> +          aCallback(null);

You are still calling the callback twice in case of exception in the callback. Please fix this.

@@ +944,5 @@
>      });
>    },
>  
>    _getAppDir: function(aId) {
> +    return FileUtils.getDir(DIRECTORY_NAME, ["webapps", aId], false, true);

Note: this could be made more I/O efficient with some caching to determine whether DIRECTORY_NAME/webapps/aId exists. This depends on how often that code is called, so perhaps that's not useful. We'll need to check with the owners of webapprt.

@@ +949,3 @@
>    },
>  
> +  _writeFile: function ss_writeFile(aFile, aData, aCallback) {

Looks like you should change the argument aFile to a string (let's call it aPath). This will save you many conversions to/from nsIFile.

@@ +956,5 @@
> +    promise.then(function onSuccess() {
> +      if (aCallback) {
> +        aCallback();
> +      }
> +    }, Cu.reportError.bind(this));

bind(this)?
That looks fishy.

@@ +1090,5 @@
>                                   ["webapps", id,
>                                    isUpdate ? "staged-update.webapp"
>                                             : "update.webapp"],
>                                   true);
>  

Nit: Could you turn all of this into one big Task? This would make code easier to read.

For the readManifests callback, you will need to do things along the lines of:

let promiseManifest = Promise.defer();
this._readManifests([{ id: id }], promiseManifest.resolve);
yield promiseManifest;

@@ +1180,3 @@
>        // Move the application.zip and manifest.webapp files out of TmpD
> +      Task.spawn(function() {
> +        let tmpDir = FileUtils.getDir("TmpD", ["webapps", id], false, true);

Are you sure that TmpD/webapps already exists?
If so, please document why.

@@ +1180,4 @@
>        // Move the application.zip and manifest.webapp files out of TmpD
> +      Task.spawn(function() {
> +        let tmpDir = FileUtils.getDir("TmpD", ["webapps", id], false, true);
> +        yield OS.File.makeDir(tmpDir.path, { ignoreExisting: true });

You might as well define somewhere a function along the lines of:

function makeDirs(existing, subdirs) {
  return Task.spawn(function() {
    for (let i = 1; i < subdirs.length; ++i) {
      let path = [];
      for (let j = 0; j < i; j++) {
        path.push(subdirs[j]);
      }
      yield OS.File.makeDir(OS.Path.join(existing, path), { ignoreExisting: true });
    }
    return OS.Path.join(existing, subdirs);
  });
}

and replace calls to FileUtils.getDir(foo, bar, true) by 

yield makeDirs(fooEquivalent, bar)

@@ +1190,5 @@
> +        let dir = FileUtils.getDir(DIRECTORY_NAME, ["webapps", id], false, true);
> +        yield OS.File.makeDir(dir.path, { ignoreExisting: true });
> +
> +        let manFileTo = dir.clone();
> +        manFileTo.append("manifest.webapp");

Why not do directly
 let manFileTo = OS.Path.join(dir.path, "manifest.webapp")
?
Same thing for the other instances of clone + append.

@@ +1206,5 @@
> +
> +        try {
> +          yield OS.File.move(staged.path, stagedTo.path);
> +          // If we are applying after a restarted download, we have no
> +          // staged update manifest.

Nit: Could you move this comment to the catch branch?

@@ +1227,5 @@
> +        zipFile.append("application.zip");
> +        Services.obs.notifyObservers(zipFile, "flush-cache-entry", null);
> +
> +        // Get the manifest, and set properties.
> +        this.getManifestFor(app.origin, (function(aData) {

Here, too, could you use the promise trick above to get this to play nicely with Task?

@@ +1245,5 @@
>            }
> +
> +          delete app.retryingDownload;
> +
> +          this._saveApps((function() {

Here, too.

@@ +1266,2 @@
>          }).bind(this));
> +      }.bind(this), Cu.reportError.bind(this));

Wrong bind for Cu.reportError.
You should test it.

@@ +1408,5 @@
>        }
>  
>        // Store the new update manifest.
> +      let dir = FileUtils.getDir(DIRECTORY_NAME, ["webapps", id], false, true);
> +      OS.File.makeDir(dir.path, { ignoreExisting: true }).then(function() {

If you have written makeDirs, reuse it here.

@@ +1454,5 @@
> +        if (aNewManifest) {
> +          this.updateAppHandlers(aOldManifest, aNewManifest, app);
> +
> +          let dir = FileUtils.getDir(DIRECTORY_NAME, ["webapps", id], false, true);
> +          yield OS.File.makeDir(dir.path, { ignoreExisting: true });

Here, too, good site for makeDirs.

@@ +1458,5 @@
> +          yield OS.File.makeDir(dir.path, { ignoreExisting: true });
> +
> +          // Store the new manifest.
> +          let manFile = dir.clone();
> +          manFile.append("manifest.webapp");

Here too, clone/append.

@@ +1509,4 @@
>            }
> +          delete app.manifest;
> +        });
> +      }.bind(this), Cu.reportError.bind(this));

bind

@@ +1900,5 @@
>  
>    denyInstall: function(aData) {
>      let packageId = aData.app.packageId;
>      if (packageId) {
> +      let dir = FileUtils.getDir("TmpD", ["webapps", packageId], false, true);

Good.

@@ +1981,5 @@
> +      appObject.id = id;
> +      appObject.localId = localId;
> +
> +      let basePath = FileUtils.getDir(DIRECTORY_NAME, ["webapps"], false, true);
> +      yield OS.File.makeDir(basePath.path, { ignoreExisting: true });

As above, makeDirs.

@@ +1989,5 @@
>        let dir = this._getAppDir(id);
> +      yield OS.File.makeDir(dir.path, { ignoreExisting: true });
> +
> +      let manFile = dir.clone();
> +      manFile.append(manifestName);

As above, get rid of clone/append.

@@ +2051,5 @@
> +      // We notify about the successful installation via mgmt.oninstall and the
> +      // corresponging DOMRequest.onsuccess event as soon as the app is properly
> +      // saved in the registry.
> +      if (!aFromSync) {
> +        this._saveApps((function() {

As above, you should use the promise trick to get this to play nice with Task.

@@ +2070,5 @@
> +        // origin for install apps is meaningless here, since it's app:// and this
> +        // can't be used to resolve package paths.
> +        manifest = new ManifestHelper(jsonManifest, app.manifestURL);
> +        this.downloadPackage(manifest, appObject, false, (function(aId, aManifest) {
> +          Task.spawn(function() {

As above, promise trick.

@@ +2114,5 @@
> +              if (aInstallSuccessCallback) {
> +                aInstallSuccessCallback(aManifest);
> +              }
> +            }).bind(this));
> +          }.bind(this), Cu.reportError.bind(this));

bind

@@ +2119,3 @@
>          }).bind(this));
> +      }
> +    }.bind(this), Cu.reportError.bind(this));

bind

@@ +2204,5 @@
> +        } else {
> +          this._readManifests(aData, aFinalCallback, index + 1);
> +        }
> +      }).bind(this));
> +    }.bind(this), Cu.reportError.bind(this));

bind

@@ +2727,2 @@
>        try {
>          checkDownloadSize(dir.diskSpaceAvailable);

What does dir.diskSpaceAvaailable do if the directory doesn't exist?

Comment 13

[Myk Melez [:myk] [@mykmelez]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #9)
> @@ +980,5 @@
> > +    promise.then(function onSuccess() {
> > +      if (aCallback) {
> > +        aCallback();
> > +      }
> > +    }, Components.utils.reportError);
> 
> According to my experiments, you'll need to bind
> Components.utils.reportError if you intend to pass it like this.

My experimentation isn't comprehensive, but these two lines appear to behave identically in the browser console:

Cu.import("resource://gre/modules/Promise.jsm").Promise.reject("fail").then(null, Cu.reportError);
Cu.import("resource://gre/modules/Promise.jsm").Promise.reject("fail").then(null, Cu.reportError.bind(Cu));

And Promise.jsm recommends use of the non-bound function:

https://github.com/mozilla/mozilla-central/blob/e93dc31b824e2cc2a3e1d356e15e25f62b1a2227/toolkit/modules/Promise.jsm#L86-L87

What problems did you notice in your experiments?

Comment 14

[David Teller [:Yoric] - still alive but not very active]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #13)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #9)
> > @@ +980,5 @@
> > > +    promise.then(function onSuccess() {
> > > +      if (aCallback) {
> > > +        aCallback();
> > > +      }
> > > +    }, Components.utils.reportError);
> > 
> > According to my experiments, you'll need to bind
> > Components.utils.reportError if you intend to pass it like this.
> 
> My experimentation isn't comprehensive, but these two lines appear to behave
> identically in the browser console:
> 
> Cu.import("resource://gre/modules/Promise.jsm").Promise.reject("fail").
> then(null, Cu.reportError);
> Cu.import("resource://gre/modules/Promise.jsm").Promise.reject("fail").
> then(null, Cu.reportError.bind(Cu));
> 
> And Promise.jsm recommends use of the non-bound function:
> 
> https://github.com/mozilla/mozilla-central/blob/
> e93dc31b824e2cc2a3e1d356e15e25f62b1a2227/toolkit/modules/Promise.jsm#L86-L87
> 
> What problems did you notice in your experiments?

I have encountered cases in which foo.then(null, Cu.reportError) reported nothing. I haven't really investigated them, though.

I guess the thing to do is test whether WebApps.jsm displays errors in case of error, with just Cu.reportError.

Comment 15

[Marco Castelluccio [:marco]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #14)
> I have encountered cases in which foo.then(null, Cu.reportError) reported
> nothing. I haven't really investigated them, though.

Maybe that's because of bug 904929. Exceptions in the function passed to Task.spawn are, at the moment, just discarded.

Comment 16

[Marco Castelluccio [:marco]]

Attachment: Part 1

I've changed my mind, I'll write several independent patches that also improve the readability of the code in Webapps.jsm, this way we'd kill two birds with one stone.

This first patch:
1) Improves the code called to load the registry at startup using Task.jsm, removing all the callbacks.
2) Makes _readManifests, _writeFile, _saveApps, _loadJSONAsync return a promise. This will ease the usage of Task.jsm in the following patches.
3) Rewrites _readManifests in a non-recursive way.
4) Removes some getDir(…, …, true, …), where it can be done safely.

Comment 17

[Marco Castelluccio [:marco]]

Attachment: Part 1

Forgot to qref...

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 797006 [details] [diff] [review]
Part 1

Review of attachment 797006 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. I have a few nitpicks, some possible small errors, and I'd like a little more documentation.
After that, we'll get an owner of that module to review it.

::: dom/apps/src/Webapps.jsm
@@ +89,5 @@
>  // store even by error.
>  const STORE_ID_PENDING_PREFIX = "#unknownID#";
>  
>  this.DOMApplicationRegistry = {
> +  appsFilePath: null,

Nit: Could you take the opportunity to document this field? Both role and type.

@@ +126,5 @@
>      AppDownloadManager.registerCancelFunction(this.cancelDownload.bind(this));
>  
> +    this.appsFilePath = FileUtils.getFile(DIRECTORY_NAME,
> +                                          ["webapps", "webapps.json"],
> +                                          true).path;

Could you do take the opportunity to make this
OS.Path.join(DIRECTORY_NAME, "webapps", "webapps.json")
?

@@ +131,5 @@
>  
>      this.loadAndUpdateApps();
>    },
>  
>    // loads the current registry, that could be empty on first run.

Could you document the return type? In particular, it is important to mention whether the caller needs to report asynchronous errors.
(also true for the other methods you make async).

@@ +132,5 @@
>      this.loadAndUpdateApps();
>    },
>  
>    // loads the current registry, that could be empty on first run.
> +  loadCurrentRegistry: function() {

Haven't you forgotten a |Task.spawn|?

@@ +139,5 @@
> +      return;
> +    }
> +
> +    this.webapps = data;
> +    let appDir = OS.Path.dirname(this.appsFilePath);

That works, but |OS.Path.join(DIRECTORY_NAME, "webapps")| would probably be more natural.

@@ +253,3 @@
>  
>        // Nothing else to do but notifying we're ready.
>        this.notifyAppsRegistryReady();

So you notify that you're ready before the manifests are fully read? Are you sure that this will work?

@@ +353,1 @@
>                              .path;

Again, OS.Path.join should be simpler.

@@ +394,5 @@
>    //   b. uninstall any core app from the current registry but not in the
>    //      new core apps registry.
>    //   c. for all apps in the new core registry, install them if they are not
>    //      yet in the current registry, and run installPermissions()
> +  installSystemApps: function() {

Have you forgotten Task.spawn?

@@ +401,5 @@
>        file = FileUtils.getFile("coreAppsDir", ["webapps", "webapps.json"], false);
>      } catch(e) { }
>  
> +    let data = yield this._loadJSONAsync(file.path);
> +    if (data) {

I believe that you could just return if !data, couldn't you?

@@ +402,5 @@
>      } catch(e) { }
>  
> +    let data = yield this._loadJSONAsync(file.path);
> +    if (data) {
> +      // b : core apps are not removable.

That "b" is not very clear. Same thing for "c" below.

@@ +416,5 @@
> +        this._clearPrivateData(localId, false);
> +        delete this.webapps[id];
> +      }
> +
> +      let appDir = FileUtils.getDir("coreAppsDir", ["webapps"], false);

Ok, that one is probably annoying to replace.

@@ +499,5 @@
>          // installPreinstalledApp() removes the ones failing to install.
>          this._saveApps();
>        }
>        this.registerAppsHandlers(runUpdate);
> +    }.bind(this)).then(null, Cu.reportError);

Generally, I believe that you should return the tasks that you spawn.

@@ +751,5 @@
> +      Cu.reportError("DOMApplicationRegistry: Error while reading: " +
> +                     aPath + " " + ex + "\n" + ex.stack);
> +
> +      throw new Task.Result(null);
> +    });

I believe that the code would be easier to read if the then(null, ...) were replaced with a try/catch inside the Task.spawn.

@@ +1973,5 @@
>      appNote.id = id;
>  
>      appObject.id = id;
>      appObject.localId = localId;
> +    appObject.basePath = OS.Path.dirname(this.appsFilePath);

As above, OS.Path.join is probably more natural.

@@ +2095,5 @@
>      */
>  
>    _manifestCache: {},
>  
> +  _readManifests: function(aData, aIndex) {

I believe that you don't use aIndex anymore.

@@ +2114,5 @@
> +            file = FileUtils.getFile(baseDir, ["webapps", id, "update.webapp"], true);
> +          }
> +          if (!(yield OS.File.exists(file.path))) {
> +            file = FileUtils.getFile(baseDir, ["webapps", id, "manifest.json"], true);
> +          }

Nit: I would probably rewrite this as a loop, something along the lines of:

let dir = FileUtils.getDir(baseDir, ["webapps", id], false, false);
let path;
for (let leaf in ["manifest.webapp", ...]) {
  let path2 = OS.Path.join(dir, leaf);
  if ((yield OS.File.exists(path2)) {
    path = path2;
    break;
  }
}

This can, of course, wait for a followup bug.

@@ +2825,4 @@
>      });
>    },
>  
> +  doGetAll: function(aData, aMm) {

Weird method name. I realize it's not yours, but could you take the opportunity to document what it does?

Comment 19

[Marco Castelluccio [:marco]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #18)
> @@ +126,5 @@
> >      AppDownloadManager.registerCancelFunction(this.cancelDownload.bind(this));
> >  
> > +    this.appsFilePath = FileUtils.getFile(DIRECTORY_NAME,
> > +                                          ["webapps", "webapps.json"],
> > +                                          true).path;
> 
> Could you do take the opportunity to make this
> OS.Path.join(DIRECTORY_NAME, "webapps", "webapps.json")
> ?

We don't have the OS.Constants.Path constants needed to modify this at the moment. I'd rather improve this in another bug to avoid stalling.

> @@ +132,5 @@
> >      this.loadAndUpdateApps();
> >    },
> >  
> >    // loads the current registry, that could be empty on first run.
> > +  loadCurrentRegistry: function() {
> 
> Haven't you forgotten a |Task.spawn|?

It's a generator function, so it will work automagically if called inside a task.

> @@ +253,3 @@
> >  
> >        // Nothing else to do but notifying we're ready.
> >        this.notifyAppsRegistryReady();
> 
> So you notify that you're ready before the manifests are fully read? Are you
> sure that this will work?

Yes, I think this is the expected behavior (and it's also the current behavior).
Any function that will need to access the manifests will call _readManifests that in turn will load the manifest file if it isn't already loaded.

> @@ +402,5 @@
> >      } catch(e) { }
> >  
> > +    let data = yield this._loadJSONAsync(file.path);
> > +    if (data) {
> > +      // b : core apps are not removable.
> 
> That "b" is not very clear. Same thing for "c" below.

Those comments are referring to the comment on top of the function.

> @@ +751,5 @@
> > +      Cu.reportError("DOMApplicationRegistry: Error while reading: " +
> > +                     aPath + " " + ex + "\n" + ex.stack);
> > +
> > +      throw new Task.Result(null);
> > +    });
> 
> I believe that the code would be easier to read if the then(null, ...) were
> replaced with a try/catch inside the Task.spawn.

I think it would complicate things, because I'd need to have something like:
try {
  …
} catch (ex if ex instanceof Task.Result) {
  throw(ex);
} catch (ex) {
  Cu.reportError(…);
  throw new Task.Result(null);
}

> This looks good. I have a few nitpicks, some possible small errors, and I'd
> like a little more documentation.
> After that, we'll get an owner of that module to review it.

Thank you for the quick review! I'll ask Fabrice to review the next patch.

Comment 20

[David Teller [:Yoric] - still alive but not very active]

(In reply to Marco Castelluccio [:marco] from comment #19)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #18)
> > @@ +126,5 @@
> > >      AppDownloadManager.registerCancelFunction(this.cancelDownload.bind(this));
> > >  
> > > +    this.appsFilePath = FileUtils.getFile(DIRECTORY_NAME,
> > > +                                          ["webapps", "webapps.json"],
> > > +                                          true).path;
> > 
> > Could you do take the opportunity to make this
> > OS.Path.join(DIRECTORY_NAME, "webapps", "webapps.json")
> > ?
> 
> We don't have the OS.Constants.Path constants needed to modify this at the
> moment. I'd rather improve this in another bug to avoid stalling.

My bad, you are right. Let's keep this for a followup bug.

> > @@ +132,5 @@
> > >      this.loadAndUpdateApps();
> > >    },
> > >  
> > >    // loads the current registry, that could be empty on first run.
> > > +  loadCurrentRegistry: function() {
> > 
> > Haven't you forgotten a |Task.spawn|?
> 
> It's a generator function, so it will work automagically if called inside a
> task.

Ok. I would suggest avoiding that automatic style, however, as it makes reading/code reuse harder.

> 
> > @@ +253,3 @@
> > >  
> > >        // Nothing else to do but notifying we're ready.
> > >        this.notifyAppsRegistryReady();
> > 
> > So you notify that you're ready before the manifests are fully read? Are you
> > sure that this will work?
> 
> Yes, I think this is the expected behavior (and it's also the current
> behavior).
> Any function that will need to access the manifests will call _readManifests
> that in turn will load the manifest file if it isn't already loaded.

Ok. Could you please document this somewhere?

> 
> > @@ +402,5 @@
> > >      } catch(e) { }
> > >  
> > > +    let data = yield this._loadJSONAsync(file.path);
> > > +    if (data) {
> > > +      // b : core apps are not removable.
> > 
> > That "b" is not very clear. Same thing for "c" below.
> 
> Those comments are referring to the comment on top of the function.

Well, they were already not very clear in the original, and now that "a" has disappeared, that's worse :)

> 
> > @@ +751,5 @@
> > > +      Cu.reportError("DOMApplicationRegistry: Error while reading: " +
> > > +                     aPath + " " + ex + "\n" + ex.stack);
> > > +
> > > +      throw new Task.Result(null);
> > > +    });
> > 
> > I believe that the code would be easier to read if the then(null, ...) were
> > replaced with a try/catch inside the Task.spawn.
> 
> I think it would complicate things, because I'd need to have something like:
> try {
>   …
> } catch (ex if ex instanceof Task.Result) {
>   throw(ex);
> } catch (ex) {
>   Cu.reportError(…);
>   throw new Task.Result(null);
> }

or

try {
  ...
} catch (ex if !(ex instanceof Task.result)) {
  Cu.reportError(...)
  throw new Task.Result(null);
}

I think that this is a little easier to read than your code. I am not going to insist strongly, though. Your call.

> > This looks good. I have a few nitpicks, some possible small errors, and I'd
> > like a little more documentation.
> > After that, we'll get an owner of that module to review it.
> 
> Thank you for the quick review! I'll ask Fabrice to review the next patch.

Sounds good.

Comment 21

[Marco Castelluccio [:marco]]

Attachment: Patch

This second patch includes the changed made in the other one and also removes many other callbacks using promises. Now most of the main-thread IO is gone (the remaining things will probably need bug 772538).

I realize the patch is pretty big, but it's mostly code moved around (mostly callbacks removed thanks to Task.jsm).

Try push here: https://tbpl.mozilla.org/?tree=Try&rev=e122cba93bab

Comment 22

[[:fabrice] Fabrice Desré]

Comment on attachment 797533 [details] [diff] [review]
Patch

Review of attachment 797533 [details] [diff] [review]:
-----------------------------------------------------------------

r- for the debug actor breakage.

This looks generally ok, but I confess I didn't do a line by line review. In any case, we need to get many more tests before being able to land big refactorings like that.

::: dom/apps/src/Webapps.jsm
@@ +90,5 @@
>  // store even by error.
>  const STORE_ID_PENDING_PREFIX = "#unknownID#";
>  
>  this.DOMApplicationRegistry = {
> +  appsFilePath: null,

Add the comment Yoric asked for in comment #18

@@ +391,5 @@
>  #endif
>    },
>  
>    // Implements the core of bug 787439
> +  // On first run and updates, installs the core apps.

Please keep the full comment here.

@@ +941,3 @@
>    },
>  
> +  launch: function (aData, aMm) {

You're breaking the webapps actor at https://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webapps.js#422

@@ +1023,5 @@
>      AppDownloadManager.remove(aManifestURL);
>    },
>  
> +  startDownload: function(aManifestURL) {
> +    Task.spawn(function() {

You usually use the => style, so be consistent.

@@ +2534,2 @@
>                          let dir = FileUtils.getDir(aDir,
> +                          ["webapps", id], false, true);

Are you sure it's safe to use "false" in these places?

@@ +2651,3 @@
>    },
>  
> +  uninstall: function(aData, aMm) {

and... https://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webapps.js#329

Comment 23

[Marco Castelluccio [:marco]]

(In reply to Fabrice Desré [:fabrice] from comment #22)
> r- for the debug actor breakage.

Ah, this is where those functions were used. I wonder why we have:

  let reg = DOMApplicationRegistry;
  reg.launch(

This just makes more difficult to find it :D

> 
> @@ +391,5 @@
> >  #endif
> >    },
> >  
> >    // Implements the core of bug 787439
> > +  // On first run and updates, installs the core apps.
> 
> Please keep the full comment here.

I've moved the comments near the lines they were actually describing, instead of having weird links with letters, it looked more clear to me. If you prefer I can move them back.

> @@ +1023,5 @@
> >      AppDownloadManager.remove(aManifestURL);
> >    },
> >  
> > +  startDownload: function(aManifestURL) {
> > +    Task.spawn(function() {
> 
> You usually use the => style, so be consistent.

We basically can't use arrow functions in Task.spawn because arrow functions can't be generators. It's really sad because they'd remove the need for |bind(this)|.

> @@ +2534,2 @@
> >                          let dir = FileUtils.getDir(aDir,
> > +                          ["webapps", id], false, true);
> 
> Are you sure it's safe to use "false" in these places?

I've looked at all the places where |true| was used and it looks like it mostly wasn't necessary (for example sometimes it's used right before removing the directory).

What tests could I add?

Comment 24

[[:fabrice] Fabrice Desré]

(In reply to Marco Castelluccio [:marco] from comment #23)

> 
> I've moved the comments near the lines they were actually describing,
> instead of having weird links with letters, it looked more clear to me. If
> you prefer I can move them back.

Yes I prefer to have the full algorithm described in one place.


> We basically can't use arrow functions in Task.spawn because arrow functions
> can't be generators. It's really sad because they'd remove the need for
> |bind(this)|.

Ha. :(

> 
> > @@ +2534,2 @@
> > >                          let dir = FileUtils.getDir(aDir,
> > > +                          ["webapps", id], false, true);
> > 
> > Are you sure it's safe to use "false" in these places?
> 
> I've looked at all the places where |true| was used and it looks like it
> mostly wasn't necessary (for example sometimes it's used right before
> removing the directory).

"looks like" is not strong enough to risk this changes.

> What tests could I add?

Onecyrenus is working on ading many update tests that we lack. We won't land any big changes before we have these.

Comment 25

[Marco Castelluccio [:marco]]

(In reply to Fabrice Desré [:fabrice] from comment #24)
> Onecyrenus is working on ading many update tests that we lack. We won't land
> any big changes before we have these.

I thought we had packaged apps update tests, this is unfortunate!

Comment 27

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 28

[Marco Castelluccio [:marco]]

Comment on attachment 807980 [details] [diff] [review]
Patch

Let's wait for the try run results before.

Comment 29

[Marco Castelluccio [:marco]]

Attachment: Reduced patch

As I thought the emulator failures were due to the initialization refactoring, I think we can start to land this part first (so that I don't have to rebase it so often).

Comment 30

[Marco Castelluccio [:marco]]

I'm using Promise.jsm because in my opinion it makes code more readable than DOM promises (because when you create a DOM promise you need to specify a callback).
I think Promise.jsm implementation is going to be ported to DOM promises, so this shouldn't be a problem.
I can switch to DOM promises anyway, if you prefer.

Comment 31

[David Teller [:Yoric] - still alive but not very active]

(In reply to Marco Castelluccio [:marco] from comment #30)
> I'm using Promise.jsm because in my opinion it makes code more readable than
> DOM promises (because when you create a DOM promise you need to specify a
> callback).
> I think Promise.jsm implementation is going to be ported to DOM promises, so
> this shouldn't be a problem.
> I can switch to DOM promises anyway, if you prefer.

At the moment, our policy on m-c is to use Promise.jsm, until DOM Promise are considered stable. The specification of DOM Promise is too much in flux to make it worth using them yet. When DOM Promise have stabilized, we will certainly port Promise.jsm to DOM Promise anyway.

Comment 32

[Marco Castelluccio [:marco]]

Try runs: https://tbpl.mozilla.org/?tree=Try&rev=d4544fc541da and https://tbpl.mozilla.org/?tree=Try&rev=4e3e06e27cd9

Comment 33

[Marco Castelluccio [:marco]]

We have update tests now, so I'm clearing the needinfo request.

Comment 34

[Marco Castelluccio [:marco]]

I'd guess the patch is heavily bitrotten now. Fabrice, if I rewrite a new patch, do you have some time to review it?

Comment 35

[[:fabrice] Fabrice Desré]

Sure, but we'll land that after we branch b2g 1.3.

Comment 36

[Marco Castelluccio [:marco]]

The Apps API code has been removed in bug 1261019.