747552 - During webapp install, icon retrieval may return non-image files

Status: RESOLVED FIXED

(Firefox Graveyard :: Web Apps, defect, P2)

Description

[Dan Walkowski]

Some of the test apps on the marketplace contain references to non-existent icons.
These can and do return 404 pages, which makes the webapp install code believe that a valid icon was returned, leaving the icon with no icon.

Comment 1

[Asa Dotzler [:asa]]

We should do something here but I don't think this should block k9o

Comment 2

[Jason Smith [:jsmith]]

(In reply to Asa Dotzler [:asa] from comment #1)
> We should do something here but I don't think this should block k9o

Note - This does occur with some marketplace apps.

Comment 3

[Asa Dotzler [:asa]]

(In reply to Jason Smith [:jsmith] from comment #2)
> (In reply to Asa Dotzler [:asa] from comment #1)
> > We should do something here but I don't think this should block k9o
> 
> Note - This does occur with some marketplace apps.

can we tell those folks to fix it?

Comment 4

[Jason Smith [:jsmith]]

(In reply to Asa Dotzler [:asa] from comment #3)
> (In reply to Jason Smith [:jsmith] from comment #2)
> > (In reply to Asa Dotzler [:asa] from comment #1)
> > > We should do something here but I don't think this should block k9o
> > 
> > Note - This does occur with some marketplace apps.
> 
> can we tell those folks to fix it?

Andrew - Can we keep a look out for this issue during app reviews?

Comment 5

[Andrew Williamson [:eviljeff]]

(In reply to Jason Smith [:jsmith] from comment #4)
> (In reply to Asa Dotzler [:asa] from comment #3)
> > (In reply to Jason Smith [:jsmith] from comment #2)
> > > (In reply to Asa Dotzler [:asa] from comment #1)
> > > > We should do something here but I don't think this should block k9o
> > > 
> > > Note - This does occur with some marketplace apps.
> > 
> > can we tell those folks to fix it?
> 
> Andrew - Can we keep a look out for this issue during app reviews?

Its a common issue - normally because the developer assumes the icon path is relative to the manifest or launch url rather the origin.  I've been rejecting their submission and requesting they fix it.

I hoping it will be something that will become a lot less common now developers can use the consumer pages to try installing and launching their own app.

Comment 6

[John Drinkwater (:beta)]

(In reply to Andrew Williamson [:eviljeff] from comment #5)
> I hoping it will be something that will become a lot less common now
> developers can use the consumer pages to try installing and launching their
> own app.

Came across this problem with https://marketplace-dev.allizom.org/en-US/app/sandglaz-1/
There’s no information in the web console about failed/missing icons, some feedback there may improve things for webapp deployers. Lest they assume Firefox is broken.

Comment 7

[Marco Castelluccio [:marco]]

I'm a bit reworking the icon related stuff in the installer code.

Comment 8

[Marco Castelluccio [:marco]]

Attachment: Patch

With this patch we're trying to use the icon provided by the application, if the download fails we'll use the chrome:// icon.

Comment 9

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

Comment on attachment 786377 [details] [diff] [review]
Patch

Marco and I talked about this in person and agreed to make a couple changes to the approach to simplify the control flow and reduce the brittleness of fallback icon retrieval:

1. use promises to report errors consistently during the installation process;
2. rework fallback icon retrieval to eliminate the loop.

So I'm cancelling this review, and Marco is going to follow up with a new patch.

Comment 10

[Marco Castelluccio [:marco]]

Attachment: Patch v2

Comment 11

[Marco Castelluccio [:marco]]

Comment on attachment 789689 [details] [diff] [review]
Patch v2

Turns out on Windows I need to use the mimeService because channel.contentType isn't correct.

Comment 12

[Marco Castelluccio [:marco]]

Attachment: Patch v2

Comment 13

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

Comment on attachment 789914 [details] [diff] [review]
Patch v2

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

This is looking much better!  But I'd like to make a few more improvements, as noted.

::: browser/modules/webappsUI.jsm
@@ +114,5 @@
>  
>            DOMApplicationRegistry.confirmInstall(aData, false, localDir, null,
>              function (aManifest) {
> +              WebappsInstaller.install(aData, aManifest).then(
> +                function onSuccess() {

Nit: the Developer Tools folks tell me that the JS engine has become very good at identifying anonymous function expressions to debuggers and other developer tools, and they recommend no longer giving them names.

(The one exception is when a function needs to refer to itself, f.e. a event/notification listener/handler/observer function that unregisters itself, in which case the expression needs a name for the function to refer to.)

::: toolkit/webapps/WebappsIconHelpers.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/. */
>  
> +const DEFAULT_ICON_URL = "chrome://global/skin/icons/webapps-64.png";

Nice refactoring!

@@ +42,3 @@
>   *                 - processIcon()
> + * @param aUseTmpForIcon Whether to use a temp file to store
> + *                       the downloaded icon.

It looks like the value of this parameter is platform-specific, and it doesn't otherwise vary, so it would be better to use a preprocessor directive inside downloadIcon to include only the relevant code rather than passing an argument through multiple functions, which complicates the call signatures and increases the risk of misuse.

@@ +53,5 @@
>  
> +      // If we've already tried with the chrome icon, trying again is useless.
> +      if (aShell.iconURI.spec == DEFAULT_ICON_URL) {
> +        return;
> +      }

Returning here resolves the Task promise instead of rejecting it, even though we failed to get an icon for the app.  It seems like we should throw in this case instead.

But it also seems unnecessary to check for this case in the first place, since we shouldn't try retrieving the default icon first; and if we did for some reason, then it shouldn't fail.

@@ +60,5 @@
> +      // correct icon.
> +      aShell.iconURI = Services.io.newURI(DEFAULT_ICON_URL, null, null);
> +
> +      let [ mimeType, icon ] = yield downloadIcon(aShell.iconURI, aUseTmpFile);
> +      yield aShell.processIcon(mimeType, icon);

Nit: (it shouldn't matter, because retrieving the default icon shouldn't fail, but) it seems more logical to change the iconURI property *after* retrieving and processing the icon, to ensure the new value is successfully downloaded and processed before we change iconURI to it.

@@ +79,5 @@
>      } else {
> +      mimeType = mimeService.getTypeFromURI(aIconURI);
> +     }
> +  } catch(e) {
> +    deferred.reject("Failed to determine icon MIME type");

Nit: like other such rejections, this should include the error message, i.e.:

  deferred.reject("Failure determining icon MIME type: " + e);

@@ +87,5 @@
> +  function onIconDownloaded(aStatusCode, aIcon) {
> +    if (Components.isSuccessCode(aStatusCode)) {
> +      deferred.resolve([ mimeType, aIcon ]);
> +    } else {
> +      deferred.reject("Failure downloading icon");

Nit: here too it would be useful to include the status code representing the failure, i.e.:

  deferred.reject("Failure downloading icon: " + aStatusCode);

@@ +102,5 @@
>        };
>  
>        let tmpIcon = Services.dirsvc.get("TmpD", Ci.nsIFile);
>        tmpIcon.append("tmpicon." + mimeService.getPrimaryExtension(mimeType, ""));
>        tmpIcon.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, 0666);

While you're making changes nearby, can you replace this deprecated octal literal with its recommended replacement? parseInt("666", 8) is the new hotness.

@@ +131,5 @@
>      channel.notificationCallbacks = new CertUtils.BadCertHandler(true);
>  
>      channel.asyncOpen(listener, null);
>    } catch(e) {
> +    deferred.reject("Failure getting icon (" + e + ")");

Nit: it'd be nice to make this message even more consistent with the others in this file and being a bit more specific about what failed than the generic term "getting", i.e.:

  deferred.reject("Failure initiating download of icon: " + e);

::: toolkit/webapps/WebappsInstaller.jsm
@@ +237,2 @@
>  
> +      yield getIconForApp(this, false);

I'm not a fan of the way these install functions pass the shell into getIconForApp, which then modifies it.  I know that's the way the current code is written, but I'd much rather the shell modify itself, to better encapsulate object mutation.  And now that you've moved most of the logic that was in getIconForApp into downloadIcon, it should be pretty easy to do this by calling downloadIcon and processIcon from within the install functions themselves, i.e.:

    try {
      let [ mimeType, icon ] = yield downloadIcon(this.iconURI);
      yield this.processIcon(mimeType, icon);
    }
    catch(ex) {
      let [ mimeType, icon ] = yield downloadIcon(DEFAULT_ICON_URL);
      yield this.processIcon(mimeType, icon);
    
      // Set the iconURI property so that the user notification will have the
      // correct icon.
      this.iconURI = Services.io.newURI(DEFAULT_ICON_URL, null, null);
    }

Even better, put this cross-platform code into a getIcon method of the NativeApp prototype that the install methods call to get the icon.

Comment 14

[Marco Castelluccio [:marco]]

Attachment: Patch v3

Comment 15

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

Comment on attachment 792950 [details] [diff] [review]
Patch v3

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

Looks good, only a couple minor nits, r=myk!

::: toolkit/webapps/WebappsInstaller.jsm
@@ +255,5 @@
> +      } catch (ex) {
> +        this._removeInstallation(false);
> +        throw(ex);
> +      }
> +    }.bind(this));

Nit: now that we have arrow functions <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/arrow_functions>, we should start using them in places like this where we currently *bind(this)*.

@@ +726,5 @@
> +
> +    let process = Cc["@mozilla.org/process/util;1"]
> +                  .createInstance(Ci.nsIProcess);
> +    let sipsFile = Cc["@mozilla.org/file/local;1"]
> +                  .createInstance(Ci.nsILocalFile);

Nit: the formatting here is unusual.  These should be formatted in one of these two conventional ways:

    let process = Cc["@mozilla.org/process/util;1"].
                  createInstance(Ci.nsIProcess);
    let sipsFile = Cc["@mozilla.org/file/local;1"].
                   createInstance(Ci.nsILocalFile);

    let process = Cc["@mozilla.org/process/util;1"]
                    .createInstance(Ci.nsIProcess);
    let sipsFile = Cc["@mozilla.org/file/local;1"]
                     .createInstance(Ci.nsILocalFile);

Comment 16

[Marco Castelluccio [:marco]]

Attachment: Patch v3

Carrying r+.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backed out because one of the 4 webapps patches landed on your behalf was causing mochitest-other assertions. You really need to start running your patches through Try before requesting checkin. This is far from the first bustage we've had to backout for.
https://hg.mozilla.org/integration/fx-team/rev/3a3d9846ceca

https://tbpl.mozilla.org/php/getParsedLog.php?id=27007390&tree=Fx-Team

Comment 19

[Marco Castelluccio [:marco]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #18)
> You really need to start running your patches through Try before requesting checkin.

I did: I did: https://tbpl.mozilla.org/?tree=Try&rev=30a4e8eb935c

Comment 20

[Marco Castelluccio [:marco]]

The problem was bug 899353, that patch wasn't in my try push.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d05cea4a905b