1302414 - Theme preview is not working

Status: VERIFIED FIXED

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

Description

[u549602]

Environment: 49.0
Device: Nexus 7 (Android 5.1.1 );
Build: RC 49.0 (2016-09-12);

Steps to reproduce:
1. Go to addons.mozilla.org - Theme
2. Tap on any theme


Expected result:
Theme is in preview mode and "you're trying it on" message is displayed

Actual result:
"you're trying it on" message is displayed but the theme is not in preview

Notes:
If the user taps on "Keep it", the theme is correctly applied

https://www.youtube.com/watch?v=AF48GzM_cZc&feature=youtu.be

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Works for me on a Nexus 5x. Can reproduce on a Nexus 10. Appears to be tablet only.

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: regression. Wontfix for 49 makes sense. Users with themes installed will continue to function and the workaround is to apply the theme using the keep it button.

Release Note Request (optional, but appreciated)
[Why is this notable]: Broken feature with workaround
[Suggested wording]: Previewing themes on Tablets is not functioning the workaround is to use the "Keep it" button to apply the theme to Firefox for Android.
[Links (documentation, blog post, etc)]: This bug

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

I've bisected this twice and got this one day range twice https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6cf0089510fad8deb866136f5b92bbced9498447&tochange=0502bd9e025edde29777ba1de4280f9b52af4663 

All the integration tree bisection has lead to incorrect bugs, as whatever caused this landed on 51 and was uplifted. I did get one that narrowed to that might be correct https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b83235ed0ea5a46e58ee9e6f402171bf908ac25f&tochange=ed7a4daa831ac3c547959854e44585dd62fad508 maybe bug 1293128 or bug 1274332 which I don't know if I trust.

Comment 4

[Marcia Knous [:marcia]]

Tracking 51+ for this regression, even though there is a workaround.

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

Assigning to Sebastian to have an owner.

Comment 6

[u549602]

After further investigation, this issue could be also reproduced on phone devices - Samsung Galaxy S6 EDGE (Android 6.0), one plus two (Android 6.0), Xiaomi mi i4 (Android 5.1.1).

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Reverting my checkout to the oldest changeset in the regression ranges above doesn't create a build without the bug :(

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Sebastian Kaspari (:sebastian) from comment #7)
> Reverting my checkout to the oldest changeset in the regression ranges above
> doesn't create a build without the bug :(

Oh, the bug seems to depend on the state of the profile. With a clean profile it's either always reproducible or never. Looking at the changesets in the regression range now.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Looking at the regression range with hg-bisect the patch from bug 1274332 seems to cause this:

> The first bad revision is:
> changeset:   308803:0b1fc540eace
> user:        Cory Price <[e-mail]>
> date:        Tue Aug 09 14:27:14 2016 -0700
> summary:     Bug 1274332 - Add testpilot.firefox.com to the xpinstall whitelist. r=dolske,s.kaspari

And indeed with a backout this is fixed locally.

Looking at some unit tests for "xpinstall.whitelist.add" [1], I thinks this

> pref("xpinstall.whitelist.add", "https://addons.mozilla.org");
> pref("xpinstall.whitelist.add", "https://testpilot.firefox.com");

should probably be this:

> pref("xpinstall.whitelist.add", "https://addons.mozilla.org,https://testpilot.firefox.com");


[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_permissions.js#19

Comment 10

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Attachment: Theme preview is not working

Comment 11

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Patch attached - thanks for catching Sebastian!

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Cory, looks promising, is this waiting for review or landing? I'm not sure why you unassigned yourself here. 

Wontfix for 49 but once this lands on m-c you may want to request uplift.

Comment 13

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #12)
> Cory, looks promising, is this waiting for review or landing? I'm not sure
> why you unassigned yourself here. 
> 
> Wontfix for 49 but once this lands on m-c you may want to request uplift.
This is waiting for review and landing. I unassigned myself because I won't be doing either of those things (but it sounds like I should keep myself assigned when working in this realm?).

I'm going to NI :sebastian for a review. I'm not entirely sure how important this is (I submitted this patch because I introduced the bug), so maybe sebastian can also add comments/uplift if needed.

Thank you!

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8794299 [details] [diff] [review]
Theme preview is not working

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

Patch is looking good. Thanks!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1b0aaa20f10
Theme preview is not working. r=sebastian

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e1b0aaa20f10

Comment 17

[Gerry Chang [:gchang]]

Hi :ckprice,
Since this bug is a regression and also affects 50/51, do you think the patch is worth uplifting to 50/51?

Comment 18

[Teodora Vermesan (:TeoVermesan)]

Tapping on a theme, the theme is applied, so:
Verified as fixed using:
Device: Moto X (Android 4.4.4)
Build: Firefox for Android 52.0a1 (2016-10-02)

Comment 19

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Comment on attachment 8794299 [details] [diff] [review]
Theme preview is not working

Approval Request Comment
[Feature/regressing bug #]:

This regression was introduced in Bug 1274332.

[User impact if declined]:

Theme preview does not work.

[Describe test coverage new/current, TreeHerder]:

Looks like this was verified in comment 18. Also tested in https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_permissions.js#19

[Risks and why]: 

Low risk, updating an existing list that matches the convention of https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_permissions.js#19

[String/UUID change made/needed]:

N/A

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Cory, do you want to request uplift to Aurora51/Beta50 as well?

Comment 21

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

Comment on attachment 8794299 [details] [diff] [review]
Theme preview is not working

Hi Ritu - yes please! Added flags. See comment 19 for reasons.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8794299 [details] [diff] [review]
Theme preview is not working

Recent regression, fix was verified on Nightly, Aurora51+, Beta50+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2c6b7e9342d7

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2a516eed50a5

Comment 25

[u421692]

Verified as fixed on Firefox 50 Beta 6 and latest Aurora

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8794299 [details] [diff] [review]
Theme preview is not working

Regression fix that I think we can keep in 50.

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Too late for the release notes, sorry