Import / export API for apps

RESOLVED FIXED in mozilla36

Status

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: fabrice, Assigned: fabrice)

Tracking

({dev-doc-needed})

Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(tracking-b2g:backlog)

Details

Attachments

(2 attachments, 16 obsolete attachments)

13.71 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
41.32 KB, patch
marco
: review+
Details | Diff | Splinter Review
We were thinking about something similar for desktop (bug 964069 comment 1).
I'd like to hear from our android web runtime team about how we would implement mozApps.mgmt.import/export on android in conjunction with the APK factory.
Flags: needinfo?(myk)
Given a blob, Fennec's implementation of mozApps.mgmt.import() could extract the app resources from the blob (manifest, package) and POST them to the APK Factory service for packaging as an APK.  Currently, the APK Factory service doesn't support POSTing of resources, it only accepts a manifest URL (and then it GETs the manifest and package); but it could be made to do so, and we're planning to do it in bug 962879.

Alternately, Fennec's implementation could generate an APK locally, although it would require a bunch more work to implement client-side APK generation, and we've foresworn that option for now.

As for mozApps.mgmt.export(), it isn't clear how it supports the use case identified in the discussion referenced in comment 0.  And I'm cautious here of premature generalization.  But in any case it should be possible to implement it by extracting the app's resources from Fennec's datastores and blobifying them.
Flags: needinfo?(myk)
Posted patch import-export-apps.patch (obsolete) — Splinter Review
very early wip, doesn't do anything really interesting yet.
(In reply to Fabrice Desré [:fabrice] from comment #4)
> Created attachment 8403555 [details] [diff] [review]
> import-export-apps.patch
> 
> very early wip, doesn't do anything really interesting yet.

The ImportExport.jsm file is missing.
Posted patch import-export-apps.patch (obsolete) — Splinter Review
Attachment #8403555 - Attachment is obsolete: true
Posted patch wip: export() (obsolete) — Splinter Review
This patch implements export() and works almost properly. The almost part is that the on disk blob is not removed when the references to the object go away.

Ben, can you take a look at my changes to nsDOMMultipartFile? I added an option to the FilePropertyBag dictionary to switch to a nsDOMTemporaryFileBlob but the underlying file never gets removed.
Assignee: nobody → fabrice
Attachment #8404177 - Attachment is obsolete: true
Attachment #8411450 - Flags: feedback?(bent.mozilla)
Attachment #8411450 - Flags: feedback?(bent.mozilla)
Actually it seems that nsDOMTemporaryFileBlob is not doing what I want. It uses nsTemporaryFileInputStream that just closes the file descriptor but doesn't remove the file.
Posted patch import-export-apps.patch (obsolete) — Splinter Review
Ben, how does that look? I'm relying on nsDOMFilFile() being destroyed when it's appropriate. Basic tests are working fine, but I haven't check yet more complex ones like sending the blob in an activity.
Attachment #8411450 - Attachment is obsolete: true
Attachment #8411556 - Flags: feedback?(bent.mozilla)
Comment on attachment 8411556 [details] [diff] [review]
import-export-apps.patch

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

Hm, I don't think this is quite what we want... We should be using NS_OpenAnonymousTemporaryFile *somewhere* here, otherwise we'll just fill the temp dir I think...
Attachment #8411556 - Flags: feedback?(bent.mozilla)
Posted patch import-export-apps.patch (obsolete) — Splinter Review
updated version:
- export() works.
- getAppManifest() works.
- import() works for hosted apps only.

Still need to add packaged app import support, with the signature verification and all the other bells and whistles that packaged apps can have - directly reusing code from webapps.jsm is not super easy, and I don't want to refactor too much because this patch will need to be backported to 1.3t.

I'm posting my gaia changes in bug 1001704 for people that want to test.
Attachment #8411556 - Attachment is obsolete: true
Attachment #8414151 - Flags: feedback?(myk)
Posted patch import-export-apps.patch (obsolete) — Splinter Review
Importing packaged apps now works, but we still need to do more security checks.
Attachment #8414151 - Attachment is obsolete: true
Attachment #8414151 - Flags: feedback?(myk)
Attachment #8414849 - Flags: feedback?(myk)
Attachment #8414849 - Flags: feedback?(mar.castelluccio)
Comment on attachment 8414849 [details] [diff] [review]
import-export-apps.patch

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

::: content/base/public/nsDOMFile.h
@@ +208,5 @@
>  class nsDOMFileFile : public nsDOMFile
>  {
>  public:
>    // Create as a file
> +  nsDOMFileFile(nsIFile *aFile, bool aRemoveWhenDead = false)

I'd name this parameter (and the member variable) some variant of "temporary" (like a/mTemporary or a/mIsTemporary), not only because the parameter's only current consumer uses that term, but also because "temporary" describes what the object is, while "remove when dead" describes what it should do, and the former is less likely to change than the latter.

::: dom/apps/src/ImportExport.jsm
@@ +22,5 @@
> +  "resource://gre/modules/PermissionsInstaller.jsm");
> +
> +this.EXPORTED_SYMBOLS = ["ImportExport"];
> +
> +const kAppArchiveMimeType  = "x-application/webapp-package";

RFC 6648 <http://tools.ietf.org/html/rfc6648> says not to use "x-" prefixes anymore.

RFC 6839 <http://tools.ietf.org/html/rfc6839> says to use the "+zip" suffix to identify a media type whose "underlying representation" is ZIP.

So I'd make this application/webapp-package+zip, or even application/openwebapp+zip, adding the word "open" to identify this as being created by an Open Web Apps implementation of webapps, and removing "package" because it doesn't add much meaning (especially once you add "+zip").

@@ +56,5 @@
> +          oid: aMsg.oid,
> +          error: aError,
> +          success: false
> +        });
> +    }

I'd isolate all the cross-process message handling in Webapps.jsm and simplify this module's APIs so they just return promises, to isolate that handling in Webapps.jsm, which already has to do a bunch of it, and so other consumers can access this API directly, without having to go through the DOM API or create a fake message manager.

