Closed Bug 727702 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect)

x86
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla15
Tracking Status
firefox13 + verified
firefox14 --- verified

People

(Reporter: mconley, Assigned: Unfocused)

References

Details

(Keywords: regression)

Attachments

(1 file)

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.
Blocks: 714741
Keywords: regression
Blocks: 714841
No longer blocks: 714741
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.
Hey Blair,

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

-Mike
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
Summary: Cannot undo applying a Persona → Undoing applying a Persona reverts back to default theme instead of old Persona
Component: Theme → Add-ons Manager
Product: Thunderbird → Toolkit
QA Contact: theme → add-ons.manager
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.
OS: All → Windows 7
Blocks: 727571
(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).
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.
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
(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?
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.
Sending over to you, Blair, since you landed the regressing bug.
Assignee: nobody → bmcbride
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
Blair - do you expect to fix this issue in the next week, or should we consider backing out bug 714841?
(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.
Attached patch Backout patchSplinter Review
This applies cleanly on central, aurora, and beta.
Attachment #623022 - Flags: review?(dtownsend+bugmail)
Attachment #623022 - Flags: review?(dtownsend+bugmail) → review+
https://hg.mozilla.org/integration/fx-team/rev/8d96e68d55e7
Status: NEW → ASSIGNED
Whiteboard: [fixed-in-fx-team]
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).
(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?
https://hg.mozilla.org/mozilla-central/rev/8d96e68d55e7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
(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 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.
Attachment #623022 - Flags: approval-mozilla-beta?
Attachment #623022 - Flags: approval-mozilla-aurora?
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.
Attachment #623022 - Flags: approval-mozilla-beta?
Attachment #623022 - Flags: approval-mozilla-beta+
Attachment #623022 - Flags: approval-mozilla-aurora?
Attachment #623022 - Flags: approval-mozilla-aurora+
Whiteboard: [qa+]
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
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)
Status: RESOLVED → VERIFIED
Whiteboard: [qa+]
You need to log in before you can comment on or make changes to this bug.