Closed Bug 556334 Opened 11 years ago Closed 10 years ago

Allow LightweightThemeManager to accept file URIs

Categories

(Toolkit :: Add-ons Manager, defect)

1.9.2 Branch
x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5
Tracking Status
blocking1.9.2 --- needed
status1.9.2 --- .5-fixed

People

(Reporter: rdoherty, Assigned: dao)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

[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.
Blocks: 544753
the same is true of a clean install of Firefox 3.6.3 and Personas 1.5.2 on a slackware 13 machine.
This is still an issue for Firefox 3.6.3, Personas 1.5.2 on Win7x64.
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
Keywords: regression
(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)
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #437276 - Flags: review?(dtownsend)
Version: unspecified → 1.9.2 Branch
blocking1.9.2: --- → ?
status1.9.2: --- → ?
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.
Attachment #437276 - Flags: review?(dtownsend) → review-
(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).
(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.
(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.
(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.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #437276 - Attachment is obsolete: true
Attachment #437356 - Flags: review?(dtownsend)
Blocks: 554220
Please nominate for approval once it's reviewed and baked.
blocking1.9.2: ? → needed
Dave, ping?
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.
Attachment #437356 - Flags: review?(dtownsend) → review-
(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.
(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.
Attached patch patch v3Splinter Review
Attachment #437356 - Attachment is obsolete: true
Attachment #441449 - Flags: review?(dtownsend)
Attachment #441449 - Flags: review?(dtownsend) → review+
http://hg.mozilla.org/mozilla-central/rev/8db5461a5404
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Attachment #441449 - Flags: approval1.9.2.5?
What version of Firefox will this be a part of?
Attachment #441449 - Flags: approval1.9.2.5? → approval1.9.2.5+
(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
Keywords: checkin-needed
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"}
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.