777400 - disable mozApps.installPackage API in Firefox for Desktop and Android

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Jason Smith [:jsmith]]

Currently, the installPackage function is accessible via the mozapps API on desktop and android via JavaScript. However, the implementation support for desktop and android has not been implemented yet. We need to pref off the installPackage function on desktop and android in FF 16 and on until the implementation support is added for desktop and android.

Comment 1

[Jason Smith [:jsmith]]

From today's triage, the packaged apps should "theoretically" work on Android and desktop. I understand some of the logic with packaged apps, but I don't have a concrete example to work off of to test this functionality. Could someone create an example html page with a zip making use of the installPackage API to demonstrate use of it? I'm stuck trying to figure out how to use the API right now.

Comment 2

[Jason Smith [:jsmith]]

Tested this on desktop and android with Fabrice's test app here (http://people.mozilla.org/~fdesre/openwebapps/test.html). Results below:

Firefox OS will have a prompt come up. Origin seems odd in the prompt for a packaged app (bug forthcoming), but installing the app worked. Launching it showed the correct content (a TEF facebook-like app).

Desktop will have the doorhanger come up. Clicking install will cause a failure to install with the following error in the console:

Timestamp: 8/6/2012 6:07:19 PM
Error: NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService2.newURI]
Source File: resource:///modules/WebappsInstaller.jsm
Line: 63

Android will have the prompt come up. Clicking install does install the app, but with an incorrect icon. Launching the app shows an error indicating that Firefox can't find the file at /fbint.html.

Therefore, I think the pref off makes sense until we implement any necessary fixes to get basic functionality for packaged apps working on desktop and android.

Comment 3

[Jason Smith [:jsmith]]

What we could do here btw possibly throw a NOT_IMPLEMENTED error for desktop and android to fail gracefully if someone tries to install a packaged app on desktop or android.

Comment 4

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #3)
> What we could do here btw possibly throw a NOT_IMPLEMENTED error for desktop
> and android to fail gracefully if someone tries to install a packaged app on
> desktop or android.

Anant - What's your opinion on this? What do you think would be the best way to fail gracefully on the platforms that we do not have packaged apps support on the installPackage function? Would a NOT_IMPLEMENTED error be sufficient?

Comment 5

[Lawrence Mandel [:lmandel] (use needinfo)]

Poke. If we do want to disable this function in Fx16 it should be done soon. Who an make this call?

Comment 6

[Bill Walker [:bwalker] [@wfwalker]]

I would prefer that we at least assess the difficulty of making packaged apps work on android and desktop before disabling this API there

Comment 7

[Jason Smith [:jsmith]]