@@ +62,5 @@
> +
> +    let app = aRegistry.getAppByManifestURL(aMsg.manifestURL);
> +    if (!app) {
> +      // Should not happen!
> +      sendError("NoSuchApp");

We use UNDERSCORE_CAPS format for errors in Webapps.jsm, so it seems like we should do the same here.

@@ +89,5 @@
> +        let s = JSON.stringify(aObject);
> +        stream.setData(s, s.length);
> +        zipWriter.addEntryStream(aName, Date.now(),
> +                                 Ci.nsIZipWriter.COMPRESSION_DEFAULT,
> +                                 stream, false);

It looks like addObjectToZip ignores its aWriter parameter and assumes a zipWriter in its scope.

I'd move it out of the enclosing "export" function, making it a global that does use its aWriter parameter, to make it easier to read the logic in the long enclosing function.

@@ +177,5 @@
> +
> +    // Reads back the file as Blob.
> +    let blob = new File(zipFile, { name: app.id + kAppArchiveExtension,
> +                                   type: kAppArchiveMimeType,
> +                                   temporary: true });

It's too bad we can't write the ZIP archive directly to a blob output stream.

@@ +368,5 @@
> +                                  manifest,
> +                                  meta);
> +
> +      // Save the app registry, and sends the various notifications.
> +      aRegistry._saveApps().then(() => {

I'd move this chunk into Webapps.jsm, given that _saveApps is intended to be private to DOMApplicationRegistry.

::: dom/apps/src/Webapps.js
@@ +921,5 @@
> +        }
> +        break;
> +      case "Webapps:Import:Return":
> +        if (msg.success) {
> +          req.resolve();

It might be worth resolving this with some value that the caller is likely to want to access next, like a reference to the imported app, perhaps even an instance of mozIDOMApplication.

::: dom/interfaces/apps/nsIDOMApplicationRegistry.idl
@@ +170,5 @@
> +  /**
> +   * Returns the app manifest from a Blob
> +   * This function returns a promise.
> +   */
> +  nsISupports getAppManifest(in nsIDOMBlob blob);

I'd call this simply "getManifest", as there aren't other things for which we expect to implement "get manifest" functionality in this API in the future (hence "import" and "export", "getAll", etc.).
Attachment #8414849 - Flags: feedback?(myk) → feedback+
Comment on attachment 8414849 [details] [diff] [review]
import-export-apps.patch

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

Is this B2G specific for the time being?
After bug 910473, it will probably be easy to extend the code to
desktop (and I guess Android) too.

::: dom/apps/src/ImportExport.jsm
@@ +45,5 @@
> +    return uuidGenerator.generateUUID().toString();
> +  },
> +
> +  // Exports to a Blob.
> +  export: function(aMsg, aMm, aRegistry) {

I guess you've created a new module for these features to avoid adding
too much code to DOMApplicationRegistry, but I don't like so much the
fact that we're passing the registry as the third parameter here.

I can see three alternatives:
1) Modify the code a bit so that ImportExport doesn't call
   DOMApplicationRegistry functions.
2) Make it clearer that ImportExport depends on DOMApplicationRegistry
   by importing Webapps.jsm and using DOMApplicationRegistry when needed
   instead of aRegistry.
3) Don't create a new module for these functions but include them in
   DOMApplicationRegistry.

I don't know which one is the best (maybe there are other better solutions),
it depends on how tightly coupled ImportExport and DOMApplicationRegistry are.
If they're tightly coupled, I'd go with 2 or 3.
If they aren't tightly coupled, I'd go with 1.

If you go with 1 (that is what I would prefer), I have the following
suggestions:
1) Let DOMApplicationRegistry send the "Webapps:" messages.
2) For export, make DOMApplicationRegistry get the app object for the
   specified manifestURL and pass the object to ImportExport.export
   that then returns a blob to DOMApplicationRegistry
3) For import, make ImportExport build an app object and return it to
   DOMApplicationRegistry, that then adds it to the registry (this would allow in
   the near future a refactoring of addInstalledApp, the code you're adding
   and confirmInstall)

Note that I haven't looked at the code in detail, so I'm not sure if any of
this is feasible.
Attachment #8414849 - Flags: feedback?(mar.castelluccio) → feedback+
Posted patch import-export-apps.patch (obsolete) — Splinter Review
I think I addressed all comments, except the ones I disagreed with ;)
Namely:
- CamelCase for error strings: using SNAKE_CAPS was an error actually, so even if it's unconsistent I prefer the new code to be right.
- I'm still using some "private" apis from DOMApplicationRegistry, but I believe this is the simplest and safest thing to do for now.

I changed import() so that the promise returns the app that was just imported. That should help with building the apps.
Attachment #8414849 - Attachment is obsolete: true
Attachment #8416216 - Flags: review?(myk)
Attachment #8416216 - Flags: review?(mar.castelluccio)
Comment on attachment 8416216 [details] [diff] [review]
import-export-apps.patch

Forgot to add Ben to review the DOMFile changes.
Attachment #8416216 - Flags: review?(bent.mozilla)
Paul, I'd like you to take a look at what we do there. This is only available to certified apps that have the webapps-manage permission.

- The export() method creates a Blob that contains some metadata about the app being exported (installOrigin, receipts, manifestURL and a version number), and the manifest (for hosted apps) or package. We prevent exporting certified apps.

- The import() method takes a Blob, and re-install the wrapped app, doing all checks we do usually except the "allowed-install-from" one since there's no way we could prevent the blob to be built with a faked one. We check signature for package apps.

- The getManifest() method takes a Blob and just extracts the manifest file.
Flags: needinfo?(ptheriault)
Comment on attachment 8416216 [details] [diff] [review]
import-export-apps.patch

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

r=me on content/base changes.

