1035420 - [geckoview] Implement Java <-> Javascript bridge

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GRE Core, enhancement)

Description

[Heath Borders]

GeckoView should have a bridge from javascript into Java similar to Android's WebView#addJavascriptInterface [1]. GeckoView should also have a bridge from Java into javascript, which would block the calling thread until the call completes, similar to iOS' -[UIWebView stringByEvaluatingJavaScriptFromString:] [2]. Callbacks from javascript into Java should always run on the main thread (unlike Android where they run on an unspecified background thread).

[1] http://developer.android.com/reference/android/webkit/WebView.html#addJavascriptInterface(java.lang.Object, java.lang.String)
[2] https://developer.apple.com/library/ios/documentation/uikit/reference/UIWebView_Class/Reference/Reference.html#//apple_ref/occ/instm/UIWebView/stringByEvaluatingJavaScriptFromString:

Comment 1

[Nick Alexander :nalexander [he/him]]

In Fennec, we pass messages [1] and it works pretty well.  I'd rather stick with explicit messaging than try to do some fancy reflection.  You can, of course, build reflection on top of message passing; that would be an awesome third-party library.  While I'm here, you can actually do this statically using an AnnotationProcessor, but down that path lies madness...

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/Messaging.jsm

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: geckoview-injectscript v0.1

Mostly simple patch that allows host apps to load JS files from their assests folder into the GeckoView chrome context. This works almost exactly like a bootstrap.js file. I used the same sandboxing code used for bootstrap.js files.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: geckoview-messaging v0.1

This patch allows JS scripts to send requests to Java, handled by GeckoViewChrome.onScriptMessage, which also has an async way to signal a callback in the JS side.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Some discussion here: https://mail.mozilla.org/pipermail/mobile-firefox-dev/2014-October/000909.html

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

I have an example of this working in my sample browser: https://github.com/mfinkle/geckobrowser
I'll push the changes once this code lands.

Comment 6

[Brian Nicholson (:bnicholson)]

Comment on attachment 8499115 [details] [diff] [review]
geckoview-injectscript v0.1

