Last Comment Bug 876293 - Installation of web apps from Firefox Marketplace fails on Linux during mapping of categories
: Installation of web apps from Firefox Marketplace fails on Linux during mappi...
Status: RESOLVED FIXED
[qa+]
: regression
Product: Marketplace
Classification: Server Software
Component: Consumer Pages (show other bugs)
: 1.0
: x86_64 Linux
: P2 normal with 2 votes (vote)
: 2013-07-25
Assigned To: Kevin Ngo [:ngoke]
:
:
Mentors:
: 879176 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-26 15:08 PDT by Eike Hein
Modified: 2013-10-30 14:34 PDT (History)
8 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix-webapps-install-on-linux.patch (1.20 KB, patch)
2013-05-26 15:08 PDT, Eike Hein
no flags Details | Diff | Splinter Review

Description Eike Hein 2013-05-26 15:08:23 PDT
Created attachment 754286 [details] [diff] [review]
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] 2013-05-26 16:14:06 PDT
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 2013-05-26 16:36:25 PDT
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] 2013-05-27 21:57:25 PDT
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] 2013-05-27 22:04:54 PDT
(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 2013-05-28 13:01:21 PDT
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] 2013-05-29 11:02:12 PDT
It sounds like Kevin added this so assigning to him.
Comment 7 Kevin Ngo [:ngoke] 2013-05-30 13:35:08 PDT
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 2013-05-30 15:03:47 PDT
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 2013-06-02 10:16:39 PDT
Has this been deployed already? Marketplace installation on Linux still fails with current Nightly (sans my patch).
Comment 10 Jason Smith [:jsmith] 2013-06-02 17:08:29 PDT
(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 2013-06-03 09:21:39 PDT
Missed that metadata, thanks and sorry for the noise :).
Comment 12 ralf tauscher 2013-06-24 14:43:24 PDT
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 13 Marco Castelluccio [:marco] 2013-06-28 10:48:13 PDT
*** Bug 879176 has been marked as a duplicate of this bug. ***
Comment 14 Marco Castelluccio [:marco] 2013-06-28 11:16:22 PDT
Has this been deployed? The bug is still valid.
Comment 15 Antoine.Saroufim 2013-06-29 05:56:59 PDT
Bug is still valid as of the nightly build of today
Comment 16 Kevin Ngo [:ngoke] 2013-07-01 10:32:10 PDT
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 2013-07-01 10:34:46 PDT
I'm happy to let people remote control my Linux machine for testing if that is useful?
Comment 18 Kevin Ngo [:ngoke] 2013-07-01 11:39:26 PDT
Thanks, but I guess I should just install a VM.
Comment 19 Eike Hein 2013-07-01 12:01:26 PDT
FWIW, clients patched with my patch continue to work if any of you need a workaround.
Comment 20 Marco Castelluccio [:marco] 2013-07-01 13:39:24 PDT
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] 2013-07-01 14:01:54 PDT
Marco: Ah okay, I was mistaken, I'll get on that right now.
Comment 22 Kevin Ngo [:ngoke] 2013-07-12 16:10:29 PDT
Got a patch, wasn't too trivial. Should be in within the week.
Comment 23 Kevin Ngo [:ngoke] 2013-07-12 16:18:59 PDT
https://github.com/mozilla/zamboni/pull/891
Comment 25 EddyCarr 2013-07-18 15:07:48 PDT
Amazing! It is working. Thanks very much for fixing it.
Comment 26 Kevin Ngo [:ngoke] 2013-07-18 15:08:45 PDT
Sweeettt
Comment 27 Eike Hein 2013-10-29 15:13:43 PDT
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] 2013-10-30 08:57:15 PDT
Was this related to bug 912279?
Comment 29 Eike Hein 2013-10-30 09:07:01 PDT
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] 2013-10-30 14:12:56 PDT
This is a different bug. The installation isn't failing.
Filed bug 932994.
Comment 31 Eike Hein 2013-10-30 14:25:24 PDT
Good point, thanks for the triage Marco.
Comment 32 Marco Castelluccio [:marco] 2013-10-30 14:34:45 PDT
(In reply to Eike Hein from comment #31)
> Good point, thanks for the triage Marco.

Thank you for noticing the problem!

Note You need to log in before you can comment on or make changes to this bug.