::: content/base/public/nsDOMFile.h
@@ +303,5 @@
>    NS_IMETHOD GetInternalStream(nsIInputStream**) MOZ_OVERRIDE;
>  
>    void SetPath(const nsAString& aFullPath);
>  
> +  virtual ~nsDOMFileFile() {

Nit: Let's move this to the cpp file.

@@ +307,5 @@
> +  virtual ~nsDOMFileFile() {
> +    if (mFile && mIsTemporary) {
> +      // Ignore errors if any, not much we can do. Clean-up will be done by
> +      // https://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsAnonymousTemporaryFile.cpp?rev=6c1c7e45c902#127
> +      mFile->Remove(false);

Might as well add NS_WARN_IF_FALSE(NS_SUCCEEDED(...)) here to at least give us something in stdout.
Attachment #8416216 - Flags: review?(bent.mozilla) → review+
Posted patch domfile-tests.patch (obsolete) — Splinter Review
Ben, here's a chrome test that checks that we properly get rid of files created when the DOMFile goes out of scope.
Attachment #8416653 - Flags: review?(bent.mozilla)
Comment on attachment 8416653 [details] [diff] [review]
domfile-tests.patch

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

This looks ok but it's not a very realistic use case... Could you put the blob through a FileReader before you trigger the GC? That would seem more reasonable.
Attachment #8416653 - Flags: review?(bent.mozilla) → review+
(In reply to Fabrice Desré [:fabrice] from comment #17)
> Paul, I'd like you to take a look at what we do there. This is only
> available to certified apps that have the webapps-manage permission.
> 
> - The export() method creates a Blob that contains some metadata about the
> app being exported (installOrigin, receipts, manifestURL and a version
> number), and the manifest (for hosted apps) or package. We prevent exporting
> certified apps.
I am a little concerned about exporting receipts (if I read this correctly).  Since receipts are verified by the developer to authenticate the app and purchaser, and a legitimate use case would be to back up your apps, I don't think that this is what the real intent of this feature, it seems it is also an exploit if the exported receipt would pass verification by another user who imports that app.
My understanding is that receipts needs to be validated online by the app, and is tied to the user login in some way. If that's correct, the "damage" is limited to the time before this receipt is verified.
Posted patch domfile-tests.patch v2 (obsolete) — Splinter Review
Updated test, where we actually add content to the file and read it back with a file reader.
Attachment #8416653 - Attachment is obsolete: true
Attachment #8416715 - Flags: review?(bent.mozilla)
Posted patch import-export-apps.patch (obsolete) — Splinter Review
Sorry for the churn, I fixed a bug in getManifest(). Carrying over Ben's r+.
Attachment #8416216 - Attachment is obsolete: true
Attachment #8416216 - Flags: review?(myk)
Attachment #8416216 - Flags: review?(mar.castelluccio)
Attachment #8416717 - Flags: review?(myk)
Attachment #8416717 - Flags: review?(mar.castelluccio)
Comment on attachment 8416715 [details] [diff] [review]
domfile-tests.patch v2

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

Thanks!
Attachment #8416715 - Flags: review?(bent.mozilla) → review+
Comment on attachment 8416216 [details] [diff] [review]
import-export-apps.patch

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

Erm, I know there's a new patch, but I was in the middle of this review, so I'm going to post it and then finish up the review on the new patch!

::: content/base/public/nsDOMFile.h
@@ +208,5 @@
>  class nsDOMFileFile : public nsDOMFile
>  {
>  public:
>    // Create as a file
> +  nsDOMFileFile(nsIFile *aFile, bool aRemoveWhenDead = false)

Nit: I would change this parameter name too, making it aIsTemporary.

::: dom/apps/src/Webapps.js
@@ +813,5 @@
> +          oid: this._id,
> +          requestID: this.getPromiseResolverId({
> +            resolve: aResolve,
> +            reject: aReject
> +          })});

Nit: I'd put the outer closing bracket and parenthesis on its own line and align it to the opening bracket to make the structure here clearer.

@@ +867,5 @@
> +         .indexOf(aMessage.name) == -1) {
> +      req = this.getRequest(msg.requestID);
> +    } else {
> +      req = this.takePromiseResolver(msg.requestID);
> +    }

Nit: I'd reverse the logic here, so the list of messages is grouped with the conditional block that handles those messages:

if (["Webapps:Export:Return",
     "Webapps:Import:Return",
     "Webapps:GetManifest:Return"]
     .indexOf(aMessage.name) != -1) {
  req = this.takePromiseResolver(msg.requestID);
} else {
  req = this.getRequest(msg.requestID);
}

::: dom/apps/src/Webapps.jsm
@@ +1234,5 @@
> +  doImport: function(aMsg, aMm) {
> +    ImportExport.import(aMsg.blob).then(
> +      ([aManifestURL, aManifest]) => {
> +        let app = this.getAppByManifestURL(aManifestURL);
> +        app.manifest= aManifest;

Nit: add space before equals sign.

@@ +1272,5 @@
> +            success: false
> +          });
> +      }
> +    );
> +  },

Nit: I know this is totally trivial, but the message handler handles the messages in the order Export, Import, GetManifest, so I would put these functions in the same order.

