946296 - Defect - Desktop add-ons loading into Metro browser

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Browser, defect, P2)

Description

[Tim Nguyen :ntim]

Now that metro and desktop share the same profile, desktop addons also work on Metro mode. For example, my stylish userstyles st

Comment 1

[Tim Nguyen :ntim]

End of the description : 
my stylish userstyles used on desktop still work on metro mode.

Comment 2

[Jim Mathies [:jimm]]

Can you clarify, are you saying that desktop add-ons are loading in metrofx? If so which add-on?

Comment 3

[Tim Nguyen :ntim]

Well, basically all the ones that affect the page content : AdBlock Plus, Stylish, GreaseMonkey ...

Comment 4

[Tim Nguyen :ntim]

All of them actually load but the ones affecting page content have visible effect

Comment 5

[Jim Mathies [:jimm]]

Confirmed with Stylish and the "youtube black 2013" theme. Not sure if this is good or bad, user styles are pretty harmless. Some might consider this a neat feature. I didn't see adblock plus loading though, I think we're just getting user styles here. We should confirm that though.

Comment 6

[Tim Nguyen :ntim]

(In reply to Jim Mathies [:jimm] from comment #5)
> Confirmed with Stylish and the "youtube black 2013" theme. Not sure if this
> is good or bad, user styles are pretty harmless. Some might consider this a
> neat feature. I didn't see adblock plus loading though, I think we're just
> getting user styles here. We should confirm that though
Maybe metro firefox could have extra code for desktop add-ons to run and then add a new platform setting for metro ffx on AMO's website

Comment 7

[Jim Mathies [:jimm]]

(In reply to Tim Nguyen [:ntim] from comment #6)
> Maybe metro firefox could have extra code for desktop add-ons to run and
> then add a new platform setting for metro ffx on AMO's website

We do plan to support addons, and yes there will be a new amo category for them. We won't be able to support desktop addons out of the box due to user interface differences. Developers will be able to update their desktop addons to make them compatible with metrofx if they choose to.

Comment 8

[Marco Mucci [:MarcoM]]

Hey Juan, can you confirm Desktop addons are not loading?

Comment 9

[juan becerra [:juanb]]

Tested on 2013-12-09 with latest nightly and AdBlock Plus. First I installed the latest nightly and I installed Adblock Plus from their site. I tested that Adblock was working by visiting a few sites with lots of ads. Then I went to the drop-down menu on the top right and selected "Windows 8 Touch" to go into Metro mode.

It looks like the ads on those pages were blocked in Metro mode as well. I also tested using NoScript, and searching for flights on hipmunk.com (which didn't work because NoScript prevented it from loading). When I loaded that site in Metro, it also did not work. Disabling NoScript in desktop mode then made it work in Metro, again, too.

Comment 10

[Jim Mathies [:jimm]]

blocking!

Comment 11

[Brian R. Bondy [:bbondy]]

<bbondy> gotcha
[11:36:00] <mbrubeck> http://mxr.mozilla.org/mozilla-central/source/browser/metro/profile/metro.js#203
[11:37:01] <bbondy> what do we want to do about that btw? because we want to be able to use things like dom inspector. I think I heard an idea of just allow it on nightly and block everywhere else?
[11:37:41] <mbrubeck> bbondy: We could set the pref to 0 by default, and developers could switch it to 1
[11:37:51] <mbrubeck> documentation of the values, for reference: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm#2336
[11:38:14] <bbondy> there's already some manual setup needed so that sounds easiest/best to me
[11:38:19] <bbondy> and just update the wiki

Comment 12

[Matt Brubeck (:mbrubeck)]

Taking this for iteration 21, p=1.

Comment 13

[Marco Mucci [:MarcoM]]

Added to IT#21.

Comment 14

[Matt Brubeck (:mbrubeck)]

Revising my point estimate based on new information.  From conversation in IRC:

<mbrubeck> I hoped we could use extensions.enableScopes to disable the profile scope, but we can't (currently): http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#2003
<Mossop> I mainly force-enabled the profile folder because all sorts of things wouldn't work right in the add-ons manager if the profile scope wasn't there. I can't guarantee sanity. The better solution is just to not load the add-ons manager at all in metro tbh
<Mossop> So generally the patch should make sure that any use of KEY_APP_PROFILE in the add-ons manager does something sane when it doesn't exist. There are a few cases where right now a bad exception would be thrown, including at startup I think

Comment 15

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This is based on an alternate, simpler idea from Mossop.  It completely disables the default XPIProvider and LightweightThemeManager while Firefox is running in Metro mode.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8345411 [details] [diff] [review]
patch

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

Looks good, but needs a test. Should be enough to just check AddonManager.isInstallEnabled("application/x-xpinstall")==false

Comment 17

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

Now with test!

Comment 18

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8345975 [details] [diff] [review]
patch v2

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

::: toolkit/mozapps/extensions/test/xpcshell/test_default_providers_pref.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Tests the preference-related properties of AddonManager
> +// eg: AddonManager.checkCompatibility, AddonManager.updateEnabled, etc

Oops, copy-paste error.  I've updated this locally with a description of the actual test.

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8345975 [details] [diff] [review]
patch v2

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

Ship it!

Comment 20

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/fx-team/rev/c237f02b3578

Comment 21

[Matt Brubeck (:mbrubeck)]

Backed out because it broke mochitest-metro-chrome:
https://hg.mozilla.org/integration/fx-team/rev/5fba9c37392d

I think we need to set the pref to true in the test profile in order for the test harness to load.

Comment 22

[Matt Brubeck (:mbrubeck)]

Pushed to Try with a fix for the test profile:
https://tbpl.mozilla.org/?tree=Try&rev=f6ac61675322

Comment 23

[Matt Brubeck (:mbrubeck)]

Re-landed with test fix:
https://hg.mozilla.org/integration/fx-team/rev/690537e87173

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/690537e87173

Comment 25

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8345975 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Shared profiles (bug 924860)

User impact if declined: Certain add-ons load in Metro even though the Metro app does not yet support add-on installation or management.

Testing completed (on m-c, etc.): Landed on m-c 12/12; includes an automated test.

Risk to taking this patch (and alternatives if risky): Low-risk patch; all changes are behind a hidden pref that is enabled only for Metro.

String or IDL/UUID changes made by this patch: None.

Comment 26

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bab4f38bfe7c

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This change here totally broke our Mozmill test framework. See bug 951715. Thankfully there is a pref we can make use of to re-enable Mozmill for Metro. It would be very helpful for us to let us know about major changes in Metro which could affect testruns. Juan, can you please make that happen? Thanks.

Comment 28

[Matt Brubeck (:mbrubeck)]

(In reply to Henrik Skupin (:whimboo) from comment #27)
> This change here totally broke our Mozmill test framework. See bug 951715.

Oops, sorry!  I didn't even know we had Mozmill tests that ran the Metro browser.  We'll have to add that to our development toolbox...

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Matt Brubeck (:mbrubeck) from comment #28)
> Oops, sorry!  I didn't even know we had Mozmill tests that ran the Metro
> browser.  We'll have to add that to our development toolbox...

Those tests are not running in our CI yet, but we are working heavily on getting this accomplished soon. The last remaining blocker for us is mainly bug 911257. That means I expect to have our tests running in CI by end of January at latest. Would you mind to give me some more information about your dev toolbox? What is it? To not spam this bug I would also appreciate an email. Thanks.

Comment 30

[Cornel Ionce [:noni]]

Verified as fixed on latest Firefox Aurora (build ID:20140115004003).