1035282 - Marketplace apps fail to install with DUPLICATE_ORIGIN

Status: RESOLVED FIXED

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

Description

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

The two Marketplace packaged apps in Marketplace both fail to install with INVALID_PACKAGE.

  https://marketplace.firefox.com/app/marketplace-package
  https://marketplace.firefox.com/app/marketplace

Possible regression from bug 1029674?

Comment 1

[Marco Castelluccio [:marco]]

This is the only place where we throw INVALID_PACKAGE: http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#3157

So there should be also an error reported in the console, could you copy & paste it here?

Bug 1029674 basically didn't change anything in Webapps.jsm, so I think this isn't a regression from that bug.

Comment 2

[Marco Castelluccio [:marco]]

I've just tried and I can't reproduce on Linux, I'll try on Mac (but I wouldn't expect this to be a platform-specific bug).

Comment 3

[Marco Castelluccio [:marco]]

I can't reproduce on Mac too.

Comment 4

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

Attachment: webapps.json file from profile exhibiting problem

Hmm, it appears to be specific to the state of my profile.  Upon retrying to install either app, I get a DUPLICATE_ORIGIN error, although the app is not installed.  But I can successfully install one of the apps using the same installation of Firefox with a new profile.

Here's the webapps.json file from my original profile (modified only to hide the random string in the basePath).  It shows two Marketplace apps, "packaged.marketplace.firefox.com" and "marketplace.firefox.com".  The first one is the one I subsequently successfully installed using a new profile, while the second one is not installed.

And the Marketplace website correctly shows a Launch button for the first app and an Install button for the second.  But pressing the Launch button (after hacking the website to enable it, of course) for the first app does nothing; while pressing the Install button for the second app reports DUPLICATE_ORIGIN.

Comment 5

[Marco Castelluccio [:marco]]

OK, I can reproduce if I remove the app and reinstall it.

Comment 6

[Marco Castelluccio [:marco]]

Attachment: Patch

This should fix the problem.

Comment 7

[Marco Castelluccio [:marco]]

This fixes the DUPLICATE_ORIGIN problem, but there's still another problem left. I'll investigate.

Comment 8

[Marco Castelluccio [:marco]]

Attachment: Patch

This should fix the problem by:
1) Throwing DUPLICATE_ORIGIN only if the app is launchable
2) Modifying the ID only if it's needed

Comment 9

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

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

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

This doesn't fix the "Launch" button, but it fixes the issue in this bug.

This reminds me that we should revisit isLaunchable.  We originally expected DOMApplicationRegistry to include all the user's apps, whether or not they're installed on this particular machine.  But we never implemented sync across devices, so it is effectively now just the set of apps installed natively—except when it gets out of sync with local installs, as in this case.

Perhaps we should be more aggressive about making sure DOMApplicationRegistry and native installations stay in sync, as we do on Android (and which is the case by default on FxOS).

::: dom/apps/src/Webapps.jsm
@@ +3414,5 @@
>          debug("Setting origin to " + uri.prePath +
>                " for " + aOldApp.manifestURL);
>          let newId = uri.prePath.substring(6); // "app://".length
> +        if (newId in this.webapps &&
> +            this._isLaunchable(this.webapps[newId])) {

Nit: this fits all on one line.

@@ +3424,5 @@
> +
> +        if (oldId == newId) {
> +          // This could happen when we have an app in the registry
> +          // that is not launchable.
> +          return;

I wonder if there's anything else we should do at this point.

Comment 10

[Marco Castelluccio [:marco]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #9)
> Comment on attachment 8453154 [details] [diff] [review]
> Patch
> 
> Review of attachment 8453154 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This doesn't fix the "Launch" button, but it fixes the issue in this bug.

Could you file a new bug for this?

> This reminds me that we should revisit isLaunchable.  We originally expected
> DOMApplicationRegistry to include all the user's apps, whether or not
> they're installed on this particular machine.  But we never implemented sync
> across devices, so it is effectively now just the set of apps installed
> natively—except when it gets out of sync with local installs, as in this
> case.
> 
> Perhaps we should be more aggressive about making sure
> DOMApplicationRegistry and native installations stay in sync, as we do on
> Android (and which is the case by default on FxOS).

There's bug 892020, that was stuck at the design phase (I'll probably restart
it in the following days).
What solution have you adopted on Android?

> @@ +3424,5 @@
> > +
> > +        if (oldId == newId) {
> > +          // This could happen when we have an app in the registry
> > +          // that is not launchable.
> > +          return;
> 
> I wonder if there's anything else we should do at this point.

To make sure, I'll ask Fabrice's review too.

Comment 11

[[:fabrice] Fabrice Desré]

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

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

::: dom/apps/src/Webapps.jsm
@@ +3424,5 @@
> +
> +        if (oldId == newId) {
> +          // This could happen when we have an app in the registry
> +          // that is not launchable.
> +          return;

Not changing the origin and not throwing an error means that we let the app be installed but not in the state that is expected. So it will very likely not work as expected. I think I would throw...

Comment 12

[Marco Castelluccio [:marco]]

(In reply to Fabrice Desré [:fabrice] from comment #11)
> Comment on attachment 8453154 [details] [diff] [review]
> Patch
> 
> Review of attachment 8453154 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/Webapps.jsm
> @@ +3424,5 @@
> > +
> > +        if (oldId == newId) {
> > +          // This could happen when we have an app in the registry
> > +          // that is not launchable.
> > +          return;
> 
> Not changing the origin and not throwing an error means that we let the app
> be installed but not in the state that is expected. So it will very likely
> not work as expected. I think I would throw...

The origin is changed a few lines before, the lines following this branch are changing the ID, so this branch returns early if we don't really need to change the ID.

Comment 13

[Marco Castelluccio [:marco]]

Attachment: Patch

The patch now comes with a test:
1) Install app with custom origin
2) Uninstall the app from the OS but not from the registry
3) Install app again

Comment 14

[Marco Castelluccio [:marco]]

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

Fabrice this problem appears when the registry isn't in sync with apps installed. One example is the test case. The app already has the correct ID, because in confirmInstall we re-use the ID (http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm?rev=340b19c14d3d#2461).

Comment 15

[Marco Castelluccio [:marco]]

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

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ef82665a6e6