892837 - Support permissions in desktop webrt

Status: RESOLVED FIXED

(Firefox Graveyard :: Webapp Runtime, defect, P1)

Comment 1

[Marco Castelluccio [:marco]]

We need to set the permissions during the first run of the application.

Comment 2

[Marco Castelluccio [:marco]]

Attachment: Add marketplace certificate

Comment 3

[Marco Castelluccio [:marco]]

Attachment: privileged_apps_permissions_v2

This patch reworks the webapprt startup.

The module Startup.jsm will be loaded when the runtime is executed, not when the first window is opened. It will take care of waiting for the webapps registry and the xul window to be loaded, then will install the permissions and set the correct appId for the <browser> principal.
It uses GRE_BUILDID to know when an update occurs (it's similar to what Android does, only that they use MOZ_APP_VERSION).

I've also done some other simplifications in tests code.

I think we could now get rid of ContentPolicy.js, as it's now implemented in Gecko itself with the use of principals. Do you agree?

Comment 4

[Marco Castelluccio [:marco]]

Attachment: ContentPermission.js changes

Comment 5

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

Comment on attachment 778147 [details] [diff] [review]
privileged_apps_permissions_v2

My initial feedback is that I'm having trouble applying this patch to the tip of the trunk:

07-19 11:43 > git apply ~/privileged_apps_permissions_v2
/Users/myk/privileged_apps_permissions_v2:115: trailing whitespace.
 * loaded. */ 
/Users/myk/privileged_apps_permissions_v2:174: space before tab in indent.
  	  savedBuildID = Services.prefs.getCharPref("webapprt.buildid");
/Users/myk/privileged_apps_permissions_v2:179: space before tab in indent.
  	  Services.prefs.setCharPref("webapprt.buildid", ourBuildID);
/Users/myk/privileged_apps_permissions_v2:180: space before tab in indent.
  	  return savedBuildID ? "upgrade" : "new";
/Users/myk/privileged_apps_permissions_v2:189: space before tab in indent.
  	// Wait for the webapps registry to be loaded.
error: patch failed: toolkit/webapps/WebappsInstaller.jsm:155
error: toolkit/webapps/WebappsInstaller.jsm: patch does not apply

Comment 6

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

Comment on attachment 778147 [details] [diff] [review]
privileged_apps_permissions_v2

A few thoughts:

* The build scripts shouldn't need to preprocess Startup.jsm to get it the build ID, as it can retrieve it at runtime from nsIXULAppInfo::platformBuildID.

* We should be careful about how we update the webapprt.buildid pref.  If any code that depends on it could fail, and we would want to reexecute that code on next startup, then we should make sure to execute it before update the pref.  By the same token, if code that depends on it could fail, but we *don't* want to try it again, then we should make sure to execute it after updating the pref.

* It's worth displaying the window as soon as possible, even if there isn't anything in it, to give the earliest possible visual cue that the app is loading.  And I wonder if the window would open slightly faster if the command line handler continued to open the window right before importing Startup.jsm.

* Perhaps I just don't understand Task well enough, but it isn't clear why the Startup module needs to spawn a task.  It seems like StartupObserver.init() could just return StartupObserver.deferredRegistry.promise, and the init() caller could then chain a callback to it that returns StartupObserver.deferredWindowLoad.promise, which then chains a callback that handles window load, i.e.:

this.StartupObserver = {
  init: function() {
    …
    return this.deferredRegistry.promise;
  }
}

StartupObserver.init().then(function() {
  // get app ID, set permissions, etc.
  return StartupObserver.deferredWindowLoad.promise;
}).then(function() {
  // run the app in the loaded window
}, Components.utils.reportError);


* I'm not even sure we benefit from encapsulating functionality in a StartupObserver object, given that we can chain both synchronous and asynchronous functions together to perform all the actions in StartupObserver.init() and initializationTask(), something like:

Promise.resolve(
  Services.ww.openWindow(null,
                         "chrome://webapprt/content/webapp.xul",
                         "_blank",
                         "chrome,dialog=no,resizable,scrollbars,centerscreen",
                         null);
).
then(function(window) {
  let deferredRegistry = Promise.defer(), deferredWindowLoad = Promise.defer();

  Services.obs.addObserver(function onRegistryStart() {
    Services.obs.removeObserver(onRegistryStart, "webapps-registry-start");
    deferredRegistry.resolve(deferredWindowLoad.promise);
  }, "webapps-registry-start", true);

  window.addEventListener("load", function onLoad() {
    window.removeEventListener("load", onLoad, false);
    deferredWindowLoad.resolve(window);
  });

  return deferredRegistry.promise;
}).
then(function(deferredWindowLoadPromise) deferredWindowLoadPromise).
then(function(window) {
  // the rest of the work, which could be broken up into multiple synchronous
  // functions, f.e. one function to handle installation permissions on update
  // and another function to handle running the app in the window
}).
then(null, Components.utils.reportError);


That version avoids all global variables, but Startup.jsm has its own context, and we probably don't need to worry about setting globals (except possibly for memory leaks due to window references), so we could do something like this instead:

let window = 
  Services.ww.openWindow(null,
                         "chrome://webapprt/content/webapp.xul",
                         "_blank",
                         "chrome,dialog=no,resizable,scrollbars,centerscreen",
                         null);

let deferredWindowLoad = Promise.defer();

window.addEventListener("load", function onLoad() {
  window.removeEventListener("load", onLoad, false);
  deferredWindowLoad.resolve();
});

let deferredRegistry = Promise.defer();

Services.obs.addObserver(function onRegistryStart() {
  Services.obs.removeObserver(onRegistryStart, "webapps-registry-start");
  deferredRegistry.resolve();
}, "webapps-registry-start", true);

deferredRegistry.promise.
then(function() {
  // anything that doesn't need the window to be loaded
}).
then(function() deferredWindowLoad).
then(function() {
  // anything that needs the window to be loaded
}).
then(function() {
  // if needed, clean up global values
}).
then(null, Components.utils.reportError);


Nevertheless, I'm open to using Task if it makes more sense.  Perhaps I'm just not familiar enough with it.

Comment 7

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

Erm, and now that I've taken a closer look at Task, it appears to nicely abstract some of the complexity of chaining promises, making a sequence of sometimes-asynchronous operations even simpler to chain together than they are using promises alone.

So I'm now totally in favor of implementing this module's functionality using Task.  However, it still seems that we don't need the StartupObserver object, as we could include all its functionality in the task, such that the module is basically a single long task with an easy-to-read sequence of initialization and configuration operations.

Comment 8

[Marco Castelluccio [:marco]]

Attachment: privileged_apps_permissions_v2

I've removed the StartupObserver object.

Who could review the certificate patch?

Comment 9

[Marco Castelluccio [:marco]]

Attachment: ContentPermission.js changes

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 777434 [details] [diff] [review]
Add marketplace certificate

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

Marco,

In order to review this patch I need to know more details about how the desktop RT works. Does the desktop RT use a separate Gecko profile for each installed app? Will this desktop privileged app support be enabled for Firefox itself?

See bug 892288 for an example of the type of problem that doesn't apply to B2G but which probably applies to other products. It seems like, at a minimum, we'd need to remove the content handlers for X.509 certificates (PSMContentListener.cpp) and do other things to ensure that other code signing certs are not added to the certificate datbase in the profile that the desktop RT.

When I wrote the B2G_CERTDATA hack, I was actually expecting that we'd have to rewrite packaged app certificate verification completely to avoid these issues. The B2G_CERTDATA hack was written the way it was because at the time I was told I didn't have time to do it right.

Comment 11

[Marco Castelluccio [:marco]]

(In reply to Brian Smith (:briansmith), was [e-mail] (:bsmith) from comment #10)

Opened bug 896620.

Comment 12

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

Comment on attachment 779309 [details] [diff] [review]
privileged_apps_permissions_v2

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

Looks great!

Comment 13

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

Comment on attachment 779310 [details] [diff] [review]
ContentPermission.js changes

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

Other than that one issue, this looks good!

::: webapprt/ContentPermission.js
@@ +74,5 @@
> +      // Persist the choice if the user wants to remember
> +      Services.perms.addFromPrincipal(request.principal, request.type, action);
> +    } else {
> +      // Otherwise allow the permission for the current session
> +      Services.perms.addFromPrincipal(request.principal, request.type, action, Ci.nsIPermissionManager.EXPIRE_SESSION);

request.type is used both to construct the locale string names (f.e. geolocation.allow) and as the *type* argument to addFromPrincipal, but addFromPrincipal takes "geo" for the geolocation permission, while the locale string names use "geolocation", so one of them must be wrong.

We can change the locale string names if necessary, especially since this patch already does so; but keep in mind that our localization processes will flag those strings as changed, even though they haven't actually changed.  So it's worth notifying the l10n folks (f.e. :pike specifically or <http://www.mozilla.org/about/forums/#dev-l10n> generally) about the name change so they can tell localizers not to worry about retranslating those strings.

Comment 14

[Marco Castelluccio [:marco]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #13)
> request.type is used both to construct the locale string names (f.e.
> geolocation.allow) and as the *type* argument to addFromPrincipal, but
> addFromPrincipal takes "geo" for the geolocation permission, while the
> locale string names use "geolocation", so one of them must be wrong.
> 
> We can change the locale string names if necessary, especially since this
> patch already does so; but keep in mind that our localization processes will
> flag those strings as changed, even though they haven't actually changed. 
> So it's worth notifying the l10n folks (f.e. :pike specifically or
> <http://www.mozilla.org/about/forums/#dev-l10n> generally) about the name
> change so they can tell localizers not to worry about retranslating those
> strings.

There are some places where "geolocation" is passed to addFromPrincipal and testExactPermissionFromPrincpal (for example: http://mxr.mozilla.org/mozilla-central/source/dom/permission/tests/test_permission_basics.html or also the ContentPermissionPrompt.js of the metro browser, b2g and android). And there are other places where "geo" is passed.
Maybe they're both usable?

Comment 15

[Marco Castelluccio [:marco]]

Attachment: privileged apps permissions v3

Comment 16

[Marco Castelluccio [:marco]]

Attachment: Add manifestURL field to webapp.json

I've had to rework the code in order to enable permissions testing.

Comment 17

[Marco Castelluccio [:marco]]

Attachment: Install permissions on first run/update and set principal appId

Comment 18

[Marco Castelluccio [:marco]]

Attachment: Test permissions installation

Comment 19

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

Comment on attachment 785427 [details] [diff] [review]
Add manifestURL field to webapp.json

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

::: toolkit/webapps/WebappsInstaller.jsm
@@ +177,5 @@
>        "registryDir": registryFolder.path,
>        "app": {
>          "manifest": aManifest,
> +        "origin": aData.app.origin,
> +        "manifestURL": aData.app.manifestURL

Nit: the emerging convention is to append a trailing comma to the property list so a future addition doesn't trivially modify the previous line.

Comment 20

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

Comment on attachment 785428 [details] [diff] [review]
Install permissions on first run/update and set principal appId

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

Requesting review from Fabrice on the dom/apps/ changes.

::: dom/apps/src/AppsUtils.jsm
@@ +404,5 @@
>  
>    /**
>     * Determines if an update or a factory reset occured.
>     */
> +  isFirstRunOrUpdate: function isFirstRun() {

Nit: rename the function expression now that the name of the property to which it is being assigned has changed.

::: webapprt/CommandLineHandler.js
@@ +33,5 @@
> +                                      "chrome://webapprt/content/webapp.xul",
> +                                      "_blank",
> +                                      "chrome,dialog=no,resizable,scrollbars,centerscreen",
> +                                      null);
> +      // Load the module to startup the applications

Nit: startup -> start up (because you're using it as a verb here rather than a noun), applications -> application (or simply "app", which is the most common term we use to describe them).

@@ +35,5 @@
> +                                      "chrome,dialog=no,resizable,scrollbars,centerscreen",
> +                                      null);
> +      // Load the module to startup the applications
> +      Cu.import("resource://webapprt/modules/Startup.jsm");
> +      startup(window);

Nit: it's worth adding a comment explaining that we're opening the window here in order to open it as soon as possible.  Otherwise it won't necessarily be obvious why we do it here rather than simply letting Startup.jsm handle it.

::: webapprt/Startup.jsm
@@ +4,5 @@
>  
> +/* This module is imported at the startup of an application. It takes care of
> + * permissions installation, application url loading, security settings. Put
> + * stuff here that you want to happen once on startup before the webapp is
> + * loaded. */ 

Nit: trailing whitespace.

@@ -19,5 @@
>  Cu.import("resource://gre/modules/Webapps.jsm");
>  
>  // Initialize window-independent handling of webapps- notifications.
>  Cu.import("resource://webapprt/modules/WebappsHandler.jsm");
> -WebappsHandler.init();

Yup, this makes sense.  There's no reason to do it here when WebappsHandler.jsm can do it!

@@ +46,5 @@
> +    });
> +    // For tests, the window could be already loaded
> +    if (window.document.getElementById("content")) {
> +      deferredWindowLoad.resolve();
> +    }

If the window is already loaded, then we shouldn't register the load handler.

Also, is window.document guaranteed to be defined at this point?  It seems like it wouldn't be (or at least wouldn't be guaranteed to be) in the non-test case, in which case this would throw an exception.

@@ +52,5 @@
> +    // Wait for webapps registry loading.
> +    yield deferredRegistry.promise;
> +
> +    // Get the appId.
> +    let appId = Ci.nsIScriptSecurityManager.NO_APP_ID;

Nit: the convention in this file (and the other webapprt/ files) is to capitalize acronyms, so this should be appID.

Also "get the appId" seems pretty obvious, since the next line sets appId; while at the same time it isn't quite accurate, since the rest of the code segment does more than just get the ID: it also installs permissions.

@@ +55,5 @@
> +    // Get the appId.
> +    let appId = Ci.nsIScriptSecurityManager.NO_APP_ID;
> +    let manifestURL = WebappRT.config.app.manifestURL;
> +    if (manifestURL) {
> +      appId = DOMApplicationRegistry.getAppLocalIdByManifestURL(WebappRT.config.app.manifestURL);

Nit: since we've defined manifestURL, this line should use it.

::: webapprt/WebappRT.jsm
@@ +29,5 @@
>      let inputStream = Cc["@mozilla.org/network/file-input-stream;1"].
>                        createInstance(Ci.nsIFileInputStream);
>      inputStream.init(webappFile, -1, 0, 0);
>      let json = Cc["@mozilla.org/dom/json;1"].createInstance(Ci.nsIJSON);
> +    let config = json.decodeFromStream(inputStream, webappFile.fileSize);

Nice simplification!

Comment 21

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

Comment on attachment 785429 [details] [diff] [review]
Test permissions installation

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

::: dom/apps/src/PermissionsInstaller.jsm
@@ +30,5 @@
>    //dump("-*-*- PermissionsInstaller.jsm : " + aMsg + "\n");
>  }
>  
> +this.PermissionsInstaller = {
> +  // An array carring all the possible (expanded) permission names.

Nit: carring -> carrying or containing (not your typo, but we might as well fix it in the process of making changes here).

@@ +31,5 @@
>  }
>  
> +this.PermissionsInstaller = {
> +  // An array carring all the possible (expanded) permission names.
> +  allPossiblePermissions: [],

I don't mind making this a property rather than a global, but I don't like that doing so adds an initialization step.  And checking the length of this array to determine whether or not to initialize it seems brittle.  You could instead make the property a memoizing getter that replaces itself with a populated array the first time it is accessed:

https://wiki.mozilla.org/Performance/Addons/BestPractices#Memoization

But note that you don't actually need to make this a property in order to access it from the test files, because you can access the global scope of PermissionsInstaller.jsm from those files via:

let globalScope = Cu.import("resource://gre/modules/PermissionsInstaller.jsm");
// Now you have access to globalScope.AllPossiblePermissions.

::: webapprt/test/chrome/browser_noperm.js
@@ +17,5 @@
> +    is(scope.AppsUtils.getAppManifestStatus(app.manifest), Ci.nsIPrincipal.APP_STATUS_INSTALLED, "The app is not privileged");
> +
> +    // Check that the app principal has the correct appId
> +    let principal = document.getElementById("content").contentDocument.defaultView.document.nodePrincipal;
> +    is(scope.DOMApplicationRegistry.getAppLocalIdByManifestURL(app.manifestURL), principal.appId, "Principal appID correct")

Nit: appID -> appId (or "app ID", since this is intended to be a human-readable message)

::: webapprt/test/chrome/browser_webperm.js
@@ +17,5 @@
> +    is(scope.AppsUtils.getAppManifestStatus(app.manifest), Ci.nsIPrincipal.APP_STATUS_INSTALLED, "The app is not privileged");
> +
> +    // Check that the app principal has the correct appId
> +    let principal = document.getElementById("content").contentDocument.defaultView.document.nodePrincipal;
> +    is(scope.DOMApplicationRegistry.getAppLocalIdByManifestURL(app.manifestURL), principal.appId, "Principal appID correct")

Nit: appID -> appId (or "app ID", since this is intended to be a human-readable message)

Comment 22

[Marco Castelluccio [:marco]]

Attachment: Install permissions on first run/update and set principal appId v2

Comment 23

[Marco Castelluccio [:marco]]

Attachment: Test permissions installation v2

Comment 24

[[:fabrice] Fabrice Desré]

Comment on attachment 785873 [details] [diff] [review]
Install permissions on first run/update and set principal appId v2

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

r=me with comments addressed. I did not review the tests.

::: dom/apps/src/AppsUtils.jsm
@@ +404,5 @@
>  
>    /**
>     * Determines if an update or a factory reset occured.
>     */
> +  isFirstRunOrUpdate: function isFirstRunOrUpdate() {

why did you change that? The aPrefBranch parameter is there on purpose.

::: dom/apps/src/Webapps.jsm
@@ +481,5 @@
>    },
>  #endif
>  
>    loadAndUpdateApps: function loadAndUpdateApps() {
> +    let runUpdate = AppsUtils.isFirstRunOrUpdate();

Why did you change that?

::: webapprt/Startup.jsm
@@ +56,5 @@
> +    let appID = Ci.nsIScriptSecurityManager.NO_APP_ID;
> +    let manifestURL = WebappRT.config.app.manifestURL;
> +    if (manifestURL) {
> +      appID = DOMApplicationRegistry.getAppLocalIdByManifestURL(manifestURL);
> +

You should use the appsService here, not directly the DOMApplication registry, even if in this case (parent process) that doesn't matter much.

::: webapprt/content/mochitest.js
@@ +73,5 @@
> +
> +    // During chrome tests, we use the same window to load all the tests. We
> +    // need to change the buildID so that the permissions for the currently
> +    // tested application get installed.
> +    Services.prefs.setCharPref("gecko.buildID", WebappRT.config.app.manifestURL);

That's a strange build ID... Maybe just using Date().now() would be more realistic.

Comment 25

[[:fabrice] Fabrice Desré]

Comment on attachment 785873 [details] [diff] [review]
Install permissions on first run/update and set principal appId v2

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

reverting to r- for the docshell issue

::: webapprt/Startup.jsm
@@ +74,5 @@
> +    // Get the <browser> element in the webapp.xul window.
> +    let appBrowser = window.document.getElementById("content");
> +
> +    // Set the principal to the correct appID and launch the application.
> +    appBrowser.docShell.setIsApp(appID);

The correct way to switch into app mode is to set the mozapp attribute when creating the iframe. Setting the app id on the docShell is not guaranteed to work.

Comment 26

[Marco Castelluccio [:marco]]

(In reply to Fabrice Desré [:fabrice] from comment #25)
> reverting to r- for the docshell issue
> 
> ::: webapprt/Startup.jsm
> @@ +74,5 @@
> > +    // Get the <browser> element in the webapp.xul window.
> > +    let appBrowser = window.document.getElementById("content");
> > +
> > +    // Set the principal to the correct appID and launch the application.
> > +    appBrowser.docShell.setIsApp(appID);
> 
> The correct way to switch into app mode is to set the mozapp attribute when
> creating the iframe. Setting the app id on the docShell is not guaranteed to
> work.

We aren't using an iframe but a browser element, I think there isn't a mozapp attribute for <browser>s.
What else would be needed to allow using a browser element other than setting the docShell?

Comment 27

[Marco Castelluccio [:marco]]

(In reply to Marco Castelluccio [:marco] from comment #26)
> (In reply to Fabrice Desré [:fabrice] from comment #25)
> > reverting to r- for the docshell issue
> > 
> > ::: webapprt/Startup.jsm
> > @@ +74,5 @@
> > > +    // Get the <browser> element in the webapp.xul window.
> > > +    let appBrowser = window.document.getElementById("content");
> > > +
> > > +    // Set the principal to the correct appID and launch the application.
> > > +    appBrowser.docShell.setIsApp(appID);
> > 
> > The correct way to switch into app mode is to set the mozapp attribute when
> > creating the iframe. Setting the app id on the docShell is not guaranteed to
> > work.
> 
> We aren't using an iframe but a browser element, I think there isn't a
> mozapp attribute for <browser>s.
> What else would be needed to allow using a browser element other than
> setting the docShell?

I've looked at the code in content/html/content/src/nsGenericHTMLFrameElement.cpp and in content/base/src/nsFrameLoader.cpp.
AFAICS (I haven't experience with that code), the manifest attribute is only used to set the docShell appID and to allow switching between apps in an iframe (to handle the "embed-apps" permission). All the code that handles permissions and security is then based on the docShell appID.
In our runtime, we don't need to switch apps in an iframe, because our <browser> will only contain a single application, so I think the docShell appID should be enough.

Comment 28

[[:fabrice] Fabrice Desré]

Comment on attachment 785873 [details] [diff] [review]
Install permissions on first run/update and set principal appId v2

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

the docshell trick will work if no app is using the embed-apps permission functionality. Let's do that for now.

r=me with comments addressed.

::: webapprt/Startup.jsm
@@ +74,5 @@
> +    // Get the <browser> element in the webapp.xul window.
> +    let appBrowser = window.document.getElementById("content");
> +
> +    // Set the principal to the correct appID and launch the application.
> +    appBrowser.docShell.setIsApp(appID);

The correct way to switch into app mode is to set the mozapp attribute when creating the iframe. Setting the app id on the docShell is not guaranteed to work.

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Fabrice Desré [:fabrice] from comment #28)

> > +    // Set the principal to the correct appID and launch the application.
> > +    appBrowser.docShell.setIsApp(appID);
> 
> The correct way to switch into app mode is to set the mozapp attribute when
> creating the iframe. Setting the app id on the docShell is not guaranteed to
> work.

Just an FYI. Fennec is using the same approach on it's <browser>s too. Any bugs would at least show up in both products.

Comment 30

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

Comment on attachment 785873 [details] [diff] [review]
Install permissions on first run/update and set principal appId v2

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

Looks really good!  Just a few minor issues, mostly nits.

::: webapprt/CommandLineHandler.js
@@ +28,5 @@
>                               "_blank",
>                               "chrome,dialog=no",
>                               args);
>      } else {
> +      // We're opening the window here in order to show it as soon as possible.

After recommending this approach, I became worried that we were engaging in premature optimization <http://c2.com/cgi/wiki?PrematureOptimization>.  So I instrumented the loading of Startup.jsm and found that warm loading consumes 14-20ms on my fast dev machine.

And that isn't nearly enough time for users to notice a delay on its own.  But it's a non-trivial portion of total startup time, and it'll cost more on an average user machine.  Plus, lessons from Firefox suggest that minor expenses like this one become a "death by a thousand cuts" over time.

So I still think it's worth the additional complexity to open the window before loading Startup.jsm, even though it feels a bit like premature optimization.

::: webapprt/Startup.jsm
@@ +4,5 @@
>  
> +/* This module is imported at the startup of an application. It takes care of
> + * permissions installation, application url loading, security settings. Put
> + * stuff here that you want to happen once on startup before the webapp is
> + * loaded. */

Nit: in code comments, which are typically rendered in a monospace font, put two spaces after each period.

@@ +22,5 @@
> +Cu.import("resource://gre/modules/AppsUtils.jsm");
> +Cu.import("resource://gre/modules/PermissionsInstaller.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/Task.jsm");
> +Cu.import("resource://gre/modules/Promise.jsm");

Nit: unless the modules need to be loaded in this order, I prefer to load runtime modules after GRE modules, on the general principle that the GRE is the platform on which the runtime runs, so GRE code loads first and runtime code second.

@@ +32,5 @@
> +    // Observe registry loading.
> +    let deferredRegistry = Promise.defer();
> +    function observeRegistryLoading(subj, topic, data) {
> +      if (topic == "webapps-registry-start") {
> +        deferredRegistry.resolve();

Hmm, we only ever want to resolve this deferred once, even if webapps-registry-start were to get called multiple times, so this observer should remove itself from the list of observers for this notification.

Also, it doesn't need to test the topic, because we only ever add it as an observer of that specific topic, so the observer service will never notify it about any other topic.

@@ +46,5 @@
> +      window.addEventListener("load", function onLoad() {
> +        window.removeEventListener("load", onLoad, false);
> +        deferredWindowLoad.resolve();
> +      });
> +    }

Nit: the logic here would be a bit easier to grok like this:

    // Observe XUL window loading.
    // For tests, it could be already loaded.
    let deferredWindowLoad = Promise.defer();
    if (window.document && window.document.getElementById("content")) {
      deferredWindowLoad.resolve();
    } else {
      window.addEventListener("load", function onLoad() {
        window.removeEventListener("load", onLoad, false);
        deferredWindowLoad.resolve();
      });
    }
    
    …
    
    // Wait for XUL window loading
    yield deferredWindowLoad.promise;

(Also, ideally we wouldn't have any test-specific code in this module; but I don't see how to avoid it.)

::: webapprt/WebappsHandler.jsm
@@ +85,5 @@
>      }
>    }
>  };
> +
> +WebappsHandler.init();

Nit: now that initialization happens unconditionally inside the module, I would take the opportunity to remove the *init* method and instead initialize the WebappsHandler object in the global scope, i.e.:

    this.WebappsHandler = {
    …
    };

    Services.obs.addObserver(WebappsHandler, "webapps-ask-install", false);
    Services.obs.addObserver(WebappsHandler, "webapps-launch", false);
    Services.obs.addObserver(WebappsHandler, "webapps-uninstall", false);

That's also an opportunity to make the object support weak references from the observer service, reducing the risk of memory leaks:

    Cu.import("resource://gre/modules/XPCOMUtils.jsm");

    this.WebappsHandler = {
      QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
                                             Ci.nsISupportsWeakReference]),
    …
    };

    Services.obs.addObserver(WebappsHandler, "webapps-ask-install", true);
    Services.obs.addObserver(WebappsHandler, "webapps-launch", true);
    Services.obs.addObserver(WebappsHandler, "webapps-uninstall", true);

And it removes the now-private *init* method from the public interface of the module, which is a good idea in general.

::: webapprt/content/mochitest.js
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/* Note: this script is loaded by both mochitest-start.js and head.js, so make sure

Hmm, I want to preserve the alignment between the name of the XUL file and the name of its primary script, so let's rename this file to mochitest-shared.js and then use mochitest.js as the name of the new file that mochitest.xul now loads instead of this one.

Also, nit: wrap this line at 80 columns (which will happen automatically when you rename the files).

::: webapprt/content/webapp.js
@@ -68,5 @@
> -  // When content calls openWindow(), there are no window.arguments,
> -  // but something in the platform loads the URL specified by the content.
> -  if (args && args.hasKey("url")) {
> -    gAppBrowser.setAttribute("src", args.get("url"));
> -  }

I really like this simplification of the load handler, whose behavior has been complex and brittle.  What a great side-effect of your changes!

Comment 31

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

Comment on attachment 785875 [details] [diff] [review]
Test permissions installation v2

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

Looking good, just a few issues!  In addition to the code comments, note that one test is failing because of the "wappush" permission, which presumably has landed since you rolled this patch:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_webperm.js | Permission wappush correctly set. - Got 0, expected 2
Stack trace:
    JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 560
    JS frame :: chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_webperm.js :: onLoad :: line 30
    JS frame :: chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/head.js :: onLoadApp :: line 31
    native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

::: webapprt/test/chrome/browser_noperm.js
@@ +1,2 @@
> +let scope = {};
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

This doesn't appear to be used, so the test shouldn't import it.

@@ +4,5 @@
> +Cu.import("resource://gre/modules/PermissionSettings.jsm", scope);
> +let globalScope = Cu.import("resource://gre/modules/PermissionsInstaller.jsm", scope);
> +Cu.import("resource://gre/modules/PermissionsTable.jsm", scope);
> +Cu.import("resource://gre/modules/AppsUtils.jsm", scope);
> +Cu.import("resource://gre/modules/Webapps.jsm", scope);

PermissionsTable.jsm doesn't appear to be used either, so it shouldn't be imported.

Also, nit: to make it clearer what is being imported and shorten the lengths of the lines that use imported symbols, use destructuring assignment:

    let { AllPossiblePermissions } =
      Cu.import("resource://gre/modules/PermissionsInstaller.jsm", {});
    let { AppsUtils } = Cu.import("resource://gre/modules/AppsUtils.jsm", {});
    let { DOMApplicationRegistry } =
      Cu.import("resource://gre/modules/Webapps.jsm", {});

It's also fine to import symbols into the global scope, especially for modules like AppsUtils.jsm and Webapps.jsm that only export a single symbol:

    Cu.import("resource://gre/modules/AppsUtils.jsm");
    Cu.import("resource://gre/modules/Webapps.jsm");

But I would avoid importing into a "scope" object unless it's particularly important to protect the global scope from unexpected symbols, since that adds verbosity to the code using the symbols.

@@ +17,5 @@
> +    is(scope.AppsUtils.getAppManifestStatus(app.manifest), Ci.nsIPrincipal.APP_STATUS_INSTALLED, "The app is not privileged");
> +
> +    // Check that the app principal has the correct appId
> +    let principal = document.getElementById("content").contentDocument.defaultView.document.nodePrincipal;
> +    is(scope.DOMApplicationRegistry.getAppLocalIdByManifestURL(app.manifestURL), principal.appId, "Principal app ID correct")

Nit: trailing semi-colon.

@@ +22,5 @@
> +
> +    // Check if all the permissions of the app are unknown
> +    for (let idx in globalScope.allPossiblePermissions) {
> +      // Get the values for all the possible permissions.
> +      let permName = globalScope.allPossiblePermissions[idx];

allPossiblePermissions -> AllPossiblePermissions

Also, note that you can iterate the values of an array slightly more simply, when you don't care about its indices, with for…of <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of>:

    for (let permName of AllPossiblePermissions) {
      …
    }

::: webapprt/test/chrome/browser_webperm.js
@@ +4,5 @@
> +Cu.import("resource://gre/modules/PermissionSettings.jsm", scope);
> +let globalScope = Cu.import("resource://gre/modules/PermissionsInstaller.jsm", scope);
> +Cu.import("resource://gre/modules/PermissionsTable.jsm", scope);
> +Cu.import("resource://gre/modules/AppsUtils.jsm", scope);
> +Cu.import("resource://gre/modules/Webapps.jsm", scope);

Same points here regarding unused modules and import style.

@@ +17,5 @@
> +    is(scope.AppsUtils.getAppManifestStatus(app.manifest), Ci.nsIPrincipal.APP_STATUS_INSTALLED, "The app is not privileged");
> +
> +    // Check that the app principal has the correct appId
> +    let principal = document.getElementById("content").contentDocument.defaultView.document.nodePrincipal;
> +    is(scope.DOMApplicationRegistry.getAppLocalIdByManifestURL(app.manifestURL), principal.appId, "Principal app ID correct")

Nit: trailing semi-colon.

::: webapprt/test/chrome/webperm.webapp
@@ +48,5 @@
> +  	"audio-channel-telephony": { "description": "Desc" },
> +  	"audio-channel-ringer": { "description": "Desc" },
> +  	"audio-channel-publicnotification": { "description": "Desc" },
> +  	"open-remote-window": { "description": "Desc" },
> +  	"keyboard": { "description": "Desc" }

These permissions are all indented with tab characters, resulting in:

    08-08 13:32 > git apply ~/test_permissions
    /Users/myk/test_permissions:162: space before tab in indent.
            "storage": { "description": "I need to store 1 million dollars in your bank account" },
    /Users/myk/test_permissions:163: space before tab in indent.
            "geolocation": { "description": "Desc" },
    /Users/myk/test_permissions:164: space before tab in indent.
            "camera": { "description": "Desc" },
    /Users/myk/test_permissions:165: space before tab in indent.
            "alarms": { "description": "Desc" },
    /Users/myk/test_permissions:166: space before tab in indent.
            "tcp-socket": { "description": "Desc" },
    warning: squelched 42 whitespace errors
    warning: 47 lines add whitespace errors.

Use spaces to indent except where absolutely necessary (i.e. in Makefile recipes).

Also, nit: if you add a trailing comma to this property list, then you won't have to modify its last line the next time you add a property to it.

Comment 32

[Marco Castelluccio [:marco]]

Attachment: Install permissions on first run/update and set principal appId v3

(In reply to Fabrice Desré [:fabrice] from comment #24)
> why did you change that? The aPrefBranch parameter is there on purpose.

I changed that because it was used only by one caller and it was using Services.prefs.
Anyway I've removed the changes to AppsUtils.jsm and Webapps.jsm from the patch.

> That's a strange build ID... Maybe just using Date().now() would be more
> realistic.

Date.now() can give a lot of problems in tests, it's better to use something that is guaranteed to be unique here.

(In reply to Myk Melez [:myk] [@mykmelez] from comment #30)
> (Also, ideally we wouldn't have any test-specific code in this module; but I
> don't see how to avoid it.)

I've tried really hard to find a way to avoid test-specific code, but I couldn't manage to avoid this "hack".

Comment 33

[Marco Castelluccio [:marco]]

Attachment: test_permissions

(In reply to Myk Melez [:myk] [@mykmelez] from comment #31)
> PermissionsTable.jsm doesn't appear to be used either, so it shouldn't be
> imported.
> 
> Also, note that you can iterate the values of an array slightly more simply,
> when you don't care about its indices, with for…of
> <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Statements/for...of>:
> 

I've modified the tests' code several times, at the beginning I needed PermissionsTable.jsm and the index, but then I forgot them.
 
> Also, nit: if you add a trailing comma to this property list, then you won't
> have to modify its last line the next time you add a property to it.

It's a JSON file, so a trailing comma isn't valid.

Comment 34

[Marco Castelluccio [:marco]]

Attachment: Install permissions on first run/update and set principal appId v3

There was a typo.

Comment 35

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

Comment on attachment 787835 [details] [diff] [review]
test_permissions

(In reply to Marco Castelluccio [:marco] from comment #33)
> > Also, nit: if you add a trailing comma to this property list, then you won't
> > have to modify its last line the next time you add a property to it.
> 
> It's a JSON file, so a trailing comma isn't valid.

D'oh!  Indeed!


This looks good, but I see a strange error when running these tests on Mac:

08-12 16:10 > make -C obj-x86_64-apple-darwin12.4.0/ webapprt-test-chrome
…
Mochitest INFO | runtests.py | Running tests: start.
…
TEST-INFO | checking window state
TEST-UNEXPECTED-FAIL | (browser-test.js) | Found an unexpected
TEST-START | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_noperm.js


Strangely, the test runner also reports that all tests pass:

INFO TEST-START | Shutdown
Browser Chrome Test Summary
	Passed: 134
	Failed: 0
	Todo: 0


As well as a test failure:

*** End BrowserChrome Test Results ***
…
Mochitest INFO | runtests.py | Running tests: end.
webapprt-test-chrome failed:
TEST-UNEXPECTED-FAIL | (browser-test.js) | Found an unexpected 
make: *** [webapprt-test-chrome] Error 1


Nevertheless tbpl will probably still report this as a test run failure, so we need to resolve the failure.

Comment 36

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

Comment on attachment 788285 [details] [diff] [review]
Install permissions on first run/update and set principal appId v3

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

::: webapprt/test/chrome/head.js
@@ +6,5 @@
>  
>  // Some of the code we want to provide to chrome mochitests is in another file
>  // so we can share it with the mochitest shim window, thus we need to load it.
> +Services.scriptloader
> +        .loadSubScript("chrome://webapprt/content/mochitest-shared.js", this);

Nit: runtime and Firefox code (although not always platform code) typically appends the member operator to the end of the first line in such multi-line statements that break on the operator, i.e.:

Services.scriptloader.
         loadSubScript(…);

Comment 37

[Marco Castelluccio [:marco]]

Attachment: Install permissions on first run/update and set principal appId v3

The problem was in this patch (testUrl in mochitest.js is null for chrome tests, I've just added a check).

Comment 38

[Marco Castelluccio [:marco]]

Comment on attachment 787835 [details] [diff] [review]
test_permissions

I didn't see the last comment where you r+ed the patch, anyway the only change is a |if (testUrl)| check to avoid the test error.

Comment 39

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

Comment on attachment 787835 [details] [diff] [review]
test_permissions

Looks good, mostly works, but just got broken again by bug 883923.  In the long run, the test script will run automatically, and developers will update it when landing APIs.  In the short run, however, that isn't going to happen, and the test shouldn't be so brittle (and perhaps not even in the long run, especially if the permissions in question are for APIs that don't make sense on desktop).

Can we rework the test so it only checks the permissions table for the set of permissions listed in the test app's manifest?

Comment 40

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

Comment on attachment 789308 [details] [diff] [review]
Install permissions on first run/update and set principal appId v3

Looks great!

(Note: I think we're a bit past "v2" and "v3", which are in the names of the file and the attachment, respectively. ;-)

Comment 41

[Marco Castelluccio [:marco]]

Attachment: test_permissions

(In reply to Myk Melez [:myk] [@mykmelez] from comment #39)
> Can we rework the test so it only checks the permissions table for the set
> of permissions listed in the test app's manifest?

Now it checks only a small set of permissions. Basically here we just need to check that permissions are installed, we could even do that with a single permission.

(In reply to Myk Melez [:myk] [@mykmelez] from comment #40)
> (Note: I think we're a bit past "v2" and "v3", which are in the names of the
> file and the attachment, respectively. ;-)

That's true :D I usually change the patch version number only when the new patch is really different.

Could you also re-look the first patch?

Comment 42

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

Comment on attachment 789708 [details] [diff] [review]
test_permissions

(In reply to Marco Castelluccio [:marco] from comment #41)
> Now it checks only a small set of permissions. Basically here we just need
> to check that permissions are installed, we could even do that with a single
> permission.

Looks good!


> (In reply to Myk Melez [:myk] [@mykmelez] from comment #40)
> > (Note: I think we're a bit past "v2" and "v3", which are in the names of the
> > file and the attachment, respectively. ;-)
> 
> That's true :D I usually change the patch version number only when the new
> patch is really different.

Perhaps there's a case for minor version numbers here. ;-)


> Could you also re-look the first patch?

Yup, it still looks good!  These two patches are ready to land.

Comment 43

[Marco Castelluccio [:marco]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #42)
> Yup, it still looks good!  These two patches are ready to land.

I meant the patch with r- (comment 13 and comment 14).

Comment 44

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

(In reply to Marco Castelluccio [:marco] from comment #43)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #42)
> > Yup, it still looks good!  These two patches are ready to land.
> 
> I meant the patch with r- (comment 13 and comment 14).

Ah, sorry, I misunderstood!

Ok, I traced the code, and addFromPrincipal and testExactPermissionFromPrincipal both end up calling GetTypeIndex, which has no hardcoded list of permissions: it either returns the index of the existing type or creates a new one for it.

So the string can be anything for the purposes of this API, provided we use it consistently.  The test you reference uses "geolocation", but it merely uses it as a "permission explicit for privileged and certified apps", which is presumably the case for any unknown permission type.

And the browser chrome code seems to use "geo" with nsIPermissionsManager (even if it uses "geolocation" internally):

https://github.com/mozilla/mozilla-central/blob/5bc8297959fb1f802b4138c86fbe9af08d3d4b8b/browser/components/nsBrowserGlue.js#L1977

So we can use either, and we don't have to use the same string as the browser (B2G appears to use "geolocation"); but we're already using "geo", so we should probably keep using it, or we'll reset the permission for existing users of apps that have requested it.

Comment 45

[Marco Castelluccio [:marco]]

Attachment: Install permissions on first run/update and set principal appId v3

Unbitrotted, carrying r+.

Comment 46

[Marco Castelluccio [:marco]]

Attachment: Install permissions on first run/update and set principal appId

We can't use AppsUtils.isFirstRun, I've reverted that part to an earlier version of the patch.
(We can't use it because it's already used by Webapps.jsm, so when it's called the second time in Startup.jsm it returns false).

The difference should be negligible, but I'm asking for review anyway (Myk don't kill me :D)

Comment 47

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

Comment on attachment 790509 [details] [diff] [review]
Install permissions on first run/update and set principal appId

Erm, nit:

08-16 11:15 > git apply ~/privileged_apps_permissions_v2
/Users/myk/privileged_apps_permissions_v2:105: trailing whitespace.
 
/Users/myk/privileged_apps_permissions_v2:107: trailing whitespace.
 
/Users/myk/privileged_apps_permissions_v2:112: trailing whitespace.
 
warning: 3 lines add whitespace errors.

Comment 48

[Marco Castelluccio [:marco]]

Attachment: Install permissions on first run/update and set principal appId

Carrying r+.
And I've changed the default indentation in my text editor :D

Comment 49

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

Removing checkin-needed temporarily while awaiting clarification of dependency relationship between this bug and bug 905397, per bug 905397, comment 2.

Comment 50

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

Back to checkin-needed per bug 905397, comment 3!

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/4a6b9390ff5a
https://hg.mozilla.org/integration/fx-team/rev/1cac9051df04
https://hg.mozilla.org/integration/fx-team/rev/859d42fcc526

Comment 52

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4a6b9390ff5a
https://hg.mozilla.org/mozilla-central/rev/1cac9051df04
https://hg.mozilla.org/mozilla-central/rev/859d42fcc526

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/3bfa589adaae

Comment 54

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #53)
> https://hg.mozilla.org/integration/fx-team/rev/3bfa589adaae

This was from bug 905397.