610686 - Cached copies of Addon objects for lightweight themes don't have the correct permissions and pendingOperations during the onEnabling event

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[ahoza]

Steps to reproduce:

1. Wear 2-3 lightweight themes from www.getpersonas.com
2. Install a theme from AMO. (e.g: https://addons.mozilla.org/en-US/firefox/addon/122/)
3. Open Add-ons Manager-> Appearance.
4. Enable one of the lightweight themes from step 1.

Expected results:
Lightweight themes is worn on top of the installed theme from AMO.
or
A restart is required and after restart the lightweight theme is worn on top of default theme.
or 
"Enable" button is disabled for lightweight themes until "Default" theme is selected.

Actual results:
Nothing happens

Comment 1

[Dave Townsend [:mossop]]

For me after step 4 I see Firefox using the lightweight theme on top of the default theme with no restart required. This is what I would expect. You said that "Nothing happens" for you, does that mean the lightweight theme you enabled in step 4 didn't get used?

Comment 2

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

The Restart Now button should appear for the theme you have installed from AMO, once you try to enable a Personas. Doesn't it happen? Can you please attach a screenshot? Also which build are you using?

Comment 3

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

If that is not obvious enough, bug 553460 could be the possible fix.

Comment 4

[ahoza]

(In reply to comment #2)
> The Restart Now button should appear for the theme you have installed from AMO,
> once you try to enable a Personas. Doesn't it happen? Can you please attach a
> screenshot? Also which build are you using?

You're right, the Restart button appears for the theme installed from AMO.
But this could be confusing as one chose to enable the lightweight persona and did nothing with the theme (Disable or Remove it)
 
I'm using release candidate build for 4.0 beta 7, buildID: Mozilla/5.0 (Windows NT 6.1; rv:2.0b7) Gecko/20100101 Firefox/4.0b7

Comment 5

[ahoza]

Attachment: Screenshot

Comment 6

[Dave Townsend [:mossop]]

(In reply to comment #5)
> Created attachment 489432 [details]
> Screenshot

At what point in the process is that? What themes did you install leading up to that?

Comment 7

[Dave Townsend [:mossop]]

Nevermind I see what is happening. The UI isn't updating properly when making changes that affect both a theme and a lightweight theme. Switching panes shows the correct display.

Comment 8

[Dave Townsend [:mossop]]

The problem is that there is a small hack in place to ensure that the permissions and pendingOperations of an Addon for a lightweight theme are correct during the onEnabling event. This hack only applies to the object sent with the event, older cached versions of the Addon object do not have the right values. The UI works with cached copies so it fails to update the state correctly.

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Rather than caching in the AddonWrapper instance that it is being enabled instead just cache it globally so old instances can see it.

Comment 10

[Robert Strong (they/them - no direct email)]

Comment on attachment 489530 [details] [diff] [review]
patch rev 1

>diff --git a/toolkit/mozapps/extensions/LightweightThemeManager.jsm b/toolkit/mozapps/extensions/LightweightThemeManager.jsm
>--- a/toolkit/mozapps/extensions/LightweightThemeManager.jsm
>+++ b/toolkit/mozapps/extensions/LightweightThemeManager.jsm
>@@ -82,16 +82,21 @@ __defineGetter__("_maxUsedThemes", funct
>   return this._maxUsedThemes;
> });
> 
> __defineSetter__("_maxUsedThemes", function(aVal) {
>   delete this._maxUsedThemes;
>   return this._maxUsedThemes = aVal;
> });
> 
>+// Holds the ID of the theme being enabled while sending out the events so
>+// cached AddonWrapper instances can return correct values for permissions and
>+// pendingOperations
>+var _themeBeingEnabled = null;
nit: might be better named as _themeIDBeingEnabled or _enablingThemeID but I'll leave that up to you since I think the comment is enough

>...
>@@ -396,17 +403,17 @@ var LightweightThemeManager = {
>     aCallback([new AddonWrapper(a) for each (a in this.usedThemes)]);
>   },
> };
> 
> /**
>  * The AddonWrapper wraps lightweight theme to provide the data visible to
>  * consumers of the AddonManager API.
>  */
nit: would be nice to cleanup the comments in this file so they specify param and return but last I looked there was enough cleanup to justify a bug for it.

r=me with or without the above though I'd like either a bug to cleanup the comments or AddonWrapper cleaned up if a bug isn't filed.

Comment 11

[Dave Townsend [:mossop]]

(In reply to comment #10)
> Comment on attachment 489530 [details] [diff] [review]
> patch rev 1
> 
> >diff --git a/toolkit/mozapps/extensions/LightweightThemeManager.jsm b/toolkit/mozapps/extensions/LightweightThemeManager.jsm
> >--- a/toolkit/mozapps/extensions/LightweightThemeManager.jsm
> >+++ b/toolkit/mozapps/extensions/LightweightThemeManager.jsm
> >@@ -82,16 +82,21 @@ __defineGetter__("_maxUsedThemes", funct
> >   return this._maxUsedThemes;
> > });
> > 
> > __defineSetter__("_maxUsedThemes", function(aVal) {
> >   delete this._maxUsedThemes;
> >   return this._maxUsedThemes = aVal;
> > });
> > 
> >+// Holds the ID of the theme being enabled while sending out the events so
> >+// cached AddonWrapper instances can return correct values for permissions and
> >+// pendingOperations
> >+var _themeBeingEnabled = null;
> nit: might be better named as _themeIDBeingEnabled or _enablingThemeID but I'll
> leave that up to you since I think the comment is enough

_themeIDBeingEnabled seems to be a better choice.

> nit: would be nice to cleanup the comments in this file so they specify param
> and return but last I looked there was enough cleanup to justify a bug for it.

Filed bug 611380

Comment 12

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/fa89e65fc219

Comment 13

[Dave Townsend [:mossop]]

Backed out due to windows debug test failures

Comment 14

[Dave Townsend [:mossop]]

Re-landed as this wasn't the cause of the bustage: http://hg.mozilla.org/mozilla-central/rev/82da3fec03fc

Comment 15

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

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110209 Firefox/4.0b12pre ID:20110209030359