876293 - Installation of web apps from Firefox Marketplace fails on Linux during mapping of categories

Status: RESOLVED FIXED

(Marketplace Graveyard :: Consumer Pages, defect, P2)

Description

[Eike Hein]

Attachment: fix-webapps-install-on-linux.patch

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20130526 Firefox/24.0 (Nightly/Aurora)
Build ID: 20130526031046

Steps to reproduce:

I tried to install an application from Firefox Marketplace (http://marketplace.firefox.com).


Actual results:

Installation failed silently. The Error Console provides the following error:

  Error installing app: TypeError: (intermediate value).toLowerCase is not a function


Expected results:

The application should have finished successfully.

Now, I've looked at the relevant code (toolkit/webapps/WebappsInstaller.jsm) and debugged it a little, and it turns out that the reason for the failure is that it expects the list of application categories (this.app.categories) to be a list of strings, however Marketplace appears to hand it a list of integers. (At least I _think_ that the installPackage()[1] call is issued by JavaScript code served by the Marketplace web app - I'm super-new to this whole ecosystem and piecing things together by prodding ...)

The integers I'm seeing correspond to the numeric category id's available via the Marketplace API:https://marketplace.firefox.com/api/v1/apps/category/

So it seems Marketplace has changed behavior, intentionally or not, and the client code wasn't updated to match.

Since I'd like to ship a copy of Firefox with working Marketplace install in a product within a short timeframe, I've decided to work around this problem by way of the patch I've attached to this ticket. It extends the translation table from Marketplace categories to freedesktop.org categories by a set of numeric keys, and only attempts to call toLowerCase() on a category if typeof concludes that it's not a number.

This seemed to be the only way to go for me, since in case Marketplace decides to send down strings again as a result of this bug report -- the resulting code should handle both.


1 = https://developer.mozilla.org/en-US/docs/Web/API/Apps.installPackage?redirectlocale=en-US&redirectslug=DOM%2FApps.installPackage

Comment 1

[Jason Smith [:jsmith]]

This actually probably isn't the right place to do the workaround actually - I'd fix this on the marketplace side, not the client side code. The numbers carry no meaning in the client side code, but they do on the marketplace side.

In that argument, I'm bouncing this over to the Marketplace product.

Comment 2

[Eike Hein]

Aye, personally I agree that it makes more sense to change Marketplace back(?) to using strings, considering clients expecting them are already out there now. But since I can do little about Marketplace locally, I dug into the client sources instead.

Comment 3

[Wil Clouser [:clouserw]]

Sorry, since this jumped products I think I may have lost context.  I'm looking at the link from comment 0 (https://marketplace.firefox.com/api/v1/apps/category/) and I see an array of categories in this format:

> {"id": "155", "name": "Games", "resource_uri": "/api/v1/apps/category/155/", "slug": "games"}

Which gives you names, slugs, and IDs.  What are you asking for in this bug?

Comment 4

[Jason Smith [:jsmith]]