Review of attachment 8499115 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoView.java
@@ +241,5 @@
>          return Collections.unmodifiableList(browsers);
>      }
>  
> +    public void importScript(final String url) {
> +        if (url.startsWith("resource://android/assets")) {

Nit: Additional "/" after assets.

::: mobile/android/chrome/content/EmbedRT.js
@@ +33,5 @@
> +
> +    sandbox["console"] = new ConsoleAPI({ consoleID: "script/" + scriptURL });
> +    sandbox["GeckoView"] = {
> +      sendRequest: function(data, callback) {
> +        let message = { type: "GeckoView:Message", data : data };

Nit: s/data :/data:/

@@ +35,5 @@
> +    sandbox["GeckoView"] = {
> +      sendRequest: function(data, callback) {
> +        let message = { type: "GeckoView:Message", data : data };
> +        if (callback) {
> +          Messaging.sendRequestForResult(message).then(result => callback(result, null), error => callback(null, error));

Since we just transitioned away from this pattern in the Messaging API, it would be nice to avoid it here. I'd prefer having separate sendRequest/sendRequestForResult methods in GeckoView, then just directly returning Messaging.sendRequestForResult(message) for the latter.

I know you hate promises, but since this is a new API, I think callbacks are the wrong direction....

@@ +43,5 @@
> +      }
> +    };
> +
> +    sandbox.__SCRIPT_URI_SPEC__ = scriptURL;
> +    Cu.evalInSandbox("Components.classes['@mozilla.org/moz/jssubscript-loader;1'].createInstance(Components.interfaces.mozIJSSubScriptLoader).loadSubScript(__SCRIPT_URI_SPEC__);", sandbox, "ECMAv5");

Is this outer Cu.evalInSandbox() necessary? loadSubScript accepts a context as the second argument which can be used with the same effect as evalInSandbox (according to [1]). I would expect to be able to replace this huge line with this:

Services.scriptloader.loadSubScript(scriptURL, sandbox);

[1] https://developer.mozilla.org/en-US/Add-ons/Overlay_Extensions/XUL_School/Appendix_D:_Loading_Scripts#The_Sub-Script_Loader

Comment 7

[Brian Nicholson (:bnicholson)]

Comment on attachment 8499117 [details] [diff] [review]
geckoview-messaging v0.1

Review of attachment 8499117 [details] [diff] [review]:
-----------------------------------------------------------------

r- since this is all built over our obsolete messaging API, and we shouldn't be adding more code on top of it. Switching over to our newer EventCallback API should simplify things and allow you to remove the MessageResult/EventCallback redundancy.

::: mobile/android/base/GeckoView.java
@@ +69,5 @@
>                                  mode == View.IMPORTANT_FOR_ACCESSIBILITY_AUTO) {
>                                  GeckoAccessibility.sendAccessibilityEvent(message);
>                              }
> +                        } else if (event.equals("GeckoView:Message")) {
> +                            handleScriptMessage(message);

It looks like GeckoView is still using the obsolete GeckoEventListener API [1]. This should be using the newer NativeEventListener [2]. That will give you access to the EventCallback object (example at [3]), which will allow you to remove your MessageResult class.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/EventDispatcher.java?rev=986e8a9f6195#122
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/EventDispatcher.java?rev=986e8a9f6195#109
[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java?rev=5da210c60d38#617

@@ +337,5 @@
>  
> +    private void handleScriptMessage(final JSONObject message) throws JSONException {
> +        if (mChromeDelegate != null) {
> +            JSONObject data = message.optJSONObject("data");
> +            mChromeDelegate.onScriptMessage(GeckoView.this, data, new MessageResult(message));

handleScriptMessage should take a second argument, EventCallback, and pass it through to onScriptMessage instead of a new MessageResult. This will EventCallback will come from the handleMessage in GeckoView once making the changes above.

Of course, if you want another layer of abstraction for GeckoView messaging, you could pass something like "new MessageResult(callback)", where MessageResult is just a wrapper over our existing EventCallback implementation.

@@ +535,5 @@
>  
> +    /* Provides a means for the client to respond to a script message with some data.
> +     * An instance of this class is passed to GeckoViewChrome.onScriptMessage.
> +     */
> +    public class MessageResult {

You shouldn't need any of this -- we already have EventCallback/GeckoEventCallback.

@@ +562,5 @@
> +            JSONObject result = makeResult(RESULT_SUCCESS);
> +            try {
> +                result.put("data", data);
> +            } catch(JSONException ex) { }
> +            EventDispatcher.sendResponse(mMessage, result);

EventDispatcher.sendResponse is obsolete -- you should forward the EventCallback given to you by the listener in GeckoView. But this won't matter if you ditch this class anyway.

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8499115 [details] [diff] [review]
geckoview-injectscript v0.1

Review of attachment 8499115 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoView.java
@@ +246,5 @@
> +            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("GeckoView:ImportScript", url));
> +            return;
> +        }
> +
> +        throw new IllegalArgumentException("Must import script from 'resources://android/assets' location.");

Why?  Let's really empower folks to do things.  If somebody wants to slurp a script from Java, or build it some other way, why stop them?

::: mobile/android/chrome/content/EmbedRT.js
@@ +5,5 @@
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "ConsoleAPI",
> +                                  "resource://gre/modules/devtools/Console.jsm");
> +
> +var EmbedRT = {

A whole file with nary a comment, let alone a comment explaining what the file is for.  Also, I think you want RT to stand for run-time, and I see nothing "runtime-y" about this.

@@ +26,5 @@
> +
> +    let sandbox = new Cu.Sandbox(principal,
> +      {
> +        sandboxName: scriptURL,
> +        wantGlobalProperties: ["indexedDB"]

Lift this to a constant.  Is this list complete?  Even if it is, I'd consider adding a way for the message to fiddle this.  Why limit consumers without a reason?

@@ +47,5 @@
> +    Cu.evalInSandbox("Components.classes['@mozilla.org/moz/jssubscript-loader;1'].createInstance(Components.interfaces.mozIJSSubScriptLoader).loadSubScript(__SCRIPT_URI_SPEC__);", sandbox, "ECMAv5");
> +
> +    this._scopes[scriptURL] = sandbox;
> +
> +    if ("load" in sandbox) {

You already catch errors below.  Won't this just make it hard to debug a change in the sandbox interface?

Comment 9

[Nick Alexander :nalexander [he/him]]

So I want to take a step back and understand this approach.  My main question is, why are we sandboxing *anything*?  Why not load scripts into the runtime, and be done with it?  I see no need to protect embedders from themselves.

My second question is, why are we enforcing some particular choice of message passing between JS and GeckoView embedders?  I think bnicholson's r- is saying, "don't do this: just use what we have" but I'm not 100% certain.  I see lots of value in making it really easy to do easy things, and maybe that's what this does; but I think they should be built from exposing the more powerful primitives.  In this case, those primitives are the gory glory of the Java <-> JS event handling that Fennec uses.

Comment 10

[Brian Nicholson (:bnicholson)]

(In reply to Nick Alexander :nalexander from comment #9)
> My second question is, why are we enforcing some particular choice of
> message passing between JS and GeckoView embedders?  I think bnicholson's r-
> is saying, "don't do this: just use what we have" but I'm not 100% certain. 
> I see lots of value in making it really easy to do easy things, and maybe
> that's what this does; but I think they should be built from exposing the
> more powerful primitives.  In this case, those primitives are the gory glory
> of the Java <-> JS event handling that Fennec uses.

So this brings us back to the discussion here: https://bugzilla.mozilla.org/show_bug.cgi?id=1052841#c5.

An argument against exposing all of the gory details is that we don't have any frozen APIs, and our messages are constantly evolving. Exposing our messaging API without some level of abstraction is begging for things to get broken.

Comment 11

[Nick Alexander :nalexander [he/him]]

(In reply to Brian Nicholson (:bnicholson) from comment #10)
> (In reply to Nick Alexander :nalexander from comment #9)
> > My second question is, why are we enforcing some particular choice of
> > message passing between JS and GeckoView embedders?  I think bnicholson's r-
> > is saying, "don't do this: just use what we have" but I'm not 100% certain. 
> > I see lots of value in making it really easy to do easy things, and maybe
> > that's what this does; but I think they should be built from exposing the
> > more powerful primitives.  In this case, those primitives are the gory glory
> > of the Java <-> JS event handling that Fennec uses.
> 
> So this brings us back to the discussion here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1052841#c5.

It gives me some faith in myself that my position is more or less consistent: https://bugzilla.mozilla.org/show_bug.cgi?id=1052841#c3

> An argument against exposing all of the gory details is that we don't have
> any frozen APIs, and our messages are constantly evolving. Exposing our
> messaging API without some level of abstraction is begging for things to get
> broken.

Anybody who wants to plumb the message flow is in for a world of hurt.  But the APIs to register and handle messages on already can't move too fast on the JS side, 'cuz they're exposed to JS already.  If we're really concerned about breaking GV consumers, okay, wrap it up.

But the only way to actually keep this working is to land with tests.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Brian Nicholson (:bnicholson) from comment #6)

> ::: mobile/android/base/GeckoView.java

> > +    public void importScript(final String url) {
> > +        if (url.startsWith("resource://android/assets")) {
> 
> Nit: Additional "/" after assets.

Makes sense

> ::: mobile/android/chrome/content/EmbedRT.js

> > +    sandbox["GeckoView"] = {
> > +      sendRequest: function(data, callback) {
> > +        let message = { type: "GeckoView:Message", data : data };
> 
> Nit: s/data :/data:/
> 
> @@ +35,5 @@
> > +    sandbox["GeckoView"] = {
> > +      sendRequest: function(data, callback) {
> > +        let message = { type: "GeckoView:Message", data : data };
> > +        if (callback) {
> > +          Messaging.sendRequestForResult(message).then(result => callback(result, null), error => callback(null, error));
> 
> Since we just transitioned away from this pattern in the Messaging API, it
> would be nice to avoid it here. I'd prefer having separate
> sendRequest/sendRequestForResult methods in GeckoView, then just directly
> returning Messaging.sendRequestForResult(message) for the latter.
> 
> I know you hate promises, but since this is a new API, I think callbacks are
> the wrong direction....

This part should not have apeared in this patch. It should only be in the second patch. I guess I used | hg qcrecord xxx | incorectly. I'll act like it''s in the messaging part

> > +    sandbox.__SCRIPT_URI_SPEC__ = scriptURL;
> > +    Cu.evalInSandbox("Components.classes['@mozilla.org/moz/jssubscript-loader;1'].createInstance(Components.interfaces.mozIJSSubScriptLoader).loadSubScript(__SCRIPT_URI_SPEC__);", sandbox, "ECMAv5");
> 
> Is this outer Cu.evalInSandbox() necessary? loadSubScript accepts a context
> as the second argument which can be used with the same effect as
> evalInSandbox (according to [1]). I would expect to be able to replace this
> huge line with this:

I want to treat this script as much as possible as a bootstrap.js file, which uses this aproach. Reasons given here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4329

I can add the same comment.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Brian Nicholson (:bnicholson) from comment #7)
> Comment on attachment 8499117 [details] [diff] [review]
> geckoview-messaging v0.1
> 
> Review of attachment 8499117 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- since this is all built over our obsolete messaging API, and we shouldn't
> be adding more code on top of it. Switching over to our newer EventCallback
> API should simplify things and allow you to remove the
> MessageResult/EventCallback redundancy.

There is one main reason I ended up using the older messaging approach: I did not want to expose internal Gecko/Fennec implementation details to embedders.

EventCallback is tied to internal messaging processes. We already changed it once. I saw no reason at all to expose the nuances of our internal code to embedders. Doing so makes EventCallback a "public API" and locks us into some choices. MessageResult is a super simple wrapper that does one job. It's very easy for embedders to use.

Another internal implementation detail that the new messaging system introduces is NativeJSObject. I did not feel like forcing embedders to use more of our internal coding classes. JSONObject is part of the normal Android API.

We are already hiding the NativeEventListener and GeckoEventListener impls in GeckoView from embedders.

I'd like us to consdier these reasons before moving ahead with your suggestions.

> ::: mobile/android/base/GeckoView.java
> @@ +69,5 @@
> >                                  mode == View.IMPORTANT_FOR_ACCESSIBILITY_AUTO) {
> >                                  GeckoAccessibility.sendAccessibilityEvent(message);
> >                              }
> > +                        } else if (event.equals("GeckoView:Message")) {
> > +                            handleScriptMessage(message);
> 
> It looks like GeckoView is still using the obsolete GeckoEventListener API
> [1]. This should be using the newer NativeEventListener [2]. That will give
> you access to the EventCallback object (example at [3]), which will allow
> you to remove your MessageResult class.

GeckoView is using the older GeckoEventListener because a message (like "Content:LoadError") can only be registered with GeckoEventListener or NativeEventListener... but not both. Until Fennec completely switches over, or Fennec uses GeckoView directly and stop using the raw messages, we can't change GeckoView to completely use NativeEventListener.

> > +    private void handleScriptMessage(final JSONObject message) throws JSONException {
> > +        if (mChromeDelegate != null) {
> > +            JSONObject data = message.optJSONObject("data");
> > +            mChromeDelegate.onScriptMessage(GeckoView.this, data, new MessageResult(message));
> 
> handleScriptMessage should take a second argument, EventCallback, and pass
> it through to onScriptMessage instead of a new MessageResult. This will
> EventCallback will come from the handleMessage in GeckoView once making the
> changes above.

I'll look into the first part of this suggestion.

> Of course, if you want another layer of abstraction for GeckoView messaging,
> you could pass something like "new MessageResult(callback)", where
> MessageResult is just a wrapper over our existing EventCallback
> implementation.

I strongly want to keep MessageResult.

> You shouldn't need any of this -- we already have
> EventCallback/GeckoEventCallback.

Like I said above, I want this.

> EventDispatcher.sendResponse is obsolete -- you should forward the
> EventCallback given to you by the listener in GeckoView. But this won't
> matter if you ditch this class anyway.

I'll look into using EventCallback

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Nick Alexander :nalexander from comment #8)

> ::: mobile/android/base/GeckoView.java

> > +        throw new IllegalArgumentException("Must import script from 'resources://android/assets' location.");
> 
> Why?  Let's really empower folks to do things.  If somebody wants to slurp a
> script from Java, or build it some other way, why stop them?

I'll think about it.

> ::: mobile/android/chrome/content/EmbedRT.js

> A whole file with nary a comment, let alone a comment explaining what the
> file is for.  Also, I think you want RT to stand for run-time, and I see
> nothing "runtime-y" about this.

Comments should be added. I expect this file to grow, like WebappRT.js is used for running WebApps. I hope to grow a BrowserRT.js sometime too. The goal is to remove unneeded code from browser.js.

> > +    let sandbox = new Cu.Sandbox(principal,
> > +      {
> > +        sandboxName: scriptURL,
> > +        wantGlobalProperties: ["indexedDB"]
> 
> Lift this to a constant.  Is this list complete?  Even if it is, I'd
> consider adding a way for the message to fiddle this.  Why limit consumers
> without a reason?

This is what we do for bootstrap.js files. I have no reason to do more.

> @@ +47,5 @@
> > +    Cu.evalInSandbox("Components.classes['@mozilla.org/moz/jssubscript-loader;1'].createInstance(Components.interfaces.mozIJSSubScriptLoader).loadSubScript(__SCRIPT_URI_SPEC__);", sandbox, "ECMAv5");
> > +
> > +    this._scopes[scriptURL] = sandbox;
> > +
> > +    if ("load" in sandbox) {
> 
> You already catch errors below.  Won't this just make it hard to debug a
> change in the sandbox interface?

Again. It's what we do for bootstrap.js scripts, where the sandbox can potentially eat exceptions (I'm guessing). But I am stealing the approach without spending the time to dig into the nuances. I (we) can learn the nuances as we experience them.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Nick Alexander :nalexander from comment #9)
> So I want to take a step back and understand this approach.  My main
> question is, why are we sandboxing *anything*?  Why not load scripts into
> the runtime, and be done with it?  I see no need to protect embedders from
> themselves.

The sandbox is setup to give scripts access to everything, just like a bootstrap.js based add-on. The scripts has full access that allow them to break stuff.

> My second question is, why are we enforcing some particular choice of
> message passing between JS and GeckoView embedders?  I think bnicholson's r-
> is saying, "don't do this: just use what we have" but I'm not 100% certain. 
> I see lots of value in making it really easy to do easy things, and maybe
> that's what this does; but I think they should be built from exposing the
> more powerful primitives.  In this case, those primitives are the gory glory
> of the Java <-> JS event handling that Fennec uses.

As I responded to Brian, I want to create some simplicity, some isolation of our internal code, some minimizing of API surface, and some protection against locking us into public APIs.

I have stated previously, in general and specifically about GeckoView, that simple API abstractions should exist to make some shitty look and feel like something pretty. I do not want to force embedders to work in to code that we work with. By minimizing the API surface we also create something that can easily be documented, even with JavaDoc. Try doing that with our raw internal code.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #13)

> > EventDispatcher.sendResponse is obsolete -- you should forward the
> > EventCallback given to you by the listener in GeckoView. But this won't
> > matter if you ditch this class anyway.
> 
> I'll look into using EventCallback

It looks like I can't use EventCallback with GeckoEventListener, and I want GeckoEventListener for JSONObject. I suppose I could use NativeEventListener and the EventCallback (wrapped by MessageResult), but use a NativeJSObject <-> JSONObject system. I'm honesty not in a hurry to do soemthing like that though. I am still not comfortable using NativeJSObject in a public API.

Comment 17

[Roman Tsisyk]

Hi,

I compiled GeckoView library with Mark's patches above.
My app calls geckoView.importScript("resource://android/assets/android.js") to load initialization script:
---
function load(params) {
    console.log("window", params.toString());
    console.log("window", params.window.toString());
    console.log("window.document", params.window.document.toString());
    console.log("android.js loaded");
}
--

How can I add custom JavaScript function available in all browsers tabs like, say, `window' or `console'? I want to use this custom API from my html5 app.

Thanks!

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: geckoview-injectscript v0.2

Addressed comments. Carrying r+ forward.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: geckoview-messaging v0.2

The crux of this patch remains:
* Internally using deprecated GeckoEventListener because I want to pass JSONObject to consumers.
* MessageResult is wrapping the EventCallback/sendResponse system to minimize API exposure to internals

I did convert the messaging API exposed to scripts to sendRequest/sendRequestForResult which uses promises.

Back to r? to see where we can move forward at this point.

Comment 20

[Brian Nicholson (:bnicholson)]

Comment on attachment 8501953 [details] [diff] [review]
geckoview-messaging v0.2

Review of attachment 8501953 [details] [diff] [review]:
-----------------------------------------------------------------

As discussed, it's worth looking into using NativeJSObject.toBundle() so clients can deal with Android-friendly Bundles instead of the more specific NativeJSObject or JSONObject classes. While we already have NativeJSObject -> Bundle conversion, it looks like we'll need to add support for going in the Bundle -> JSONObject direction.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: geckoview-messaging v0.3

This patch:
* Moves "GeckoView:Message" to be a NativeEventListener and MessageResult uses an EventCallback.
* More tightly focuses the ThreadUtils.postToUiThread since NativeJSObjects must be accessed on the Gecko thread. "Accessibility:Ready" already has code to post to the UI thread in it's handler.
* Switch to use Bundles for incoming requests and outgoing responses. We use NativeJSObject.toBundle() for the incoming requests and I have a simple Bundle->JSON helper for the outgoing responses.

I tested in a GeckoView sample application using the sendRequest and sendRequestForResult APIs. Worked fine.

Notes for the API:

public void onScriptMessage(GeckoView view, Bundle data, GeckoView.MessageResult result);

1. "data" should never be null. If "data" is null, there is no reason to fire the method.
2. "result" can be null when handling sendRequest calls, since the underlying EventCallback is null.

I might file a followup to improve the Bundle->JSON helper.

Comment 22

[Brian Nicholson (:bnicholson)]

Comment on attachment 8504712 [details] [diff] [review]
geckoview-messaging v0.3

Review of attachment 8504712 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with an explanation of why we require data to be non-null (and we should catch this in JS, not Java, as I commented below).

::: mobile/android/base/GeckoView.java
@@ +89,5 @@
> +                } else if ("GeckoView:Message".equals(event)) {
> +                    // We need to pull out the bundle while on the Gecko thread.
> +                    NativeJSObject json = message.optObject("data", null);
> +                    if (json == null) {
> +                        // Must have payload to call the message handler.

I'm confused -- why do we require the script to send data? What if it just wants to send a simple message?

If this is really a requirement, we should check this on the JS side before sending anything over at all. Otherwise, we'll leak if there's a registered callback that we don't respond to, and it's pointless to send if we don't handle it anyway.

@@ +93,5 @@
> +                        // Must have payload to call the message handler.
> +                        return;
> +                    }
> +                    final Bundle data = json.toBundle();
> +                    ThreadUtils.postToUiThread(new Runnable() {

I like this. Android is designed such that every callback happens on the UI thread, and by following that pattern for clients, that will minimize any surprises. If they have background work that needs to be done off the main thread, they can take care of that themselves.

@@ +96,5 @@
> +                    final Bundle data = json.toBundle();
> +                    ThreadUtils.postToUiThread(new Runnable() {
> +                        @Override
> +                        public void run() {
> +                            handleScriptMessage(data, callback);

Nothing you're doing wrong here, but our callback contract concerns me. Remember that listeners responding to sendRequestForResult() must use the callback to respond *exactly* once; not responding at all will cause a leak, and responding more than once will crash. I'm worried this will be a common source of errors for GeckoView clients.

@@ +103,2 @@
>                  }
> +            } catch (Exception e) {

Catching generic Exceptions is usually a red flag, and it can make it hard to find bugs. Say we introduce a NPE here -- we won't be able to find it easily without digging into the logs.

Nothing here throws a checked exception (e.g., JSONException), so do we need this catch block at all?

@@ +547,5 @@
>  
> +    /* Provides a means for the client to respond to a script message with some data.
> +     * An instance of this class is passed to GeckoViewChrome.onScriptMessage.
> +     */
> +    public class MessageResult {

Nit: MessageResult isn't a result. It's a callback interface used to send the result.

@@ +557,5 @@
> +            }
> +            mCallback = callback;
> +        }
> +
> +        private void bundleToJSON(Bundle data, JSONObject json) {

Instead of taking a json arg, can you just create a new JSONObject() here and return it?

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Brian Nicholson (:bnicholson) from comment #22)
> Comment on attachment 8504712 [details] [diff] [review]
> geckoview-messaging v0.3
> 
> Review of attachment 8504712 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with an explanation of why we require data to be non-null (and we
> should catch this in JS, not Java, as I commented below).
> 
> ::: mobile/android/base/GeckoView.java
> @@ +89,5 @@
> > +                } else if ("GeckoView:Message".equals(event)) {
> > +                    // We need to pull out the bundle while on the Gecko thread.
> > +                    NativeJSObject json = message.optObject("data", null);
> > +                    if (json == null) {
> > +                        // Must have payload to call the message handler.
> 
> I'm confused -- why do we require the script to send data? What if it just
> wants to send a simple message?

Take a closer look at the signature of onScriptMessage. There is only the data Bundle. There is no other hint about what the message is. Even a simple message will still require the data Bundle.

> If this is really a requirement, we should check this on the JS side before
> sending anything over at all. Otherwise, we'll leak if there's a registered
> callback that we don't respond to, and it's pointless to send if we don't
> handle it anyway.

Good catch. I'll make the change.

> > +                    final Bundle data = json.toBundle();
> > +                    ThreadUtils.postToUiThread(new Runnable() {
> > +                        @Override
> > +                        public void run() {
> > +                            handleScriptMessage(data, callback);
> 
> Nothing you're doing wrong here, but our callback contract concerns me.
> Remember that listeners responding to sendRequestForResult() must use the
> callback to respond *exactly* once; not responding at all will cause a leak,
> and responding more than once will crash. I'm worried this will be a common
> source of errors for GeckoView clients.

I can null out the EventHandler after the first use, and check for null the next time. This will avoid the crash.

> >                  }
> > +            } catch (Exception e) {
> 
> Catching generic Exceptions is usually a red flag, and it can make it hard
> to find bugs. Say we introduce a NPE here -- we won't be able to find it
> easily without digging into the logs.
> 
> Nothing here throws a checked exception (e.g., JSONException), so do we need
> this catch block at all?

Both handleMessage methods have this check. If we want to remove it, I can file a new bug to remove it from both places.

> > +
> > +        private void bundleToJSON(Bundle data, JSONObject json) {
> 
> Instead of taking a json arg, can you just create a new JSONObject() here
> and return it?

Done

Comment 24

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #23)
> I can null out the EventHandler after the first use, and check for null the
> next time. This will avoid the crash.

Well, we choose to crash specifically because we don't want to cover up errors. If I were a consumer of GeckoView, I'd rather it just crash (indicating that I'm using the API wrong) over my response being ignored in some places. We just need to make it clear in the docs that responding exactly once using the callback is a strict requirement.

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

Kept the crashes on re-use
https://hg.mozilla.org/integration/fx-team/rev/b4bfcde216b9
https://hg.mozilla.org/integration/fx-team/rev/f7c918458900

Comment 26

[Carsten Book [:Tomcat]]

Hi Mark, sorry had to back this out for bustages like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=906122&repo=fx-team on Android

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

Stupid last-second patch change bustage. Fixed and built fine:
https://hg.mozilla.org/integration/fx-team/rev/b33d90f51fae
https://hg.mozilla.org/integration/fx-team/rev/9c6cd6e14e74

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b33d90f51fae
https://hg.mozilla.org/mozilla-central/rev/9c6cd6e14e74

Comment 29

[Ali Kasimoglu]

We are implementing GeckoView in our project. But we have a problem with clear cookies.
Can somebody say, what class clear cookies in geckoview? Urgent! Best Regards

Comment 30

[Nick Alexander :nalexander [he/him]]

(In reply to Ali Kasimoglu from comment #29)
> We are implementing GeckoView in our project. But we have a problem with
> clear cookies.
> Can somebody say, what class clear cookies in geckoview? Urgent! Best Regards

I answered via email.  In short, use https://dxr.mozilla.org/mozilla-central/source/mobile/android/modules/Sanitizer.jsm and https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoView.java#491 to trigger the message from Java.

Comment 31

[Ali Kasimoglu]

(In reply to Nick Alexander :nalexander from comment #30)
> (In reply to Ali Kasimoglu from comment #29)
> > We are implementing GeckoView in our project. But we have a problem with
> > clear cookies.
> > Can somebody say, what class clear cookies in geckoview? Urgent! Best Regards
> 
> I answered via email.  In short, use
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/modules/
> Sanitizer.jsm and
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/
> mozilla/gecko/GeckoView.java#491 to trigger the message from Java.

Thank you so much Nick. I im looking it now.

Comment 32

[qiulong li]

Hi,I want to use Geckoview to call javascript functions just like using addJavaScriptInterface in android WebView,please tell me how to do,thank you!!

Comment 33

[qiulong li]

(In reply to qiulong li from comment #32)
> Hi,I want to use Geckoview to call javascript functions just like using
> addJavaScriptInterface in android WebView,please tell me how to do,thank
> you!!

oh,I found something,I can use onScriptMessage to pass parameters from Geckoview to js,and use onAlert to pass parameters from js to Geckoview,am I right?

Comment 34

[qiulong li]

Hi,I have another question.
I want to play Adobe Flash with Geckoview,and it's working.But before loading the swf,I got this message,"tap here to active plugin".
So,how can I remove this message and play Flash directly?
Please tell me,Thank you!