Bug 91529 – Make Animation, TimingFunction and TransformOperation thread safe

• Component: New Bugs
• Status: NEW

[Noam Rosenthal]

Make Animation, TimingFunction and TransformOperation thread safe

[Noam Rosenthal]

Created attachment 152794 [details]
Patch

[David Levin]

Comment on attachment 152794 [details]
Patch

This doesn't have much context. It would be nice to have a over arching bug with more context on what is being done.

Anyway, I'm dubious of this patch since a cursory glance revealed something that seems fundamentally incorrect in it.
 
I'm hesitant to point it out because it makes me feel like the implications of this change haven't been examined. Please look carefully at these classes and make sure it is safe to do this. (Hint: It isn't right now.)

[Noam Rosenthal]

(In reply to comment #2)
> (From update of attachment 152794 [details])
> This doesn't have much context. It would be nice to have a over arching bug with more context on what is being done.

Fair enough, I'll submit the patch that uses it when it's ready.
 
> I'm hesitant to point it out because it makes me feel like the implications of this change haven't been examined. Please look carefully at these classes and make sure it is safe to do this. (Hint: It isn't right now.)

Possible :)

Looking at those classes, the only thing I can think of is the assignment operator for Animation. Is there problems with the other classes as well? I can't spot any but it's possible I've missed something. 

I can probably do without making Animation ThreadSafeRefCounted. Is there a particular problem with TimingFunction and TransformOperation?

[David Levin]

Thanks for trying :)

I'll be more direct. String uses non-threadsafe ref counting of StringImpl (so it isn't safe to destruct it on any thread). 

I think the only use of them is in Animation, so if you can avoid making it ThreadSafeRefCounted, I think you are on a better track,


 > I'll submit the patch that uses it when it's ready.
fwiw, I appreciate the incremental approach that you're doing. I was just hoping to see something with more of any overview as well.

[Noam Rosenthal]

(In reply to comment #4)
> Thanks for trying :)
> 
> I'll be more direct. String uses non-threadsafe ref counting of StringImpl (so it isn't safe to destruct it on any thread). 
> 
> I think the only use of them is in Animation, so if you can avoid making it ThreadSafeRefCounted, I think you are on a better track,


ah, thanks for the catch. This actually does make me rethink of the approach, as later on we'd need to do the same for filters which have lots of strings inside. So probably the best approach would be to provide isolatedCopy functionality for these classes.