Mojo: Armed Watchers (issue 2725133002 by rockot@chromium.org)

[roc...@chromium.org]

Reviewers: yzshen1
CL: https://codereview.chromium.org/2725133002/

Message:
PTAL - Looks like I have some red to fix, but the CL is pretty large so I wanted
to send it out. Please look at EDK changes, public cpp watcher changes, and
bindings changes. Thanks!

Description:
Mojo: Armed Watchers

Changes the Watcher C API as described in this post:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/UcA97R4IznI

Updates the Watcher class in mojo/public/cpp/system to reflect the
new model, adding an explicit ArmingPolicy to allow users to select
manual or automatic arming.

Automatic arming provides imperfect edge-triggered behavior, which is
still an improvement over the old behavior. All existing Watcher users
use this behavior except for the bindings Connector.

Manual arming is used in the bindings Connector to ensure that all
messages are flushed from a pipe before control returns from a
handle-ready notification.

Other users of the Watcher C API (namely Blink's MojoWatcher and
content's MessagePort) have also been adapted to the new API.

BUG=693595

Affected files (+1022, -404 lines):
M chrome/browser/media/cast_remoting_sender.cc
M content/browser/loader/mojo_async_resource_handler.cc
M content/child/url_response_body_consumer.cc
M content/child/web_data_consumer_handle_impl.cc
M content/common/message_port.h
M content/common/message_port.cc
M ios/web/webui/mojo_facade.mm
M ipc/ipc_sync_channel.cc
M media/mojo/common/mojo_decoder_buffer_converter.cc
M media/remoting/demuxer_stream_adapter.cc
M mojo/android/system/watcher_impl.cc
M mojo/common/data_pipe_drainer.cc
M mojo/edk/embedder/entrypoints.cc
M mojo/edk/js/drain_data.cc
M mojo/edk/js/waiting_callback.cc
M mojo/edk/system/awakable_list.h
M mojo/edk/system/awakable_list.cc
M mojo/edk/system/core.h
M mojo/edk/system/core.cc
M mojo/edk/system/data_pipe_consumer_dispatcher.h
M mojo/edk/system/data_pipe_consumer_dispatcher.cc
M mojo/edk/system/data_pipe_producer_dispatcher.h
M mojo/edk/system/data_pipe_producer_dispatcher.cc
M mojo/edk/system/data_pipe_unittest.cc
M mojo/edk/system/dispatcher.h
M mojo/edk/system/dispatcher.cc
M mojo/edk/system/message_pipe_dispatcher.h
M mojo/edk/system/message_pipe_dispatcher.cc
M mojo/edk/system/multiprocess_message_pipe_unittest.cc
M mojo/edk/system/watch_unittest.cc
M mojo/edk/system/watcher.h
M mojo/edk/system/watcher.cc
M mojo/edk/system/watcher_set.h
M mojo/edk/system/watcher_set.cc
M mojo/public/c/system/functions.h
M mojo/public/c/system/thunks.h
M mojo/public/c/system/thunks.cc
M mojo/public/c/system/types.h
M mojo/public/cpp/bindings/BUILD.gn
M mojo/public/cpp/bindings/binding.h
M mojo/public/cpp/bindings/connector.h
M mojo/public/cpp/bindings/interface_ptr.h
M mojo/public/cpp/bindings/lib/binding_state.h
M mojo/public/cpp/bindings/lib/binding_state.cc
M mojo/public/cpp/bindings/lib/connector.cc
A mojo/public/cpp/bindings/lib/destruction_tracker.h
A mojo/public/cpp/bindings/lib/destruction_tracker.cc
M mojo/public/cpp/bindings/lib/interface_ptr_state.h
M mojo/public/cpp/bindings/lib/multiplex_router.h
M mojo/public/cpp/bindings/lib/multiplex_router.cc
M mojo/public/cpp/bindings/tests/binding_set_unittest.cc
M mojo/public/cpp/bindings/tests/connector_unittest.cc
M mojo/public/cpp/system/tests/watcher_unittest.cc
M mojo/public/cpp/system/watcher.h
M mojo/public/cpp/system/watcher.cc
M third_party/WebKit/Source/core/mojo/MojoWatcher.h
M third_party/WebKit/Source/core/mojo/MojoWatcher.cpp

[yzs...@chromium.org]

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/c/system/functions.h
File mojo/public/c/system/functions.h (right):

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/c/system/functions.h#newcode162
mojo/public/c/system/functions.h:162: // |handle|: The handle to
watcher. Must be an open message pipe or data pipe
watchter -> watch?

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/c/system/types.h
File mojo/public/c/system/types.h (right):

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/c/system/types.h#newcode185
mojo/public/c/system/types.h:185: // |MojoWatchNotificationFlags|:
Passed to a callback invoked by an armed
nit: strictly speaking the callback may be called while the watcher is
unarmed.

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/bindings/lib/connector.cc
File mojo/public/cpp/bindings/lib/connector.cc (right):

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/bindings/lib/connector.cc#newcode193
mojo/public/cpp/bindings/lib/connector.cc:193: CancelWait();
Will this cause problem while we are in the middle of a sync watch? For
example:
1) Endpoint a uses Connector::SyncWatch() to wait for an incoming sync
response.
2) The same thread is re-entered by another endpoint b.
3) Endpoint b calls EnableNestedDispatch() on a.
4) It will destroy the sync_watcher_ of a, which will cause the sync
watch to quit.

