Bug 90132 – [chromium] CCThreadTask should not depend on CrossThreadTask and does not need to depend on CrossThreadCopier

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] CCThreadTask should not depend on CrossThreadTask and does not need to depend on CrossThreadCopier

[James Robinson]

Created attachment 149862 [details]
Patch

[David Levin]

You should consider something which helps you automatically enforce proper calls across the thread boundary. Otherwise, you're asking for more subtle bugs. The current code makes it very explicit about which items need to be looked at carefully but this change looses that.

Another solution to your problem is to break out CrossThreadTaskTraits into a platform piece that has what you need.

[James Robinson]

I agree with the general principle of static checks. In practice, these checks have not provided us any benefit and the dependencies are proving problematic.

[David Levin]

(In reply to comment #3)
> I agree with the general principle of static checks. In practice, these checks have not provided us any benefit and the dependencies are proving problematic.

Couldn't the dependency be refactored to not be problematic?

Anyway, if you feel this is the best thing for the future maintenance of this piece of code, go for it. I just wanted to push a little about an alternative (because of the problems I've seen in other places in the code).

[Adrienne Walker]

I'm not entirely convinced here either.  I realize that the majority of the time we're allowing cross-thread access, but I think it's good that it's explicitly called out. At least in terms of reviewing, I'll think twice about whether it's the right thing to do when one of these gets added.

It looks like CrossThreadCopier is already in Platform.  Can you just split CrossThreadTaskTraits out of CrossThreadTask and put it in Platform as well?  Would that fix your dependency issues?

[Adrienne Walker]

(In reply to comment #5)
> I'm not entirely convinced here either.  I realize that the majority of the time we're allowing cross-thread access, but I think it's good that it's explicitly called out. At least in terms of reviewing, I'll think twice about whether it's the right thing to do when one of these gets added.
> 
> It looks like CrossThreadCopier is already in Platform.  Can you just split CrossThreadTaskTraits out of CrossThreadTask and put it in Platform as well?  Would that fix your dependency issues?

Oops, I misread the platform directory.  I guess they'd have to move to WTF or capital P platform.  Yuck.

[James Robinson]

Any new thoughts on this?  I still think this patch is a strict improvement on the current state.

[David Levin]

(In reply to comment #7)
> Any new thoughts on this?  I still think this patch is a strict improvement on the current state.

imo, it isn't but I've stated that already in comment #2. Ideally CrossThreadCopier would be refactored.

[Nat Duca]

Its always hard to say whether something like this has succeeded. Is the absence of bugs about cross-thread copying a signal, nor a non signal? 

My personal guess is that this hasn't saved us from enough errors to warrant bringing it over to Chromium as part of GTFO. Note, for example, it doesn't prevent the type of errors where you post to yourself but the target has died. That's where Chrome's ThreadSafeRefCounted stuff really works wonders. As a result, we've had to introduce things like CCTimer.

There's a much bigger source of issues here than cross-thread copies: the place where we've had our fragility is CCThreadProxy, where you can get to pointers from either thread. We've had a number of regressions where we dereferenced a pointer on the wrong side, simply because we dont have a ThreadSpecific<T>.

Net/net, I see less value in preventing types from moving across thread boundaries and more value in managing lifetimes and preventing access to pointers once you're on another thread.

Long winded way of saying, I like this change.

[David Levin]

(In reply to comment #9)
> Its always hard to say whether something like this has succeeded. Is the absence of bugs about cross-thread copying a signal, nor a non signal?  My personal guess is that this hasn't saved us from enough errors to warrant bringing it over to Chromium as part of GTFO. 

As a hard data point, I know that it has found a number of errors in other places (especially when we tightened it up to force the AllowCrossThreadAccess annotation but I don't think there were any of those in this graphics area (CCThread). Perhaps your coding practices will always keep you immune from the errors that others make. 

To me, it appears that this patch takes the easy way out instead of simply refactoring refactor CrossThreadCopier.

> Note, for example, it doesn't prevent the type of errors where you post to yourself but the target has died. That's where Chrome's ThreadSafeRefCounted stuff really works wonders. As a result, we've had to introduce things like CCTimer.

Yep it prevents one type of error. Ideally there would be something to prevent the other type of error as well.

>There's a much bigger source of issues here than cross-thread copies: the place where we've had our fragility is CCThreadProxy

Ideally those pointers wouldn't be in a structure that either thread could get to.

> Net/net, I see less value in preventing types from moving across thread boundaries and more value in managing lifetimes and preventing access to pointers once you're on another thread.

Are the two mutually exclusive?


Anyway, I will not approve this patch but I also will not r- this patch, so if the folks who work on this code want to r+ go for it (as I tried to state in comment #2.)

[David Levin]

Comment on attachment 149862 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 149862 [details]
Patch

Clearing flags on attachment: 149862

Committed r124077: <http://trac.webkit.org/changeset/124077>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.