Closed Bug 1035282 Opened 6 years ago Closed 6 years ago

Marketplace apps fail to install with DUPLICATE_ORIGIN

Categories

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

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: myk, Assigned: marco)

Details

Attachments

(2 files, 2 obsolete files)

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?
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.
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).
I can't reproduce on Mac too.
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.
Priority: -- → P2
OK, I can reproduce if I remove the app and reinstall it.
Summary: Marketplace apps fail to install with INVALID_PACKAGE → Marketplace apps fail to install with DUPLICATE_ORIGIN
Attached patch Patch (obsolete) — Splinter Review
This should fix the problem.
This fixes the DUPLICATE_ORIGIN problem, but there's still another problem left. I'll investigate.
Flags: needinfo?(mar.castelluccio)
Attached patch Patch (obsolete) — Splinter Review
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
Attachment #8453136 - Attachment is obsolete: true
Attachment #8453154 - Flags: review?(myk)
Flags: needinfo?(mar.castelluccio)
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.
Attachment #8453154 - Flags: review?(myk) → review+
(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.
Attachment #8453154 - Flags: review?(fabrice)
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...
Attachment #8453154 - Flags: review?(fabrice)
(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.
Attached patch PatchSplinter Review
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
Assignee: nobody → mar.castelluccio
Attachment #8453154 - Attachment is obsolete: true
Status: NEW → ASSIGNED
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).
Attachment #8455382 - Flags: review?(fabrice)
Attachment #8455382 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/4ef82665a6e6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
No longer blocks: 1029674
Flags: in-testsuite+
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.