WDYT?

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/bindings/lib/connector.cc#newcode221
mojo/public/cpp/bindings/lib/connector.cc:221: for (;;) {
nit: Does it make sense to merge ReadAllAvailableMessages() into this
method or merge this loop into ReadAllAvailableMessages()?

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/bindings/lib/destruction_tracker.h
File mojo/public/cpp/bindings/lib/destruction_tracker.h (right):

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/bindings/lib/destruction_tracker.h#newcode5
mojo/public/cpp/bindings/lib/destruction_tracker.h:5: #ifndef
MOJO_PUBLIC_CPP_BINDINGS_LIB_DELETION_TRACKER_H_
Please make it consistent with the file name.

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/bindings/lib/destruction_tracker.h#newcode34
mojo/public/cpp/bindings/lib/destruction_tracker.h:34: explicit operator
bool() const { return was_destroyed_; }
can we use !tracker_ to indicate destructed?

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/system/watcher.h
File mojo/public/cpp/system/watcher.h (right):

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/system/watcher.h#newcode166
mojo/public/cpp/system/watcher.h:166: bool* was_deleted_flag_ = nullptr;
Is the reason to use this instead of DestructionTracker that we don't
need to worry about reentrancy here?

https://codereview.chromium.org/2725133002/diff/60001/third_party/WebKit/Source/core/mojo/MojoWatcher.cpp
File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):

https://codereview.chromium.org/2725133002/diff/60001/third_party/WebKit/Source/core/mojo/MojoWatcher.cpp#newcode147
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:147: arm();
Will it make things simpler if we use mojo::Watcher instead of the raw C
API?

https://codereview.chromium.org/2725133002/

[roc...@chromium.org]

Thanks! In addition to the inline comments, I've also reverted the redundant
NotifyState in MessagePipeDispatcher and removed the unnecessary use of
AtomicFlag from EDK's Watcher.

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/c/system/functions.h
File mojo/public/c/system/functions.h (right):

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/c/system/functions.h#newcode162
mojo/public/c/system/functions.h:162: // |handle|: The handle to
watcher. Must be an open message pipe or data pipe

On 2017/03/03 at 00:03:50, yzshen1 wrote:
> watchter -> watch?

done

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/c/system/types.h
File mojo/public/c/system/types.h (right):

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/c/system/types.h#newcode185
mojo/public/c/system/types.h:185: // |MojoWatchNotificationFlags|:
Passed to a callback invoked by an armed

On 2017/03/03 at 00:03:50, yzshen1 wrote:
> nit: strictly speaking the callback may be called while the watcher is
unarmed.

done

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/bindings/lib/connector.cc
File mojo/public/cpp/bindings/lib/connector.cc (right):

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/bindings/lib/connector.cc#newcode193
mojo/public/cpp/bindings/lib/connector.cc:193: CancelWait();

On 2017/03/03 at 00:03:50, yzshen1 wrote:
> Will this cause problem while we are in the middle of a sync watch?
For example:
> 1) Endpoint a uses Connector::SyncWatch() to wait for an incoming sync
response.
> 2) The same thread is re-entered by another endpoint b.
> 3) Endpoint b calls EnableNestedDispatch() on a.
> 4) It will destroy the sync_watcher_ of a, which will cause the sync
watch to quit.
>
> WDYT?

Good point. Now I just reset handle_watcher_ before WaitToReadMore.

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/bindings/lib/connector.cc#newcode221
mojo/public/cpp/bindings/lib/connector.cc:221: for (;;) {

On 2017/03/03 at 00:03:50, yzshen1 wrote:
> nit: Does it make sense to merge ReadAllAvailableMessages() into this
method or merge this loop into ReadAllAvailableMessages()?

Done

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/bindings/lib/destruction_tracker.h
File mojo/public/cpp/bindings/lib/destruction_tracker.h (right):

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/bindings/lib/destruction_tracker.h#newcode5
mojo/public/cpp/bindings/lib/destruction_tracker.h:5: #ifndef
MOJO_PUBLIC_CPP_BINDINGS_LIB_DELETION_TRACKER_H_

On 2017/03/03 at 00:03:50, yzshen1 wrote:
> Please make it consistent with the file name.

N/A - delete this code and used WeakPtrs instead per
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/lTXF4ejhbnQ

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/bindings/lib/destruction_tracker.h#newcode34
mojo/public/cpp/bindings/lib/destruction_tracker.h:34: explicit operator
bool() const { return was_destroyed_; }

On 2017/03/03 at 00:03:50, yzshen1 wrote:
> can we use !tracker_ to indicate destructed?

N/A

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/system/watcher.h
File mojo/public/cpp/system/watcher.h (right):

https://codereview.chromium.org/2725133002/diff/60001/mojo/public/cpp/system/watcher.h#newcode166
mojo/public/cpp/system/watcher.h:166: bool* was_deleted_flag_ = nullptr;

On 2017/03/03 at 00:03:50, yzshen1 wrote:
> Is the reason to use this instead of DestructionTracker that we don't
need to worry about reentrancy here?

N/A - using WeakPtr now.

But for the record, the reason was that I didn't think it made sense to
put DestructionTracker in an arbitrary public API (it's not a feature of
bindings or system really), so I decided to hide it in bindings where I
know we need reentrancy. Alas, it's irrelevant now :)

https://codereview.chromium.org/2725133002/diff/60001/third_party/WebKit/Source/core/mojo/MojoWatcher.cpp
File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):