(In reply to Wil Clouser [:clouserw] from comment #3)
> Sorry, since this jumped products I think I may have lost context.  I'm
> looking at the link from comment 0
> (https://marketplace.firefox.com/api/v1/apps/category/) and I see an array
> of categories in this format:
> 
> > {"id": "155", "name": "Games", "resource_uri": "/api/v1/apps/category/155/", "slug": "games"}
> 
> Which gives you names, slugs, and IDs.  What are you asking for in this bug?

I think a while back marketplace implemented something where when you called the install function to install hosted app on linux, we would provide the category of the app as part of the dictionary in the second parameter (e.g. Games). The issue filed here is the fact that we're now passing the ID instead, not the category, so the client-side code is erroring out given that we're expecting a string in letters, not a number.

Comment 5

[Eike Hein]

That's correct.

And to perhaps drive this home a bit more, the consequence of this is that no Firefox release made since October 2012 (when a commit by Marco Castelluccio turned the reliance on strings into a runtime failure by introducing the toLowerCase() call) is capable of installing applications from Marketplace on Linux, and no version of Firefox ever released is capable of correctly associating Marketplace applications with menu categories on Linux (because the translation table from Marketplace categories to Linux menu categories is keyed by string).

Comment 6

[Wil Clouser [:clouserw]]

It sounds like Kevin added this so assigning to him.

Comment 7

[Kevin Ngo [:ngoke]]

https://github.com/mozilla/zamboni/commit/4398814875b6170f3ea0a0b95899c008ea7ba279

Not sure if this is correct, but when installing Wikipedia, it nows shows up under the "Other" category in Ubuntu. Before, it would not show up at all.

Comment 8

[Eike Hein]

That's fine, and common behavior among menu implementations which only show a defined set of top-level categories and treat the rest as "Other". Thanks!

Comment 9

[Eike Hein]

Has this been deployed already? Marketplace installation on Linux still fails with current Nightly (sans my patch).

Comment 10

[Jason Smith [:jsmith]]

(In reply to Eike Hein from comment #9)
> Has this been deployed already? Marketplace installation on Linux still
> fails with current Nightly (sans my patch).

Not yet. See the target milestone - this will be deployed on June 6th.

Comment 11

[Eike Hein]

Missed that metadata, thanks and sorry for the noise :).

Comment 12

[ralf tauscher]

hi,

I'm still getting this error using tonights nightly build for linux 64.

[23:39:00.157] Error installing app: TypeError: (intermediate value).toLowerCase is not a function @ resource://gre/modules/WebappsInstaller.jsm:40
[23:39:00.184] "[install] Error installing app: " "DENIED"

can I do something to help?

Comment 14

[Marco Castelluccio [:marco]]

Has this been deployed? The bug is still valid.

Comment 15

[Antoine.Saroufim]

Bug is still valid as of the nightly build of today

Comment 16

[Kevin Ngo [:ngoke]]

I don't have access to a Linux box right now...but will later in the week. I could look at it then.

Comment 17

[EddyCarr]

I'm happy to let people remote control my Linux machine for testing if that is useful?

Comment 18

[Kevin Ngo [:ngoke]]

Thanks, but I guess I should just install a VM.

Comment 19

[Eike Hein]

FWIW, clients patched with my patch continue to work if any of you need a workaround.

Comment 20

[Marco Castelluccio [:marco]]

Kevin, Marketplace is still passing an array of integers. This is why I asked if the patch had been deployed.
For example, for Wikipedia, it's passing [161,163].

Looking at your patch, if you're using this data (https://marketplace.firefox.com/api/v1/apps/category/) you should use cat.slug and not cat.id.

However Marketplace also changed the categories, in the future it would be good to keep them in sync with Firefox (for example "photos-media" has become "photo-video").

Comment 21

[Kevin Ngo [:ngoke]]

Marco: Ah okay, I was mistaken, I'll get on that right now.

Comment 22

[Kevin Ngo [:ngoke]]

Got a patch, wasn't too trivial. Should be in within the week.

Comment 23

[Kevin Ngo [:ngoke]]

https://github.com/mozilla/zamboni/pull/891

Comment 24

[Kevin Ngo [:ngoke]]

https://github.com/mozilla/zamboni/commit/65663ae4e4b62a3e7879bb6fb070d90c19291d98

Comment 25

[EddyCarr]

Amazing! It is working. Thanks very much for fixing it.

Comment 26

[Kevin Ngo [:ngoke]]

Sweeettt

Comment 27

[Eike Hein]

This has regressed again; neither Firefox 24 nor current nightly (28.0a1 (2013-10-29)) write any "Categories=" keys to the .desktop files.

This is obviously never going to work well this way. I suggest to move the category mapping server-side (perhaps even letting content authors edit it) and just write to the .desktop file whatever Marketplace pushes down to the client, removing complexity from the client.

Until then however a short-term fix would be much appreciated.

Comment 28

[Wil Clouser [:clouserw]]

Was this related to bug 912279?

Comment 29

[Eike Hein]

The original bug was, yes - the server started returning integer IDs instead of strings, and so the baked-in mapping table was no longer useful for lookup, plus the code crashed trying to toLowerCase() an integer. I don't know if this is the same failure vector again, though.

Comment 30

[Marco Castelluccio [:marco]]

This is a different bug. The installation isn't failing.
Filed bug 932994.

Comment 31

[Eike Hein]

Good point, thanks for the triage Marco.

Comment 32

[Marco Castelluccio [:marco]]

(In reply to Eike Hein from comment #31)
> Good point, thanks for the triage Marco.

Thank you for noticing the problem!