@@ +1281,5 @@
> +        aMm.sendAsyncMessage("Webapps:GetManifest:Return",
> +          { requestID: aMsg.requestID,
> +            oid: aMsg.oid,
> +            success: true,
> +            manifest: aManifest

Nit: another totally trivial issue, but the other message structures list the message-specific property ("app", "blob", or "error") before the "success" property, so I would do that here as well.

::: dom/interfaces/apps/nsIDOMApplicationRegistry.idl
@@ +170,5 @@
> +  /**
> +   * Returns the app manifest from a Blob
> +   * This function returns a promise.
> +   */
> +  nsISupports getManifest(in nsIDOMBlob blob);

I wonder if this'll be confusing, as "getManifest" sounds like the kind of thing you would want to do for an app that is already in the registry.  Perhaps extractManifest would be clearer; or getManifestFromBlob, but that's unwieldy.
Comment on attachment 8416717 [details] [diff] [review]
import-export-apps.patch

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

This review is not quite complete, but here's what I've got so far.

::: dom/apps/src/ImportExport.jsm
@@ +39,5 @@
> +
> +function debug(aMsg) {
> +//#ifdef DEBUG
> +  dump("-*- ImportExport.jsm : " + aMsg + "\n");
> +//#endif

If you uncomment these when committing the patch, then you'll need to move the file to the EXTRA_PP_JS_MODULES list in moz.build.

@@ +43,5 @@
> +//#endif
> +}
> +
> +/*
> +The full metat-data for an app looks like:

Nit: metat -> meta

@@ +68,5 @@
> +  "installerIsBrowser":false,
> +  "storeId":"https://marketplace.firefox.com#7b41976f-cb97-4b90-a711-a71376d21d42",
> +  "storeVersion":1540885,
> +  "role":"",
> +  "redirects":null}

It might be better to reference _setAppProperties in AppsUtils.jsm instead of giving an example here that is likely to grow out-of-date.

@@ +90,5 @@
> +  } catch(e) {
> +    debug("Error: " + e);
> +    return false;
> +  }
> +  return true;

Nit: instead of catching the exception and returning a boolean here, I would catch it in the caller, so the error has to be handled in only one place.

@@ +96,5 @@
> +
> +// Reads a JSON object from a zip, throws aError if something goes wrong.
> +function readObjectFromZip(aZipReader, aPath, aError) {
> +  if (!aZipReader.hasEntry(aPath)) {
> +    throw aError;

Nit: I would also log a message about the ZIP file not having the entry, which will provide additional context to someone debugging this code.

@@ +111,5 @@
> +  try {
> +    res = JSON.parse(converter.ConvertToUnicode(
> +      NetUtil.readInputStreamToString(istream, istream.available()) || ""));
> +  } catch(e) {
> +    throw aError;

Nit: I would also log the exception.

@@ +128,5 @@
> +  // rejected with an error string.
> +  // Possible errors are:
> +  // NoSuchApp, AppNotFullyInstalled, DontExportCertifiedApps, ZipWriteError,
> +  // NoAppDirectory
> +  export: function(aApp) {

Nit: this function and getAppManifest would be easier to read if they were implemented using Task.async, like *import*.

@@ +147,5 @@
> +
> +    // Exporting certified apps is forbidden, as it is to import them.
> +    // We *have* to do this check in the parent process.
> +    if (aApp.appStatus == Ci.nsIPrincipal.APP_STATUS_CERTIFIED) {
> +      deferred.reject("DontExportCertifiedApps"); // XXX: find a better error name.

CertifiedAppExportForbidden?

@@ +179,5 @@
> +    if (!addObjectToZip(meta, "metadata.json", zipWriter)) {
> +      zipWriter.close();
> +      zipFile.remove(false);
> +      deferred.reject("ZipWriteError");
> +      return deferred.promise;

Nit: instead of doing things that can fail in three different places and duplicating the four lines of code needed to handle those failures, it'd be better to prepare them all and then do them all at once.

@@ +187,5 @@
> +
> +    debug("Adding files from " + aApp.basePath + "/" + aApp.id);
> +    let dir = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> +    dir.initWithPath(aApp.basePath);
> +    dir.append(aApp.id);

This doesn't need the ZIP writer to be open, so it should happen before opening the writer.

@@ +287,5 @@
> +  // [manifestURL, manifest]
> +  // Possible errors are:
> +  // NoBlobFound, UnsupportedBlobArchive, MissingMetadataFile, IncorrectVersion,
> +  // AppAlreadyInstalled, DontImportCertifiedApps, InvalidManifest,
> +  // InvalidPrivilegeLevel, InvalidOrigin, DuplicateOrigin

Nit: these'd be easier to read as Javadoc-formatted comments with @return and @throws tags.

@@ +304,5 @@
> +    } else {
> +      // We can't QI the DOMFile to nsIFile, so we need to create one.
> +      zipFile = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> +      zipFile.initWithPath(aBlob.mozFullPath);
> +    }

Nit: since the code in the *if* block throws, the code in the *else* block should be unconditional.

@@ +453,5 @@
> +                                               manifest,
> +                                               meta);
> +
> +      // Save the app registry, and sends the various notifications.
> +      yield DOMApplicationRegistry._saveApps();

Nit: I would add a TODO here explaining that we should get rid of external access of a private method.

@@ +468,5 @@
> +    } catch(e) {
> +      error = e;
> +      let dir = FileUtils.getDir(DOMApplicationRegistry.dirKey,
> +                                 ["webapps", appId], true);
> +      dir.remove(true);

If we're here because the directory couldn't be created, then this call to getDir will also attempt to create it, and it will probably also fail, so we should check that the dir exists before trying to remove it.

@@ +469,5 @@
> +      error = e;
> +      let dir = FileUtils.getDir(DOMApplicationRegistry.dirKey,
> +                                 ["webapps", appId], true);
> +      dir.remove(true);
> +    }

There should be a *finally* block here that closes the zip reader.

@@ +474,5 @@
> +
> +    // Remove the temporary file we create from the memory blob.
> +    if (!isFile) {
> +      zipFile.remove(false);
> +    }

This code (and the equivalent in getAppManifest) will never execute, since the function returns early if !isFile.  Perhaps this is left over from an earlier plan to implement storing the blob on disk, which now has a TODO comment?

In any case, it causes no harm, and an eventual implementation will probably want to do it, but it'd be better to remove it and re-add it if/when it makes sense in a future implementation.

@@ +478,5 @@
> +    }
> +
> +    if (error) {
> +      throw error;
> +    }

There's no need to assign the exception to the *error* variable and rethrow it here, as we can simply rethrow it in the *catch* block.

@@ +487,5 @@
> +  // Extracts the manifest from a blob, returning a Promise that resolves to
> +  // the manifest
> +  // Possible errors are:
> +  // NoBlobFound, UnsupportedBlobArchive, MissingMetadataFile.
> +  getAppManifest: function(aBlob) {

Nit: I would call this function getManifest for consistency with the name of the Webapps:GetManifest event.

@@ +492,5 @@
> +    let deferred = Promise.defer();
> +
> +    // First, do we even have a blob?
> +    if (!aBlob || !aBlob instanceof Ci.nsIDOMBlob) {
> +      sendError("NoBlobFound");

This needs to be a deferred.reject() call.

@@ +506,5 @@
> +    } else {
> +      // We can't QI the DOMFile to nsIFile, so we need to create one.
> +      zipFile = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> +      zipFile.initWithPath(aBlob.mozFullPath);
> +    }

Nit: since the code in the *if* block returns, the code in the *else* block should be unconditional.

@@ +519,5 @@
> +      zipReader.open(zipFile);
> +
> +      if (!zipReader.hasEntry("manifest.webapp")) {
> +        throw "MissingManifestFile";
> +      }

This code throws if !zipReader.hasEntry("manifest.webapp"), but the code right below it handles that case by extracting the manifest from the inner ZIP.  Only one of these can be correct!

@@ +532,5 @@
> +                            .createInstance(Ci.nsIZipReader);
> +        innerReader.openInner(zipReader, "application.zip");
> +        manifest = readObjectFromZip(innerReader,
> +                                     "manifest.webapp",
> +                                     "NoManifest");

This block should close the inner reader once it's done with it.

@@ +548,5 @@
> +      deferred.reject(error);
> +    } else {
> +      deferred.resolve(manifest);
> +    }
> +

The function should close the reader now that it's done with it.
Attachment #8416717 - Flags: review?(myk) → review-
(In reply to Fabrice Desré [:fabrice] from comment #22)
> My understanding is that receipts needs to be validated online by the app,
> and is tied to the user login in some way. If that's correct, the "damage"
> is limited to the time before this receipt is verified.

I do not believe that is correct as the receipt is issued to an email address but do not believe that there is anything tied to a login.   I am not an expert in receipts, but can't think of a legitimate use case to export a paid app from Marketplace as Marketplace will allow a user to restore it.  We should not be enabling transfer of receipts.

From documentation on MDN:
Even if you validate receipts for your paid app, it can be pirated if someone passes around the receipt. The receipt validation methods given above do not prevent this.
https://developer.mozilla.org/en-US/Marketplace/Monetization/Validating_a_receipt

I do not think you should be exporting receipts as receipts, according to our terms of use, should not be transferable.
(In reply to Myk Melez [:myk] [@mykmelez] from comment #27)
> @@ +128,5 @@
> > +  // rejected with an error string.
> > +  // Possible errors are:
> > +  // NoSuchApp, AppNotFullyInstalled, DontExportCertifiedApps, ZipWriteError,
> > +  // NoAppDirectory
> > +  export: function(aApp) {
> 
> Nit: this function and getAppManifest would be easier to read if they were
> implemented using Task.async, like *import*.

By way of illustration, here's what it looks like to convert *export* to Task.async:

https://github.com/mykmelez/gecko-dev/commit/4ea02c062bd4c21c2a7e53ae4cb20acd82b66afc


> @@ +179,5 @@
> > +    if (!addObjectToZip(meta, "metadata.json", zipWriter)) {
> > +      zipWriter.close();
> > +      zipFile.remove(false);
> > +      deferred.reject("ZipWriteError");
> > +      return deferred.promise;
> 
> Nit: instead of doing things that can fail in three different places and
> duplicating the four lines of code needed to handle those failures, it'd be
> better to prepare them all and then do them all at once.

And this is what that change looks like (made easier by the use of Task.async):

https://github.com/mykmelez/gecko-dev/commit/acd3ed227832d052871f343da3d153ceb2a056d9


It's also safer to delay opening the ZIP writer until right before the try/finally block that unconditionally closes it, so there's no chance of an exception in between that leaves the writer open:

https://github.com/mykmelez/gecko-dev/commit/fca4200306c3bd912bf7b2dca796d83b395ef358


> @@ +187,5 @@
> > +
> > +    debug("Adding files from " + aApp.basePath + "/" + aApp.id);
> > +    let dir = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> > +    dir.initWithPath(aApp.basePath);
> > +    dir.append(aApp.id);
> 
> This doesn't need the ZIP writer to be open, so it should happen before
> opening the writer.

This change mostly falls out of the others, but here's the last bit to make it happen:

https://github.com/mykmelez/gecko-dev/commit/bbd6a3b0e94777c82828eb4172ad67f1c0f93174


After those changes, the function looks like this:

https://github.com/mykmelez/gecko-dev/blob/bbd6a3b0e94777c82828eb4172ad67f1c0f93174/dom/apps/src/ImportExport.jsm#L132-L221

(I haven't actually tested it, but it should work.  One question is whether an exception in the forEach function will propagate correctly.  It should, but if it doesn't, then a simple fix would be to convert the statement into a for each…in on for…of block.)
(In reply to David Bialer [:dbialer] from comment #28)
> (In reply to Fabrice Desré [:fabrice] from comment #22)
> > My understanding is that receipts needs to be validated online by the app,
> > and is tied to the user login in some way. If that's correct, the "damage"
> > is limited to the time before this receipt is verified.
> 
> I do not believe that is correct as the receipt is issued to an email
> address but do not believe that there is anything tied to a login.   I am
> not an expert in receipts, but can't think of a legitimate use case to
> export a paid app from Marketplace as Marketplace will allow a user to
> restore it.  We should not be enabling transfer of receipts.
> 
> From documentation on MDN:
> Even if you validate receipts for your paid app, it can be pirated if
> someone passes around the receipt. The receipt validation methods given
> above do not prevent this.
> https://developer.mozilla.org/en-US/Marketplace/Monetization/
> Validating_a_receipt
> 
> I do not think you should be exporting receipts as receipts, according to
> our terms of use, should not be transferable.

Some thoughts on the risks:
- Easier to pirate apps: receipts are already accessible in the app-manager, and this change doesn't make it any easier (third-party homescreen apps will have the same access to receipt objects too). If we had a way to restore receipts from the marketplace, then maybe it would be better not to export them. I'm 

- User data will be stored in clear text on the SDCard: if we just export the data in the app without any protection, that data is trivially exposed should the user lose their phone etc. 

- Export data in apps: makes it a little easier to extract data from apps if you have physical access. Not really any different to current app-manager access though. (Maybe we should have a preference which allows exporting certified app data, similar to the debug-certified-apps preference? Seems like email export would be a useful feature)

Those are the first that come to mind, but this probably needs formal sec-review (there are probably threats related to importing that I haven't listed here, and I'd like to test this). I'll get on it asap.
Flags: needinfo?(ptheriault) → sec-review?(ptheriault)
Comment on attachment 8416717 [details] [diff] [review]
import-export-apps.patch

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

I'll review the next iteration. Could you write a simple test for the feature?
Attachment #8416717 - Flags: review?(mar.castelluccio)
(In reply to Paul Theriault [:pauljt] from comment #30)
> (In reply to David Bialer [:dbialer] from comment #28)
> > (In reply to Fabrice Desré [:fabrice] from comment #22)
> > > My understanding is that receipts needs to be validated online by the app,
> > > and is tied to the user login in some way. If that's correct, the "damage"
> > > is limited to the time before this receipt is verified.
> > 
> > I do not believe that is correct as the receipt is issued to an email
> > address but do not believe that there is anything tied to a login.   I am
> > not an expert in receipts, but can't think of a legitimate use case to
> > export a paid app from Marketplace as Marketplace will allow a user to
> > restore it.  We should not be enabling transfer of receipts.
> > 
> > From documentation on MDN:
> > Even if you validate receipts for your paid app, it can be pirated if
> > someone passes around the receipt. The receipt validation methods given
> > above do not prevent this.
> > https://developer.mozilla.org/en-US/Marketplace/Monetization/
> > Validating_a_receipt
> > 
> > I do not think you should be exporting receipts as receipts, according to
> > our terms of use, should not be transferable.
> 
> Some thoughts on the risks:
> - Easier to pirate apps: receipts are already accessible in the app-manager,
> and this change doesn't make it any easier (third-party homescreen apps will
> have the same access to receipt objects too). If we had a way to restore
> receipts from the marketplace, then maybe it would be better not to export
> them. I'm 
> 

There is no need to export a receipt as these are only issued for paid apps.  Paid apps are restorable once a user logs in and wished to restore it.   But exporting the receipt for the use case of allowing another user to import an app, is copying a 'proof of purchase', which is what the receipt is issued for - as a proof of purchase tied to a user.  The receipt should be considered as something separate from the app itself and not be exported.  You could still export the app, but if the imported app does a receipt verification, it should fail since either the receipt is missing or is not a authentic.


> - User data will be stored in clear text on the SDCard: if we just export
> the data in the app without any protection, that data is trivially exposed
> should the user lose their phone etc. 
> 
> - Export data in apps: makes it a little easier to extract data from apps if
> you have physical access. Not really any different to current app-manager
> access though. (Maybe we should have a preference which allows exporting
> certified app data, similar to the debug-certified-apps preference? Seems
> like email export would be a useful feature)
> 
> Those are the first that come to mind, but this probably needs formal
> sec-review (there are probably threats related to importing that I haven't
> listed here, and I'd like to test this). I'll get on it asap.
Hi. I wanted to make a few corrections.

Receipts are completely portable; they can and should be copied to all devices that a user owns. If a paid app cannot cope with copied receipts then the app has a bug in its receipt validator. 

Furthermore, if a user purchases an app and the app is copied to another device, the app *will not work* without a valid receipt so it seems pointless to copy paid apps without copying their purchase receipts. A user can always re-install their purchased app from the Marketplace (which will install a new receipt) but this will be confusing -- they'll have to delete the old app first.
Fabrice asked if I had time to apply the changes I suggested, and since my comments were relatively concise, and it wasn't always clear how to apply them, I figured that would make sense.

So here's a patch with fixes for the issues I identified in review.  It still isn't tested and doesn't have tests, but it's ready for another look, after which perhaps Fabrice can pick this back up and add some basic tests.
Attachment #8416717 - Attachment is obsolete: true
Attachment #8421046 - Flags: feedback?(mar.castelluccio)
Comment on attachment 8421046 [details] [diff] [review]
patch with fixes for Myk's review issues

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

::: dom/apps/src/ImportExport.jsm
@@ +50,5 @@
> +*/
> +
> +// Adds a js object to a zip.
> +// Returns true for success, false otherwise.
> +function addObjectToZip(aObject, aName, aWriter) {

Given that you're using this function just once and it doesn't contain so
much logic, you could just write the code in the caller.

@@ +62,5 @@
> +                         stream, false);
> +}
> +
> +// Reads a JSON object from a zip, throws aError if something goes wrong.
> +function readObjectFromZip(aZipReader, aPath, aError) {

I think it would be clearer (even if more verbose) to remove the third argument
from readObjectFromZip and let the callers throw the error they want to throw.

@@ +117,5 @@
> +      throw "CertifiedAppExportForbidden"; // XXX: find a better error name.
> +    }
> +
> +    // Add the metadata we'll need to recreate the app object.
> +    let fields = ["installOrigin", "receipts", "manifestURL"];

Unused?

@@ +323,5 @@
> +      // TODO: stop accessing internal methods of other objects.
> +      yield DOMApplicationRegistry._writeFile(manifestFile.path,
> +                                              manifestString);
> +
> +      // Default values for the common fields.

As I said in my previous comment, I think it would be really better if
we could share the "store app in registry" logic in DOMApplicationRegistry
(also if in the future we want to make this module support other platforms
too and not only B2G). This would also make us stop using
DOMApplicationRegistry internal methods here.
If we don't want to do it now, could we file a follow-up?

@@ +389,5 @@
> +          meta.downloadAvailable = true;
> +        }
> +      }
> +
> +      DOMApplicationRegistry.webapps[meta.id] = meta;

