828863 - Add a remote debugger actor to install apps

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[[:fabrice] Fabrice Desré]

The goal is to let developers install applications on their phones with privileged status without to use a signing service.

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: patch v1

Panagiotis, can you review the remote debugging part? I also have a xpcshell script to test it if you want to try it.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 700246 [details] [diff] [review]
patch v1

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

I have a few comments about making the packet form fit well with the rest of the protocol in some cases, but it looks great overall.

As an aside: have you considered adding this actor to desktop Firefox as well? I'm not sure if it will be of much benefit to a developer, but we could add tests to exercise it and consider supporting it in Firefox Developer Tools.

::: b2g/chrome/content/dbg-webapps-actors.js
@@ +55,5 @@
> +                         type: "webappsEvent",
> +                         event: "InstallDone",
> +                         subject: null,
> +                         data: { status: "ok" }
> +                       });

I see you mostly copied the eventNotification structure verbatim, which essentially mirrors the observer notifications, but you could simplify a bit. I'd go for:
{ from: self.actorID,
  type: "webappsEvent",
  appId: aId }

appId seems useful, otherwise there is no indication which installation this event corresponds to. Note that in the protocol in general we don't return a success status, since the lack of an "error" property denotes success.

@@ +72,5 @@
> +        subject: null,
> +        data: { status: "error",
> +                message: aMsg
> +              }
> +      });

In this case I'd just return:
{ from: this.actorID,
  type: "webappsEvent",
  appId, aId,
  error: "installationFailed",
  message: aMsg }

and make sure that _sendError is called with human-readable descriptive text. aId would have to be provided as a parameter of course.

@@ +139,5 @@
> +
> +          // Move application.zip to the destination directory.
> +          let zipFile = aDir.clone();
> +          zipFile.append("application.zip");
> +          zipFile.moveTo(appDir, "application.zip");

Nit: aDir and appDir were slightly confusing to read. If you care, perhaps consider renaming one to installDir or somesuch.

@@ +178,5 @@
> +
> +  // @param appId : The id of the app we want to install. We will look for the
> +  //                files for the app in $TMP/b2g/$appId
> +  //                For packaged apps: application.zip
> +  //                For hosted apps:   metadata.json and manifest.webapp

A multi-line comment (/* */) would be more consistent, but I won't be picky about it. It would also be cool if the comment described all expected request parameters.

@@ +186,5 @@
> +
> +    let appId = aRequest.appId;
> +    if (!appId) {
> +      return { status: "error",
> +               message: "MISSING_PARAMETER_APPID"}

The standard protocol response in this case is:
{ error: "missingParameter",
  message: "missing parameter appId" }

See: https://wiki.mozilla.org/Remote_Debugging_Protocol#Requests_and_Replies
Feel free to make the message as explanatory or verbose as you want.

@@ +196,5 @@
> +    if (!appDir || !appDir.exists()) {
> +      return { status: "error",
> +               message: "MISSING_DIRECTORY",
> +               detail: appDir.path
> +             }

Same as above, this should return missingParameter if no appDir is specified and badParameterType if the directory is invalid in some way:
{ error: "badParameterType",
  message: "missing directory " + appDir.path }

@@ +217,5 @@
> +      if (missing) {
> +        try {
> +          aDir.remove(true);
> +        } catch(e) {}
> +        return { status: "error", message: "MISSING_HOSTED_APP_FILE" }

Same here:
{ error: "badParameterType",
  message: missing + " hosted app file is missing" }

@@ +223,5 @@
> +
> +      this.installHostedApp(appDir, appId, appType);
> +    }
> +
> +    return { status: "ok", appId: appId, path: appDir.path }

Protocol responses in general are considered successful if they don't contain an error property. In this case you'd just return:
{ appId: appId,
  path: appDir.path }

Comment 3

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e1fe4baad60

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 700246 [details] [diff] [review]
patch v1

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

::: b2g/chrome/content/dbg-webapps-actors.js
@@ +118,5 @@
> +            self._registerApp(app, aId, aDir);
> +          });
> +        } catch(e) {
> +          // If anything goes wrong, just send it back.
> +          self.sendError(e.toString());

Don't know why I didn't notice this before, but this should have been self._sendError... (notice the underscore).

@@ +166,5 @@
> +
> +          self._registerApp(app, aId, aDir);
> +        } catch(e) {
> +          // If anything goes wrong, just send it back.
> +          self.sendError(e.toString());

Ditto.

Comment 5

[[:fabrice] Fabrice Desré]

followup for the .sendError I didn't caught myself

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd7d549c42ad

Comment 6

[Johnny Stenback (:jst)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/3a5ba563fa69
https://hg.mozilla.org/releases/mozilla-b2g18/rev/608961f27944

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4e1fe4baad60
https://hg.mozilla.org/mozilla-central/rev/bd7d549c42ad