Make AsyncHandleWaiter reenterancy-safe

Make AsyncHandleWaiter reenterancy-safe

On Windows, message pump can re-enter. AsyncHandleWaiter should
be prepared for that.

R=viettrungluu@chromium.org
BUG=none
TEST=ExtensionApiTest.SharedModuleWhitelist (this hits the DCHECK)

Committed: https://crrev.com/a8c3c7dfa409152914df3363a931f7ed1348c760
Cr-Commit-Position: refs/heads/master@{#319202}

[Hajime Morrita, 2015-03-03 20:48:15]

Trung: PTAL? This is blocking https://codereview.chromium.org/960693003/

[viettrungluu, 2015-03-03 22:03:33]

https://codereview.chromium.org/973213002/diff/20001/ipc/mojo/async_handle_wa...
File ipc/mojo/async_handle_waiter.cc (right):

https://codereview.chromium.org/973213002/diff/20001/ipc/mojo/async_handle_wa...
ipc/mojo/async_handle_waiter.cc:70: return 0 < processing_;
nit: "processing_ > 0" would probably be more readable for most people

https://codereview.chromium.org/973213002/diff/20001/ipc/mojo/async_handle_wa...
ipc/mojo/async_handle_waiter.cc:93: if (1 < processing_) {
processing_ > 1

https://codereview.chromium.org/973213002/diff/20001/ipc/mojo/async_handle_wa...
ipc/mojo/async_handle_waiter.cc:95: return;
Why do we not want to do any of the stuff below in this case?

https://codereview.chromium.org/973213002/diff/20001/ipc/mojo/async_handle_wa...
ipc/mojo/async_handle_waiter.cc:123: int processing_;
nit: Maybe rename this to something slightly more descriptive, now that it's no
longer a bool?

(I'm not sure what though. process_io_event_enter_count_?
processing_enter_count_?)

[viettrungluu, 2015-03-03 22:04:29]

(Also, how hard would it be to write a more minimal test, rather than relying on
some incidental external test?)

[Hajime Morrita, 2015-03-03 22:12:49]

On 2015/03/03 22:04:29, viettrungluu wrote:
> (Also, how hard would it be to write a more minimal test, rather than relying
on
> some incidental external test?)
Well, I hasted to do this as this is Windows only behavior. But probably I
should do. Wait a minute.

[Hajime Morrita, 2015-03-04 23:16:09]

Found that if needs a real UI message loop to hit the case,
which is hard to exercise. 

Instead, I exposed UIHandler to the test and call it 
so that we can reproduce the call sequence without the
MessageLoop instance. Turned out it allows us to write
fine grained tests in an intuitive way.

https://codereview.chromium.org/973213002/diff/20001/ipc/mojo/async_handle_wa...
File ipc/mojo/async_handle_waiter.cc (right):

https://codereview.chromium.org/973213002/diff/20001/ipc/mojo/async_handle_wa...
ipc/mojo/async_handle_waiter.cc:93: if (1 < processing_) {
On 2015/03/03 22:03:33, viettrungluu wrote:
> processing_ > 1

Done.

https://codereview.chromium.org/973213002/diff/20001/ipc/mojo/async_handle_wa...
ipc/mojo/async_handle_waiter.cc:95: return;
On 2015/03/03 22:03:33, viettrungluu wrote:
> Why do we not want to do any of the stuff below in this case?
It can cause nested IO callback, which isn't what we want here.
It would be safe to let the outermost handler take it.

https://codereview.chromium.org/973213002/diff/20001/ipc/mojo/async_handle_wa...
ipc/mojo/async_handle_waiter.cc:123: int processing_;
On 2015/03/03 22:03:33, viettrungluu wrote:
> nit: Maybe rename this to something slightly more descriptive, now that it's
no
> longer a bool?
> 
> (I'm not sure what though. process_io_event_enter_count_?
> processing_enter_count_?)

Done.

[viettrungluu, 2015-03-05 01:10:46]

LGTM w/nit.

https://codereview.chromium.org/973213002/diff/40001/ipc/mojo/async_handle_wa...
File ipc/mojo/async_handle_waiter.cc (right):

https://codereview.chromium.org/973213002/diff/40001/ipc/mojo/async_handle_wa...
ipc/mojo/async_handle_waiter.cc:69: return 0 < io_loop_level_;
nit: io_loop_level_ > 0

[Hajime Morrita, 2015-03-05 01:31:18]

Thanks! Landing with a fix.

[Hajime Morrita, 2015-03-05 01:31:27]

The CQ bit was checked by morrita@chromium.org

[Hajime Morrita, 2015-03-05 01:31:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
viettrungluu@chromium.org
Link to the patchset: https://codereview.chromium.org/973213002/#ps60001 (title:
"Landing")

[commit-bot: I haz the power, 2015-03-05 01:32:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/973213002/60001

[commit-bot: I haz the power, 2015-03-05 03:06:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-05 03:07:30]

Patchset 4 (id:??) landed as
https://crrev.com/a8c3c7dfa409152914df3363a931f7ed1348c760
Cr-Commit-Position: refs/heads/master@{#319202}