mek@google.com:

The two ways I believe it to be broken:
- if the second call to AppWebMessagePort.setMessageCallback passes a null handler (indicating main thread) it'll keep dispatching on the old handler
- it is possible that the old handler is about to dispatch messages as setMessageCallback is called on the main thread. That can then result in a race condition where mMessageCallback is changed before the old handler dispatches messages, resulting in the old handler thread being used to call the new message callback.

tobiasjs@:

1) https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java?rcl=849244ee60caf4ccc6a7defeddf0a221d4bdfb3a&l=176 would tend to suggest you're right. You can't reset a handler after one has been set.
2) also looks correct. mLock is dropped after Hander#sendMessage is called and reobtained when the message handler calls dispatchReceivedMessages, but mMessageCallback may have been changed in the interim. It seems like the callback should be stored as a member of the handler.

It's possible that 2) is not completely valid. It's possible for the second handler/callback to receive messages that caused the first handler to be triggered, depending on scheduling. I'm not sure whether that should be considered a problem.

The alternative would be to acquire the messages greedily, and pass them via msg.obj - this is what was done before https://codereview.chromium.org/2422793002 - I'm not certain why this was changed.

torne@:

Just banning calling it multiple times is probably easiest, though - I suspect usage of this API at all is extremely low, and it seems pretty unlikely that anyone is changing the callback (though we should still check).

Banning calling it multiple times would definitely be my favorite options, as I think that'll also allow some other simplifications/refactorings.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bcfc3dce5db3aeb16c1403fa8431bd313f7bf6d7

commit bcfc3dce5db3aeb16c1403fa8431bd313f7bf6d7
Author: Tobias Sargeant <tobiasjs@google.com>
Date: Tue Jan 02 20:03:49 2018

Allow AppWebMessagePort mHandler to be reset to null.

Nothing currently prevents a WebView application calling
setWebMessageCallback multiple times.

If a handler has previously been set via setMessageCallback, then it is
currently impossible to reset it back to null, meaning that the effect
of passing null for handler differs depending on previous calls.

Bug: 798449
Change-Id: I93dd8fbb3a5dce7be0b787ee78fecf739f7c4199
Reviewed-on: https://chromium-review.googlesource.com/847486
Commit-Queue: Tobias Sargeant <tobiasjs@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526525}
[modify] https://crrev.com/bcfc3dce5db3aeb16c1403fa8431bd313f7bf6d7/content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java

I think the problem with banning it from being called multiple times is that we didn't document it that way up front. Conservatively we'd only be able to change the behaviour for apps that target the SDK level where we made the documentation change. We don't really have a way of knowing whether there are any applications that would be broken by this change.

Torne, do you think it's justified to be more aggressive in this case?

If it's relatively straightforward to make calling it multiple times do the right thing then that's fine - suggesting banning it was just to avoid having to solve that, and I'm justifying it purely by speculating that it's rarely used enough to not break any apps (which I guess we should check if we actually are going to).

Either way sounds good to me (although somebody really should write some tests for the handler argument to setMessageCallback. In a very much not complete reimplementation of AppWebMessagePort in https://chromium-review.googlesource.com/c/chromium/src/+/853231 I'm completely ignoring the handler argument while still passing all the tests, so it doesn't seem like there is even a single test for that argument...

Is there any point doing the second half of this (addressing point 2, in comment 1) if you're planning on changing the implementation?

Good question. There would be some value in that it would help clarify what the expected behavior is, and hopefully result in tests to make sure any potential rewrite would match that behavior. But I could indeed try to just come up with something somewhat sensible directly...

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/323f3e4a1aa50e82dab2f79e5c2638e7ca590531

commit 323f3e4a1aa50e82dab2f79e5c2638e7ca590531
Author: Marijn Kruisselbrink <mek@chromium.org>
Date: Sat Jan 13 00:57:23 2018

Change android MessagePort code to no longer use blink::MessagePortChannel.

This follows similar changes to blink::MessagePort to use mojo's Connector
API directly, eventually paving the way to removing the now no longer needed
several hundred lines of fragile code that is blink::MessagePortChannel.

Also fixes a bug in AppWebMessagePort where if setMessageCallback is called
with a different thread/handler than the existing hanler, it was possible
for the old callback to be called on the new thread. With this fix a callback
will always be called on the same thread that was passed with it to
setMessageCallback (although there is still some race condition where the old
callback might be called on the old thread after the callback was changed).

Bug: 798449
Change-Id: I784b23f74a41cab6e37ca6ec6d645822ee135b0d
Reviewed-on: https://chromium-review.googlesource.com/853231
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Yusuf Ozuysal <yusufo@chromium.org>
Reviewed-by: Yuzhu Shen <yzshen@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529124}
[modify] https://crrev.com/323f3e4a1aa50e82dab2f79e5c2638e7ca590531/android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java
[modify] https://crrev.com/323f3e4a1aa50e82dab2f79e5c2638e7ca590531/android_webview/test/BUILD.gn
[modify] https://crrev.com/323f3e4a1aa50e82dab2f79e5c2638e7ca590531/chrome/android/java/src/org/chromium/chrome/browser/browserservices/PostMessageHandler.java
[modify] https://crrev.com/323f3e4a1aa50e82dab2f79e5c2638e7ca590531/content/browser/android/app_web_message_port.cc
[modify] https://crrev.com/323f3e4a1aa50e82dab2f79e5c2638e7ca590531/content/browser/android/app_web_message_port.h
[modify] https://crrev.com/323f3e4a1aa50e82dab2f79e5c2638e7ca590531/content/public/android/BUILD.gn
[modify] https://crrev.com/323f3e4a1aa50e82dab2f79e5c2638e7ca590531/content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java
[modify] https://crrev.com/323f3e4a1aa50e82dab2f79e5c2638e7ca590531/content/public/android/java/src/org/chromium/content_public/browser/MessagePort.java