New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 612944 link

Starred by 1 user
Status: Fixed
Owner:
█ roc...@chromium.org
please use my google.com address
Closed: May 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 612358


Sign in to add a comment

Enable thread-safe send for ChannelMojo

Project Member Reported by roc...@chromium.org, May 18 2016 
ChannelMojo is thread-safe, which means ChannelProxy is allowed to call Send from any thread rather than requiring a thread-hop first.

We cannot take advantage of this because a bunch of code today assumes ChannelProxy::Send will always post a task to the IO thread runner, leading to subtle races wherever such assumptions are made.

For now, ChannelMojo will be marked as not thread-safe to avoid these races.

This bug tracks progress toward re-enabling thread-safe send.
Project Member

Comment 1 by bugdroid1@chromium.org, May 18 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ff0d8038dd81bb025313a393cb8a6fcde9a79603

commit ff0d8038dd81bb025313a393cb8a6fcde9a79603
Author: rockot <rockot@chromium.org>
Date: Wed May 18 22:03:37 2016

Disable thread-safe Send on ChannelMojo

The world is not yet ready for thread-safe Send. D:

BUG= 612944 
TBR=jam@chromium.org

Review-Url: https://codereview.chromium.org/1991913003
Cr-Commit-Position: refs/heads/master@{#394568}

[modify] https://crrev.com/ff0d8038dd81bb025313a393cb8a6fcde9a79603/ipc/mojo/ipc_channel_mojo.cc

Comment 2 by kbr@chromium.org, May 23 2016

Blockedon: 612358

Comment 3 by roc...@chromium.org, May 23 2016 

The plan is to re-enable thread-safe sending without changing the behavior of existing Send calls. Callers will have to opt into the new behavior.

 Issue 612358  is already resolved, so I don't understand why it's been added as a blocker.
Project Member

Comment 4 by bugdroid1@chromium.org, May 25 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/868f89e764ab0e807adb237f0cbaf79ca5f49257

commit 868f89e764ab0e807adb237f0cbaf79ca5f49257
Author: rockot <rockot@chromium.org>
Date: Wed May 25 16:25:45 2016

Send input event IPCs directly from the UI thread

This CL:

1. Adds SendNow and SendOnIOThread to ChannelProxy
  2a. SendNow sends immediately from the current thread if the
      underlying Channel implementation claims to have a
      thread-safe Send.
  2b. SendOnIOThread is simply a more explicit alias for Send and
      and does not acknowledge the thread-safety of the
      underlying Channel implementation.
2. Flags ChannelMojo for thread-safe Send once again
3. Adds GetImmediateSender and GetIOThreadSender interfaces to
   RenderProcessHost. These are safe alternatives to using its
   IPC::Sender implementation directly and each corresponds to
   SendNow or SendOnIOThread behavior, respectively.
4. Changes RenderWidgetHostImpl so that the input router uses
   the RPH's immediate Sender interface.

The net result here is that input events are now sent to renderers
directly from the UI thread, and we have a reusable path forward
for porting more IPCs to the SendNow interface.

BUG= 612944 
TBR=jam@chromium.org

Review-Url: https://codereview.chromium.org/1991323002
Cr-Commit-Position: refs/heads/master@{#395906}

[modify] https://crrev.com/868f89e764ab0e807adb237f0cbaf79ca5f49257/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/868f89e764ab0e807adb237f0cbaf79ca5f49257/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/868f89e764ab0e807adb237f0cbaf79ca5f49257/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/868f89e764ab0e807adb237f0cbaf79ca5f49257/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/868f89e764ab0e807adb237f0cbaf79ca5f49257/content/public/browser/render_process_host.h
[modify] https://crrev.com/868f89e764ab0e807adb237f0cbaf79ca5f49257/content/public/test/mock_render_process_host.cc
[modify] https://crrev.com/868f89e764ab0e807adb237f0cbaf79ca5f49257/content/public/test/mock_render_process_host.h
[modify] https://crrev.com/868f89e764ab0e807adb237f0cbaf79ca5f49257/ipc/ipc_channel_proxy.cc
[modify] https://crrev.com/868f89e764ab0e807adb237f0cbaf79ca5f49257/ipc/ipc_channel_proxy.h
[modify] https://crrev.com/868f89e764ab0e807adb237f0cbaf79ca5f49257/ipc/ipc_sync_channel.cc
[modify] https://crrev.com/868f89e764ab0e807adb237f0cbaf79ca5f49257/ipc/ipc_sync_channel.h
[modify] https://crrev.com/868f89e764ab0e807adb237f0cbaf79ca5f49257/ipc/ipc_sync_message_filter.cc
[modify] https://crrev.com/868f89e764ab0e807adb237f0cbaf79ca5f49257/ipc/ipc_sync_message_filter.h
[modify] https://crrev.com/868f89e764ab0e807adb237f0cbaf79ca5f49257/ipc/mojo/ipc_channel_mojo.cc

Comment 5 by roc...@chromium.org, May 25 2016

Status: Fixed (was: Assigned)
This is now turned on but doesn't affect the behavior of existing Send calls by default. We can convert callers individually if/when it seems worthwhile to do so.
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 16 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f14a8ae8f52b814820b29752996c5cee903a8d4d

commit f14a8ae8f52b814820b29752996c5cee903a8d4d
Author: rockot <rockot@chromium.org>
Date: Thu Jun 16 19:28:41 2016

Revert "Send input event IPCs directly from the UI thread"

This reverts commit 868f89e764ab0e807adb237f0cbaf79ca5f49257.
See https://codereview.chromium.org/1991323002 for the original
CL.

Per offline discussions: there was no clear benefit from
using SendNow for input events, and having the extra interface
adds unnecessary complexity given that nobody is going to work
on porting Send call sites.

BUG= 612944 
R=jam@chromium.org

Review-Url: https://codereview.chromium.org/2068343003
Cr-Commit-Position: refs/heads/master@{#400227}

[modify] https://crrev.com/f14a8ae8f52b814820b29752996c5cee903a8d4d/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/f14a8ae8f52b814820b29752996c5cee903a8d4d/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/f14a8ae8f52b814820b29752996c5cee903a8d4d/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/f14a8ae8f52b814820b29752996c5cee903a8d4d/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/f14a8ae8f52b814820b29752996c5cee903a8d4d/content/public/browser/render_process_host.h
[modify] https://crrev.com/f14a8ae8f52b814820b29752996c5cee903a8d4d/content/public/test/mock_render_process_host.cc
[modify] https://crrev.com/f14a8ae8f52b814820b29752996c5cee903a8d4d/content/public/test/mock_render_process_host.h
[modify] https://crrev.com/f14a8ae8f52b814820b29752996c5cee903a8d4d/ipc/ipc_channel_proxy.cc
[modify] https://crrev.com/f14a8ae8f52b814820b29752996c5cee903a8d4d/ipc/ipc_channel_proxy.h
[modify] https://crrev.com/f14a8ae8f52b814820b29752996c5cee903a8d4d/ipc/ipc_sync_channel.cc
[modify] https://crrev.com/f14a8ae8f52b814820b29752996c5cee903a8d4d/ipc/ipc_sync_channel.h
[modify] https://crrev.com/f14a8ae8f52b814820b29752996c5cee903a8d4d/ipc/ipc_sync_message_filter.cc
[modify] https://crrev.com/f14a8ae8f52b814820b29752996c5cee903a8d4d/ipc/ipc_sync_message_filter.h
[modify] https://crrev.com/f14a8ae8f52b814820b29752996c5cee903a8d4d/ipc/mojo/ipc_channel_mojo.cc
►

Sign in to add a comment