[NOT FOR COMMIT] Hacks to merge render compositor thread with UI thread

[NOT FOR COMMIT] Hacks to merge render compositor thread with UI thread

Alex is right, this does mostly "just work"

Hacks involved:
* Allow UI thread to wait on condition variables. Individual fixes in PS2. Alex says the command buffer will be rewritten for the merged thread case to not block.
* Do not create a SyncChannel. This breaks all function related to sync messages like find in page, capture picture etc. Depreate sync find in page, capture picture will be rewritten for merged thread case.

Here is a hang/deadlock somewhere. Easier to reproduce with AwSettingsTest#testBlockNetworkImagesBlocksNetworkImageAndReloadInPlace; hangs about once every 20 runs.


To get merged threads:
Apply Patch Set 3 in this patch

Failing with timeout:
ClientOnReceivedErrorTest#testOnReceivedErrorOnInvalidUrl

Already landed patches:
Apply crrev.com/12386078
Apply crrev.com/12383056
Apply crrev.com/12639002
Apply crrev.com/12383030

BUG=

[boliu, 2013-02-27 20:10:09]

Building android to see how this actually works in apps

https://codereview.chromium.org/12356002/diff/1/chrome/browser/metrics/thread...
File chrome/browser/metrics/thread_watcher.h (right):

https://codereview.chromium.org/12356002/diff/1/chrome/browser/metrics/thread...
chrome/browser/metrics/thread_watcher.h:408: static void StartWatching(
Oops, this has nothing to do with this patch.

[Leandro Graciá Gil, 2013-02-27 20:52:47]

On 2013/02/27 20:10:09, boliu wrote:
> Building android to see how this actually works in apps
> 
>
https://codereview.chromium.org/12356002/diff/1/chrome/browser/metrics/thread...
> File chrome/browser/metrics/thread_watcher.h (right):
> 
>
https://codereview.chromium.org/12356002/diff/1/chrome/browser/metrics/thread...
> chrome/browser/metrics/thread_watcher.h:408: static void StartWatching(
> Oops, this has nothing to do with this patch.

We do need to create a sync channel for synchronous find-in-page. What we need
to do in that case is to prevent compositor commits (or anything else
deadlocking) from happening while a synchronous find-in-page operation is
ongoing.

[boliu, 2013-02-27 22:24:17]

On 2013/02/27 20:52:47, Leandro Graciá Gil wrote:
> We do need to create a sync channel for synchronous find-in-page. What we need
> to do in that case is to prevent compositor commits (or anything else
> deadlocking) from happening while a synchronous find-in-page operation is
> ongoing.

Sorry, I was not suggesting that we stop using SyncChannel. Alex already pointed
out to me the place where it's deadlocking and I'll work on that next.

I'm just listing the things are broken/need fixes here after we merge the
threads. And admittedly admittedly the list is not that long.

[boliu, 2013-02-28 02:37:40]

About sync ipcs: It's not enough to "prevent compositor commits". By the time
findAll is called on the UI thread, webkit maybe already be blocked on the
impl/UI thread. Resolving this case would involve major acrobatics to make it
work. Already considered:

1) Adding ability to back out of all synchronous compositor calls to impl thread
2) Make webkit->impl thread interaction asynchronous
3) Some how unblock the compositor first by pulling out the synchronous task
before sending the ipc

All seems to hacky so right now the preference we (joth, alex, me) decided is to
break synchrous find by returning 0 and using async version instead.

I'm going ignore this issue for now and continue working on other issues.

[boliu, 2013-03-01 00:19:12]

So patch set 2 contains all the new ScopedAllowWait's that need to be added to
*run* all the webview tests. This means I might have missed sync messages that
were not/rarely sent during tests.

There is a deadlock that happens about 1/50 times, not sure if it's the one I
mentioned in gpu_message_filter.cc yet.

https://codereview.chromium.org/12356002/diff/7001/android_webview/browser/sc...
File android_webview/browser/scoped_allow_wait_for_legacy_web_view_api.h (left):

https://codereview.chromium.org/12356002/diff/7001/android_webview/browser/sc...
android_webview/browser/scoped_allow_wait_for_legacy_web_view_api.h:10: #include
"base/logging.h"
nothign to do with this patch set

https://codereview.chromium.org/12356002/diff/7001/content/browser/renderer_h...
File content/browser/renderer_host/gpu_message_filter.cc (right):