https://codereview.chromium.org/2725133002/diff/60001/third_party/WebKit/Source/core/mojo/MojoWatcher.cpp#newcode147
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:147: arm();

On 2017/03/03 at 00:03:50, yzshen1 wrote:
> Will it make things simpler if we use mojo::Watcher instead of the raw
C API?

Yes, most certainly, but I assume it will result in an argument over
Blink depending on mojo/public/cpp/system, so I'd rather hold off on
that.

https://codereview.chromium.org/2725133002/

[roc...@chromium.org]

OK, I believe this is really ready for review now. Still an issue with ios's use
of Watcher which I'm looking into, but all //mojo changes are in a good state.
PTAL!

https://codereview.chromium.org/2725133002/

[roc...@chromium.org]

Just adding some inline comments to preemptively explain some parts of the CL,
hope it helps :)


https://codereview.chromium.org/2725133002/diff/190001/mojo/edk/system/message_pipe_dispatcher.cc
File mojo/edk/system/message_pipe_dispatcher.cc (right):

https://codereview.chromium.org/2725133002/diff/190001/mojo/edk/system/message_pipe_dispatcher.cc#newcode314
mojo/edk/system/message_pipe_dispatcher.cc:314:
watchers_.NotifyState(GetHandleSignalsStateNoLock());
I've had to add these state notifications to ReadMessage again after
all. The reason is that for WatcherDispatcher::Arm to be efficient, it
must always have up-to-date information about a watched handle's signal
state. Before we only needed to notify when state changed positively,
but now we need to notify any time the state changes, positive or
negative.

https://codereview.chromium.org/2725133002/diff/190001/mojo/edk/system/request_context.cc
File mojo/edk/system/request_context.cc (right):

https://codereview.chromium.org/2725133002/diff/190001/mojo/edk/system/request_context.cc#newcode44
mojo/edk/system/request_context.cc:44: // Pump potentially multiple
finalizer lists in sequence in order to avoid
BTW this change to handling nested finalizers is not necessary for this
patch. I can always land it separately if you prefer, but I liked the
idea of avoiding recursive destructor invocations.

https://codereview.chromium.org/2725133002/diff/190001/mojo/edk/system/request_context.h
File mojo/edk/system/request_context.h (left):

https://codereview.chromium.org/2725133002/diff/190001/mojo/edk/system/request_context.h#oldcode60
mojo/edk/system/request_context.h:60: void
AddWatchCancelFinalizer(scoped_refptr<Watcher> watcher);
This mechanism only existed to support cancelling watches from within
notification callbacks. Before we forced mutual exclusion and so this
was used to defer cancellation. That was quite difficult to understand
and now that we have manual arming I don't believe it's necessary to
require that exclusion.

https://codereview.chromium.org/2725133002/

[roc...@chromium.org]

Sorry, please ignore again for now. I have some more changes to make :>

https://codereview.chromium.org/2725133002/

[roc...@chromium.org]

OK, really ready for review this time.

Sorry for the large CL. I felt a rename of mojo::Watcher was appropriate given
that it's now a limited subset of watcher functionality.

Most of the core work is still in EDK code.

https://codereview.chromium.org/2725133002/

[roc...@chromium.org]

On 2017/03/08 at 23:07:59, Ken Rockot wrote:
> OK, really ready for review this time.
>
> Sorry for the large CL. I felt a rename of mojo::Watcher was appropriate given
that it's now a limited subset of watcher functionality.
>
> Most of the core work is still in EDK code.

Also worth noting that my previous comments regarding RequestContext are invalid
and can be ignored.

Because it's undesirable to make watch cancellation illegal from within
notification callbacks, I decided that the safest thing to do is to ensure that
a MOJO_RESULT_CANCELLED notification is always sent for any watch regardless of
how it's cancelled.

This is a bit safer than the status quo, which sends a MOJO_RESULT_CANCELLED
notification only when the watched handle is closed. Otherwise users assume that
once MojoCancelWatch is called, no more notifications can fire. That's the
guarantee made by the API today, but it requires explicit cancellation to be
exclusive to notification and thus the only way to cancel watch from within a
notification handler is to close the handle (MojoCancelWatch would deadlock).

Guaranteeing a final MOJO_RESULT_CANCELLED for all watch contexts allows users
to safely treat the context value as an owned, thread-safe object pointer which
can be reliably deleted on MOJO_RESULT_CANCELLED. You can see how it's used this
way in mojo::SimpleWatcher and content::MessagePort, where it's used as a
scoped_refptr instance.

Some of the burden of sorting out cross-thread behavior from the notification
callback shifted onto the user, but this gives them more freedom to decide how
to implement synchronization, and as mojo::SimpleWatcher demonstrates, we can
still provide helpful wrappers to this end.

https://codereview.chromium.org/2725133002/

[yzs...@chromium.org]

Some more comments.


https://codereview.chromium.org/2725133002/diff/60001/content/child/url_response_body_consumer.cc
File content/child/url_response_body_consumer.cc (right):

