Lightweight theme preview is broken

VERIFIED FIXED in Firefox 33

Status

()

defect
VERIFIED FIXED
6 years ago
10 months ago

People

(Reporter: AdrianT, Assigned: mfinkle)

Tracking

(Blocks 1 bug)

Trunk
Firefox 35
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox23 wontfix, firefox24 wontfix, firefox25 wontfix, firefox32 wontfix, firefox33 verified, firefox34 verified, firefox35 verified, fennec+)

Details

Attachments

(3 attachments)

Reporter

Description

6 years ago
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
I take it you can reproduce this on a new profile and you don't need to do step #1? Is this a regression?
Reporter

Comment 3

6 years ago
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.
Sriram - Sounds like something is clearing the whitelist that controls showing the prompts for AMO. Can you investigate?
Assignee: nobody → sriram
tracking-fennec: ? → +
Duplicate of this bug: 1062994
'Theme Preview' from AMO is still broken for over a year now.
Assignee: sriram.mozilla → nobody
Summary: Lightweight theme preview is not displayed and the user is asked to allow the installation of the theme if private data is cleared → Lightweight theme preview is broken
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.
Assignee: nobody → mark.finkle
Attachment #8486969 - Flags: review?(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
(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 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.
Attachment #8486969 - Flags: review?(margaret.leibovic) → review+
Duplicate of this bug: 933168
https://hg.mozilla.org/mozilla-central/rev/7854e1b5d181
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
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
Attachment #8486969 - Flags: approval-mozilla-beta?
Attachment #8486969 - Flags: approval-mozilla-aurora?
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: anthony.s.hughes
Comment on attachment 8486969 [details] [diff] [review]
lwt-preview v0.1

Anthony - Thanks for verifying. 

Approved for Beta and Aurora
Attachment #8486969 - Flags: approval-mozilla-beta?
Attachment #8486969 - Flags: approval-mozilla-beta+
Attachment #8486969 - Flags: approval-mozilla-aurora?
Attachment #8486969 - Flags: approval-mozilla-aurora+
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

10 months ago
I will remove the qe-verify flag based on comment 18.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.