https://codereview.chromium.org/12356002/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/gpu_message_filter.cc:154: // TODO(boliu): This
will deadlock with threads merged. Can this ever
So GpuHostMsg_CreateViewCommandBuffer is sent from UI thread, so if this is
waiting on the UI thread, that would be a deadlock.

Can this happen?

https://codereview.chromium.org/12356002/diff/7001/content/common/gpu/client/...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/12356002/diff/7001/content/common/gpu/client/...
content/common/gpu/client/command_buffer_proxy_impl.cc:154: if (!Send(new
GpuCommandBufferMsg_Initialize(route_id_, handle, &result))) {
handler does not need DELAYED_RESPONSE, so should be ok?

https://codereview.chromium.org/12356002/diff/7001/content/common/gpu/client/...
content/common/gpu/client/command_buffer_proxy_impl.cc:175: if (Send(new
GpuCommandBufferMsg_GetState(route_id_, &state)))
Again doesn't really need DELAYED_RESPONSE except for using it to send back an
error, so should be ok?

https://codereview.chromium.org/12356002/diff/7001/content/common/gpu/client/...
content/common/gpu/client/command_buffer_proxy_impl.cc:225: if (Send(new
GpuCommandBufferMsg_GetStateFast(route_id_,
I changed handler to not use DELAYED_RESPONSE

https://codereview.chromium.org/12356002/diff/7001/content/common/gpu/client/...
content/common/gpu/client/command_buffer_proxy_impl.cc:243: Send(new
GpuCommandBufferMsg_SetGetBuffer(route_id_, shm_id));
Changed handler to not use DELAYED_RESPONSE

https://codereview.chromium.org/12356002/diff/7001/content/common/gpu/gpu_com...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/12356002/diff/7001/content/common/gpu/gpu_com...
content/common/gpu/gpu_command_buffer_stub.cc:40: #include
"base/threading/thread_restrictions.h"
oops, no needed

https://codereview.chromium.org/12356002/diff/7001/content/common/gpu/gpu_com...
content/common/gpu/gpu_command_buffer_stub.cc:339: void
GpuCommandBufferStub::OnInitialize(
This doesn't really need flexibility of DELAYED_RESPONSE

https://codereview.chromium.org/12356002/diff/7001/content/renderer/render_th...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/12356002/diff/7001/content/renderer/render_th...
content/renderer/render_thread_impl.cc:774:
base::ThreadRestrictions::ScopedAllowWait allow_wait;
This one just allocates shared mem and does not use DELAYED_RESPONSE, should be
ok.

https://codereview.chromium.org/12356002/diff/7001/content/renderer/render_th...
content/renderer/render_thread_impl.cc:1016:
base::ThreadRestrictions::ScopedAllowWait allow_wait;
potential deadlock? See comment in gpu_message_filter.cc

[boliu, 2013-03-05 01:52:06]

3-file change for the additional hacks to get merged threads, but have to apply
a series of yet-to-land "real" patches first. Instruction in the description

[boliu, 2013-03-07 18:28:26]

https://codereview.chromium.org/12356002/diff/7003/android_webview/lib/main/a...
File android_webview/lib/main/aw_main_delegate.cc (right):

https://codereview.chromium.org/12356002/diff/7003/android_webview/lib/main/a...
android_webview/lib/main/aw_main_delegate.cc:88:
content::BrowserThread::UnsafeGetMessageLoopForThread(
Just realized this has been returning null since code runs before any message
loops are started.

AFAICT this has always been the case, although I swear it was triggering
ThreadRestriction checks on the UI thread when I was hacking this...

Going to look for a better (ie functional) way to do this

[boliu, 2013-03-07 19:04:45]

https://codereview.chromium.org/12356002/diff/7003/android_webview/lib/main/a...
File android_webview/lib/main/aw_main_delegate.cc (right):

https://codereview.chromium.org/12356002/diff/7003/android_webview/lib/main/a...
android_webview/lib/main/aw_main_delegate.cc:88:
content::BrowserThread::UnsafeGetMessageLoopForThread(
On 2013/03/07 18:28:26, boliu wrote:
> Just realized this has been returning null since code runs before any message
> loops are started.
> 
> AFAICT this has always been the case, although I swear it was triggering
> ThreadRestriction checks on the UI thread when I was hacking this...
> 
> Going to look for a better (ie functional) way to do this

Fix here: crrev.com/12639002