WeakPtrFactory::GetWeakPtr() is not thread safe |
||
Issue descriptionThanks to zijiehe@'s for pointing out this problem. WeakPtrFactory::GetWeakPtr() doesn't seem to be thread safe nor providing thread consistency check. When calling GetWeakPtr() on different thread, there may be race condition in WeakReferenceOwner::GetRef() such that the refcounted |flag_| may be created twice. There is a pending CL calling out the lack of thread safety of GetWeakPtr(): 1602443003 For remoting code base there are code that call GetWeakPtr() on multiple threads, such as JniDisplayHandler. We need to call WeakPtrFactory::GetWeakPtr() and store the weak pointer in their constructors rather than calling it every time we need the weak pointer.
,
Jul 12 2016
I think this should be a bug for tracking problematic GetWeakPtr() calls in Chromoting...
,
Jul 13 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bdbc56ba8839e6bb32a3677bf199234641f36806 commit bdbc56ba8839e6bb32a3677bf199234641f36806 Author: yuweih <yuweih@chromium.org> Date: Wed Jul 13 03:17:42 2016 [Remoting Android] Fix GetWeakPtr() calls on multiple threads According to recent discovery of the thread safety issue of WeakPtrFactory, WeakptrFactory::GetWeakPtr() can't be called on multiple threads. This CL makes Android JNI classes that expose GetWeakPtr() function to store the WeakPtr in constructor and simply return the stored WeakPtr in GetWeakPtr(). BUG= 627602 Review-Url: https://codereview.chromium.org/2142253002 Cr-Commit-Position: refs/heads/master@{#404948} [modify] https://crrev.com/bdbc56ba8839e6bb32a3677bf199234641f36806/remoting/client/jni/chromoting_jni_instance.cc [modify] https://crrev.com/bdbc56ba8839e6bb32a3677bf199234641f36806/remoting/client/jni/chromoting_jni_instance.h [modify] https://crrev.com/bdbc56ba8839e6bb32a3677bf199234641f36806/remoting/client/jni/jni_client.cc [modify] https://crrev.com/bdbc56ba8839e6bb32a3677bf199234641f36806/remoting/client/jni/jni_client.h [modify] https://crrev.com/bdbc56ba8839e6bb32a3677bf199234641f36806/remoting/client/jni/jni_display_handler.cc [modify] https://crrev.com/bdbc56ba8839e6bb32a3677bf199234641f36806/remoting/client/jni/jni_display_handler.h [modify] https://crrev.com/bdbc56ba8839e6bb32a3677bf199234641f36806/remoting/client/jni/jni_pairing_secret_fetcher.cc [modify] https://crrev.com/bdbc56ba8839e6bb32a3677bf199234641f36806/remoting/client/jni/jni_pairing_secret_fetcher.h
,
Jul 13 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bdbc56ba8839e6bb32a3677bf199234641f36806 commit bdbc56ba8839e6bb32a3677bf199234641f36806 Author: yuweih <yuweih@chromium.org> Date: Wed Jul 13 03:17:42 2016 [Remoting Android] Fix GetWeakPtr() calls on multiple threads According to recent discovery of the thread safety issue of WeakPtrFactory, WeakptrFactory::GetWeakPtr() can't be called on multiple threads. This CL makes Android JNI classes that expose GetWeakPtr() function to store the WeakPtr in constructor and simply return the stored WeakPtr in GetWeakPtr(). BUG= 627602 Review-Url: https://codereview.chromium.org/2142253002 Cr-Commit-Position: refs/heads/master@{#404948} [modify] https://crrev.com/bdbc56ba8839e6bb32a3677bf199234641f36806/remoting/client/jni/chromoting_jni_instance.cc [modify] https://crrev.com/bdbc56ba8839e6bb32a3677bf199234641f36806/remoting/client/jni/chromoting_jni_instance.h [modify] https://crrev.com/bdbc56ba8839e6bb32a3677bf199234641f36806/remoting/client/jni/jni_client.cc [modify] https://crrev.com/bdbc56ba8839e6bb32a3677bf199234641f36806/remoting/client/jni/jni_client.h [modify] https://crrev.com/bdbc56ba8839e6bb32a3677bf199234641f36806/remoting/client/jni/jni_display_handler.cc [modify] https://crrev.com/bdbc56ba8839e6bb32a3677bf199234641f36806/remoting/client/jni/jni_display_handler.h [modify] https://crrev.com/bdbc56ba8839e6bb32a3677bf199234641f36806/remoting/client/jni/jni_pairing_secret_fetcher.cc [modify] https://crrev.com/bdbc56ba8839e6bb32a3677bf199234641f36806/remoting/client/jni/jni_pairing_secret_fetcher.h |
||
►
Sign in to add a comment |
||
Comment 1 by dah...@chromium.org
, Jul 12 2016