556334 - Allow LightweightThemeManager to accept file URIs

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Ryan Doherty (:rdoherty)]

[spin-off of bug 554220]

The Personas Plus add-on has a 'custom persona' feature where users can select images from their computer to use. Previous to 3.6.2 the LightweightThemeManager allowed local file urls. A change in 3.6.2 now does not allow this, which breaks this functionality for many users.

STR:
1) Firefox 3.6.2 + Personas Plus 1.5.2
2) Tools => Personas => Preferences => Check 'Show Custom Persona in Menu'
3) Tools => Personas => Custom Persona => Edit
4) Select header and footer images from computer
5) See preview
6) Close tab
7) Custom persona is removed

Expected results:
Custom persona stays applied.

Comment 1

[Shae Rivard [:crimius]]

the same is true of a clean install of Firefox 3.6.3 and Personas 1.5.2 on a slackware 13 machine.

Comment 2

[Aesir Rising]

This is still an issue for Firefox 3.6.3, Personas 1.5.2 on Win7x64.

Comment 3

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

Dao: do we know what caused this? It would be something in this list:

https://bugzilla.mozilla.org/buglist.cgi?quicksearch=ALL%20status1.9.2:.2-fixed

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #3)
> Dao: do we know what caused this? It would be something in this list:
> 
> https://bugzilla.mozilla.org/buglist.cgi?quicksearch=ALL%20status1.9.2:.2-fixed

It was bug 544753 (the one in the blockers for this bug)

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch

We probably don't want to allow file URIs generally, at least not on a stable branch, and doing it only for the currentTheme setter seems strange, so I suggest we make it opt-in.

I renamed callback to successCallback because I erroneously thought it should be called in any case.

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 437276 [details] [diff] [review]
patch

I don't think this is the right way to go. As soon as the personas extension allows local resources webpages will be able to use file urls too which we don't want. I actually think that allowing file uri's through directly setting currentTheme isn't all that strange, it just means webpages have more restrictions than local extensions do which is pretty normal.

How about in _sanitizeTheme if aBaseURI is http/https then we restrict to http/https, otherwise we allow file too. That should fix this without the personas extension having to change anything. It would also allow webpages accessed via a file:/// url to use local files but that seems reasonable to me.

Or if we wanted to go the whole hog and use the content policy service to tell us if a particular URI is allowed to use the images it requests but I don't think that is needed for this bug.

Comment 7

[Dão Gottwald [:dao]]

(In reply to comment #6)
> (From update of attachment 437276 [details] [diff] [review])
> I don't think this is the right way to go. As soon as the personas extension
> allows local resources webpages will be able to use file urls too which we
> don't want.

No, they won't, as it's only allowed once, i.e. personas should use it when setting the custom/local persona.

> I actually think that allowing file uri's through directly setting
> currentTheme isn't all that strange, it just means webpages have more
> restrictions than local extensions do which is pretty normal.

It would actually lead to the problem you mentioned above when extensions use currentTheme to install extensions from the web (e.g. through the personas menu).

Comment 8

[Dave Townsend [:mossop]]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 437276 [details] [diff] [review] [details])
> > I don't think this is the right way to go. As soon as the personas extension
> > allows local resources webpages will be able to use file urls too which we
> > don't want.
> 
> No, they won't, as it's only allowed once, i.e. personas should use it when
> setting the custom/local persona.

Oh I see, but no setting a property that modifies the behavior of currentTheme for one time is not the right thing to do here.

> > I actually think that allowing file uri's through directly setting
> > currentTheme isn't all that strange, it just means webpages have more
> > restrictions than local extensions do which is pretty normal.
> 
> It would actually lead to the problem you mentioned above when extensions use
> currentTheme to install extensions from the web (e.g. through the personas
> menu).

That's a good point, but extensions could call parseTheme first including a base URI to verify the theme is valid.

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
> > No, they won't, as it's only allowed once, i.e. personas should use it when
> > setting the custom/local persona.
> 
> Oh I see, but no setting a property that modifies the behavior of currentTheme
> for one time is not the right thing to do here.

Would a dedicated setter or method be better?

> > It would actually lead to the problem you mentioned above when extensions use
> > currentTheme to install extensions from the web (e.g. through the personas
> > menu).
> 
> That's a good point, but extensions could call parseTheme first including a
> base URI to verify the theme is valid.

They could, but I'm not convinced they will... They might as well use JSON.parse and currentTheme. I still find it hard to follow why parsing a string would verify its integrity but setting a theme wouldn't.

Comment 10

[Dave Townsend [:mossop]]

(In reply to comment #9)
> (In reply to comment #8)
> > > No, they won't, as it's only allowed once, i.e. personas should use it when
> > > setting the custom/local persona.
> > 
> > Oh I see, but no setting a property that modifies the behavior of currentTheme
> > for one time is not the right thing to do here.
> 
> Would a dedicated setter or method be better?

Yeah something dedicated would seem much better to me.

Comment 11

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 12

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

Please nominate for approval once it's reviewed and baked.

Comment 13

[Dão Gottwald [:dao]]

Dave, ping?

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 437356 [details] [diff] [review]
patch v2

Sorry this slipped off my list.

Setting a variable to change the behaviour of sanitizeTheme is still not the right way to go. Instead I think separate out the bulk of the currentTheme setter into a private function and then have currentTheme and currentThemeWithLocalResources call sanitizeTheme with an argument saying whether to allow resources and then call the new private function.

Not entirely sure I'm sold on currentThemeWithLocalResources as a name either, what do you think about just setLocalTheme? It should be a method rather than a setter with no matching getter I think.

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #14)
> Setting a variable to change the behaviour of sanitizeTheme is still not the
> right way to go.

I don't understand why that's a problem, as the variable can't be accessed from the outside.

Comment 16

[Dave Townsend [:mossop]]

(In reply to comment #15)
> (In reply to comment #14)
> > Setting a variable to change the behaviour of sanitizeTheme is still not the
> > right way to go.
> 
> I don't understand why that's a problem, as the variable can't be accessed from
> the outside.

Using global variables to alter a function's behaviour makes sense when they are essentially caching preferences but for one time changes it is bad design.

Comment 17

[Dão Gottwald [:dao]]

Attachment: patch v3

Comment 18

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/8db5461a5404

Comment 19

[Ryan Doherty (:rdoherty)]

What version of Firefox will this be a part of?

Comment 20

[Dave Townsend [:mossop]]

(In reply to comment #19)
> What version of Firefox will this be a part of?

We'll get it landed for Firefox 3.6.5

Comment 21

[Dave Townsend [:mossop]]

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

Comment 22

[Jose E. Bolanos]

This bug seems to have reappeared in Firefox 3.6.8.

When the Personas add-on tries to use the LightweightThemeManager.setLocalTheme method, the following error is reported:

"Error: uncaught exception: 2147942487"

The persona object being passed is the following:

{"id":0,
"name":"My Custom Persona",
"headerURL":"file:///Users/josenriq/Pictures/rayTrace1.jpg?298",
"footerURL":"file:///Users/josenriq/Pictures/rayTrace3.jpg?6901",
"custom":true,
"textcolor":"#FFCC00",
"accentcolor":"#006600"}

Comment 23

[Dave Townsend [:mossop]]

(In reply to comment #22)
> This bug seems to have reappeared in Firefox 3.6.8.

It's probably better to open new bugs when finding problems that are believed to be fixed.

In this case the problem is that the id property is expected to be a string and not a plain integer as you've used. Changing it to a string makes it work correctly.