887755 - Lightweight theme preview is broken

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Add-on Manager, defect)

Description

[Adrian Tamas (:AdrianT)]

Attachment: premission request screenshot

Aurora 24.0a2 2013-06-27 / Nightly 25.0a1 2013-06-27
Acer Iconia Tab A500 (Android 3.0) / Samsung Galaxy Tab 2 (Android 4.1)

Steps to reproduce: 
1) Open the settings menu and choose to clear all the private data
2) Open addons.mozilla.org and go to the "Themes" section
3) Preview a theme
4) Choose to keep it

Expected results:
When the user chooses the theme a preview is applied to the browser. If the user chooses to "Keep it" the theme is added without any additional permissions from the user

Actual results:
The preview is never displayed. If the user choose to keep the theme he is first asked to allow addons.mozilla.org to install the addon

Comment 1

[Adrian Tamas (:AdrianT)]

Attachment: preview not displayed sceenshot

Comment 2

[Aaron Train [:aaronmt]]

I take it you can reproduce this on a new profile and you don't need to do step #1? Is this a regression?

Comment 3

[Adrian Tamas (:AdrianT)]

This only happens after clearing private data as mentioned in the title and steps and not with a new profile. It works fine with a new profile until the user clears the private data. Also the permission for installation seems to be needed for any addon not just themes after clearing private data.

Comment 4

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

Sriram - Sounds like something is clearing the whitelist that controls showing the prompts for AMO. Can you investigate?

Comment 6

[Aaron Train [:aaronmt]]

'Theme Preview' from AMO is still broken for over a year now.

Comment 7

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

Attachment: lwt-preview v0.1

This is happening because the xpinstall.whitelist.* preferences are not being loading into the PermissionManager. So this check fails:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2823

The same thing is done when installing add-ons here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#3332

In fact, if you install an add-on and then try to preview a theme, it will work.

Also note that you can still install a lightweight theme, but Fennec will show a prompt. If the permission was set, you'd see no prompt.

This patch will force the permissions to get setup. The PermissionsUtil code will not let the same permissions get setup more than once, so we don't need our own state flag.

With this patch, previews work. Just remember, it takes a few seconds for the preview to display.

Comment 8

[:Margaret Leibovic]

Comment on attachment 8486969 [details] [diff] [review]
lwt-preview v0.1

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

::: mobile/android/chrome/content/browser.js
@@ +2754,5 @@
>      BrowserApp.deck.removeEventListener("ResetBrowserThemePreview", this, false, true);
>    },
>  
>    handleEvent: function (event) {
> +    PermissionsUtils.importFromPrefs("xpinstall.", "install");

To better match what's in XPIProvider, I think this should go in _isAllowed:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2848

Also, looking at that method, are we missing some logic? Should we be checking the whitelist pref as well?

Also, it looks like if _isAllowed is true we would just automatically install the theme without a doorhanger. Is that expected?

Maybe what we actually want is to just be getting rid of this _isAllowed check in _preview:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2823

Comment 9

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

(In reply to :Margaret Leibovic from comment #8)
> Comment on attachment 8486969 [details] [diff] [review]
> lwt-preview v0.1
> 
> Review of attachment 8486969 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +2754,5 @@
> >      BrowserApp.deck.removeEventListener("ResetBrowserThemePreview", this, false, true);
> >    },
> >  
> >    handleEvent: function (event) {
> > +    PermissionsUtils.importFromPrefs("xpinstall.", "install");
> 
> To better match what's in XPIProvider, I think this should go in _isAllowed:

I can do that

> Also, looking at that method, are we missing some logic? Should we be
> checking the whitelist pref as well?

isAllow checks the Permissions and PermissionUtils.importFromPrefs imports the xpinstall.whitelist prefs into the permissions.

> Also, it looks like if _isAllowed is true we would just automatically
> install the theme without a doorhanger. Is that expected?

Yes

> Maybe what we actually want is to just be getting rid of this _isAllowed
> check in _preview:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#2823

Desktop has the isAllow check in _preview too

Comment 10

[:Margaret Leibovic]

Comment on attachment 8486969 [details] [diff] [review]
lwt-preview v0.1

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

r+ if you move the check to _isAllowed.

Comment 11

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

moved the check to _isAllowed
https://hg.mozilla.org/integration/fx-team/rev/7854e1b5d181

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7854e1b5d181

Comment 14

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

Comment on attachment 8486969 [details] [diff] [review]
lwt-preview v0.1

Approval Request Comment
[Feature/regressing bug #]: Unknown, but it's been broken for a while
[User impact if declined]: Unable to preview themes
[Describe test coverage new/current, TBPL]: Waiting for QA to verify
[Risks and why]: Low risk, simple fix
[String/UUID change made/needed]: none

Comment 15

[u279076]

I was able to reproduce this bug on a Nexus 4 running Android 4.4.2 and Firefox Nightly 32.0a1 2014-06-02. I am unable to reproduce this bug on the same device after updating to today's Firefox Nightly 35.0a1, even with a new profile. Marking this bug verified fixed for Firefox 35 based on this testing.

Comment 16

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

Comment on attachment 8486969 [details] [diff] [review]
lwt-preview v0.1

Anthony - Thanks for verifying. 

Approved for Beta and Aurora

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/60c967fb8311
https://hg.mozilla.org/releases/mozilla-beta/rev/c21b3ccb9c19

Comment 18

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in
Builds:
Firefox for Android 34.0a2 (2014-09-17)
Firefox for Android 33 Beta 4

Device: Asus Transformer Pad TF300T (Android 4.2.1)

Comment 19

[Stefan Deiac]

I will remove the qe-verify flag based on comment 18.