(In reply to Bill Walker [:bwalker] [@wfwalker] from comment #6)
> I would prefer that we at least assess the difficulty of making packaged
> apps work on android and desktop before disabling this API there

I don't think this is a question of how much work is involved. It's when the work is actually planning to be targeted. Desktop has no ETA, because it's been heavily deprioritized in favor of shifting to a heavy mobile focus. Android isn't targeted for v1 (FF 16), but might be targeted for v2 (FF 17 possibly?).

I'd suggest we generally pref off the API on desktop overall due to the lowered focus. For Android, let's pref off only on FF 16. Make sense?

Comment 8

[Bill Walker [:bwalker] [@wfwalker]]

(In reply to Jason Smith [:jsmith] from comment #7)
> (In reply to Bill Walker [:bwalker] [@wfwalker] from comment #6)
> > I would prefer that we at least assess the difficulty of making packaged
> > apps work on android and desktop before disabling this API there
> 
> I don't think this is a question of how much work is involved. It's when the
> work is actually planning to be targeted. Desktop has no ETA, because it's
> been heavily deprioritized in favor of shifting to a heavy mobile focus.
> Android isn't targeted for v1 (FF 16), but might be targeted for v2 (FF 17
> possibly?).
> 
> I'd suggest we generally pref off the API on desktop overall due to the
> lowered focus. For Android, let's pref off only on FF 16. Make sense?

Yes, that makes sense.

Comment 9

[Alex Keybl [:akeybl]]

Bill - can we move forward with this? Once this is landed, should we remove

http://www.mozilla.org/en-US/firefox/16.0beta/releasenotes/
"Initial web app support (Windows/Mac/Linux)"

from the release notes?

Comment 10

[Anant Narayanan [:anant]]

(In reply to Alex Keybl [:akeybl] from comment #9)
> Bill - can we move forward with this? Once this is landed, should we remove
> 
> http://www.mozilla.org/en-US/firefox/16.0beta/releasenotes/
> "Initial web app support (Windows/Mac/Linux)"
> 
> from the release notes?

We can move forward with this, but removing that line from the release notes is not necessary. Web app support for Mac/Win/Linux will still be present, we just wont support a particular type of installation (packaged apps) which is what this bug is about.

Comment 11

[Alex Keybl [:akeybl]]

(In reply to Anant Narayanan [:anant] from comment #10)
> (In reply to Alex Keybl [:akeybl] from comment #9)
> > Bill - can we move forward with this? Once this is landed, should we remove
> > 
> > http://www.mozilla.org/en-US/firefox/16.0beta/releasenotes/
> > "Initial web app support (Windows/Mac/Linux)"
> > 
> > from the release notes?
> 
> We can move forward with this, but removing that line from the release notes
> is not necessary. Web app support for Mac/Win/Linux will still be present,
> we just wont support a particular type of installation (packaged apps) which
> is what this bug is about.

OK Thanks. What non-marketplace install type will still be supported?

Comment 12

[Anant Narayanan [:anant]]

Hosted apps from both the Marketplace and other websites will still be supported by the Desktop WebRT.

Comment 13

[Alex Keybl [:akeybl]]

Thanks Anant.

Bill/Anant - please prepare the preff-off patch for landing prior to Tuesday's beta.Thanks!

Comment 14

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

Taking, as Anant is traveling.

Comment 15

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

Attachment: patch v1: disable installPackage for Desktop

This patch for mozilla-central does the trick for Desktop in my depend build, although I haven't built for a few days and will do a clobber as well to confirm it is correct.  I'll have to adapt it for the aurora and beta branches, where at least the test code is different enough to conflict.  And I'm not sure how best to disable the API for Fennec (ifdef ANDROID, ifndef MOZ_WIDGET_GONK?).

Requesting review from Fabrice on the code, feedback from Mark on doing this for Fennec, and superreview from Jonas on consistency with DOM error reporting generally, mozApps error reporting specifically, and any standardization issues (name of error, etc.).

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 659427 [details] [diff] [review]
patch v1: disable installPackage for Desktop

Why not just use:

#ifdef MOZ_WIDGET_GONK
//do the current code
#else
//NOT IMPLEMENTED
#endif

Comment 17

[[:fabrice] Fabrice Desré]

(In reply to Mark Finkle (:mfinkle) from comment #16)
> Comment on attachment 659427 [details] [diff] [review]
> patch v1: disable installPackage for Desktop
> 
> Why not just use:
> 
> #ifdef MOZ_WIDGET_GONK
> //do the current code
> #else
> //NOT IMPLEMENTED
> #endif

We can't do that because that would also disable installPackage on b2g desktop builds.

Comment 18

[[:fabrice] Fabrice Desré]

Comment on attachment 659427 [details] [diff] [review]
patch v1: disable installPackage for Desktop

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

looks good, if MOZ_PHOENIX is what I think it is!

::: dom/apps/src/Webapps.js
@@ +169,5 @@
>  
>    installPackage: function(aPackageURL, aParams) {
>      let request = this.createRequest();
> +
> +#ifdef MOZ_PHOENIX

so, MOZ_PHOENIX is only true for fx desktop?

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Fabrice Desré [:fabrice] from comment #17)
> (In reply to Mark Finkle (:mfinkle) from comment #16)
> > Comment on attachment 659427 [details] [diff] [review]
> > patch v1: disable installPackage for Desktop
> > 
> > Why not just use:
> > 
> > #ifdef MOZ_WIDGET_GONK
> > //do the current code
> > #else
> > //NOT IMPLEMENTED
> > #endif
> 
> We can't do that because that would also disable installPackage on b2g
> desktop builds.

Then I assume you'll need some duplicate block of "not impl" code for #ifdef ANDROID

Comment 20

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

Attachment: patch v2: duplicate code for Fennec

(In reply to Fabrice Desré [:fabrice] from comment #18)
> so, MOZ_PHOENIX is only true for fx desktop?

Yes, as it is set only in /browser/confvars.sh:

http://mxr.mozilla.org/mozilla-central/source/browser/confvars.sh#9


(In reply to Mark Finkle (:mfinkle) from comment #19)
> Then I assume you'll need some duplicate block of "not impl" code for #ifdef
> ANDROID

Yes, but only if #ifndef MOZ_WIDGET_GONK, as B2G appears to set ANDROID also (hence code like <http://hg.mozilla.org/mozilla-central/annotate/12dad118c02f/content/media/plugins/nsMediaPluginHost.cpp#l76>).


Here's the patch with a duplicate code block for Fennec.  Carrying forward review, as this is the same code, just duplicated for Fennec, per mfinkle's feedback.


The tryserver run I kicked off last night for this patch came up green (modulo two orangefactors; but I suspect there aren't any installPackage tests, so this doesn't mean much):

https://tbpl.mozilla.org/?tree=Try&rev=b3dc72f4906b


So this just needs superreview.

Jonas: any chance you can do that today so I can commit this (and get approval to commit its branch equivalents) by the time we spin the next beta tomorrow?

Comment 21

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

Attachment: patch v2 for beta branch

Comment 22

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

Attachment: patch v2 for aurora branch

Comment 23

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

Attachment: patch v2 for beta branch (w/fix for copy/paste typo)

Comment 24

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

Attachment: patch v2 for aurora branch (w/fix for copy/paste typo)

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 659736 [details] [diff] [review]
patch v2: duplicate code for Fennec

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

Hmm... I'd really prefer if the .installPackaged function wasn't there at all. This way makes it sort of painful to feature-test for. Would you be able to do that?

Comment 26

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

Attachment: patch v3: remove installPackage from interface unless B2G

(In reply to Jonas Sicking (:sicking) from comment #25)
> Hmm... I'd really prefer if the .installPackaged function wasn't there at
> all. This way makes it sort of painful to feature-test for. Would you be
> able to do that?

Yes, here's a patch that does that by putting installPackage into a new  mozIDOMApplicationRegistry2 interface that the implementation only implements on B2G (i.e. #ifdef MOZ_WIDGET_GONK).

Note: the previous patch disabled installPackage specifically for Firefox and Fennec, while this one disables it for everything but B2G, which I think is reasonable, unless there are implementations of installPackage that I'm not aware of (Thunderbird? SeaMonkey?).  I could implement the original logic, but it'll be a bit hairy, and it seems better to enable this feature only where we know it works.

Comment 27

[[:fabrice] Fabrice Desré]

Comment on attachment 659915 [details] [diff] [review]
patch v3: remove installPackage from interface unless B2G

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

That doesn't work, since this will disable installPackage() on b2g desktop builds.

We really need something like #ifdef I_AM_B2G

Comment 28

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

Attachment: patch v4: remove installPackage on Firefox Desktop/Fennec

(In reply to Fabrice Desré [:fabrice] from comment #27)
> That doesn't work, since this will disable installPackage() on b2g desktop
> builds.
> 
> We really need something like #ifdef I_AM_B2G

Indeed, and the equivalent for Fennec, too.  But I'm trying to be conservative, since we intend to land this fix on beta/aurora, and I don't have a build environment for Fennec/B2G on Android that I can use to test such build flags.

So here's the same change but reusing the existing build flags, whose behavior is well known, even though they are cumbersome to use and require some code duplication.  Fortunately, I realized I don't have to remove the implementation of installPackage, just its exposure via the WebappsRegistry object, so this version leaves it in.

Comment 29

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

Comment on attachment 659923 [details] [diff] [review]
patch v4: remove installPackage on Firefox Desktop/Fennec

https://tbpl.mozilla.org/?tree=Try&rev=fd357c8992fa

Comment 30

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

Attachment: patch v4 for beta branch

Comment 31

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

Attachment: patch v4 for aurora branch

Comment 32

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

(In reply to Myk Melez [:myk] [@mykmelez] from comment #29)
> https://tbpl.mozilla.org/?tree=Try&rev=fd357c8992fa

All tests green.

Comment 33

[[:fabrice] Fabrice Desré]

Comment on attachment 659923 [details] [diff] [review]
patch v4: remove installPackage on Firefox Desktop/Fennec

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

That still doesn't solve the issue for b2g desktop properly.

::: dom/apps/src/Webapps.js
@@ +36,5 @@
> +#ifdef MOZ_PHOENIX
> +# Firefox Desktop: installPackage not implemented
> +#elifdef ANDROID
> +#ifndef MOZ_WIDGET_GONK
> +# Firefox Android (Fennec): installPackage not implemented

This will also be the code path for b2g desktop, since the WIDGET on b2g desktop is not MOZ_WIDGET_GONK but MOZ_WIDGET_GTK, etc.

Comment 34

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

(In reply to Fabrice Desré [:fabrice] from comment #33)
> > +#ifdef MOZ_PHOENIX
> > +# Firefox Desktop: installPackage not implemented
> > +#elifdef ANDROID
> > +#ifndef MOZ_WIDGET_GONK
> > +# Firefox Android (Fennec): installPackage not implemented
> 
> This will also be the code path for b2g desktop, since the WIDGET on b2g
> desktop is not MOZ_WIDGET_GONK but MOZ_WIDGET_GTK, etc.

But is ANDROID set for B2G Desktop?  If not, which is what I would expect, then B2G Desktop won't traverse that path, it'll fall through to the outer #else.

Comment 35

[[:fabrice] Fabrice Desré]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #34)

> But is ANDROID set for B2G Desktop?  If not, which is what I would expect,
> then B2G Desktop won't traverse that path, it'll fall through to the outer
> #else.

Oh you're right, mea culpa.

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 659923 [details] [diff] [review]
patch v4: remove installPackage on Firefox Desktop/Fennec

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

sr=me if you fix the comment from Fabrice. Seems like we'd want installPackage to work in the B2G desktop version.

Comment 37

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

(In reply to Jonas Sicking (:sicking) from comment #36)
> sr=me if you fix the comment from Fabrice. Seems like we'd want
> installPackage to work in the B2G desktop version.

It's already fixed in patch v4!  And after I posted comment 34, I built B2G Desktop to make sure of it, and indeed, installPackage remains enabled on it.

Comment 38

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

Comment on attachment 659926 [details] [diff] [review]
patch v4 for beta branch

https://hg.mozilla.org/releases/mozilla-beta/rev/f1902725c78a

Comment 39

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

Comment on attachment 659929 [details] [diff] [review]
patch v4 for aurora branch

https://hg.mozilla.org/releases/mozilla-aurora/rev/94277d3fb5c0

Comment 40

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

Comment on attachment 659923 [details] [diff] [review]
patch v4: remove installPackage on Firefox Desktop/Fennec

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bf62fe96867

Comment 41

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

14:07:13 - myk: sicking, fabrice: one thought i just had; if you twiddle a XPIDL, is that a binary interface change?
14:07:41 - sicking: myk: oh, yes, did you change the iids for the old interface?
14:07:49 - sicking: the iid rather
14:07:50 - myk: sicking: i did
14:08:01 - sicking: myk: cool
14:08:35 - myk: sicking: https://hg.mozilla.org/releases/mozilla-beta/rev/f1902725c78a#l4.12
14:08:54 - myk: sicking: however, tree rules <https://wiki.mozilla.org/Tree_Rules#mozilla-beta>; says not to make such changes on beta
14:08:54 - sicking: myk: good good, i think it's in the clear then
14:10:05 - sicking: myk: normally you could keep the function there (and the old iid), and add a [noscript] flag on installPackage
14:10:20 - sicking: myk: but that won't work since the function is implemented in script
14:10:34 - Standard8: iirc backouts of idl changes (i.e. revert to a previous version) are generally accepted
14:10:52 - sicking: myk: hmm.. i guess that could work actually. I might just throw
14:11:10 - sicking: myk: i wouldn't worry about it though. It seems unlikely that someone will be using this interface in a binary addon
14:11:15 - myk: sicking: ok

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9bf62fe96867

Comment 43

[Jason Smith [:jsmith]]

Verified on desktop nightly 9/12. Will need to retest on Android as well.

Comment 44

[Jason Smith [:jsmith]]

Verified on android nightly 9/12 as well.

Comment 45

[u279076]

Jason, can you please verify this against Firefox 16 and 17 Beta?

Comment 46

[Jason Smith [:jsmith]]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #45)
> Jason, can you please verify this against Firefox 16 and 17 Beta?

Desktop web apps aren't a priority anymore (to the point that our ADUs are incredibly low), so this isn't critical to verify.

Comment 47

[u279076]

Okay, thanks Jason. Flagging [qa-] to deprioritize.