978143 - Cancelling APK installation does not trigger error callback

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

[Mark Striemer [:mstriemer]]

This might be the expected behaviour (matches desktop) but it does not match fxos.

When installing an app through marketplace using `mozApps.install` cancelling the Android install dialog does not trigger the registered `onerror` callback. Hitting cancel on fxos does trigger the callback however.

Comment 1

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

I think mhaigh is working on a variant of this bug, if not a duplicate.

Comment 2

[Martyn Haigh (:mhaigh)]

Attachment: Propigate APK install cancellation back to webpage

This patch needs the patch from 957967 applied first

Comment 3

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

Comment on attachment 8392541 [details] [diff] [review]
Propigate APK install cancellation back to webpage

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

::: mobile/android/base/webapp/EventListener.java
@@ +189,4 @@
>          });
>      }
>  
> +    public static void installApk(final Activity context, final JSONObject message) {

Nice optimization!

@@ +253,5 @@
>                          receiver.cleanup();
> +                        JSONObject dataObj = null;
> +                        try {
> +                          dataObj = new JSONObject(message.getString("data"));
> +                          dataObj.put("error", "user cancelled apk install");

Nits: cancelled => canceled, apk => APK

@@ +255,5 @@
> +                        try {
> +                          dataObj = new JSONObject(message.getString("data"));
> +                          dataObj.put("error", "user cancelled apk install");
> +                        } catch (Exception ex) {
> +                          Log.e(LOGTAG, "Exception handling message \"installApk\":", ex);

Nit: make this a more specific message, like "error setting error string".

@@ +259,5 @@
> +                          Log.e(LOGTAG, "Exception handling message \"installApk\":", ex);
> +                          return;
> +                        }
> +                        GeckoAppShell.notifyGeckoOfEvent(GeckoEvent.createBroadcastEvent(
> +                                         "Webapps:InstallError", dataObj.toString()));

Nit: put this statement all on one line.

Comment 4

[Martyn Haigh (:mhaigh)]

Attachment: Propigate APK install cancellation back to webpage

Revised patch relies on patch from bug 957067 to be applied first

Comment 5

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

Comment on attachment 8403362 [details] [diff] [review]
Propigate APK install cancellation back to webpage

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

Overall, this looks great, modulo a few issues noted below.

But jhugman pointed me at Components.utils.getWeakReference <https://developer.mozilla.org/en-US/docs/Components.utils.getWeakReference>, and it looks like it'd actually make this implementation much easier.

Instead of having two maps, we can have a single map using a simple object literal:

  messageMap: {},

Store weak references to the message managers via string literal keys:

  this.messageMap[aMessage.requestID] = Cu.getWeakReference(aMessageManager);

And retrieve them via the same keys:

  let messageManager = this.messageMap[aMessage.requestID].get();

So I think we should employ this method, which jhugman is planning to use over in bug 986085.

::: mobile/android/base/webapp/EventListener.java
@@ +279,4 @@
>                          // unregistered the receiver).
>                          Log.e(LOGTAG, "error unregistering install receiver: ", e);
>                      }
> +                    GeckoAppShell.notifyGeckoOfEvent(GeckoEvent.createBroadcastEvent("Webapps:InstallError", messageData.toString()));

Since you changed the name of the event in browser.js, you'd need to change it here as well; except that you should change it back in browser.js, per my comments below!

::: mobile/android/chrome/content/browser.js
@@ +1582,5 @@
>          WebappManager.install(JSON.parse(aData), aSubject);
>          break;
>  
> +      case "Webapps:onInstallError":
> +        WebappManager.installError(JSON.parse(aData));

Erm, I actually meant that the name of the WebappManager method should be "onInstallError", as that's a conventional name for event handlers when their names aren't verb-initial (like all the other method names in WebappManager), i.e.:

  case "Webapps:InstallError":
    WebappManager.onInstallError(JSON.parse(aData));

However, the more I think about it, the more I suspect that aligning the method name to the event name (which has been the convention to date for all the verb-initial event/method names in WebappManager) is better, so let's leave it the way you had it initially:

  case "Webapps:InstallError":
    WebappManager.installError(JSON.parse(aData));

And sorry about the churn!

::: mobile/android/modules/WebappManager.jsm
@@ +43,5 @@
>  this.WebappManager = {
>    __proto__: DOMRequestIpcHelper.prototype,
>  
> +  //Used to store keys and references to message managers in case of error in
> +  //the Java which needs to be routed back to the calling web page.

Nit: add a space between the comment delimiters and their comments.

@@ +203,5 @@
>    },
>  
>    autoInstall: function(aData) {
> +    let key = this._messageKeyMap[aData.requestID];
> +    if(!!this._messageMap && key && this._messageMap.has(key)) {

Nit: add a space between the "if" keyword and the opening parenthesis.

@@ +282,5 @@
>    }).bind(this)); },
>  
> +  installError: function(aMessage) {
> +    let key = this._messageKeyMap[aMessage.requestID];
> +    if (!!this._messageMap && !!key && this._messageMap.has(key)) {

this._messageMap should always exist, since we define it unconditionally and never delete it, so we don't need to check it here.

@@ +284,5 @@
> +  installError: function(aMessage) {
> +    let key = this._messageKeyMap[aMessage.requestID];
> +    if (!!this._messageMap && !!key && this._messageMap.has(key)) {
> +      let messageManager = this._messageMap.get(key);
> +      if(messageManager) {

Nit: add a space between the "if" keyword and the opening parenthesis.

Comment 6

[Martyn Haigh (:mhaigh)]

Attachment: Propigate APK install cancellation back to webpage

Implemented getWeakReference and updated according to feedback.

Comment 7

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

Comment on attachment 8410290 [details] [diff] [review]
Propigate APK install cancellation back to webpage

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

Looking good, just minor issues.

Note that the fix in bug 957067 is back in the patch, and a bunch of my comments just repeat stuff I said about that fix.  If you'd like, we can work on both fixes together in this bug.  That way you don't have to keep separating them out.

::: mobile/android/base/webapp/EventListener.java
@@ +200,5 @@
>  
> +    public static void installApk(final Activity context, JSONObject message) {
> +        String filePath = null;
> +        final JSONObject messageData;
> +        JSONObject returnObject = new JSONObject();

Nit: I would avoid using "object" in the names of variables and simply name this after its purpose; and since we only need to use it when sending an error, I would call it either "error" or "response" (which is the name of the EventDispatcher.sendError parameter to which we pass it).

@@ +211,5 @@
> +
> +            try {
> +                returnObject.put("error", "Error getting file path and data");
> +            } catch (Exception ex) {
> +                Log.e(LOGTAG, "Exception handling message \"installApk\":", ex);

Nit: I would actually reverse the severity of these two log calls, i.e. make "Exception handling message \"installApk\" be Log.wtf and "Error getting file path and data" above be Log.e.

After all, installApk may not expect "Error getting file path and data", but it knows that it can happen, since it doesn't control the input that its caller provides.

Whereas it's in complete control of returnObject and the literal string error message that it puts into it.  So that should never fail, and failure is completely unexpected.

However, in this case we don't need returnObject at all, since EventDispatcher.sendError accepts any Object for its "response" parameter, so we should simply pass the literal string "Error getting file path and data".

@@ +213,5 @@
> +                returnObject.put("error", "Error getting file path and data");
> +            } catch (Exception ex) {
> +                Log.e(LOGTAG, "Exception handling message \"installApk\":", ex);
> +            }
> +            EventDispatcher.sendError(message, returnObject);

We don't need to send the message, since the recipient already has access to it.  So we should use the simpler, one-parameter form EventDispatcher.sendError(Object response) here.

@@ +225,3 @@
>          try {
> +            app = messageData.getJSONObject("app");
> +            manifestUrl = app.getString("manifestURL");

Nit: there's no need to use two separate try/catch blocks to retrieve the various bits of data from the message and handle any errors.  We should combine them into one such block.

::: mobile/android/modules/WebappManager.jsm
@@ +44,5 @@
>    __proto__: DOMRequestIpcHelper.prototype,
>  
> +  // Used to store keys and references to message managers in case of error in
> +  // the Java which needs to be routed back to the calling web page.
> +  _messageMap: {},

Nit: this actually maps message managers, not messages, so I would call this _messageManagers (with a plural instead of the word "map"), which is a more conventional name for such maps.

@@ +90,5 @@
>      sendMessageToJava({
>        type: "Webapps:InstallApk",
>        filePath: filePath,
> +      data: aMessage,
> +    }, (returnData, returnError) => {

sendMessageToJava takes separate success and error callbacks, so this error handling should be in a separate method.

@@ +203,5 @@
>    autoInstall: function(aData) {
> +    let messageManager = this._messageMap[aData.requestID];
> +    if (messageManager) {
> +      delete this._messageMap[aData.requestID];
> +    }

Nit: since this only uses messageManager once, it might as well simply check this._messageMap[aData.requestID] directly:

  if (this._messageMap[aData.requestID]) {
    delete this._messageMap[aData.requestID];
  }

@@ +365,4 @@
>  
>      sendMessageToJava({
>        type: "Webapps:GetApkVersions",
> +      packageNames: packageNames

Nit: to preserve history, avoid fixing whitespace on lines that aren't otherwise being changed.

Comment 8

[Martyn Haigh (:mhaigh)]

Attachment: Propigate APK install cancellation back to webpage

Newly constructed patch based on bug 957067 rewrite

Comment 9

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

Comment on attachment 8419422 [details] [diff] [review]
Propigate APK install cancellation back to webpage

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

Looks great!

::: mobile/android/modules/WebappManager.jsm
@@ +474,4 @@
>            data: JSON.stringify(msg),
>          }, (returnData, returnError) => {
>            if (!!returnError) {
> +            // There's no page to report back to so drop the error

Nit: delimit this complete sentence with a period!

Also, it'd be handy to add a note here that we should notify the user about the failure (although the functionality itself we should add in a separate bug).

@@ +474,5 @@
>            data: JSON.stringify(msg),
>          }, (returnData, returnError) => {
>            if (!!returnError) {
> +            // There's no page to report back to so drop the error
> +            debug("APK install failed : " + returnError)

Nit: delimit this statement with a semi-colon!

Comment 10

[Martyn Haigh (:mhaigh)]

Attachment: Propigate APK install cancellation back to webpage

- Moved some code from Bug 957067 dealing with the sendMessageToJava callback function.
- added delimiters
- added TODO for user notification of apk install fail on app update

Comment 11

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

Comment on attachment 8421000 [details] [diff] [review]
Propigate APK install cancellation back to webpage

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

Looks great!

Comment 12

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

Comment on attachment 8421000 [details] [diff] [review]
Propigate APK install cancellation back to webpage

Erm, I meant to give feedback+.  This still needs review from a module peer.

Comment 13

[Martyn Haigh (:mhaigh)]

https://tbpl.mozilla.org/?tree=Try&rev=27e2edbe658a

Comment 14

[Ryan VanderMeulen [:RyanVM]]

This is bitrotted. Please rebase.

Comment 15

[Martyn Haigh (:mhaigh)]

Attachment: Propigate APK install cancellation back to webpage

Updated patch to cure bitrot

Comment 16

[Ryan VanderMeulen [:RyanVM]]

I'm still hitting tons of bitrot on this (though now I'm trying to apply it on top of bug 957067 which probably isn't helping).

Comment 17

[Martyn Haigh (:mhaigh)]

Attachment: Propigate APK install cancellation back to webpage

Fixed bitrot again!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Yay persistence! :)
https://hg.mozilla.org/integration/fx-team/rev/31db3f3b1107

Comment 19

[Martyn Haigh (:mhaigh)]

Yay!  Happy days.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/31db3f3b1107