544753 - Add-ons manager is blank when a custom persona has been created by an older version of the personas extension

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Loïc Grobol]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.9.2) Gecko/20100115 Firefox/3.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.9.2) Gecko/20100115 Firefox/3.6

I hadn't used the add-on manager since the update, which was just after the release. After showing me a pop-up this morning saying that the NASA Nightlaunch theme update was ready and I clicked the hyperlink I saw what you can see now on the pic. It's the same for the others tab of the manager, they aren't displaying anything, just a blank area. I've tried downgrading FF and everything works fine with versions from 3.0.1 to 3.5.7, the bug occurs only when I upgrade to 3.6. I've also tried to change the theme and go back to the default with Personas but that changes nothing. I couldn't uninstall any add-on, but I suppose I'll try downgrade then uninstall then upgrade after this.

Reproducible: Always

Steps to Reproduce:
1.Launch firefox
2.Open Options->"Modules Comlémentaires"(Add-on manager)
3.
Actual Results:  
The add-on manager's window doesn't show any extension, theme, update or language. Just a blank area where the list should be. Even the "Catalogue" doesn't. And its link bring me to a new blank tab.

Expected Results:  
Displaying a list of theme, extensions

Build platform
target
i686-pc-mingw32

Build tools
Compiler 	Version 	Compiler flags
cl 	14.00.50727.762 	-TC -nologo -W3 -Gy -Fdgenerated.pdb -DNDEBUG -DTRIMMED -Zi -UDEBUG -DNDEBUG -GL -wd4624 -wd4952 -O1
cl 	14.00.50727.762 	-GR- -TP -nologo -Zc:wchar_t- -W3 -Gy -Fdgenerated.pdb -DNDEBUG -DTRIMMED -Zi -UDEBUG -DNDEBUG -GL -wd4624 -wd4952 -O1

Configure arguments
--enable-application=browser --enable-update-channel=release --enable-update-packaging --enable-jemalloc --enable-official-branding --enable-tests

Comment 1

[Dave Townsend [:mossop]]

Enter about:config into the address bar and then copy the value of the setting lightweightThemes.usedThemes into this bug report.

Comment 2

[Loïc Grobol]

Here it is
[{"id":"89","name":"Summerwood","accentcolor":"#befc8f","textcolor":"#e0e4ff","header":"http://getpersonas-cdn.mozilla.net/static/8/9/89/summerwood_h.jpg?1260925626","footer":"http://getpersonas-cdn.mozilla.net/static/8/9/89/summerwood_f.jpg?1260925626","category":"Nature","description":"Persona of  my \"Summerwood\" wallpaper available at: \u000d\u000a\u000d\u000ahttp://digitalblasphemy.com/freegallery.shtml#summerwood","author":"DigitalBlasphemy","username":"digitalblasphemy","detailURL":"http://getpersonas.com/persona/89","headerURL":"http://getpersonas-cdn.mozilla.net/static/8/9/89/summerwood_h.jpg?1260925626","footerURL":"http://getpersonas-cdn.mozilla.net/static/8/9/89/summerwood_f.jpg?1260925626","previewURL":"http://getpersonas-cdn.mozilla.net/static/8/9/89/preview.jpg?1260925626","iconURL":"http://getpersonas-cdn.mozilla.net/static/8/9/89/preview_small.jpg?1260925626","updateURL":"https://getpersonas.com/en-US/update_check/89","dataurl":"data:image/png;base64,/9j/4AAQSkZJRgABAQEASABIAAD/2wBDAAMCAgICAgMCAgIDAwMDBAYEBAQEBAgGBgUGCQgKCgkICQkKDA8MCgsOCwkJDRENDg8QEBEQCgwSExIQEw8QEBD/2wBDAQMDAwQDBAgEBAgQCwkLEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBD/wAARCAAQABADAREAAhEBAxEB/8QAFgABAQEAAAAAAAAAAAAAAAAABQEJ/8QAIhAAAgEDAgcAAAAAAAAAAAAAAQIDAAQREjEFBhNBUYGh/8QAFwEBAAMAAAAAAAAAAAAAAAAABAECA//EACMRAAEDAgUFAAAAAAAAAAAAAAEAAgMRIQQSQWGhFDEykbH/2gAMAwEAAhEDEQA/AMq2Oa1cVAS9/PCvLvCreLT1czySMNxlgoB9L9pUpAw0YG55RYmnqJHHYcIwW1y1u94tvIYI3WN5Qh0K7AlVJ2BIViB3wfFAdI0PyVub01t3+j2lgWUbJCJSH+IaqDUr/9k=","recent":false},{"custom":true,"name":"Night shades","header":"file:///E:/Documents/Mes%20images/Katzenmond/1.bmp","footer":"file:///E:/Documents/Mes%20images/Katzenmond/1.bmp","textcolor":"#FFFFFF","accentcolor":"#FFFF00","headerURL":"file:///E:/Documents/Mes%20images/Katzenmond/1.bmp","footerURL":"file:///E:/Documents/Mes%20images/Katzenmond/1.bmp"},{"id":"3259","name":"Celestial","author":"MaDonna","headerURL":"http://getpersonas-cdn.mozilla.net/static/5/9/3259/OuterSpaceH.jpg?1260925626","footerURL":"http://getpersonas-cdn.mozilla.net/static/5/9/3259/OuterSpaceF.jpg?1260925626","previewURL":"http://getpersonas-cdn.mozilla.net/static/5/9/3259/preview.jpg?1260925626","iconURL":"http://getpersonas-cdn.mozilla.net/static/5/9/3259/preview_small.jpg?1260925626","accentcolor":"#ebf700","textcolor":"#ebf700","updateURL":"https://www.getpersonas.com/en-US/update_check/3259","version":"1260925626"}]

