Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
374583002:
Replace CallClosureTask::create(bind()) with createCrossThreadTask() (Closed)
|
Created:
6 years, 5 months ago by hiroshige Modified:
6 years, 5 months ago CC:
eae+blinkwatch, tommyw+watchlist_chromium.org, falken, dglazkov+blink, kinuko+worker_chromium.org, horo+watch_chromium.org, rwlbuis, oilpan-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionReplace CallClosureTask::create(bind()) with createCrossThreadTask()
Background:
The code pattern of
postTask(CallClosureTask::create(bind(args)))
is thread unsafe for posting tasks crossing threads, even if args are
deep-copied, due to temporary objects (created as the return value of bind()
and deep copy functions such as String::isolatedCopy()).
Solution by this CL:
Created createCrossThreadTask() and replaced all
CallClosureTask::create(bind()) with createCrossThreadTask().
createCrossThreadTask calls bind and does deep copy (if necessary) by
CrossThreadCopier inside createCrossThreadTask, like createCallbackTask.
This is safer for cross-thread task posting because all temporary objects are
created inside createCrossThreadTask (not in its caller), and thus destroyed
before returning from createCrossThreadTask (i.e. before calling postTask).
Removed postTask() and postInspectorTask() that accepts Closure&
(i.e. return value of bind()).
BUG=390851
BUG=390174
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177815
Patch Set 1 #
Total comments: 6
Patch Set 2 : Modified comments #Patch Set 3 : Move createCallClosureTask to CrossThreadTask.h #
Total comments: 12
Patch Set 4 : Rebase, rename to createCrossThreadTask, and so #
Total comments: 4
Patch Set 5 : #Patch Set 6 : #Messages
Total messages: 33 (0 generated)
Please include the subject in the CL description. When the CL is committed, only the CL description will be used for the revision log.
On 2014/07/07 09:35:17, tyoshino wrote: > Please include the subject in the CL description. When the CL is committed, only > the CL description will be used for the revision log. s/Created and/Created/ s/createCallBackTask/createCallbackTask/
https://codereview.chromium.org/374583002/diff/1/Source/core/dom/ExecutionCon... File Source/core/dom/ExecutionContextTask.h (right): https://codereview.chromium.org/374583002/diff/1/Source/core/dom/ExecutionCon... Source/core/dom/ExecutionContextTask.h:65: // templates for member function of class C + raw pointer (C*) Start with a capital letter plz. https://codereview.chromium.org/374583002/diff/1/Source/core/dom/ExecutionCon... Source/core/dom/ExecutionContextTask.h:66: // which do not use CrossThreadCopier for a1 argument names are omitted here due to Blink coding style guide. Please use "the raw pointer of the C instance" or something than "a1". https://codereview.chromium.org/374583002/diff/1/Source/core/dom/ExecutionCon... Source/core/dom/ExecutionContextTask.h:130: */ except for copyright notice, it's more common to use // even for multi-line comments. Could you please use //?
Thank you for reviewing. I modified the comments in the code and also modified the CL description. https://codereview.chromium.org/374583002/diff/1/Source/core/dom/ExecutionCon... File Source/core/dom/ExecutionContextTask.h (right): https://codereview.chromium.org/374583002/diff/1/Source/core/dom/ExecutionCon... Source/core/dom/ExecutionContextTask.h:65: // templates for member function of class C + raw pointer (C*) On 2014/07/07 09:48:09, tyoshino wrote: > Start with a capital letter plz. Done. https://codereview.chromium.org/374583002/diff/1/Source/core/dom/ExecutionCon... Source/core/dom/ExecutionContextTask.h:66: // which do not use CrossThreadCopier for a1 On 2014/07/07 09:48:09, tyoshino wrote: > argument names are omitted here due to Blink coding style guide. Please use "the > raw pointer of the C instance" or something than "a1". Done. https://codereview.chromium.org/374583002/diff/1/Source/core/dom/ExecutionCon... Source/core/dom/ExecutionContextTask.h:130: */ On 2014/07/07 09:48:09, tyoshino wrote: > except for copyright notice, it's more common to use // even for multi-line > comments. Could you please use //? Done.
My understanding is that WTF::bind() is unusable for cross-thread posting by design, and we should use CrossThreadTask for cross-thread posting.
On 2014/07/08 01:32:02, tkent wrote: > My understanding is that WTF::bind() is unusable for cross-thread posting by > design, and we should use CrossThreadTask for cross-thread posting. Chatted with tkent offline. Currently there's no variant of createCallbackTask() for calling member functions. What this CL wants to do is adding that. We might want to put the new methods in CrossThreadTask.h. tkent's concern is that bind() may be unusable for cross-thread posting even after CrossThreadCopier wrapping. Let's investigate what's done in CrossThreadTaskN to see whether bind + CrossThreadCopier combination lacks anything done in CrossThreadTaskN.
Probably I thought WTF::bind was unusable because it didn't apply CrossThreadCopier automatically. If so, - The approach of Patch Set 2 is right. However, CrossThreadTask.h should have the implementation IMO. - We also have usecases that post a closure to the same thread to delay something. However keeping postTask(const Closure&) is confusing. We might want to have single-thread version of createFooTask().
On 2014/07/08 04:19:59, tkent wrote: > Probably I thought WTF::bind was unusable because it didn't apply > CrossThreadCopier automatically. > If so, > - The approach of Patch Set 2 is right. However, CrossThreadTask.h should have > the implementation IMO. > - We also have usecases that post a closure to the same thread to delay > something. However keeping postTask(const Closure&) is confusing. We might want > to have single-thread version of createFooTask(). I moved the implementation to CrossThreadTask.h. > However keeping postTask(const Closure&) is confusing. > We might want to have single-thread version of createFooTask(). I agree. We might need to refactor the code (e.g. give good names to createCallbackTask/createCallClosureTask and its same-thread version), possibly in separate CLs.
Please wrap the CL description in 80 columns. https://codereview.chromium.org/374583002/diff/40001/Source/core/dom/CrossThr... File Source/core/dom/CrossThreadTask.h (right): https://codereview.chromium.org/374583002/diff/40001/Source/core/dom/CrossThr... Source/core/dom/CrossThreadTask.h:481: // createCallClosureTask(...) is similar to but safer than I'd like to name this createCrossThreadTask. This actually creates CallClosureTask, but IMO CallClosureTask is an implementation detail and should hide it from users. The purpose of this function is same as the existing createCrossThreadTask, and this should have the same name. https://codereview.chromium.org/374583002/diff/40001/Source/core/dom/CrossThr... Source/core/dom/CrossThreadTask.h:503: template<typename R, typename C> Is R necessary? it's always void, right? https://codereview.chromium.org/374583002/diff/40001/Source/core/dom/Executio... File Source/core/dom/ExecutionContextTask.h (right): https://codereview.chromium.org/374583002/diff/40001/Source/core/dom/Executio... Source/core/dom/ExecutionContextTask.h:67: friend PassOwnPtr<ExecutionContextTask> createCallClosureTask(R (C::*)(), C*); These |friend|s are ugly and overkill. Keeping the factory public and adding the |do not use| comment is enough.
> Please wrap the CL description in 80 columns. Done. https://codereview.chromium.org/374583002/diff/40001/Source/core/dom/CrossThr... File Source/core/dom/CrossThreadTask.h (right): https://codereview.chromium.org/374583002/diff/40001/Source/core/dom/CrossThr... Source/core/dom/CrossThreadTask.h:481: // createCallClosureTask(...) is similar to but safer than The name createCrossThreadTask seems good, but doesn't exist in the current code. Did you mean createCallbackTask? For me, renaming createCallClosureTask to createCallbackTask might be confusing, because createCallbackTask differs in that its argument 'method' takes ExecutionContext* argument. I'd like to give better names (and refactor the functionality if needed) to the following similar but different functions: 1. createCallbackTask(method,args) (existing) - Cross-thread: Yes - Support for member functions: No - method(context,args) is called in performTask(ExecutionContext* context) 2. createCallClosureTask(method,args) (newly-added) - Cross-thread: Yes - Support for member functions: Yes - method(args) is called in performTask(ExecutionContext*) 3. same-thread version of createCallClosureTask(method,args) (not yet added, not used in this CL but can be used in future refactoring) - Cross-thread: No - Support for member functions: Yes - method(args) is called in performTask(ExecutionContext*) A candidate is (I'm not confident though): 1. createCallbackTask or createCrossThreadCallbackTask 2. createCrossThreadTask 3. createSameThreadTask Any suggestions are welcome. https://codereview.chromium.org/374583002/diff/40001/Source/core/dom/CrossThr... Source/core/dom/CrossThreadTask.h:503: template<typename R, typename C> On 2014/07/10 03:36:14, tkent wrote: > Is R necessary? it's always void, right? Right, because CallClosureTask::create require Closure as its argument. I'll replace R with void.
https://codereview.chromium.org/374583002/diff/40001/Source/core/dom/CrossThr... File Source/core/dom/CrossThreadTask.h (right): https://codereview.chromium.org/374583002/diff/40001/Source/core/dom/CrossThr... Source/core/dom/CrossThreadTask.h:481: // createCallClosureTask(...) is similar to but safer than On 2014/07/10 04:39:56, hiroshige wrote: > A candidate is (I'm not confident though): > 1. createCallbackTask or createCrossThreadCallbackTask > 2. createCrossThreadTask > 3. createSameThreadTask Yes we can also rename 1 later to align naming convention. Can we give the same name to 1 and 2 technically? It looks createCrossThreadTask makes sense for both.
https://codereview.chromium.org/374583002/diff/40001/Source/core/dom/Executio... File Source/core/dom/ExecutionContextTask.h (right): https://codereview.chromium.org/374583002/diff/40001/Source/core/dom/Executio... Source/core/dom/ExecutionContextTask.h:56: // Do not use create other than in createCallClosureTask. http://crbug.com/390851 "Do not use |create| ..." or "Do not use create() ..." is easier to understand.
https://codereview.chromium.org/374583002/diff/40001/Source/core/dom/CrossThr... File Source/core/dom/CrossThreadTask.h (right): https://codereview.chromium.org/374583002/diff/40001/Source/core/dom/CrossThr... Source/core/dom/CrossThreadTask.h:481: // createCallClosureTask(...) is similar to but safer than On 2014/07/10 04:39:56, hiroshige wrote: > The name createCrossThreadTask seems good, but doesn't exist in the current > code. > Did you mean createCallbackTask? For me, renaming createCallClosureTask to > createCallbackTask might be confusing, because createCallbackTask differs in > that its argument 'method' takes ExecutionContext* argument. > > I'd like to give better names (and refactor the functionality if needed) to the > following similar but different functions: > 1. createCallbackTask(method,args) (existing) > - Cross-thread: Yes > - Support for member functions: No > - method(context,args) is called in performTask(ExecutionContext* context) > 2. createCallClosureTask(method,args) (newly-added) > - Cross-thread: Yes > - Support for member functions: Yes > - method(args) is called in performTask(ExecutionContext*) > 3. same-thread version of createCallClosureTask(method,args) (not yet added, not > used in this CL but can be used in future refactoring) > - Cross-thread: No > - Support for member functions: Yes > - method(args) is called in performTask(ExecutionContext*) > > A candidate is (I'm not confident though): > 1. createCallbackTask or createCrossThreadCallbackTask > 2. createCrossThreadTask > 3. createSameThreadTask > > Any suggestions are welcome. I'm sorry, I meant createCallbackTask. Smaller patch is better. Should this CL be merged to a branch? So I recommend: 1. Add createCrossThreadTask for closures in this CL 2. The next CL renames existing createCallbackTask to createCrossThreadTask. 3. Another CL adds create(Same|Single)ThreadTask.
https://codereview.chromium.org/374583002/diff/40001/Source/core/dom/CrossThr... File Source/core/dom/CrossThreadTask.h (right): https://codereview.chromium.org/374583002/diff/40001/Source/core/dom/CrossThr... Source/core/dom/CrossThreadTask.h:481: // createCallClosureTask(...) is similar to but safer than > Should this CL be merged to a branch? Probably yes. I renamed createCallClosureTask to createCrossThreadTask. I'll submit separate CLs later. https://codereview.chromium.org/374583002/diff/40001/Source/core/dom/CrossThr... Source/core/dom/CrossThreadTask.h:503: template<typename R, typename C> On 2014/07/10 04:39:56, hiroshige wrote: > On 2014/07/10 03:36:14, tkent wrote: > > Is R necessary? it's always void, right? > Right, because CallClosureTask::create require Closure as its argument. > I'll replace R with void. Done. https://codereview.chromium.org/374583002/diff/40001/Source/core/dom/Executio... File Source/core/dom/ExecutionContextTask.h (right): https://codereview.chromium.org/374583002/diff/40001/Source/core/dom/Executio... Source/core/dom/ExecutionContextTask.h:56: // Do not use create other than in createCallClosureTask. http://crbug.com/390851 On 2014/07/10 04:55:42, yhirano wrote: > "Do not use |create| ..." or "Do not use create() ..." is easier to understand. Done. https://codereview.chromium.org/374583002/diff/40001/Source/core/dom/Executio... Source/core/dom/ExecutionContextTask.h:67: friend PassOwnPtr<ExecutionContextTask> createCallClosureTask(R (C::*)(), C*); On 2014/07/10 03:36:14, tkent wrote: > These |friend|s are ugly and overkill. Keeping the factory public and adding > the |do not use| comment is enough. > Done.
https://codereview.chromium.org/374583002/diff/60001/Source/core/dom/CrossThr... File Source/core/dom/CrossThreadTask.h (right): https://codereview.chromium.org/374583002/diff/60001/Source/core/dom/CrossThr... Source/core/dom/CrossThreadTask.h:510: template<typename C, typename P2, typename A2> Start from P1 and A1? We could also align with the naming rule used above. How about C, M, P1, P2, ...
lgtm Please replace createCallClosureTask in the CL description with createCrossThreadTask https://codereview.chromium.org/374583002/diff/60001/Source/core/dom/Executio... File Source/core/dom/ExecutionContextTask.h (right): https://codereview.chromium.org/374583002/diff/60001/Source/core/dom/Executio... Source/core/dom/ExecutionContextTask.h:54: // Do not use |create| other than in createCallClosureTask. createCallClosureTask -> createCrossThreadTask
https://codereview.chromium.org/374583002/diff/60001/Source/core/dom/CrossThr... File Source/core/dom/CrossThreadTask.h (right): https://codereview.chromium.org/374583002/diff/60001/Source/core/dom/CrossThr... Source/core/dom/CrossThreadTask.h:510: template<typename C, typename P2, typename A2> On 2014/07/10 07:31:46, tyoshino wrote: > Start from P1 and A1? We could also align with the naming rule used above. > > How about C, M, P1, P2, ... Done. https://codereview.chromium.org/374583002/diff/60001/Source/core/dom/Executio... File Source/core/dom/ExecutionContextTask.h (right): https://codereview.chromium.org/374583002/diff/60001/Source/core/dom/Executio... Source/core/dom/ExecutionContextTask.h:54: // Do not use |create| other than in createCallClosureTask. On 2014/07/10 07:55:13, tkent wrote: > createCallClosureTask -> createCrossThreadTask Done.
lgtm
The CQ bit was checked by tyoshino@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hiroshige@chromium.org/374583002/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
Message was sent while issue was closed.
Change committed as 177815
Message was sent while issue was closed.
Looks like this broke Oilpan compilation, http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan/...
Message was sent while issue was closed.
On 2014/07/10 11:11:59, sof wrote: > Looks like this broke Oilpan compilation, > > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan/... I'll handle this.
Message was sent while issue was closed.
On 2014/07/10 11:31:58, haraken wrote: > On 2014/07/10 11:11:59, sof wrote: > > Looks like this broke Oilpan compilation, > > > > > > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan/... > > I'll handle this. Thanks! To call member function of class C (without AllowCrossThreadAccess()), my code only supports createCrossThreadTask(func,p,...) where func is a pointer to a member function of class C and p is pointer (C*) or WeakPtr<C>, but in Oilpan WeakMember or RawPtr is passed instead. (Lines 503--641 in Source/core/dom/CrossThreadTask.h above) A possible solution is to allow these code to catch everything (other than C* and WeakPtr<C>) to be passed without CrossThreadCopier if func is a pointer to a member function, but not sure it is always correct.
Message was sent while issue was closed.
On 2014/07/10 11:50:36, hiroshige wrote: > On 2014/07/10 11:31:58, haraken wrote: > > On 2014/07/10 11:11:59, sof wrote: > > > Looks like this broke Oilpan compilation, > > > > > > > > > > > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan/... > > > > I'll handle this. > > Thanks! > > To call member function of class C (without AllowCrossThreadAccess()), my code > only supports > createCrossThreadTask(func,p,...) where func is a pointer to a member function > of class C and > p is pointer (C*) or WeakPtr<C>, but in Oilpan WeakMember or RawPtr is passed > instead. > (Lines 503--641 in Source/core/dom/CrossThreadTask.h above) > > A possible solution is to allow these code to catch everything (other than C* > and WeakPtr<C>) > to be passed without CrossThreadCopier if func is a pointer to a member > function, > but not sure it is always correct. Hmm, this is complicated. I think we need to make the member function match: template<typename T> struct CrossThreadCopierBase<false, false, true, T> { // true means the object type is GarbageCollected. ...; }; in order to use a correct CrossThreadCopier. So I added: template<typename T> struct CrossThreadCopier : public CrossThreadCopierBase<WTF::IsConvertibleToInteger<T>::value, WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, RefPtr>::Type, ThreadSafeRefCounted>::value || WTF::IsSubclassOfTemplate<typename WTF::RemovePointer<T>::Type, ThreadSafeRefCounted>::value || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, PassRefPtr>::Type, ThreadSafeRefCounted>::value, WTF::IsSubclassOfTemplate<typename WTF::RemovePointer<T>::Type, GarbageCollected>::value || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, WeakMember>::Type, GarbageCollected>::value || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, RawPtr>::Type, GarbageCollected>::value, T> { }; but then I hit another error... If you have some solution in mind, would you mind uploading a CL?
Message was sent while issue was closed.
On 2014/07/10 11:57:09, haraken wrote: > On 2014/07/10 11:50:36, hiroshige wrote: > > On 2014/07/10 11:31:58, haraken wrote: > > > On 2014/07/10 11:11:59, sof wrote: > > > > Looks like this broke Oilpan compilation, > > > > > > > > > > > > > > > > > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan/... > > > > > > I'll handle this. > > > > Thanks! > > > > To call member function of class C (without AllowCrossThreadAccess()), my code > > only supports > > createCrossThreadTask(func,p,...) where func is a pointer to a member function > > of class C and > > p is pointer (C*) or WeakPtr<C>, but in Oilpan WeakMember or RawPtr is passed > > instead. > > (Lines 503--641 in Source/core/dom/CrossThreadTask.h above) > > > > A possible solution is to allow these code to catch everything (other than C* > > and WeakPtr<C>) > > to be passed without CrossThreadCopier if func is a pointer to a member > > function, > > but not sure it is always correct. > > Hmm, this is complicated. > > I think we need to make the member function match: > > template<typename T> struct CrossThreadCopierBase<false, false, true, T> { // > true means the object type is GarbageCollected. > ...; > }; > > in order to use a correct CrossThreadCopier. So I added: > > template<typename T> struct CrossThreadCopier : public > CrossThreadCopierBase<WTF::IsConvertibleToInteger<T>::value, > WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, RefPtr>::Type, > ThreadSafeRefCounted>::value > || WTF::IsSubclassOfTemplate<typename WTF::RemovePointer<T>::Type, > ThreadSafeRefCounted>::value > || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, > PassRefPtr>::Type, ThreadSafeRefCounted>::value, > WTF::IsSubclassOfTemplate<typename WTF::RemovePointer<T>::Type, > GarbageCollected>::value > || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, > WeakMember>::Type, GarbageCollected>::value > || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, > RawPtr>::Type, GarbageCollected>::value, > T> { > > }; > > but then I hit another error... > > If you have some solution in mind, would you mind uploading a CL? Here's my attempt -- https://codereview.chromium.org/388463002/
Message was sent while issue was closed.
On 2014/07/10 11:59:33, sof wrote: > On 2014/07/10 11:57:09, haraken wrote: > > On 2014/07/10 11:50:36, hiroshige wrote: > > > On 2014/07/10 11:31:58, haraken wrote: > > > > On 2014/07/10 11:11:59, sof wrote: > > > > > Looks like this broke Oilpan compilation, > > > > > > > > > > > > > > > > > > > > > > > > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan/... > > > > > > > > I'll handle this. > > > > > > Thanks! > > > > > > To call member function of class C (without AllowCrossThreadAccess()), my > code > > > only supports > > > createCrossThreadTask(func,p,...) where func is a pointer to a member > function > > > of class C and > > > p is pointer (C*) or WeakPtr<C>, but in Oilpan WeakMember or RawPtr is > passed > > > instead. > > > (Lines 503--641 in Source/core/dom/CrossThreadTask.h above) > > > > > > A possible solution is to allow these code to catch everything (other than > C* > > > and WeakPtr<C>) > > > to be passed without CrossThreadCopier if func is a pointer to a member > > > function, > > > but not sure it is always correct. > > > > Hmm, this is complicated. > > > > I think we need to make the member function match: > > > > template<typename T> struct CrossThreadCopierBase<false, false, true, T> { // > > true means the object type is GarbageCollected. > > ...; > > }; > > > > in order to use a correct CrossThreadCopier. So I added: > > > > template<typename T> struct CrossThreadCopier : public > > CrossThreadCopierBase<WTF::IsConvertibleToInteger<T>::value, > > WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, > RefPtr>::Type, > > ThreadSafeRefCounted>::value > > || WTF::IsSubclassOfTemplate<typename WTF::RemovePointer<T>::Type, > > ThreadSafeRefCounted>::value > > || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, > > PassRefPtr>::Type, ThreadSafeRefCounted>::value, > > WTF::IsSubclassOfTemplate<typename WTF::RemovePointer<T>::Type, > > GarbageCollected>::value > > || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, > > WeakMember>::Type, GarbageCollected>::value > > || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, > > RawPtr>::Type, GarbageCollected>::value, > > T> { > > > > > }; > > > > but then I hit another error... > > > > If you have some solution in mind, would you mind uploading a CL? > > Here's my attempt -- https://codereview.chromium.org/388463002/ Thanks for the quick fix, looks correct to me!
Message was sent while issue was closed.
On 2014/07/10 12:02:18, haraken wrote: > On 2014/07/10 11:59:33, sof wrote: > > On 2014/07/10 11:57:09, haraken wrote: > > > On 2014/07/10 11:50:36, hiroshige wrote: > > > > On 2014/07/10 11:31:58, haraken wrote: > > > > > On 2014/07/10 11:11:59, sof wrote: > > > > > > Looks like this broke Oilpan compilation, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan/... > > > > > > > > > > I'll handle this. > > > > > > > > Thanks! > > > > > > > > To call member function of class C (without AllowCrossThreadAccess()), my > > code > > > > only supports > > > > createCrossThreadTask(func,p,...) where func is a pointer to a member > > function > > > > of class C and > > > > p is pointer (C*) or WeakPtr<C>, but in Oilpan WeakMember or RawPtr is > > passed > > > > instead. > > > > (Lines 503--641 in Source/core/dom/CrossThreadTask.h above) > > > > > > > > A possible solution is to allow these code to catch everything (other than > > C* > > > > and WeakPtr<C>) > > > > to be passed without CrossThreadCopier if func is a pointer to a member > > > > function, > > > > but not sure it is always correct. > > > > > > Hmm, this is complicated. > > > > > > I think we need to make the member function match: > > > > > > template<typename T> struct CrossThreadCopierBase<false, false, true, T> { > // > > > true means the object type is GarbageCollected. > > > ...; > > > }; > > > > > > in order to use a correct CrossThreadCopier. So I added: > > > > > > template<typename T> struct CrossThreadCopier : public > > > CrossThreadCopierBase<WTF::IsConvertibleToInteger<T>::value, > > > WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, > > RefPtr>::Type, > > > ThreadSafeRefCounted>::value > > > || WTF::IsSubclassOfTemplate<typename > WTF::RemovePointer<T>::Type, > > > ThreadSafeRefCounted>::value > > > || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, > > > PassRefPtr>::Type, ThreadSafeRefCounted>::value, > > > WTF::IsSubclassOfTemplate<typename WTF::RemovePointer<T>::Type, > > > GarbageCollected>::value > > > || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, > > > WeakMember>::Type, GarbageCollected>::value > > > || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, > > > RawPtr>::Type, GarbageCollected>::value, > > > T> { > > > > > > > > > }; > > > > > > but then I hit another error... > > > > > > If you have some solution in mind, would you mind uploading a CL? > > > > Here's my attempt -- https://codereview.chromium.org/388463002/ > > Thanks for the quick fix, looks correct to me! Probably fixing CrossThreadCopier to pass these pointers is not correct, because such fix also passes the pointers in the arguments (i.e. not pointer to the object to be called), which can cause thread unsafe code. I'll create another CL soon.
Message was sent while issue was closed.
On 2014/07/10 12:06:14, hiroshige wrote: > On 2014/07/10 12:02:18, haraken wrote: > > On 2014/07/10 11:59:33, sof wrote: > > > On 2014/07/10 11:57:09, haraken wrote: > > > > On 2014/07/10 11:50:36, hiroshige wrote: > > > > > On 2014/07/10 11:31:58, haraken wrote: > > > > > > On 2014/07/10 11:11:59, sof wrote: > > > > > > > Looks like this broke Oilpan compilation, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan/... > > > > > > > > > > > > I'll handle this. > > > > > > > > > > Thanks! > > > > > > > > > > To call member function of class C (without AllowCrossThreadAccess()), > my > > > code > > > > > only supports > > > > > createCrossThreadTask(func,p,...) where func is a pointer to a member > > > function > > > > > of class C and > > > > > p is pointer (C*) or WeakPtr<C>, but in Oilpan WeakMember or RawPtr is > > > passed > > > > > instead. > > > > > (Lines 503--641 in Source/core/dom/CrossThreadTask.h above) > > > > > > > > > > A possible solution is to allow these code to catch everything (other > than > > > C* > > > > > and WeakPtr<C>) > > > > > to be passed without CrossThreadCopier if func is a pointer to a member > > > > > function, > > > > > but not sure it is always correct. > > > > > > > > Hmm, this is complicated. > > > > > > > > I think we need to make the member function match: > > > > > > > > template<typename T> struct CrossThreadCopierBase<false, false, true, T> { > > > // > > > > true means the object type is GarbageCollected. > > > > ...; > > > > }; > > > > > > > > in order to use a correct CrossThreadCopier. So I added: > > > > > > > > template<typename T> struct CrossThreadCopier : public > > > > CrossThreadCopierBase<WTF::IsConvertibleToInteger<T>::value, > > > > WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, > > > RefPtr>::Type, > > > > ThreadSafeRefCounted>::value > > > > || WTF::IsSubclassOfTemplate<typename > > WTF::RemovePointer<T>::Type, > > > > ThreadSafeRefCounted>::value > > > > || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, > > > > PassRefPtr>::Type, ThreadSafeRefCounted>::value, > > > > WTF::IsSubclassOfTemplate<typename WTF::RemovePointer<T>::Type, > > > > GarbageCollected>::value > > > > || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, > > > > WeakMember>::Type, GarbageCollected>::value > > > > || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, > > > > RawPtr>::Type, GarbageCollected>::value, > > > > T> { > > > > > > > > > > > > > > }; > > > > > > > > but then I hit another error... > > > > > > > > If you have some solution in mind, would you mind uploading a CL? > > > > > > Here's my attempt -- https://codereview.chromium.org/388463002/ > > > > Thanks for the quick fix, looks correct to me! > > Probably fixing CrossThreadCopier to pass these pointers is not correct, because > such fix also passes the pointers in the arguments (i.e. not pointer to the > object to be called), which can cause thread unsafe code. > I'll create another CL soon. Isn't that comment covered by the RawPtr FIXME in CrossThreadTask.h already? i.e., the changes here seem no less unsafe than what's already in place. Or?
Message was sent while issue was closed.
On 2014/07/10 12:10:05, sof wrote: > On 2014/07/10 12:06:14, hiroshige wrote: > > On 2014/07/10 12:02:18, haraken wrote: > > > On 2014/07/10 11:59:33, sof wrote: > > > > On 2014/07/10 11:57:09, haraken wrote: > > > > > On 2014/07/10 11:50:36, hiroshige wrote: > > > > > > On 2014/07/10 11:31:58, haraken wrote: > > > > > > > On 2014/07/10 11:11:59, sof wrote: > > > > > > > > Looks like this broke Oilpan compilation, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan/... > > > > > > > > > > > > > > I'll handle this. > > > > > > > > > > > > Thanks! > > > > > > > > > > > > To call member function of class C (without AllowCrossThreadAccess()), > > my > > > > code > > > > > > only supports > > > > > > createCrossThreadTask(func,p,...) where func is a pointer to a member > > > > function > > > > > > of class C and > > > > > > p is pointer (C*) or WeakPtr<C>, but in Oilpan WeakMember or RawPtr is > > > > passed > > > > > > instead. > > > > > > (Lines 503--641 in Source/core/dom/CrossThreadTask.h above) > > > > > > > > > > > > A possible solution is to allow these code to catch everything (other > > than > > > > C* > > > > > > and WeakPtr<C>) > > > > > > to be passed without CrossThreadCopier if func is a pointer to a > member > > > > > > function, > > > > > > but not sure it is always correct. > > > > > > > > > > Hmm, this is complicated. > > > > > > > > > > I think we need to make the member function match: > > > > > > > > > > template<typename T> struct CrossThreadCopierBase<false, false, true, T> > { > > > > > // > > > > > true means the object type is GarbageCollected. > > > > > ...; > > > > > }; > > > > > > > > > > in order to use a correct CrossThreadCopier. So I added: > > > > > > > > > > template<typename T> struct CrossThreadCopier : public > > > > > CrossThreadCopierBase<WTF::IsConvertibleToInteger<T>::value, > > > > > WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, > > > > RefPtr>::Type, > > > > > ThreadSafeRefCounted>::value > > > > > || WTF::IsSubclassOfTemplate<typename > > > WTF::RemovePointer<T>::Type, > > > > > ThreadSafeRefCounted>::value > > > > > || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, > > > > > PassRefPtr>::Type, ThreadSafeRefCounted>::value, > > > > > WTF::IsSubclassOfTemplate<typename WTF::RemovePointer<T>::Type, > > > > > GarbageCollected>::value > > > > > || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, > > > > > WeakMember>::Type, GarbageCollected>::value > > > > > || WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, > > > > > RawPtr>::Type, GarbageCollected>::value, > > > > > T> { > > > > > > > > > > > > > > > > > > > > }; > > > > > > > > > > but then I hit another error... > > > > > > > > > > If you have some solution in mind, would you mind uploading a CL? > > > > > > > > Here's my attempt -- https://codereview.chromium.org/388463002/ > > > > > > Thanks for the quick fix, looks correct to me! > > > > Probably fixing CrossThreadCopier to pass these pointers is not correct, > because > > such fix also passes the pointers in the arguments (i.e. not pointer to the > > object to be called), which can cause thread unsafe code. > > I'll create another CL soon. > > Isn't that comment covered by the RawPtr FIXME in CrossThreadTask.h already? > i.e., the changes here seem no less unsafe than what's already in place. Or? Yeah. I have to update the comment. The updated version of the comment is that 'FIXME: Using a RawPtr is not safe because the RawPtr does not keep anything alive while the ExecutionContextTask is holding the RawPtr. However, we cannot use Member nor Persistent for some complicated reason, so we cannot avoid using a RawPtr. Thus the caller side must guarantee the safety of the RawPtr.' hiroshige-san: What do you mean by "thread unsafe" specifically?
Message was sent while issue was closed.
On 2014/07/10 12:16:55, haraken wrote: > On 2014/07/10 12:10:05, sof wrote: > > On 2014/07/10 12:06:14, hiroshige wrote: > > > On 2014/07/10 12:02:18, haraken wrote: > > > > On 2014/07/10 11:59:33, sof wrote: > > > > > On 2014/07/10 11:57:09, haraken wrote: > > > > > > On 2014/07/10 11:50:36, hiroshige wrote: > > > > > > > On 2014/07/10 11:31:58, haraken wrote: > > > > > > > > On 2014/07/10 11:11:59, sof wrote: > > > > > > > > > Looks like this broke Oilpan compilation, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan/... > > > > > > > > > > > > > > > > I'll handle this. > > > > > > > > > > > > > > Thanks! > > > > > > > > > > > > > > To call member function of class C (without > AllowCrossThreadAccess()), > > > my > > > > > code > > > > > > > only supports > > > > > > > createCrossThreadTask(func,p,...) where func is a pointer to a > member > > > > > function > > > > > > > of class C and > > > > > > > p is pointer (C*) or WeakPtr<C>, but in Oilpan WeakMember or RawPtr > is > > > > > passed > > > > > > > instead. > > > > > > > (Lines 503--641 in Source/core/dom/CrossThreadTask.h above) > > > > > > > > > > > > > > A possible solution is to allow these code to catch everything > (other > > > than > > > > > C* > > > > > > > and WeakPtr<C>) > > > > > > > to be passed without CrossThreadCopier if func is a pointer to a > > member > > > > > > > function, > > > > > > > but not sure it is always correct. > > > > > > > > > > > > Hmm, this is complicated. > > > > > > > > > > > > I think we need to make the member function match: > > > > > > > > > > > > template<typename T> struct CrossThreadCopierBase<false, false, true, > T> > > { > > > > > > > // > > > > > > true means the object type is GarbageCollected. > > > > > > ...; > > > > > > }; > > > > > > > > > > > > in order to use a correct CrossThreadCopier. So I added: > > > > > > > > > > > > template<typename T> struct CrossThreadCopier : public > > > > > > CrossThreadCopierBase<WTF::IsConvertibleToInteger<T>::value, > > > > > > WTF::IsSubclassOfTemplate<typename WTF::RemoveTemplate<T, > > > > > RefPtr>::Type, > > > > > > ThreadSafeRefCounted>::value > > > > > > || WTF::IsSubclassOfTemplate<typename > > > > WTF::RemovePointer<T>::Type, > > > > > > ThreadSafeRefCounted>::value > > > > > > || WTF::IsSubclassOfTemplate<typename > WTF::RemoveTemplate<T, > > > > > > PassRefPtr>::Type, ThreadSafeRefCounted>::value, > > > > > > WTF::IsSubclassOfTemplate<typename > WTF::RemovePointer<T>::Type, > > > > > > GarbageCollected>::value > > > > > > || WTF::IsSubclassOfTemplate<typename > WTF::RemoveTemplate<T, > > > > > > WeakMember>::Type, GarbageCollected>::value > > > > > > || WTF::IsSubclassOfTemplate<typename > WTF::RemoveTemplate<T, > > > > > > RawPtr>::Type, GarbageCollected>::value, > > > > > > T> { > > > > > > > > > > > > > > > > > > > > > > > > > > > }; > > > > > > > > > > > > but then I hit another error... > > > > > > > > > > > > If you have some solution in mind, would you mind uploading a CL? > > > > > > > > > > Here's my attempt -- https://codereview.chromium.org/388463002/ > > > > > > > > Thanks for the quick fix, looks correct to me! > > > > > > Probably fixing CrossThreadCopier to pass these pointers is not correct, > > because > > > such fix also passes the pointers in the arguments (i.e. not pointer to the > > > object to be called), which can cause thread unsafe code. > > > I'll create another CL soon. > > > > Isn't that comment covered by the RawPtr FIXME in CrossThreadTask.h already? > > i.e., the changes here seem no less unsafe than what's already in place. Or? > > Yeah. I have to update the comment. The updated version of the comment is that > 'FIXME: Using a RawPtr is not safe because the RawPtr does not keep anything > alive while the ExecutionContextTask is holding the RawPtr. However, we cannot > use Member nor Persistent for some complicated reason, so we cannot avoid using > a RawPtr. Thus the caller side must guarantee the safety of the RawPtr.' > > hiroshige-san: What do you mean by "thread unsafe" specifically? "Thread unsafe" I mentioned is to access RefCounted (or other reference counter that is not thread safe) from multiple threads, and passing objects or pointers that can cause such thread unsafe access to reference counters. I was afraid that the change made CrossThreadCopier to pass pointers that is not necessarily thread safe, but after looking at it carefully I found it seems safe (only passes e.g. garbage collected ones). Thanks for the fix! |