https://codereview.chromium.org/2725133002/diff/60001/content/child/url_response_body_consumer.cc#newcode60
content/child/url_response_body_consumer.cc:60: FROM_HERE,
base::Bind(&URLResponseBodyConsumer::OnReadable, AsWeakPtr(),
We may not need weak ptr support for this class any more.

https://codereview.chromium.org/2725133002/diff/370001/mojo/android/javatests/src/org/chromium/mojo/system/impl/WatcherImplTest.java
File
mojo/android/javatests/src/org/chromium/mojo/system/impl/WatcherImplTest.java
(right):

https://codereview.chromium.org/2725133002/diff/370001/mojo/android/javatests/src/org/chromium/mojo/system/impl/WatcherImplTest.java#newcode74
mojo/android/javatests/src/org/chromium/mojo/system/impl/WatcherImplTest.java:74:
* @param read_pipe A MessagePipeHandle to read from when onResult
triggers success.
nit: read_pipe -> readPipe

https://codereview.chromium.org/2725133002/diff/410001/mojo/edk/system/data_pipe_consumer_dispatcher.cc
File mojo/edk/system/data_pipe_consumer_dispatcher.cc (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/edk/system/data_pipe_consumer_dispatcher.cc#newcode484
mojo/edk/system/data_pipe_consumer_dispatcher.cc:484: if
(!in_two_phase_read_ && bytes_available_ && !suppress_readable_signal_)
nit: there is no need to check |bytes_available_| because the previous
line has done that.

https://codereview.chromium.org/2725133002/diff/410001/mojo/edk/system/watcher_dispatcher.cc
File mojo/edk/system/watcher_dispatcher.cc (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/edk/system/watcher_dispatcher.cc#newcode352
mojo/edk/system/watcher_dispatcher.cc:352: for (auto& handles :
watched_handles_) {
nit: handles -> handle?

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/c/system/data_pipe.h
File mojo/public/c/system/data_pipe.h (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/c/system/data_pipe.h#newcode89
mojo/public/c/system/data_pipe.h:89: //
|MOJO_READ_DATA_FLAG_CLEAR_SIGNAL| - If reading fails due to a lack of
An idea for your consideration:
One thing is that this CLEAR_SIGNAL changes the handle state that may
also affects others.

For example, pipe_user_a does read with ALL_OR_NONE | CLEAR_SIGNAL. And
later it gives the pipe to pipe_user_b. Pipe_user_b doesn't do
ALL_OR_NONE, and it decides to wait until READABLE instead of directly
trying a read. In that case, if no new data is available, pipe_user_b
will never do a read, because the pipe appears unreadable to him.

Is it possible to define a new kind of signal? For example,
NEW_DATA_READABLE. It will signal whenever new data arrives. Anyone who
wants to do ALL_OR_NONE read should watch for NEW_DATA_READABLE instead.

WDYT?

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/c/system/watcher.h
File mojo/public/c/system/watcher.h (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/c/system/watcher.h#newcode58
mojo/public/c/system/watcher.h:58: // watch different signals) as long
as a different |context| is provided for
Are there use cases where we need to watch a handle multiple times in
the same watcher? I am thinking if we only allow a handle being added
to a watcher once, that may save some bookkeeping work in the
implementation.

WDYT? Thanks!

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/c/system/watcher.h#newcode121
mojo/public/c/system/watcher.h:121: // lastter case, all registered
contexts no the watcher are implicitly cancelled
no -> on

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/cpp/bindings/lib/connector.cc
File mojo/public/cpp/bindings/lib/connector.cc (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/cpp/bindings/lib/connector.cc#newcode313
mojo/public/cpp/bindings/lib/connector.cc:313: MojoResult rv =
handle_watcher_->Arm(&ready_result);
Please consider using a different name, it hides |rv| of the outer
scope.

https://codereview.chromium.org/2725133002/

[roc...@chromium.org]

https://codereview.chromium.org/2725133002/diff/60001/content/child/url_response_body_consumer.cc
File content/child/url_response_body_consumer.cc (right):

https://codereview.chromium.org/2725133002/diff/60001/content/child/url_response_body_consumer.cc#newcode60
content/child/url_response_body_consumer.cc:60: FROM_HERE,
base::Bind(&URLResponseBodyConsumer::OnReadable, AsWeakPtr(),

On 2017/03/11 at 00:44:58, yzshen1 wrote:
> We may not need weak ptr support for this class any more.

I think that's true, but I'd rather leave this unchanged for this CL if
you're OK with that

https://codereview.chromium.org/2725133002/diff/370001/mojo/android/javatests/src/org/chromium/mojo/system/impl/WatcherImplTest.java
File
mojo/android/javatests/src/org/chromium/mojo/system/impl/WatcherImplTest.java
(right):

https://codereview.chromium.org/2725133002/diff/370001/mojo/android/javatests/src/org/chromium/mojo/system/impl/WatcherImplTest.java#newcode74
mojo/android/javatests/src/org/chromium/mojo/system/impl/WatcherImplTest.java:74:
* @param read_pipe A MessagePipeHandle to read from when onResult
triggers success.

On 2017/03/11 at 00:44:58, yzshen1 wrote:
> nit: read_pipe -> readPipe

Done

https://codereview.chromium.org/2725133002/diff/410001/mojo/edk/system/data_pipe_consumer_dispatcher.cc
File mojo/edk/system/data_pipe_consumer_dispatcher.cc (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/edk/system/data_pipe_consumer_dispatcher.cc#newcode484
mojo/edk/system/data_pipe_consumer_dispatcher.cc:484: if
(!in_two_phase_read_ && bytes_available_ && !suppress_readable_signal_)

On 2017/03/11 at 00:44:58, yzshen1 wrote:
> nit: there is no need to check |bytes_available_| because the previous
line has done that.

Done

https://codereview.chromium.org/2725133002/diff/410001/mojo/edk/system/watcher_dispatcher.cc
File mojo/edk/system/watcher_dispatcher.cc (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/edk/system/watcher_dispatcher.cc#newcode352
mojo/edk/system/watcher_dispatcher.cc:352: for (auto& handles :
watched_handles_) {

On 2017/03/11 at 00:44:58, yzshen1 wrote:
> nit: handles -> handle?

Done

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/c/system/data_pipe.h
File mojo/public/c/system/data_pipe.h (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/c/system/data_pipe.h#newcode89
mojo/public/c/system/data_pipe.h:89: //
|MOJO_READ_DATA_FLAG_CLEAR_SIGNAL| - If reading fails due to a lack of

On 2017/03/11 at 00:44:58, yzshen1 wrote:
> An idea for your consideration:
> One thing is that this CLEAR_SIGNAL changes the handle state that may
also affects others.
>
> For example, pipe_user_a does read with ALL_OR_NONE | CLEAR_SIGNAL.
And later it gives the pipe to pipe_user_b. Pipe_user_b doesn't do
ALL_OR_NONE, and it decides to wait until READABLE instead of directly
trying a read. In that case, if no new data is available, pipe_user_b
will never do a read, because the pipe appears unreadable to him.
>
> Is it possible to define a new kind of signal? For example,
NEW_DATA_READABLE. It will signal whenever new data arrives. Anyone who
wants to do ALL_OR_NONE read should watch for NEW_DATA_READABLE instead.
>
> WDYT?

Argued with myself for a while over this. I think it's a good solution.
Done (in a separate precursor CL)

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/c/system/watcher.h
File mojo/public/c/system/watcher.h (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/c/system/watcher.h#newcode58
mojo/public/c/system/watcher.h:58: // watch different signals) as long
as a different |context| is provided for

On 2017/03/11 at 00:44:58, yzshen1 wrote:
> Are there use cases where we need to watch a handle multiple times in
the same watcher? I am thinking if we only allow a handle being added
to a watcher once, that may save some bookkeeping work in the
implementation.
>
> WDYT? Thanks!

Good point. I was trying to keep the API as flexible as possible but I
don't think this complexity needs to be there. A user can always use a
single watch to watch multiple signals, and demultiplex notifications
based on signals_state.

Done.

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/c/system/watcher.h#newcode121
mojo/public/c/system/watcher.h:121: // lastter case, all registered
contexts no the watcher are implicitly cancelled

On 2017/03/11 at 00:44:58, yzshen1 wrote:
> no -> on

Done

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/cpp/bindings/lib/connector.cc
File mojo/public/cpp/bindings/lib/connector.cc (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/cpp/bindings/lib/connector.cc#newcode313
mojo/public/cpp/bindings/lib/connector.cc:313: MojoResult rv =
handle_watcher_->Arm(&ready_result);

On 2017/03/11 at 00:44:58, yzshen1 wrote:
> Please consider using a different name, it hides |rv| of the outer
scope.

Done

https://codereview.chromium.org/2725133002/

[yzs...@chromium.org]

LGTM with a few nits.


https://codereview.chromium.org/2725133002/diff/410001/mojo/public/cpp/system/simple_watcher.h
File mojo/public/cpp/system/simple_watcher.h (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/cpp/system/simple_watcher.h#newcode25
mojo/public/cpp/system/simple_watcher.h:25: // This provides a convnient
thread-bound watcher implementation to safely watch
convnient -> convenient

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/cpp/system/simple_watcher.h#newcode94
mojo/public/cpp/system/simple_watcher.h:94: // |MOJO_RESULT_CANCELLED|
if the SimpleWatcher is currently armed. Use
I haven't understood this. Why is CANCELLED not dispatched if it is
armed?

https://codereview.chromium.org/2725133002/diff/490001/mojo/edk/system/watch.h
File mojo/edk/system/watch.h (right):

https://codereview.chromium.org/2725133002/diff/490001/mojo/edk/system/watch.h#newcode42
mojo/edk/system/watch.h:42: // dispatcher notifies the WatcherDispatcher
of a state change a state change.
nit: "a state change" occurs twice.

https://codereview.chromium.org/2725133002/diff/490001/mojo/edk/system/watcher_dispatcher.cc
File mojo/edk/system/watcher_dispatcher.cc (right):

https://codereview.chromium.org/2725133002/diff/490001/mojo/edk/system/watcher_dispatcher.cc#newcode120
mojo/edk/system/watcher_dispatcher.cc:120: if (watches_.count(context)
|| watched_handles_.count(dispatcher.get()))
[just to double check] IIUC, we want |context| to be unique because that
is the only ID information when the callback is called. (The handle
itself is not passed into the callback.) right?

https://codereview.chromium.org/2725133002/diff/490001/mojo/public/cpp/system/simple_watcher.cc
File mojo/public/cpp/system/simple_watcher.cc (right):

https://codereview.chromium.org/2725133002/diff/490001/mojo/public/cpp/system/simple_watcher.cc#newcode59
mojo/public/cpp/system/simple_watcher.cc:59: delete context_ref;
Does it make sense to use Context* as the |context_value|, which has one
ref owned by the registered watch, and we release that ref on
cancellation?

https://codereview.chromium.org/2725133002/diff/490001/mojo/public/cpp/system/simple_watcher.cc#newcode185
mojo/public/cpp/system/simple_watcher.cc:185: // unset by then. This
prevents the cancellation notification from reaching
Is the last sentence a comment for line 178 instead?

https://codereview.chromium.org/2725133002/

[roc...@chromium.org]

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/cpp/system/simple_watcher.h
File mojo/public/cpp/system/simple_watcher.h (right):

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/cpp/system/simple_watcher.h#newcode25
mojo/public/cpp/system/simple_watcher.h:25: // This provides a convnient
thread-bound watcher implementation to safely watch

On 2017/03/13 at 20:21:18, yzshen1 wrote:
> convnient -> convenient

Done

https://codereview.chromium.org/2725133002/diff/410001/mojo/public/cpp/system/simple_watcher.h#newcode94
mojo/public/cpp/system/simple_watcher.h:94: // |MOJO_RESULT_CANCELLED|
if the SimpleWatcher is currently armed. Use

On 2017/03/13 at 20:21:18, yzshen1 wrote:
> I haven't understood this. Why is CANCELLED not dispatched if it is
armed?

Sorry, wording is bad. CANCELLED is always dispatched. What I meant by
this is that, unlike CANCELLED results, non-CANCELLED results can only
be dispatched when armed. I've tried to improve the wording.

https://codereview.chromium.org/2725133002/diff/490001/mojo/edk/system/watch.h
File mojo/edk/system/watch.h (right):

https://codereview.chromium.org/2725133002/diff/490001/mojo/edk/system/watch.h#newcode42
mojo/edk/system/watch.h:42: // dispatcher notifies the WatcherDispatcher
of a state change a state change.

On 2017/03/13 at 20:21:18, yzshen1 wrote:
> nit: "a state change" occurs twice.

Fixed

https://codereview.chromium.org/2725133002/diff/490001/mojo/edk/system/watcher_dispatcher.cc
File mojo/edk/system/watcher_dispatcher.cc (right):

https://codereview.chromium.org/2725133002/diff/490001/mojo/edk/system/watcher_dispatcher.cc#newcode120
mojo/edk/system/watcher_dispatcher.cc:120: if (watches_.count(context)
|| watched_handles_.count(dispatcher.get()))

On 2017/03/13 at 20:21:18, yzshen1 wrote:
> [just to double check] IIUC, we want |context| to be unique because
that is the only ID information when the callback is called. (The handle
itself is not passed into the callback.) right?

Correct. Unlike the WaitSet API, I didn't want watchers to return handle
values, because on cancellation the handle value is no longer valid (and
in theory may even refer to a valid handle that has different meaning if
the value has been reused by the system).

https://codereview.chromium.org/2725133002/diff/490001/mojo/public/cpp/system/simple_watcher.cc
File mojo/public/cpp/system/simple_watcher.cc (right):

https://codereview.chromium.org/2725133002/diff/490001/mojo/public/cpp/system/simple_watcher.cc#newcode59
mojo/public/cpp/system/simple_watcher.cc:59: delete context_ref;

On 2017/03/13 at 20:21:18, yzshen1 wrote:
> Does it make sense to use Context* as the |context_value|, which has
one ref owned by the registered watch, and we release that ref on
cancellation?

Sure, that's effectively equivalent to this, but I guess it does seem a
little cleaner. Done.

https://codereview.chromium.org/2725133002/diff/490001/mojo/public/cpp/system/simple_watcher.cc#newcode185
mojo/public/cpp/system/simple_watcher.cc:185: // unset by then. This
prevents the cancellation notification from reaching

On 2017/03/13 at 20:21:18, yzshen1 wrote:
> Is the last sentence a comment for line 178 instead?

Ah good catch. It used to be accurate here, but I added
DisableCancellationNotifications() later after encountering issues
described in the TODO there. Moved the comment.

https://codereview.chromium.org/2725133002/

[roc...@chromium.org]

Note to any reviewers of code using data pipes: The changes here are a stop-gap
to adapt your Watchers to the new SimpleWatcher API, which is not entirely
appropriate for data pipe handles, but is functional enough to do the right
thing and not break your code. I will be following up with a CL that introduces
a new kind of Watcher specifically for data pipes and it will make this code
less ugly.

Please take a look!

+miu: chrome/browser/media
+yhirano (and +csharrison): c/b/loader and c/child loading stuff
+jam: content MessagePort
+eugenebut: ios/
+xhwang: media/
+haraken: Blink MessagePort

Thanks all

https://codereview.chromium.org/2725133002/

[roc...@chromium.org]

That should be Blink MojoWatcher* :)

>
> Thanks all



https://codereview.chromium.org/2725133002/

[j...@chromium.org]

rs lgtm

https://codereview.chromium.org/2725133002/

[xhw...@chromium.org]

media/ lgtm and looking forward to the new DataPipe watchers :) Thanks!

https://codereview.chromium.org/2725133002/

[yhi...@chromium.org]

https://codereview.chromium.org/2725133002/diff/510001/content/child/url_response_body_consumer.cc
File content/child/url_response_body_consumer.cc (right):

https://codereview.chromium.org/2725133002/diff/510001/content/child/url_response_body_consumer.cc#newcode113
content/child/url_response_body_consumer.cc:113: if (result ==
MOJO_RESULT_SHOULD_WAIT || result == MOJO_RESULT_BUSY) {
Is ArmOrNotify unnecessary on the MOJO_RESULT_BUSY case?

https://codereview.chromium.org/2725133002/

[har...@chromium.org]

https://codereview.chromium.org/2725133002/diff/510001/third_party/WebKit/Source/core/mojo/MojoWatcher.h
File third_party/WebKit/Source/core/mojo/MojoWatcher.h (right):

https://codereview.chromium.org/2725133002/diff/510001/third_party/WebKit/Source/core/mojo/MojoWatcher.h#newcode65
third_party/WebKit/Source/core/mojo/MojoWatcher.h:65:
std::unique_ptr<CrossThreadPersistent<MojoWatcher>> m_context;

This looks strange in a coupe of ways.

The only thing we need to make sure is that MojoWatcher's wrapper does
not get collected while the watcher is running. If MojoWatcher's wrapper
is alive, the MojoWatcher is kept alive as well.

To achieve that, you just need to return true from hasPendingActivity()
while the watcher is running. i.e., one boolean flag would be
sufficient, I guess.

https://codereview.chromium.org/2725133002/

[roc...@chromium.org]

https://codereview.chromium.org/2725133002/diff/510001/content/child/url_response_body_consumer.cc
File content/child/url_response_body_consumer.cc (right):

https://codereview.chromium.org/2725133002/diff/510001/content/child/url_response_body_consumer.cc#newcode113
content/child/url_response_body_consumer.cc:113: if (result ==
MOJO_RESULT_SHOULD_WAIT || result == MOJO_RESULT_BUSY) {

On 2017/03/14 at 05:41:55, yhirano wrote:
> Is ArmOrNotify unnecessary on the MOJO_RESULT_BUSY case?

No, that implies we're in a two-phase read, which ends with
EndReadDataRaw. We always ArmOrNotify after that.

https://codereview.chromium.org/2725133002/diff/510001/third_party/WebKit/Source/core/mojo/MojoWatcher.h
File third_party/WebKit/Source/core/mojo/MojoWatcher.h (right):

https://codereview.chromium.org/2725133002/diff/510001/third_party/WebKit/Source/core/mojo/MojoWatcher.h#newcode65
third_party/WebKit/Source/core/mojo/MojoWatcher.h:65:
std::unique_ptr<CrossThreadPersistent<MojoWatcher>> m_context;

On 2017/03/14 at 09:51:01, haraken wrote:
> This looks strange in a coupe of ways.
>
> The only thing we need to make sure is that MojoWatcher's wrapper does
not get collected while the watcher is running. If MojoWatcher's wrapper
is alive, the MojoWatcher is kept alive as well.
>
> To achieve that, you just need to return true from
hasPendingActivity() while the watcher is running. i.e., one boolean
flag would be sufficient, I guess.

I see, thanks for the clarification. Actually we don't even need a bool,
we can use the validity of m_handle as a signal still. I've made it so
that explicit cancel() does not reset this value, but instead only
resets the watcher handle itself. This achieves the desired behavior.

https://codereview.chromium.org/2725133002/

[har...@chromium.org]

WebKit LGTM



https://codereview.chromium.org/2725133002/diff/550001/third_party/WebKit/Source/core/mojo/MojoWatcher.cpp
File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):

https://codereview.chromium.org/2725133002/diff/550001/third_party/WebKit/Source/core/mojo/MojoWatcher.cpp#newcode90
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:90: MojoResult rv =

Nit: rv => result

Blink prefers a fully qualified name.

https://codereview.chromium.org/2725133002/

[yhi...@chromium.org]

https://codereview.chromium.org/2725133002/diff/510001/content/child/url_response_body_consumer.cc
File content/child/url_response_body_consumer.cc (right):

https://codereview.chromium.org/2725133002/diff/510001/content/child/url_response_body_consumer.cc#newcode113
content/child/url_response_body_consumer.cc:113: if (result ==
MOJO_RESULT_SHOULD_WAIT || result == MOJO_RESULT_BUSY) {

On 2017/03/14 14:17:40, Ken Rockot wrote:
> On 2017/03/14 at 05:41:55, yhirano wrote:
> > Is ArmOrNotify unnecessary on the MOJO_RESULT_BUSY case?
>
> No, that implies we're in a two-phase read, which ends with
EndReadDataRaw. We
> always ArmOrNotify after that.

Sorry, I don't understand: My understanding is "we're in a two-phase
read, which ends with EndReadDataRaw." The EndReadDataRaw is on L88, and
this ArmOrNotify will continue posting tasks until Reclaim is called.

https://codereview.chromium.org/2725133002/

[roc...@chromium.org]

https://codereview.chromium.org/2725133002/diff/510001/content/child/url_response_body_consumer.cc
File content/child/url_response_body_consumer.cc (right):

https://codereview.chromium.org/2725133002/diff/510001/content/child/url_response_body_consumer.cc#newcode113
content/child/url_response_body_consumer.cc:113: if (result ==
MOJO_RESULT_SHOULD_WAIT || result == MOJO_RESULT_BUSY) {

On 2017/03/14 at 15:32:10, yhirano wrote:
> On 2017/03/14 14:17:40, Ken Rockot wrote:
> > On 2017/03/14 at 05:41:55, yhirano wrote:
> > > Is ArmOrNotify unnecessary on the MOJO_RESULT_BUSY case?
> >
> > No, that implies we're in a two-phase read, which ends with
EndReadDataRaw. We
> > always ArmOrNotify after that.
>
> Sorry, I don't understand: My understanding is "we're in a two-phase
read, which ends with EndReadDataRaw." The EndReadDataRaw is on L88, and
this ArmOrNotify will continue posting tasks until Reclaim is called.

Ah, sorry, I misunderstood your comment and overlooked this code. I
thought you were suggesting that we did need ArmOrNotify when the result
is MOJO_RESULT_BUSY. You're right to suggest that we don't. Fixed.

https://codereview.chromium.org/2725133002/diff/550001/third_party/WebKit/Source/core/mojo/MojoWatcher.cpp
File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):

https://codereview.chromium.org/2725133002/diff/550001/third_party/WebKit/Source/core/mojo/MojoWatcher.cpp#newcode90
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:90: MojoResult rv =

On 2017/03/14 at 15:10:32, haraken wrote:
> Nit: rv => result
>
> Blink prefers a fully qualified name.

Done

https://codereview.chromium.org/2725133002/

[yhi...@chromium.org]

lgtm



https://codereview.chromium.org/2725133002/

[mme...@chromium.org]

https://codereview.chromium.org/2725133002/diff/590001/mojo/public/cpp/system/simple_watcher.h
File mojo/public/cpp/system/simple_watcher.h (right):

https://codereview.chromium.org/2725133002/diff/590001/mojo/public/cpp/system/simple_watcher.h#newcode43
mojo/public/cpp/system/simple_watcher.h:43: // Selects how this
SimpleWatcher is to be armed.
Please add a definition of "arm", either here or above the class
declaration, or above the Arm() method.

https://codereview.chromium.org/2725133002/

[roc...@chromium.org]

https://codereview.chromium.org/2725133002/diff/590001/mojo/public/cpp/system/simple_watcher.h
File mojo/public/cpp/system/simple_watcher.h (right):

https://codereview.chromium.org/2725133002/diff/590001/mojo/public/cpp/system/simple_watcher.h#newcode43
mojo/public/cpp/system/simple_watcher.h:43: // Selects how this
SimpleWatcher is to be armed.

On 2017/03/15 at 14:11:31, mmenke wrote:
> Please add a definition of "arm", either here or above the class
declaration, or above the Arm() method.

Done. I've tried to flesh out the documentation a bit more here.

https://codereview.chromium.org/2725133002/

[mme...@chromium.org]

Thanks, much appreciated!

https://codereview.chromium.org/2725133002/

[roc...@chromium.org]

I notice you're also an owner of content/browser/loader - any chance could
review the changes there in lieu of csharrison@? :)

https://codereview.chromium.org/2725133002/

[euge...@chromium.org]

ios lgtm


https://codereview.chromium.org/2725133002/diff/650001/ios/web/webui/web_ui_mojo_inttest.mm
File ios/web/webui/web_ui_mojo_inttest.mm (right):

https://codereview.chromium.org/2725133002/diff/650001/ios/web/webui/web_ui_mojo_inttest.mm#newcode169
ios/web/webui/web_ui_mojo_inttest.mm:169: // TODO(rockot): Introduce the
full watcher API to JS and get rid
On ios we use TODO(crbug.com/###): format, because otherwise we endup
with bunch of TODOs for people how left the team. Do you want a bug for
this TODO?

https://codereview.chromium.org/2725133002/

[cshar...@chromium.org]

c/b/l LGTM

https://codereview.chromium.org/2725133002/

[roc...@chromium.org]

https://codereview.chromium.org/2725133002/diff/650001/ios/web/webui/web_ui_mojo_inttest.mm
File ios/web/webui/web_ui_mojo_inttest.mm (right):

https://codereview.chromium.org/2725133002/diff/650001/ios/web/webui/web_ui_mojo_inttest.mm#newcode169
ios/web/webui/web_ui_mojo_inttest.mm:169: // TODO(rockot): Introduce the
full watcher API to JS and get rid

On 2017/03/15 at 17:56:34, Eugene But wrote:
> On ios we use TODO(crbug.com/###): format, because otherwise we endup
with bunch of TODOs for people how left the team. Do you want a bug for
this TODO?

Seems reasonable. I've filed a bug and updated the TODO.

https://codereview.chromium.org/2725133002/

[m...@chromium.org]

chrome/browser/media/* and media/remoting/* lgtm

https://codereview.chromium.org/2725133002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2725133002/670001


https://codereview.chromium.org/2725133002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #20 (id:670001) as
https://chromium.googlesource.com/chromium/src/+/9eadabae1de0cb22725450596fe324f03dd63b92

https://codereview.chromium.org/2725133002/

[gui...@chromium.org]

A revert of this CL (patchset #20 id:670001) has been created in
https://codereview.chromium.org/2750373002/ by gui...@chromium.org.

The reason for reverting is: FindIt considers this as culprit of breaking Linux
Chromium OS ASan LSan Tests (1) (stats) bot
See
https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/20033

aura_unittests aura_unittests

failed 1
Run on OS: 'Ubuntu-14.04'
failures:
UserActivityForwarderTest.ForwardActivityToDetector


Will reland if the revert doesn't fix the bot.
.

https://codereview.chromium.org/2725133002/

[Ken Rockot]

You won't be able to revert without reverting other CLs too, unfortunately. The leak is test only and a fix is out for review.

[gui...@chromium.org]

Cancelled the revert since rockot@ has a fix:
https://codereview.chromium.org/2754863002

https://codereview.chromium.org/2725133002/

[dontke...@gmail.com]

Joined superior court judge of office until Monday