Comment 3

[Loïc Grobol]

And it's the same after 3.6 update.

Comment 4

[Dave Townsend [:mossop]]

You have some kind of custom Persona there called "Night shades". Do you have any recollection of where it came from?

Comment 5

[Loïc Grobol]

It's a Persona I've created with the first version of Personas. Do you think it could be the issue?

Comment 6

[Dave Townsend [:mossop]]

(In reply to comment #5)
> It's a Persona I've created with the first version of Personas. Do you think it
> could be the issue?

It is definitely the issue, I wasn't aware the Personas extension was creating things like this.

Myk, if Personas is doing this then it really needs to ensure it includes an ID and name for the custom persona. We can somewhat mitigate a missing name but without an ID I don't think the add-ons manager window will be able to manage them properly.

Comment 7

[Loïc Grobol]

OK, fine then. I had forgotten Personas where now displayed in the add-ons manager. Should I remove this custom Persona (even if I haven't the slightest notion of how to do it) ?

Comment 8

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #6)
> Myk, if Personas is doing this then it really needs to ensure it includes an ID
> and name for the custom persona.

I think the extension automatically assigns a name to the custom persona, but it doesn't assign an ID, and you're right that it should now that the feature is integrated into the add-ons manager, which requires an ID (the extension itself doesn't need one).

I filed bug 544937 on making the extension assign IDs to custom personas.  However, I would still think it's worth fixing this bug so that it's not possible for extensions to break the add-ons manager in this way (either by not requiring an ID or by throwing an exception if some code tries to set the user's theme to a persona with no ID).

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Hmm, actually the custom persona editor assigns ID "0" to custom personas as of the latest releases per bug 532577, which was about fixing issues with the way Personas interacts with the add-ons manager.  However, that change didn't get applied to existing custom personas.

Comment 10

[Dave Townsend [:mossop]]

(In reply to comment #8)
> (In reply to comment #6)
> > Myk, if Personas is doing this then it really needs to ensure it includes an ID
> > and name for the custom persona.
> I filed bug 544937 on making the extension assign IDs to custom personas. 
> However, I would still think it's worth fixing this bug so that it's not
> possible for extensions to break the add-ons manager in this way (either by not
> requiring an ID or by throwing an exception if some code tries to set the
> user's theme to a persona with no ID).

Yep I agree, we'll try to get that fixed as best we can in the next point release

Comment 11

[Dave Townsend [:mossop]]

(In reply to comment #7)
> OK, fine then. I had forgotten Personas where now displayed in the add-ons
> manager. Should I remove this custom Persona (even if I haven't the slightest
> notion of how to do it) ?

The simplest means would be to get the add-ons manager working again would be to reset the preference lightweightThemes.usedTheme, though that will clear the list of recently used personas.

Comment 12

[Loïc Grobol]

Done, and it works. Thanks a lot.

Cheers,

Loïc

Comment 13

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Here is my suggested patch for the Firefox side. This makes us check that anything trying to set a customTheme provided the necessary properties in the object. In the add-ons manager we will just not list any themes with no ID (too much would be broken to try to handle this edge case I think). Themes with no name just have a blank name, anything else would require localisation but at least it won't break now. Additionally the service will silently drop any themes it knows about that have no ID whenever changing, adding or removing a theme just for sanity reasons.

Added a bunch of tests to verify all of that.

Comment 14

[Dão Gottwald [:dao]]

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

>   set currentTheme (aData) {
>+    if (aData) {
>+      for (let i = 0; i < MANDATORY.length; i++) {
>+        if (!(MANDATORY[i] in aData))

Can you add a helper function for this that would mostly do what parseTheme is doing, and use that in both places?

Comment 15

[Dave Townsend [:mossop]]

Attachment: patch rev 2

This folds all of the checking from parseTheme into a new function sanitizeTheme. Because we shouldn't be altering the object passed to currentTheme I've had to alter things a little so sanitizeTheme creates a new object and copies the good properties over to it.

Do you think we need to add similar roundtrip tests like we have for parseTheme?

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 426287 [details] [diff] [review]
patch rev 2

>+function sanitizeTheme(aData, aBaseURI) {

This should be moved down to the other helper functions. I'd also prefix "_" in order to make it immediately clear that it's not an exported symbol.

The tests look sufficient to me.

Comment 17

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/4b0f7a43f0a1

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 426287 [details] [diff] [review]
patch rev 2

This has cropped up in numerous support requests so I think we should take this on the branch.

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 426287 [details] [diff] [review]
patch rev 2

a1922=beltzner

Comment 24

[Dave Townsend [:mossop]]

Landed on branch: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a8b210f3a0c7

Comment 28

[Mike Kaply [:mkaply]]

Personas (the extension) is still doing something strange here.

Personas is using 0 as the ID not "0" - integer, not string.

And in the normalizePersona code, it explicitly says to ignore everything that is not a string.

So my guess is that a custom persona built with the latest Personas extension will still not work here because normalizePersona will reject it with a non string ID.

Comment 29

[Tony Chung [:tchung]]

Verified fix on using testcase from comment 2 on trunk:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a4pre) Gecko/20100321 Minefield/3.7a4pre

and Mac Branch: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.2) Gecko/20100316 Firefox/3.6.2

and Windows branch: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.2) Gecko/20100316 Firefox/3.6.2