For example, the last part of the function could be easily moved to DOMApplicationRegistry.

::: dom/apps/src/Webapps.jsm
@@ +191,5 @@
>      AppDownloadManager.registerCancelFunction(this.cancelDownload.bind(this));
>  
>      this.appsFile = FileUtils.getFile(DIRECTORY_NAME,
>                                        ["webapps", "webapps.json"], true).path;
> +    this.dirKey = DIRECTORY_NAME;

You could define this property in the object literal (like appsFile, etc.).

@@ +1240,5 @@
>      return deferred.promise;
>    },
>  
> +  doExport: function(aMsg, aMm) {
> +    ImportExport.export(this.getAppByManifestURL(aMsg.manifestURL)).then(

I'd check if the app exists here and not in |export|. (I think it should be a responsibility of the Registry)

::: dom/interfaces/apps/nsIDOMApplicationRegistry.idl
@@ +156,5 @@
> +  /**
> +   * Exports an app as a Blob.
> +   * This function returns a promise.
> +   */
> +  nsISupports export(in mozIDOMApplication app);

We only use the manifestURL property of this object, maybe we should just pass the manifestURL?
Attachment #8421046 - Flags: feedback?(mar.castelluccio) → feedback+
(In reply to Marco Castelluccio [:marco] from comment #35)
> Comment on attachment 8421046 [details] [diff] [review]
> patch with fixes for Myk's review issues
> 
> Review of attachment 8421046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/ImportExport.jsm
> @@ +50,5 @@
> > +*/
> > +
> > +// Adds a js object to a zip.
> > +// Returns true for success, false otherwise.
> > +function addObjectToZip(aObject, aName, aWriter) {
> 
> Given that you're using this function just once and it doesn't contain so
> much logic, you could just write the code in the caller.

I don't care much. I had it inlined initially, then though "someone will think this function is too big" ;)


> @@ +323,5 @@
> > +      // TODO: stop accessing internal methods of other objects.
> > +      yield DOMApplicationRegistry._writeFile(manifestFile.path,
> > +                                              manifestString);
> > +
> > +      // Default values for the common fields.
> 
> As I said in my previous comment, I think it would be really better if
> we could share the "store app in registry" logic in DOMApplicationRegistry
> (also if in the future we want to make this module support other platforms
> too and not only B2G). This would also make us stop using
> DOMApplicationRegistry internal methods here.
> If we don't want to do it now, could we file a follow-up?

Adding that in DOMApplicationRegistry is too much refactoring for something that needs to be backported to 1.3t. A follow up is fine.


> @@ +1240,5 @@
> >      return deferred.promise;
> >    },
> >  
> > +  doExport: function(aMsg, aMm) {
> > +    ImportExport.export(this.getAppByManifestURL(aMsg.manifestURL)).then(
> 
> I'd check if the app exists here and not in |export|. (I think it should be
> a responsibility of the Registry)


We should do it in both places actually.

> ::: dom/interfaces/apps/nsIDOMApplicationRegistry.idl
> @@ +156,5 @@
> > +  /**
> > +   * Exports an app as a Blob.
> > +   * This function returns a promise.
> > +   */
> > +  nsISupports export(in mozIDOMApplication app);
> 
> We only use the manifestURL property of this object, maybe we should just
> pass the manifestURL?

I don't think so. Using an app object ensure that the caller got a handle to an application he can access, and can't just test whatever url he feels like.
Can we land this feature on v1.3t?
It's our customer's requirement.
blocking-b2g: --- → 1.3T?
I would like to request a security and legal review on the issue of exporting receipts.   Receipts, which are proofs of purchase, should not be exported as these should not be transferable to other users.  If the use case if for a bulk import of apps to 1.3t devices, paid app receipts should not be exported as part of the app - as they are not part of the app.
Paul, what do you think? I have no idea who to ping on the legal side.
Flags: needinfo?(ptheriault)
The legal bug is 1011125.  Fabrice - you are copied there.
Depends on: 1011125
Flags: needinfo?(ptheriault)
No longer depends on: 1011125
Depends on: 1011125
(In reply to David Bialer [:dbialer] from comment #38)
> I would like to request a security and legal review on the issue of
> exporting receipts.   Receipts, which are proofs of purchase, should not be
> exported as these should not be transferable to other users.  If the use
> case if for a bulk import of apps to 1.3t devices, paid app receipts should
> not be exported as part of the app - as they are not part of the app.

I agree with comment 33 - in any case receipts are completely accessible in the app manager (or if the phone is rooted). 

But what about if you want to share apps between users, which would seem to be a valid use case for regions with expensive net connectivity. Maybe we should allow the user to choose if they want to include the receipt or not when they export? (and maybe also inform them that their receipt is proof of purchase, so if they share it, the app might block them)
 I think we should the option of including receipts in this API, but make the call based on the different export use cases about whether or not we decide to export the receipt or not. 

(Noting that its already possible to access ay receipt via app manager).
ni? Steven to confirm if this is really a required late feature
Flags: needinfo?(styang)
(In reply to Paul Theriault [:pauljt] from comment #41)
> (In reply to David Bialer [:dbialer] from comment #38)
> > I would like to request a security and legal review on the issue of
> > exporting receipts.   Receipts, which are proofs of purchase, should not be
> > exported as these should not be transferable to other users.  If the use
> > case if for a bulk import of apps to 1.3t devices, paid app receipts should
> > not be exported as part of the app - as they are not part of the app.
> 
> I agree with comment 33 - in any case receipts are completely accessible in
> the app manager (or if the phone is rooted). 

I don't think what App manager and rooting a phone makes a difference to this particular API.  Receipts are a weak security mechanism, which is a problem we should fix, not exacerbate.   App manager and phone rooting is a different security threat, but not relevant to this use case.  The use case, and why this feature was added at the last second,  is to allow retailers or users to bulk load apps from an SD Card.  Receipt should be transferable and portable between a user's device, but not between users.  The use case for this API is been export apps and import apps to transfer between users.    The use case, in this case, is so that phone dealers can load apps onto a user's new device in a retail store.  That is the history of this bug and the main use case.  
> 
> But what about if you want to share apps between users, which would seem to
> be a valid use case for regions with expensive net connectivity. Maybe we
> should allow the user to choose if they want to include the receipt or not
> when they export? (and maybe also inform them that their receipt is proof of
> purchase, so if they share it, the app might block them)
Paid apps should not be shared unless they are paid for by new user.  Actually the app itself can be shared, but not the receipt, which is not part of the app and issued by Marketplace as a proof of purchase.  Of course if another user tried to use the app, if the developer does a receipt verification, it should fail - as intended and designed, since the new user does not have a legitimate receipt.   Exporting essentially violation of the trust a developer has put in Marketplace to sell their apps, issue receipts, and verify those receipts.  This makes Marketplace untrustworthy.

A user can easily transfer their own apps between their devices by re-downloading their app from Marketplace after they log in.  Receipts are portable, but not be transferred to a different user by copying them.

Again the receipt is not part of the original app and is only issued by a "trusted" app store.   App Manager and rooting devices aside.
I'll remove the receipts in the next patch, no need to argue ad nauseam. And in any case, upfront paid apps are about to die, right?
(In reply to Joe Cheng [:jcheng] from comment #43)
> ni? Steven to confirm if this is really a required late feature

We don't commit the have this one in the short term.
Flags: needinfo?(styang)
(In reply to David Bialer [:dbialer] from comment #44)
> Receipts are a weak security mechanism, which is a
> problem we should fix, not exacerbate.   

How are they weak security? Allowing a user to copy their own receipt is intrinsic to the security model of receipts. Our systems should empower users to take ownership of their own receipts across all devices.

If this exporter doesn't copy receipts then why copy paid apps at all? They *will not work* without a purchase receipt. That's going to be pretty frustrating and confusing to users!

> Exporting essentially violation of the
> trust a developer has put in Marketplace to sell their apps, issue receipts,
> and verify those receipts.  This makes Marketplace untrustworthy.

I'm really not what you're basing this statement on. Receipts were designed to be copied. Exporting them doesn't violate anything at all.

If you're talking about piracy, there are lots of ways an app can tell if a user has *shared* their receipt. That's handled easily by the server verifier. For example, the verifier could detect an explosion of IP addresses accessing the same receipt. The app server could detect multiple user accounts accessing the same receipt. And so on.
(In reply to Fabrice Desré [:fabrice] from comment #45)
> I'll remove the receipts in the next patch, no need to argue ad nauseam. And
> in any case, upfront paid apps are about to die, right?

Removing receipts from the export seems likes bad idea.

I've not heard of that, what's that based on?
(In reply to Andy McKay [:andym] from comment #48)
> (In reply to Fabrice Desré [:fabrice] from comment #45)
> > I'll remove the receipts in the next patch, no need to argue ad nauseam. And
> > in any case, upfront paid apps are about to die, right?
> 
> Removing receipts from the export seems likes bad idea.
> 
> I've not heard of that, what's that based on?

See bug 1011125
> Of course if another user tried to use the app, if the developer does a receipt
> verification, it should fail - as intended and designed, since the new user
> does not have a legitimate receipt.

That is not how it was intended or designed at all.
(In reply to Fabrice Desré [:fabrice] from comment #49)
> (In reply to Andy McKay [:andym] from comment #48)
> > (In reply to Fabrice Desré [:fabrice] from comment #45)
> > > I'll remove the receipts in the next patch, no need to argue ad nauseam. And
> > > in any case, upfront paid apps are about to die, right?
> > 
> > Removing receipts from the export seems likes bad idea.
> > 
> > I've not heard of that, what's that based on?
> 
> See bug 1011125

Can you cc me and kumar on that bug please?
> Can you cc me and kumar on that bug please?

Done!
tl:dr; Kumar and I recommend we either export apps with receipts, or just ignore any apps with a receipt. The worst case is to export paid apps without receipts.

Longer answer:

* Bug 1011125 doesn't suggest upfront paid apps are going away, by my reading.
* Even if upfront paid apps are going away, the use of receipts is not, the Marketplace is going to start using them for in-app purchases, meaning there will be more receipts in the future.
* Exporting a paid app without a receipt that does receipt checking leaves you with a broken app, until the installReceipt APIs are fully implemented by the Marketplace and others. You must uninstall it and reinstall it (see next point).
* Not exporting a paid app means the user can go to the Marketplace and install it again, easily.
* Kumar and I would like to work with legal and security on bug 1011125 to make sure that people understand how receipts are currently implemented and then maybe when all teams are happy we can add exporting apps with receipts.
So receipt discuss aside (ill leave that for 1011125, but not really a security issue, the receipt is public)

Other risks that need to be considered here, access to the api itself & modifying apps on the sdcard before they are imported seem like important threats. I've only had a quick skim of the patch so far, but a couple questions:
1.  Should this functionality require the webapps-manage permission? Seems like it should to me, but I don't see any permission checks in the patch. ( maybe there is a check but I couldn't see it... )

2. So it looks like we are checking for signature checks prior to setting the privileged app type (and thus enforcing that privileged apps must have valid signature) which is good. However, in bug 926219 we added an exception for signature checking for local files. Is this going to bite us here? [1] (I.e if you just sign an with an expired certificate, will it pass validation due to this exception?)

[1] http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#3143
triage: this seem like a major change, lets not block 1.3T and put this to backlog
blocking-b2g: 1.3T? → backlog
(In reply to Paul Theriault [:pauljt] from comment #54)
> 1.  Should this functionality require the webapps-manage permission? Seems
> like it should to me, but I don't see any permission checks in the patch. (
> maybe there is a check but I couldn't see it... )

Good catch! I added that now.

> 2. So it looks like we are checking for signature checks prior to setting
> the privileged app type (and thus enforcing that privileged apps must have
> valid signature) which is good. However, in bug 926219 we added an exception
> for signature checking for local files. Is this going to bite us here? [1]
> (I.e if you just sign an with an expired certificate, will it pass
> validation due to this exception?)

We call _openPackage() with aIsLocalFileInstall set to false, so we are strictly checking signature here.
I think we should not include receipts. Including receipts seems like something that you want to do if you're doing a backup, not if you are sharing an app with a friend. But if you are doing backup, then you also want to include app data, which we are not so far.

We can't build a totally hacker proof anti-piracy feature. That would require entering the pits of despair that DRM is. But we build something that doesn't encourage accidental piracy.

Once we build a backup solution, we should definitely include receipts then. But that's a separate bug.
We also decided that this didn't need to be under the webapps-manager permission for the "share" use case. This means that the methods won't be under mozApps.mgmt but:
- app.export()
- mozApps.import()
Actually, importing might still need to be under mgmt. The import API allows setting properties that you normally can't set, like the install-origin and the install-manifesturl.

Potentially we could allow the import() API to work, but unless you have webapps-manage, we would ignore install-origin and install-manifesturl and any other parameters that you normally can't control and just treat the installation as a normal install? But that sounds like a bit of a footgun?
You're right Jonas. import() without install origin and install manifest url is not really useful I think, since that would not let people get updates for these apps.
That's kind of a good news since I didn't updated the patch yet ;)
Posted patch import-export-apps.patch (obsolete) — Splinter Review
Latest version with comments addressed.
Attachment #8421046 - Attachment is obsolete: true
quick pass unrotting the last patch against master, haven't built/tested yet.
Posted patch Part 1 - temp dom file (obsolete) — Splinter Review
Split into two parts, the file bits and the apps bits. The file code changed pretty significantly underneath this patch. The only real change in the apps code was the removal of the src dir there.
Attachment #8431909 - Attachment is obsolete: true
Attachment #8476175 - Attachment is obsolete: true
Posted patch Part 2 - import/export (obsolete) — Splinter Review
Attachment #8416715 - Attachment is obsolete: true
Posted patch Part 1 - temp dom file (obsolete) — Splinter Review
Attachment #8485597 - Attachment is obsolete: true
Rebased, carrying r+ from bent.
Attachment #8485606 - Attachment is obsolete: true
Attachment #8496334 - Flags: review+
Marco, this is a rebased version of the previous patches. The notable change is the addition of tests.
Attachment #8485598 - Attachment is obsolete: true
Attachment #8496335 - Flags: review?(mar.castelluccio)
Hi Marco, do you have an ETA for review?
Flags: needinfo?(mar.castelluccio)
Comment on attachment 8496335 [details] [diff] [review]
Part 2: dom/apps implementation

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

The test looks good to me, I've also given a quick look at the rest of the files and have just a couple of nits.

A test that exports an app, imports it and then launches it would be very valuable, because it'd make sure we can
actually launch imported apps.
Could you add this test or file a bug?

IIRC, the WebIDL change needs a review from someone else.

::: dom/apps/ImportExport.jsm
@@ +223,5 @@
> +    if (!appZipReader.hasEntry("manifest.webapp")) {
> +      throw "NoManifestFound";
> +    }
> +
> +    return [readObjectFromZip(appZipReader, "manifest.webapp"), file];

Nit: I would make this function return an object, because it's easy to forget the order of the variables.

@@ +252,5 @@
> +
> +    debug("Importing from " + zipFile.path);
> +
> +    let meta;
> +    let appId;

You're sometimes using meta.id, sometimes appId. I think you can just always use meta.id

@@ +254,5 @@
> +
> +    let meta;
> +    let appId;
> +    let appDir;
> +    let error;

Unused

@@ +317,5 @@
> +      // TODO: stop accessing internal methods of other objects.
> +      yield DOMApplicationRegistry._writeFile(manifestFile.path,
> +                                              manifestString);
> +
> +      // Default values for the common fields.

Could you add a comment explaining that we plan to share code with
the "normal installation" flow and the "addition of an installed app" flow
(DOMApplicationRegistry::addInstalledApp)?

::: dom/apps/Webapps.jsm
@@ +1502,5 @@
> +      return;
> +    }
> +
> +    ImportExport.extractManifest(aMsg.blob).then(
> +      (aManifest) => {

Nit: Here and in the next arrow function, no need for parentheses

::: dom/apps/tests/test_import_export.html
@@ +138,5 @@
> +  // Re-import the app. This time this will succeed.
> +  navigator.mozApps.mgmt.import(exported)
> +    .then((imported) => {
> +      ok(imported !== null, "Imported app is not null.");
> +      info(imported.manifest.name);

Can you check this value instead of just logging it?

@@ +239,5 @@
> +  // Import the packaged app.
> +  navigator.mozApps.mgmt.import(exported)
> +    .then((imported) => {
> +      ok(imported !== null, "Imported app is not null.");
> +      info(imported.manifest.name);

Here too.
Attachment #8496335 - Flags: review?(mar.castelluccio) → review+
(In reply to Dietrich Ayala (:dietrich) from comment #68)
> Hi Marco, do you have an ETA for review?

Just done ;)
Flags: needinfo?(mar.castelluccio)
https://hg.mozilla.org/mozilla-central/rev/56ba1d2ec264
https://hg.mozilla.org/mozilla-central/rev/e1ed7edd0166
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Fabrice, can you describe the risk and footprint size of this patch? Any performance/privacy/security/memory concerns?

This functionality is incredibly important to our target markets in the ultra-low-cost category, so I would like to lobby for an exception to backport as far as we can.
Flags: needinfo?(fabrice)
Risk is low, it's a fairly isolated feature and it's doing everything with on disk blobs to not cause memory bustage. On which branch would you like to backport?
Flags: needinfo?(fabrice)
2.0 and 2.1, at least.
(In reply to Dietrich Ayala (:dietrich) from comment #75)
> 2.0 and 2.1, at least.

2.1 is easy technically. 2.0 would need a bit more work since we moved to use webidl for the mozApps api. I'll do the backport if you manage to convince release-mgmt!
Flags: needinfo?(dietrich)
Long past the date for backports to 2.0/2.1. Wasn't worth the risk without clear ship/update avenues for the relevant market :/
Flags: needinfo?(dietrich)
See Also: → 1139602
blocking-b2g: backlog → ---
Flags: sec-review?(ptheriault)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.