[cros] Make g_browser_process's call on the UI thread.

[cros] Make g_browser_process's call on the UI thread.

BUG=chromium-os:23417
TEST=browser_tests doesn't fail


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111544

[Dmitry Polukhin, 2011-11-24 13:31:17]

LGTM

[commit-bot: I haz the power, 2011-11-24 13:39:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/altimofeev@chromium.org/8688001/1

[commit-bot: I haz the power, 2011-11-24 16:18:58]

Try job failure for 8688001-1 (retry) on mac_rel for step "ui_tests" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2011-11-24 16:54:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/altimofeev@chromium.org/8688001/1

[commit-bot: I haz the power, 2011-11-24 18:36:42]

Change committed as 111544

[stevenjb, 2011-11-28 18:45:39]

http://codereview.chromium.org/8688001/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_web_socket_proxy_private_api.cc
(right):

http://codereview.chromium.org/8688001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_web_socket_proxy_private_api.cc:66:
IOThread* io_thread = g_browser_process->io_thread();
One reason that g_browser_process->io_thread() is disallowed from other threads
is that access to the IOThread members are not inherently thread safe. This code
as written could cause a subtle shutdown crash when accessing io_thread from
ResolveHostIOPart().

Furthermore, resolver_ could get destroyed on shutdown after it is reset and
before it is accessed in ResolveHostIOPart() during shutdown. 

resolver_->Resolve() itself does not appear to be blocking, so I'm not sure why
it needs to be called from the IO thread? If it does, resolver_ should get set
on the UI thread, and some mechanism added to prevent resolver_ from being
deleted before it is accessed from the IO thread.

[altimofeev, 2011-11-29 12:30:20]

Steven, thank you for your comments.

http://codereview.chromium.org/8688001/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_web_socket_proxy_private_api.cc
(right):

http://codereview.chromium.org/8688001/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_web_socket_proxy_private_api.cc:66:
IOThread* io_thread = g_browser_process->io_thread();
Please find my answers inline.

On 2011/11/28 18:45:39, Steven Bennetts wrote:
> One reason that g_browser_process->io_thread() is disallowed from other
threads
> is that access to the IOThread members are not inherently thread safe. This
code
> as written could cause a subtle shutdown crash when accessing io_thread from
> ResolveHostIOPart().

Few points because of which I think the code is correct:
1. There is 'DCHECK(Currently on IO thread)' in IOThread::globals()  getter. So
it is designed to be called on IOThread.
2. In case IOThread was deleted, PostMessage prevents us from running task on IO
thread, thus avoiding usage of the invalid pointer.
3. There are places in the code, where we are passing io_thread pointer (e.g.
profiles code)

Anyway, I'll be glad to hear the right way of getting host_resolver.

> Furthermore, resolver_ could get destroyed on shutdown after it is reset and
> before it is accessed in ResolveHostIOPart() during shutdown. 

I think it cannot. The reason is that WebSocketProxyPrivate inherits from
RefCountedThreadSafe. And there is AddRef() call in RunImpl() method, which is
compensated only in Finalize() method.
 
> resolver_->Resolve() itself does not appear to be blocking, so I'm not sure
why
> it needs to be called from the IO thread? If it does, resolver_ should get set
> on the UI thread, and some mechanism added to prevent resolver_ from being
> deleted before it is accessed from the IO thread.

I haven't found any DCHECKs, but since host_resolver belongs to the IO thread,
it is logically correct to use it on the IO thread too.

[stevenjb (google-dont-use), 2011-11-29 19:59:24]

Yeah, sorry that some of my comments were based on my lack of knowledge of
the system. I understand it better now and made a few comments on the new
CL that might help clarify things, but in general lgtm.

On Tue, Nov 29, 2011 at 4:30 AM, <altimofeev@chromium.org> wrote:

> Steven, thank you for your comments.
>
>
>
> http://codereview.chromium.**org/8688001/diff/1/chrome/**
>
browser/extensions/extension_**web_socket_proxy_private_api.**cc<http://codereview.chromium.org/8688001/diff/1/chrome/browser/extensions/extension_web_socket_proxy_private_api.cc>
> File chrome/browser/extensions/**extension_web_socket_proxy_**
> private_api.cc
> (right):
>
> http://codereview.chromium.**org/8688001/diff/1/chrome/**
>
browser/extensions/extension_**web_socket_proxy_private_api.**cc#newcode66<http://codereview.chromium.org/8688001/diff/1/chrome/browser/extensions/extension_web_socket_proxy_private_api.cc#newcode66>
> chrome/browser/extensions/**extension_web_socket_proxy_**
> private_api.cc:66:
> IOThread* io_thread = g_browser_process->io_thread()**;
> Please find my answers inline.
>
>
> On 2011/11/28 18:45:39, Steven Bennetts wrote:
>
>> One reason that g_browser_process->io_thread() is disallowed from
>>
> other threads
>
>> is that access to the IOThread members are not inherently thread safe.
>>
> This code
>
>> as written could cause a subtle shutdown crash when accessing
>>
> io_thread from
>
>> ResolveHostIOPart().
>>
>
> Few points because of which I think the code is correct:
> 1. There is 'DCHECK(Currently on IO thread)' in IOThread::globals()
> getter. So it is designed to be called on IOThread.
> 2. In case IOThread was deleted, PostMessage prevents us from running
> task on IO thread, thus avoiding usage of the invalid pointer.
> 3. There are places in the code, where we are passing io_thread pointer
> (e.g. profiles code)
>
> Anyway, I'll be glad to hear the right way of getting host_resolver.
>
>
>  Furthermore, resolver_ could get destroyed on shutdown after it is
>>
> reset and
>
>> before it is accessed in ResolveHostIOPart() during shutdown.
>>
>
> I think it cannot. The reason is that WebSocketProxyPrivate inherits
> from RefCountedThreadSafe. And there is AddRef() call in RunImpl()
> method, which is compensated only in Finalize() method.
>
>
>  resolver_->Resolve() itself does not appear to be blocking, so I'm not
>>
> sure why
>
>> it needs to be called from the IO thread? If it does, resolver_ should
>>
> get set
>
>> on the UI thread, and some mechanism added to prevent resolver_ from
>>
> being
>
>> deleted before it is accessed from the IO thread.
>>
>
> I haven't found any DCHECKs, but since host_resolver belongs to the IO
> thread, it is logically correct to use it on the IO thread too.
>
>
http://codereview.chromium.**org/8688001/<http://codereview.chromium.org/8688...
>