Marketplace apps fail to install with DUPLICATE_ORIGIN

RESOLVED FIXED in Firefox 33

Status

Firefox Graveyard
Webapp Runtime
P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: myk, Assigned: marco)

Tracking

Trunk
Firefox 33
x86
Mac OS X
Bug Flags:
in-testsuite +

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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?
(Assignee)

Comment 1

3 years ago
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.
(Assignee)

Comment 2

3 years ago
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).
(Assignee)

Comment 3

3 years ago
I can't reproduce on Mac too.
(Reporter)

Comment 4

3 years ago
Created attachment 8453110 [details]
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.
(Reporter)

Updated

3 years ago
Priority: -- → P2
(Assignee)

Comment 5

3 years ago
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
(Assignee)

Comment 6

3 years ago
Created attachment 8453136 [details] [diff] [review]
Patch

This should fix the problem.
(Assignee)

Comment 7

3 years ago
This fixes the DUPLICATE_ORIGIN problem, but there's still another problem left. I'll investigate.
Flags: needinfo?(mar.castelluccio)
(Assignee)

Comment 8

3 years ago
Created attachment 8453154 [details] [diff] [review]
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
Attachment #8453136 - Attachment is obsolete: true
Attachment #8453154 - Flags: review?(myk)
Flags: needinfo?(mar.castelluccio)
(Reporter)

Comment 9

3 years ago
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+
(Assignee)

Comment 10

3 years ago
(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.
(Assignee)

Updated

3 years ago
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)
(Assignee)

Comment 12

3 years ago
(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.
(Assignee)

Comment 13

3 years ago
Created attachment 8455382 [details] [diff] [review]
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
Assignee: nobody → mar.castelluccio
Attachment #8453154 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 14

3 years ago
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+
(Assignee)

Comment 15

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef82665a6e6
https://hg.mozilla.org/mozilla-central/rev/4ef82665a6e6
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
(Assignee)

Updated

3 years ago
No longer blocks: 1029674
(Assignee)

Updated

3 years ago
Flags: in-testsuite+
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.