Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2608783003:
Fix ThreadSafeAssociatedInterfacePtrProvider raciness. (Closed)
|
Created:
3 years, 11 months ago by Jay Civelli Modified:
3 years, 11 months ago CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), darin-cc_chromium.org, jam, leonhsl(Using Gerrit), mlamouri+watch-content_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix ThreadSafeAssociatedInterfacePtrProvider raciness.
Ensures the thread safe interface pointer returned by
ThreadSafeAssociatedInterfacePtrProvider can be called before the
interface pointer is actually bound.
BUG=675089
TEST=Run mojo_public_bindings_unittests
Review-Url: https://codereview.chromium.org/2608783003
Cr-Commit-Position: refs/heads/master@{#441981}
Committed: https://chromium.googlesource.com/chromium/src/+/35e5ff5b38c32d2bb9b03c2bc77911e7bf75efd1
Patch Set 1 #
Total comments: 3
Messages
Total messages: 25 (13 generated)
The CQ bit was checked by jcivelli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix ThreadSafeAssociatedInterfacePtrProvider raciness. Ensures the thread safe interface pointer returned by ThreadSafeAssociatedInterfacePtrProvider can be called before the interface pointer is actually bound. BUG=675089 TEST=Run mojo_public_bindings_unittests ========== to ========== Fix ThreadSafeAssociatedInterfacePtrProvider raciness. Ensures the thread safe interface pointer returned by ThreadSafeAssociatedInterfacePtrProvider can be called before the interface pointer is actually bound. BUG=675089 TEST=Run mojo_public_bindings_unittests ==========
jcivelli@chromium.org changed reviewers: + rockot@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2608783003/diff/1/content/renderer/mojo/threa... File content/renderer/mojo/thread_safe_associated_interface_ptr_provider.h (right): https://codereview.chromium.org/2608783003/diff/1/content/renderer/mojo/threa... content/renderer/mojo/thread_safe_associated_interface_ptr_provider.h:34: channel_proxy_->RetrieveAssociatedInterfaceOnIOThread<Interface>(base::Bind( I'm not sure I fully understand,, would you please help to clarify this question? Thanks. With changes of thread_safe_interface_ptr.h in this CL, here I suppose ChannelProxy::RetrieveAssociatedInterfaceOnIOThread() is required to post a task A to ipc task runner, and task A MUST call ThreadSafeAssociatedInterfacePtrProvider::BindInterfacePtr() *directly* there, and such scenario is not allowed? : "Task A posts to the same ipc task runner another task B to call BindInterfacePtr()."
https://codereview.chromium.org/2608783003/diff/1/content/renderer/mojo/threa... File content/renderer/mojo/thread_safe_associated_interface_ptr_provider.h (right): https://codereview.chromium.org/2608783003/diff/1/content/renderer/mojo/threa... content/renderer/mojo/thread_safe_associated_interface_ptr_provider.h:34: channel_proxy_->RetrieveAssociatedInterfaceOnIOThread<Interface>(base::Bind( On 2016/12/30 06:53:15, leonhsl wrote: > I'm not sure I fully understand,, would you please help to clarify this > question? Thanks. > With changes of thread_safe_interface_ptr.h in this CL, here I suppose > ChannelProxy::RetrieveAssociatedInterfaceOnIOThread() is required to post a task > A to ipc task runner, and task A MUST call > ThreadSafeAssociatedInterfacePtrProvider::BindInterfacePtr() *directly* there, > and such scenario is not allowed? : "Task A posts to the same ipc task runner > another task B to call BindInterfacePtr()." I am not sure I fully understand the question. The IPC task runner runs on the IO thread, so posting the bind to the IPC task runner guarantees any calls made on the ThreadSafeAssociatedInterfacePtr (once it has been returned) will be posted to the same task runner and will happen after the bind, on the IO thread.
leon.han@intel.com changed reviewers: + leon.han@intel.com
https://codereview.chromium.org/2608783003/diff/1/content/renderer/mojo/threa... File content/renderer/mojo/thread_safe_associated_interface_ptr_provider.h (right): https://codereview.chromium.org/2608783003/diff/1/content/renderer/mojo/threa... content/renderer/mojo/thread_safe_associated_interface_ptr_provider.h:34: channel_proxy_->RetrieveAssociatedInterfaceOnIOThread<Interface>(base::Bind( On 2017/01/03 16:40:56, Jay Civelli wrote: > On 2016/12/30 06:53:15, leonhsl wrote: > > I'm not sure I fully understand,, would you please help to clarify this > > question? Thanks. > > With changes of thread_safe_interface_ptr.h in this CL, here I suppose > > ChannelProxy::RetrieveAssociatedInterfaceOnIOThread() is required to post a > task > > A to ipc task runner, and task A MUST call > > ThreadSafeAssociatedInterfacePtrProvider::BindInterfacePtr() *directly* there, > > and such scenario is not allowed? : "Task A posts to the same ipc task runner > > another task B to call BindInterfacePtr()." > I am not sure I fully understand the question. > The IPC task runner runs on the IO thread, so posting the bind to the IPC task > runner guarantees any calls made on the ThreadSafeAssociatedInterfacePtr (once > it has been returned) will be posted to the same task runner and will happen > after the bind, on the IO thread. > So I suppose the following scenario maybe is not allowed. After CreateUnbound returned, post a task A to the ipc task runner, but task A does not execute BindInterfacePtr directly, instead it posts task B to the same ipc task runner to execute BindInterfacePtr. In such case the returned ThreadSafeAssociatedInterfacePtr from CreateUnbound() still can not be used to make any calls at once, is my understanding correct? If so I think it'd better add some explicit comments about this restriction around CreateUnbound().
On Tue, Jan 3, 2017 at 7:35 PM, <leon.han@intel.com> wrote: > > https://codereview.chromium.org/2608783003/diff/1/content/ > renderer/mojo/thread_safe_associated_interface_ptr_provider.h > File > content/renderer/mojo/thread_safe_associated_interface_ptr_provider.h > (right): > > https://codereview.chromium.org/2608783003/diff/1/content/ > renderer/mojo/thread_safe_associated_interface_ptr_provider.h#newcode34 > content/renderer/mojo/thread_safe_associated_interface_ptr_provider.h:34: > channel_proxy_->RetrieveAssociatedInterfaceOnI > OThread<Interface>(base::Bind( > On 2017/01/03 16:40:56, Jay Civelli wrote: > > On 2016/12/30 06:53:15, leonhsl wrote: > > > I'm not sure I fully understand,, would you please help to clarify > this > > > question? Thanks. > > > With changes of thread_safe_interface_ptr.h in this CL, here I > suppose > > > ChannelProxy::RetrieveAssociatedInterfaceOnIOThread() is required to > post a > > task > > > A to ipc task runner, and task A MUST call > > > ThreadSafeAssociatedInterfacePtrProvider::BindInterfacePtr() > *directly* there, > > > and such scenario is not allowed? : "Task A posts to the same ipc > task runner > > > another task B to call BindInterfacePtr()." > > I am not sure I fully understand the question. > > The IPC task runner runs on the IO thread, so posting the bind to the > IPC task > > runner guarantees any calls made on the > ThreadSafeAssociatedInterfacePtr (once > > it has been returned) will be posted to the same task runner and will > happen > > after the bind, on the IO thread. > > > > So I suppose the following scenario maybe is not allowed. > After CreateUnbound returned, post a task A to the ipc task runner, but > task A does not execute BindInterfacePtr directly, instead it posts task > B to the same ipc task runner to execute BindInterfacePtr. In such case > the returned ThreadSafeAssociatedInterfacePtr from CreateUnbound() still > can not be used to make any calls at once, is my understanding correct? > If so I think it'd better add some explicit comments about this > restriction around CreateUnbound(). > Yes, that case would not be supported, but I am curious if you have a scenario in mind where task A would post a task again instead of calling Bind directly. The comment for CreateUnbound() mentions posting a task on the IO thread to do the bind, which means that task has to call Bind directly (as it posting again would be an unusual case in my opinion). -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
The CQ bit was checked by jcivelli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
jcivelli@chromium.org changed reviewers: + jam@chromium.org
Adding @jam for content/ review.
lgtm
The CQ bit was checked by jcivelli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1483726569743980, "parent_rev":
"1b34593a53181b7f8a06a97206219c16486b33a1", "commit_rev":
"35e5ff5b38c32d2bb9b03c2bc77911e7bf75efd1"}
Message was sent while issue was closed.
Description was changed from ========== Fix ThreadSafeAssociatedInterfacePtrProvider raciness. Ensures the thread safe interface pointer returned by ThreadSafeAssociatedInterfacePtrProvider can be called before the interface pointer is actually bound. BUG=675089 TEST=Run mojo_public_bindings_unittests ========== to ========== Fix ThreadSafeAssociatedInterfacePtrProvider raciness. Ensures the thread safe interface pointer returned by ThreadSafeAssociatedInterfacePtrProvider can be called before the interface pointer is actually bound. BUG=675089 TEST=Run mojo_public_bindings_unittests Review-Url: https://codereview.chromium.org/2608783003 Cr-Commit-Position: refs/heads/master@{#441981} Committed: https://chromium.googlesource.com/chromium/src/+/35e5ff5b38c32d2bb9b03c2bc779... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/35e5ff5b38c32d2bb9b03c2bc779... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
