Mojo: Cancel WaitingCallbacks when their HandleWrappers are closed.

Mojo: Cancel WaitingCallbacks when their HandleWrappers are closed.

Currently, if a JS connector is left to be garbage collected, the handle
and the WaitingCallback both become ready to be collected at the same
time. If the handle is collected first, this results in an asynchronous
wait on a closed handle. With this change, WaitingCallback registers
itself as an observer to be notified when the handle it's watching is
closing and cancels the wait if the handle closes while the wait is in
progress.

BUG=406487
Committed: https://crrev.com/a9e9f87c058a47db6c5709b38aae09acd56b11cc
Cr-Commit-Position: refs/heads/master@{#294775}

[Sam McNally, 2014-09-03 05:04:16]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2014-09-03 05:14:46]

sammc@chromium.org changed reviewers:
+ sky@chromium.org

[abarth-chromium, 2014-09-03 05:21:56]

https://codereview.chromium.org/534953002/diff/20001/mojo/bindings/js/handle.cc
File mojo/bindings/js/handle.cc (right):

https://codereview.chromium.org/534953002/diff/20001/mojo/bindings/js/handle....
mojo/bindings/js/handle.cc:39: (*it++)->OnHandleClosed();
What if observers_ is modified during this iteration?

[Sam McNally, 2014-09-03 05:32:19]

Patchset #2 (id:40001) has been deleted

[Sam McNally, 2014-09-03 05:37:27]

https://codereview.chromium.org/534953002/diff/20001/mojo/bindings/js/handle.cc
File mojo/bindings/js/handle.cc (right):

https://codereview.chromium.org/534953002/diff/20001/mojo/bindings/js/handle....
mojo/bindings/js/handle.cc:39: (*it++)->OnHandleClosed();
On 2014/09/03 05:21:55, abarth wrote:
> What if observers_ is modified during this iteration?

Done.

[sky, 2014-09-03 15:47:03]

sky@chromium.org changed reviewers:
+ abarth@chromium.org

[sky, 2014-09-03 15:47:04]

Is it possible to write test coverage?

I'm making abarth a real reviewer here as he knows this code better than I.

https://codereview.chromium.org/534953002/diff/60001/mojo/bindings/js/handle.cc
File mojo/bindings/js/handle.cc (right):

https://codereview.chromium.org/534953002/diff/60001/mojo/bindings/js/handle....
mojo/bindings/js/handle.cc:33: FOR_EACH_OBSERVER(CloseObserver,
close_observers_, OnHandleClosed());
Should we only notify the observers if the handle is valid when we get here? For
example, if someone explicitly invokes Close() and then deletes the
HandleWrapper do they really want two observer notifications?

https://codereview.chromium.org/534953002/diff/60001/mojo/bindings/js/handle.h
File mojo/bindings/js/handle.h (right):

https://codereview.chromium.org/534953002/diff/60001/mojo/bindings/js/handle....
mojo/bindings/js/handle.h:20: class CloseObserver {
Move this into its own file and make the destructor protected.

https://codereview.chromium.org/534953002/diff/60001/mojo/bindings/js/handle....
mojo/bindings/js/handle.h:23: virtual void OnHandleClosed() = 0;
Isn't this sent before the close? Wouldn't OnWillCloseHandle be a better name?

https://codereview.chromium.org/534953002/diff/60001/mojo/bindings/js/handle....
mojo/bindings/js/handle.h:43: mojo::ScopedHandle handle_;
nit: newline between 42/43.

https://codereview.chromium.org/534953002/diff/60001/mojo/bindings/js/waiting...
File mojo/bindings/js/waiting_callback.cc (right):

https://codereview.chromium.org/534953002/diff/60001/mojo/bindings/js/waiting...
mojo/bindings/js/waiting_callback.cc:53: : wait_id_(),
handle_wrapper_(handle_wrapper.get()) {
0?

[Sam McNally, 2014-09-08 04:31:52]

On 2014/09/03 15:47:04, sky wrote:
> Is it possible to write test coverage?
> 
Done.

https://codereview.chromium.org/534953002/diff/60001/mojo/bindings/js/handle.cc
File mojo/bindings/js/handle.cc (right):

https://codereview.chromium.org/534953002/diff/60001/mojo/bindings/js/handle....
mojo/bindings/js/handle.cc:33: FOR_EACH_OBSERVER(CloseObserver,
close_observers_, OnHandleClosed());
On 2014/09/03 15:47:04, sky wrote:
> Should we only notify the observers if the handle is valid when we get here?
For
> example, if someone explicitly invokes Close() and then deletes the
> HandleWrapper do they really want two observer notifications?

Done.

https://codereview.chromium.org/534953002/diff/60001/mojo/bindings/js/handle.h
File mojo/bindings/js/handle.h (right):

https://codereview.chromium.org/534953002/diff/60001/mojo/bindings/js/handle....
mojo/bindings/js/handle.h:20: class CloseObserver {
On 2014/09/03 15:47:04, sky wrote:
> Move this into its own file and make the destructor protected.

Done.

https://codereview.chromium.org/534953002/diff/60001/mojo/bindings/js/handle....
mojo/bindings/js/handle.h:23: virtual void OnHandleClosed() = 0;
On 2014/09/03 15:47:04, sky wrote:
> Isn't this sent before the close? Wouldn't OnWillCloseHandle be a better name?

Done.

https://codereview.chromium.org/534953002/diff/60001/mojo/bindings/js/handle....
mojo/bindings/js/handle.h:43: mojo::ScopedHandle handle_;
On 2014/09/03 15:47:04, sky wrote:
> nit: newline between 42/43.

Done.

https://codereview.chromium.org/534953002/diff/60001/mojo/bindings/js/waiting...
File mojo/bindings/js/waiting_callback.cc (right):

https://codereview.chromium.org/534953002/diff/60001/mojo/bindings/js/waiting...
mojo/bindings/js/waiting_callback.cc:53: : wait_id_(),
handle_wrapper_(handle_wrapper.get()) {
On 2014/09/03 15:47:04, sky wrote:
> 0?

Done.

[sky, 2014-09-08 16:07:02]

LGTM

https://codereview.chromium.org/534953002/diff/80001/mojo/apps/js/test/handle...
File mojo/apps/js/test/handle_unittest.cc (right):

https://codereview.chromium.org/534953002/diff/80001/mojo/apps/js/test/handle...
mojo/apps/js/test/handle_unittest.cc:30: };
private:DISALLOW...

[Sam McNally, 2014-09-09 00:02:46]

abarth: do you want to take a look?

https://codereview.chromium.org/534953002/diff/80001/mojo/apps/js/test/handle...
File mojo/apps/js/test/handle_unittest.cc (right):

https://codereview.chromium.org/534953002/diff/80001/mojo/apps/js/test/handle...
mojo/apps/js/test/handle_unittest.cc:30: };
On 2014/09/08 16:07:01, sky wrote:
> private:DISALLOW...

Done.

[Sam McNally, 2014-09-15 00:53:40]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-09-15 00:54:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/534953002/100001

[commit-bot: I haz the power, 2014-09-15 01:25:24]

Committed patchset #4 (id:100001) as e66d4f1a59f6f3db331a89dbb3603b71d29128bc

[commit-bot: I haz the power, 2014-09-15 01:29:01]

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