Bug 90132 - [chromium] CCThreadTask should not depend on CrossThreadTask and does not need to depend on CrossThreadCopier
Summary: [chromium] CCThreadTask should not depend on CrossThreadTask and does not nee...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-27 21:20 PDT by James Robinson
Modified: 2012-07-30 13:06 PDT (History)
8 users (show)

See Also:


Attachments
Patch (19.20 KB, patch)
2012-06-27 21:24 PDT, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-06-27 21:20:45 PDT
[chromium] CCThreadTask should not depend on CrossThreadTask and does not need to depend on CrossThreadCopier
Comment 1 James Robinson 2012-06-27 21:24:55 PDT
Created attachment 149862 [details]
Patch
Comment 2 David Levin 2012-06-27 22:38:12 PDT
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.
Comment 3 James Robinson 2012-06-27 22:52:00 PDT
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.
Comment 4 David Levin 2012-06-27 23:10:06 PDT
(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).
Comment 5 Adrienne Walker 2012-06-28 18:35:22 PDT
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?
Comment 6 Adrienne Walker 2012-06-28 18:42:59 PDT
(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.
Comment 7 James Robinson 2012-07-30 11:29:12 PDT
Any new thoughts on this?  I still think this patch is a strict improvement on the current state.
Comment 8 David Levin 2012-07-30 11:49:20 PDT
(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.
Comment 9 Nat Duca 2012-07-30 11:58:21 PDT
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.
Comment 10 David Levin 2012-07-30 12:16:21 PDT
(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.)
Comment 11 David Levin 2012-07-30 12:54:22 PDT
Comment on attachment 149862 [details]
Patch

ok
Comment 12 WebKit Review Bot 2012-07-30 13:06:16 PDT
Comment on attachment 149862 [details]
Patch

Clearing flags on attachment: 149862

Committed r124077: <http://trac.webkit.org/changeset/124077>
Comment 13 WebKit Review Bot 2012-07-30 13:06:21 PDT
All reviewed patches have been landed.  Closing bug.