727702 - Undoing applying a Persona reverts back to default theme instead of old Persona

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

Bisecting has revealed this to be a regression caused by the patch for bug 714841.

Steps to reproduce:

1)  Open the Addons Manager, "Get Addons"
2)  Under "More ways to Customize", choose "See all themes"
3)  In the content tab that loads, choose "Personas" in the top menu
4)  Choose some Persona and apply it.
5)  Choose a second Persona and apply it, and then hit "Undo" in the notification bar.

What happens?

Thunderbird switches back to the default theme.

What's expected?

Thunderbird should have switched back to the first applied persona.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I can't reproduce this in Firefox (I don't run Thunderbird nightlies). Looking at the code, it looks like Thunderbird didn't pick up bug 592338, which changed that code a decent amount. That *may* be the cause.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Hey Blair,

Thanks for the lead - I'll look into it.

-Mike

Comment 3

[Mike Conley (:mconley) (:⚙️)]

I finally got a chance to look at this more closely.

So, I have steps to reproduce for Firefox now:

1)  Go to getpersonas.com
2)  Find a persona (we'll call it Persona A), and choose to Wear It
3)  Undo applying Persona A
4)  Re-apply Persona A
5)  Find another persona (Persona B), and apply it
6)  Undo applying Persona B

What happens?

We revert the default theme, and I cannot change my persona anymore from the addons manager.  The Error Console displays:

Error: uncaught exception: 2147942487

What's expected:

We should undo back to Persona A

Comment 4

[Mike Conley (:mconley) (:⚙️)]

I reproduced this on Firefox Nightly on Windows 7

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:13.0a1) Gecko/20120216 Firefox/13.0a1

I'm unsure if Firefox is affected on the other platforms.

Comment 5

[Mark Banner (:standard8)]

(In reply to Blair McBride (:Unfocused) from comment #1)
> I can't reproduce this in Firefox (I don't run Thunderbird nightlies).
> Looking at the code, it looks like Thunderbird didn't pick up bug 592338,
> which changed that code a decent amount. That *may* be the cause.

Just to confirm, I tried backporting bug 592338 and that didn't fix the test failure (I'll get that patch up soon).

Comment 6

[Mark Banner (:standard8)]

Not sure if this should be tracked as its a regression from bug 714841 that may confuse users if they are trying out different lightweight themes.

Comment 7

[Alex Keybl [:akeybl]]

Tracking for 13 because of 

(In reply to Mike Conley (:mconley) from comment #3)
> We revert the default theme, and I cannot change my persona anymore from the
> addons manager.  The Error Console displays:
> 
> Error: uncaught exception: 2147942487

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Mike Conley (:mconley) from comment #3)
> Error: uncaught exception: 2147942487

This is NS_ERROR_ILLEGAL_VALUE or NS_ERROR_INVALID_ARG.


mconley: You were looking into this deeper and had a theory - any update on that?

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Blair:

Yeah, sorry - got a bit swamped with Thunderbird stuff, which took away my attention.

My theory was a bust - I was able to fix our failing test (which brought our attention to this problem), but the STR I listed above still produced the undesired result, so I think I was just plastering over a deeper problem.

Here's what I know:

The error is NS_ERROR_INVALID_ARG, and it's being thrown in the _sanitizeTheme function here:

http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/mozapps/extensions/LightweightThemeManager.jsm#689

What seems to be happening, is that the headerURL for some personas are being prefixed with the file:// scheme.  I'm not sure why or how that happens, (I'm guessing offline persistence) but that's what's happening.

When _sanitizeTheme is called, aLocal is not set to true, so the sanitizing fails by throwing the NS_ERROR_INVALID_ARG.

So, the big question is:

How did the patch for bug 714841 make it so that the headerURL for some personas has the file:// scheme, when it wasn't like that before bug 714841.

I don't have the cycles to dive much deeper into this.  Hopefully what I've found is useful.

Comment 10

[Alex Keybl [:akeybl]]

Sending over to you, Blair, since you landed the regressing bug.

Comment 11

[Mike Kaply [:mkaply]]

I've discovered that my add-on is regressed as well.

There are cases where I query currentTheme and what I get back is not the theme that is actually being displayed.

I would suggest backing out 714841 before it is too late for FF 13

Comment 12

[Alex Keybl [:akeybl]]

Blair - do you expect to fix this issue in the next week, or should we consider backing out bug 714841?

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(Sorry, I'm catching up after extended time off due to illness.)

Yes, a complete backout of bug 714841 seems appropriate. comment 11 seems to suggest it's easier to hit than first thought, and I don't have enough time to look into this right now.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Backout patch

This applies cleanly on central, aurora, and beta.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Note: Backing that out fixes the original issue. But if any lightweight theme ended up with file:/// URIs due to this bug, then that theme won't be able to be enabled. AFAICT, it will need to be removed and re-installed. Fixing that is non-trivial, as would requires implementing updating of disabled lightweight themes (we don't do that).

Comment 17

[Mike Kaply [:mkaply]]

(In reply to Blair McBride (:Unfocused) from comment #16)
> Note: Backing that out fixes the original issue. But if any lightweight
> theme ended up with file:/// URIs due to this bug, then that theme won't be
> able to be enabled. AFAICT, it will need to be removed and re-installed.
> Fixing that is non-trivial, as would requires implementing updating of
> disabled lightweight themes (we don't do that).

How widespread would this problem be?

Comment 18

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/8d96e68d55e7

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Michael Kaply (mkaply) from comment #17)
> How widespread would this problem be?

Hard to say :\ I'm not sure how easy this bug is to trigger, since I don't understand why it's happening yet. But bug 714841 never made it to a release version, and AFAIK this is the only bug on file for the issue it caused.

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 623022 [details] [diff] [review]
Backout patch

[Approval Request Comment]
Regression caused by (bug #): bug 714841
User impact if declined: Undoing applying of a lightweight theme can potentially not work, and can potentially break that theme making it unable to be enabled again.
Testing completed (on m-c, etc.): Reverts back to known good. Backout has been on m-c since the 12th.
Risk to taking this patch (and alternatives if risky): Bug 714841 was a performance optimization for when there are many lightweight themes installed (this mostly impacted the Addons Manager UI) - backing that out makes that situation perform slowly again. Alternative is to spend time figuring out cause and provide a fix - such a fix would really need time to bake (ie, it's not something I'd want to land on aurora/beta).
String changes made by this patch: None.

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 623022 [details] [diff] [review]
Backout patch

[Triage Comment]
Given the alternatives, and the low risk nature of returning ourselves to a known good state, approved for Aurora 14 and Beta 13.

Comment 22

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/defa2630e1eb
https://hg.mozilla.org/releases/mozilla-beta/rev/909c744af251

Comment 23

[Ioana (away)]

Verified as fixed using the steps from comment 0 on:
Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20100101 Firefox/13.0
BuildID: 20120528154913

Comment 24

[Ioana (away)]

Verified as fixed using the steps from comment 0 and comment 3 on:
Mozilla/5.0 (Windows NT 6.1; rv:14.0) Gecko/20100101 Firefox/14.0 (20120605113340)
Mozilla/5.0 (Windows NT 6.1; rv:16.0) Gecko/16.0 Firefox/16.0a1 (20120605030522)