disable mozApps.installPackage API in Firefox for Desktop and Android

VERIFIED FIXED in Firefox 16

Status

()

Core
DOM: Apps
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: jsmith, Assigned: myk)

Tracking

Trunk
mozilla18
Points:
---

Firefox Tracking Flags

(firefox16+ fixed, firefox17+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(3 attachments, 7 obsolete attachments)

8.10 KB, patch
fabrice
: review+
sicking
: superreview+
myk
: checkin+
Details | Diff | Splinter Review
11.68 KB, patch
myk
: checkin+
Details | Diff | Splinter Review
12.04 KB, patch
myk
: checkin+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
tracking-firefox16: --- → ?
tracking-firefox17: --- → ?
(Reporter)

Updated

5 years ago
status-firefox16: --- → affected
(Reporter)

Updated

5 years ago
Component: DOM: Mozilla Extensions → DOM: Apps
tracking-firefox16: ? → +
tracking-firefox17: ? → +
(Reporter)

Updated

5 years ago
Whiteboard: [blocking-webrtandroid1?]
(Reporter)

Updated

5 years ago
Keywords: qawanted
(Reporter)

Updated

5 years ago
QA Contact: jsmith
(Reporter)

Updated

5 years ago
Whiteboard: [blocking-webrtandroid1?]
(Reporter)

Comment 1

5 years ago
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.
Whiteboard: [blocked-on-input]
(Reporter)

Comment 2

5 years ago
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.
Keywords: qawanted
Whiteboard: [blocked-on-input]
(Reporter)

Comment 3

5 years ago
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.
(Reporter)

Comment 4

5 years ago
(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?
Poke. If we do want to disable this function in Fx16 it should be done soon. Who an make this call?
I would prefer that we at least assess the difficulty of making packaged apps work on android and desktop before disabling this API there
(Reporter)

Comment 7

5 years ago
(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?
(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

5 years ago
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?
Assignee: nobody → bwalker
(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.
(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?
Hosted apps from both the Marketplace and other websites will still be supported by the Desktop WebRT.
Thanks Anant.

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

Comment 14

5 years ago
Taking, as Anant is traveling.
Assignee: bwalker → myk
Status: NEW → ASSIGNED
(Assignee)

Comment 15

5 years ago
Created attachment 659427 [details] [diff] [review]
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.).
Attachment #659427 - Flags: superreview?(jonas)
Attachment #659427 - Flags: review?(fabrice)
Attachment #659427 - Flags: feedback?(mark.finkle)
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
Attachment #659427 - Flags: feedback?(mark.finkle) → feedback+
(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 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?
Attachment #659427 - Flags: review?(fabrice) → review+
(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
(Assignee)

Comment 20

5 years ago
Created attachment 659736 [details] [diff] [review]
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?
Attachment #659427 - Attachment is obsolete: true
Attachment #659427 - Flags: superreview?(jonas)
Attachment #659736 - Flags: superreview?(jonas)
Attachment #659736 - Flags: review+
(Assignee)

Comment 21

5 years ago
Created attachment 659770 [details] [diff] [review]
patch v2 for beta branch
(Assignee)

Comment 22

5 years ago
Created attachment 659771 [details] [diff] [review]
patch v2 for aurora branch
(Assignee)

Comment 23

5 years ago
Created attachment 659823 [details] [diff] [review]
patch v2 for beta branch (w/fix for copy/paste typo)
(Assignee)

Updated

5 years ago
Attachment #659770 - Attachment is obsolete: true
(Assignee)

Comment 24

5 years ago
Created attachment 659839 [details] [diff] [review]
patch v2 for aurora branch (w/fix for copy/paste typo)
Attachment #659771 - Attachment is obsolete: true
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?
(Assignee)

Comment 26

5 years ago
Created attachment 659915 [details] [diff] [review]
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.
Attachment #659736 - Attachment is obsolete: true
Attachment #659736 - Flags: superreview?(jonas)
Attachment #659915 - Flags: superreview?(jonas)
Attachment #659915 - Flags: review?(fabrice)
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
Attachment #659915 - Flags: review?(fabrice) → review-
(Assignee)

Comment 28

5 years ago
Created attachment 659923 [details] [diff] [review]
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.
Attachment #659915 - Attachment is obsolete: true
Attachment #659915 - Flags: superreview?(jonas)
Attachment #659923 - Flags: superreview?(jonas)
Attachment #659923 - Flags: review?(fabrice)
(Assignee)

Comment 29

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

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

Comment 30

5 years ago
Created attachment 659926 [details] [diff] [review]
patch v4 for beta branch
Attachment #659823 - Attachment is obsolete: true
(Assignee)

Comment 31

5 years ago
Created attachment 659929 [details] [diff] [review]
patch v4 for aurora branch
Attachment #659839 - Attachment is obsolete: true
(Assignee)

Comment 32

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

All tests green.
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.
Attachment #659923 - Flags: review?(fabrice) → review-
(Assignee)

Comment 34

5 years ago
(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.
(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.
Attachment #659923 - Flags: review- → review+
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.
Attachment #659923 - Flags: superreview?(jonas) → superreview+
(Assignee)

Comment 37

5 years ago
(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.

Updated

5 years ago
Attachment #659929 - Flags: approval-mozilla-aurora+

Updated

5 years ago
Attachment #659926 - Flags: approval-mozilla-beta+
(Assignee)

Comment 38

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

https://hg.mozilla.org/releases/mozilla-beta/rev/f1902725c78a
Attachment #659926 - Flags: checkin+
(Assignee)

Comment 39

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/94277d3fb5c0
Attachment #659929 - Flags: checkin+
(Assignee)

Comment 40

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bf62fe96867
Attachment #659923 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #659923 - Flags: review+ → checkin+
(Assignee)

Updated

5 years ago
status-firefox16: affected → fixed
status-firefox17: --- → fixed
Summary: Pref off the installPackage API in mozapps for desktop and android → disable mozApps.installPackage API in Firefox for Desktop and Android
Target Milestone: --- → mozilla18
(Assignee)

Comment 41

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/9bf62fe96867
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
Keywords: verifyme
(Reporter)

Comment 43

5 years ago
Verified on desktop nightly 9/12. Will need to retest on Android as well.
(Reporter)

Comment 44

5 years ago
Verified on android nightly 9/12 as well.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Jason, can you please verify this against Firefox 16 and 17 Beta?
Keywords: verifyme
(Reporter)

Comment 46

5 years ago
(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.
Keywords: verifyme
Okay, thanks Jason. Flagging [qa-] to deprioritize.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.