Open Bug 1243934 Opened 8 years ago Updated 2 years ago

AddonManager promises should reject with Error instances rather than arrays

Categories

(Toolkit :: Add-ons Manager, defect, P3)

defect

Tracking

()

People

(Reporter: rkent, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: triaged)

This bug arose when trying to modify test_install.js in bug 1211160, but that test would not work at all (without changes) in my local Windows debug builds. The particular thing that is failing is run_test_30() in test_install.js, with the zip reader claiming that the file is corrupt, but I will investigate that in another bug. This bug is instead about how that error is handled.

In XPIProvider.loadManifest, we have code like this:

  loadManifest: Task.async(function*(file) {
    let zipreader = Cc["@mozilla.org/libjar/zip-reader;1"].
                    createInstance(Ci.nsIZipReader);
    try {
      zipreader.open(file);
    }
    catch (e) {
      zipreader.close();
      return Promise.reject([AddonManager.ERROR_CORRUPT_FILE, e]);
    }

The issue is, what happens when the catch(e) is activated if zipreader.open throws? One example is in XPIProvider.initLocalInstall, where the promise error handler is specified as:

    }, ([error, message]) => {
      logger.warn("Invalid XPI", message);

Unfortunately this generates an error processing [error, message] because what the reject() method sees is not the rejection result, but an error object. This is the error object generated in a throw in Task.jsm _run method here:

      if (this._isStarGenerator) {
        try {
          let result = aSendResolved ? this._iterator.next(aSendValue)
                                     : this._iterator.throw(aSendValue);

So a throw() is executed, passed to catch (ex), then ultimately to _handleException(ex). By the time it works its way down to the reject method, it is simply an error object, not an error result. So ([error, message]) generates an error message about something not being an iterator, and the expected error processing is not executed.
OK now I see the issue and the resolution.

The error is occurring in the file.remove here in XPIProvider.jsm:

    // No valid add-on was found, delete all the temporary files
    for (let { file } of files)
      file.remove(true);

    return Promise.reject([AddonManager.ERROR_CORRUPT_FILE,
                           "Multi-package XPI does not contain any valid packages to install"]);

I'm not really sure why the file.remove fails, but possibly some sort of race condition, where Windows will not let you delete file that has not been fully processed by the system? In any case, there is an error thrown, but it is a JS exception, so the Promise.reject() is not called. That exception gets passed through the task, and ultimately ends up as the reject parameter. But the underlying code assumes that the reject parameter is an array, and does not handle the exception. The destructuring assignment in the reject() method provides a very confusing error message because of its false assumption about the type of the reject.

Lesson learned (at least MY takeaway): always assume that a reject() parameter can be an error type, probably best to create a custom error type instead of rejecting with an array or other non-error type.

I'll probably fix the underlying issue in bug 1211160 but not try to fix the error types in the reject() parameter, as otherwise it is really hard for me to test the changes I need to make in that bug.
Priority: -- → P3
Whiteboard: triaged
Summary: Promise reject handling incorrect in XPIProvider.jsm → AddonManager promises should reject with Error instances rather than arrays
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.