HTML MessagePort as mojo::MessagePipeHandle

content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java

Index: content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java b/content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java
index 30e50d02e8a3cdbd6f60a0ac75b155cf90cd1162..62a2b952d3b33187aff9de24c420bf76d9493cd9 100644
--- a/content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java
+++ b/content/public/android/java/src/org/chromium/content/browser/AppWebMessagePort.java
@@ -9,6 +9,8 @@ import android.os.Looper;
 import android.os.Message;
 
 import org.chromium.base.Log;
+import org.chromium.base.annotations.CalledByNative;
+import org.chromium.base.annotations.JNINamespace;
 import org.chromium.content_public.browser.MessagePort;
 
 import java.util.Arrays;
@@ -73,25 +75,13 @@ import java.util.Arrays;
  * transferring data. As a return, it simplifies implementation and prevents hard
  * to debug, racy corner cases while receiving/sending data.
  */
-public class AppWebMessagePort implements MessagePort, PostMessageSender.PostMessageSenderDelegate {
-    private static final String TAG = "MessagePort";
-    private static final int PENDING = -1;
+@JNINamespace("content")
+public class AppWebMessagePort implements MessagePort {
+    private static final String TAG = "AppWebMessagePort";
+    private static final long UNINITIALIZED_PORT_NATIVE_PTR = 0;
 
-    // the what value for POST_MESSAGE
-    private static final int POST_MESSAGE = 1;
-
-    private static class PostMessageFromWeb {
-        public AppWebMessagePort port;
-        public String message;
-        public AppWebMessagePort[] sentPorts;
-
-        public PostMessageFromWeb(
-                AppWebMessagePort port, String message, AppWebMessagePort[] sentPorts) {
-            this.port = port;
-            this.message = message;
-            this.sentPorts = sentPorts;
-        }
-    }
+    // The |what| value for handleMessage.
+    private static final int MESSAGES_AVAILABLE = 1;
 
     // Implements the handler to handle messageport messages received from web.
     // These messages are received on IO thread and normally handled in main
@@ -102,9 +92,9 @@ public class AppWebMessagePort implements MessagePort, PostMessageSender.PostMes
         }
         @Override
         public void handleMessage(Message msg) {
-            if (msg.what == POST_MESSAGE) {
-                PostMessageFromWeb m = (PostMessageFromWeb) msg.obj;
-                m.port.onMessage(m.message, m.sentPorts);
+            if (msg.what == MESSAGES_AVAILABLE) {
+                AppWebMessagePort port = (AppWebMessagePort) msg.obj;
+                port.dispatchReceivedMessages();
                 return;
             }
             throw new IllegalStateException("undefined message");
@@ -114,35 +104,39 @@ public class AppWebMessagePort implements MessagePort, PostMessageSender.PostMes
     private static final MessageHandler sDefaultHandler =
             new MessageHandler(Looper.getMainLooper());
 
-    private int mPortId = PENDING;
+    private long mNativeAppWebMessagePort = UNINITIALIZED_PORT_NATIVE_PTR;
+
     private MessageCallback mMessageCallback;
-    private AppWebMessagePortService mMessagePortService;
     private boolean mClosed;
     private boolean mTransferred;
     private boolean mStarted;
-    private boolean mReleasedMessages;
-    private PostMessageSender mPostMessageSender;
     private MessageHandler mHandler;
     private final Object mLock = new Object();
 
-    public AppWebMessagePort(AppWebMessagePortService messagePortService) {
-        mMessagePortService = messagePortService;
-        mPostMessageSender = new PostMessageSender(this, mMessagePortService);
-        mMessagePortService.addObserver(mPostMessageSender);
+    // Called to create an entangled pair of ports.
+    public static AppWebMessagePort[] createPair() {
+        AppWebMessagePort[] ports =
+            new AppWebMessagePort[] { new AppWebMessagePort(), new AppWebMessagePort() };
+        nativeInitializeAppWebMessagePortPair(ports);
+        return ports;
     }
 
     @Override
     public boolean isReady() {
-        return mPortId != PENDING;
+        return mNativeAppWebMessagePort != UNINITIALIZED_PORT_NATIVE_PTR;
     }
 
-    public int portId() {
-        return mPortId;
+    @CalledByNative
+    private void setNativeAppWebMessagePort(long nativeAppWebMessagePort) {
+        mNativeAppWebMessagePort = nativeAppWebMessagePort;
     }
 
-    public void setPortId(int id) {
-        mPortId = id;
-        releaseMessages();
+    @CalledByNative
+    private long releaseNativePortForTransfer() {

[sgurun-gerrit only, 2017/02/08 21:47:50]

who is deleting the native message port after it is transferred? unless is
triggering closemessageport() (which I don't see), we are leaking them.

[darin (slow to review), 2017/02/09 00:13:17]

Thanks, I need to delete the native side from
AppWebMessagePort::UnwrapJavaArray. Presently, I am just leaking it. Good catch!

+        mTransferred = true;
+        long port = mNativeAppWebMessagePort;
+        mNativeAppWebMessagePort = UNINITIALIZED_PORT_NATIVE_PTR;
+        return port;
     }
 
     @Override
@@ -154,18 +148,16 @@ public class AppWebMessagePort implements MessagePort, PostMessageSender.PostMes
             if (mClosed) return;
             mClosed = true;
         }
-        // If the port is already ready, and no messages are waiting in the
-        // queue to be transferred, onPostMessageQueueEmpty() callback is not
-        // received (it is received only after messages are purged). In this
-        // case do the cleanup here.
-        if (isReady() && mPostMessageSender.isMessageQueueEmpty()) {
+        if (isReady()) {
             cleanup();
         }
     }
 
     @Override
     public boolean isClosed() {
-        return mClosed;
+        synchronized (mLock) {
+            return mClosed;
+        }
     }
 
     @Override
@@ -173,10 +165,6 @@ public class AppWebMessagePort implements MessagePort, PostMessageSender.PostMes
         return mTransferred;
     }
 
-    public void setTransferred() {
-        mTransferred = true;
-    }
-
     @Override
     public boolean isStarted() {
         return mStarted;
@@ -192,43 +180,41 @@ public class AppWebMessagePort implements MessagePort, PostMessageSender.PostMes
                 mHandler = new MessageHandler(handler.getLooper());
             }
         }
-        releaseMessages();
+        nativeStartReceivingMessages(mNativeAppWebMessagePort);
     }
 
-    // Only called on IO thread.
-    public void onReceivedMessage(String message, AppWebMessagePort[] sentPorts) {
+    // Called on a background thread.
+    @CalledByNative
+    private void onMessagesAvailable() {
         synchronized (mLock) {
-            PostMessageFromWeb m = new PostMessageFromWeb(this, message, sentPorts);
             Handler handler = mHandler != null ? mHandler : sDefaultHandler;
-            Message msg = handler.obtainMessage(POST_MESSAGE, m);
+            Message msg = handler.obtainMessage(MESSAGES_AVAILABLE, this);
             handler.sendMessage(msg);
         }
     }
 
-    private void releaseMessages() {
-        if (mReleasedMessages || !isReady() || mMessageCallback == null) {
-            return;
-        }
-        mReleasedMessages = true;
-        mMessagePortService.releaseMessages(mPortId);
-    }
-
     // This method may be called on a different thread than UI thread.
-    public void onMessage(String message, AppWebMessagePort[] ports) {
+    @CalledByNative
+    private void onReceivedMessage(String message, AppWebMessagePort[] ports) {
         synchronized (mLock) {
             if (isClosed()) {
-                Log.w(TAG, "Port [" + mPortId + "] received message in closed state");
+                Log.w(TAG, "Port [" + mNativeAppWebMessagePort
+                        + "] received message in closed state");
                 return;
             }
             if (mMessageCallback == null) {
-                Log.w(TAG,
-                        "No handler set for port [" + mPortId + "], dropping message " + message);
+                Log.w(TAG, "No handler set for port [" + mNativeAppWebMessagePort
+                        + "], dropping message " + message);
                 return;
             }
             mMessageCallback.onMessage(message, ports);
         }
     }
 
+    private void dispatchReceivedMessages() {
+        nativeDispatchReceivedMessages(mNativeAppWebMessagePort);
+    }
+
     @Override
     public void postMessage(String message, MessagePort[] sentPorts) throws IllegalStateException {
         if (isClosed() || isTransferred()) {
@@ -240,37 +226,29 @@ public class AppWebMessagePort implements MessagePort, PostMessageSender.PostMes
                 if (port.equals(this)) {
                     throw new IllegalStateException("Source port cannot be transferred");
                 }
+                if (port.isClosed() || port.isTransferred()) {
+                    throw new IllegalStateException("Port is already closed or transferred");
+                }
+                if (port.isStarted()) {
+                    throw new IllegalStateException("Port is already started");
+                }
             }
             ports = Arrays.copyOf(sentPorts, sentPorts.length, AppWebMessagePort[].class);
         }
         mStarted = true;
-        mPostMessageSender.postMessage(null, message, null, ports);
+        nativePostMessage(mNativeAppWebMessagePort, message, ports);

[sgurun-gerrit only, 2017/02/08 21:47:50]

I wonder if these changes create an opportunity for further optimization here
wrt to how we transfer ports. The current logic is:
postMessage -> nativePostMessage -> UnwrapJavaArray { for each port transferred:
Go back to java i.e. releaseNativePortForTransfer: set port as transferred and
uninitialize native ptr. }

Can we do:
postMessage { for each port: mark as transferred/ store native ptr/ reset native
ptr} -> nativePostMessage passing native ptrs. 

But then I guess the question is messageportprovider::postmessagetoframe can
also transfer ports. Himm, ok, nevermind.

[darin (slow to review), 2017/02/09 00:13:17]

Yeah, exactly. I had the same thoughts. I landed on this for the reason you
state. 

     }
 
-    // Implements PostMessageSender.PostMessageSenderDelegate interface method.
-    @Override
-    public boolean isPostMessageSenderReady() {
-        return isReady();
-    }
-
-    // Implements PostMessageSender.PostMessageSenderDelegate interface method.
-    @Override
-    public void onPostMessageQueueEmpty() {
-        if (isClosed()) {
-            cleanup();
-        }
+    private void cleanup() {
+        nativeCloseMessagePort(mNativeAppWebMessagePort);
+        mNativeAppWebMessagePort = UNINITIALIZED_PORT_NATIVE_PTR;
     }
 
-    // Implements PostMessageSender.PostMessageSenderDelegate interface method.
-    @Override
-    public void postMessageToWeb(
-            String frameName, String message, String targetOrigin, int[] sentPortIds) {
-        mMessagePortService.postMessage(mPortId, message, sentPortIds);
-    }
+    private static native void nativeInitializeAppWebMessagePortPair(AppWebMessagePort[] ports);
 
-    private void cleanup() {
-        mMessagePortService.removeObserver(mPostMessageSender);
-        mPostMessageSender = null;
-        mMessagePortService.closePort(mPortId);
-    }
+    private native void nativeCloseMessagePort(long nativeAppWebMessagePort);
+    private native void nativePostMessage(long nativeAppWebMessagePort, String message,
+                                          AppWebMessagePort[] ports);
+    private native void nativeDispatchReceivedMessages(long nativeAppWebMessagePort);
+    private native void nativeStartReceivingMessages(long nativeAppWebMessagePort);
 }