993195 - Add support for sending callbacks from NativeJSObject listeners

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

EventDispatcher#sendResponse takes a JSONObject message as its first parameter, meaning it can be used only with the deprecated GeckoEventListener API. We should update sendResponse to support NativeJSObject.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Add only one observer for messages with callbacks

Minor fix to set up one listener for responses instead of two.

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Add GeckoCallback parameter to handleMessage

My first attempt simply copied sendResponse/sendError to accept a NativeJSObject in addition to JSONObject, but that won't work since the callbacks may be executed after the handleMessage(), meaning the message may have been disposed.

Since we're changing our handleMessage API anyway, this seemed like a good opportunity to build in proper event response callbacks. With this patch, handleMessage now takes a third argument, a GeckoCallback, which listeners can use to send a response back to Gecko.

This patch has several changes from our old approach:
1) We no longer have to hold onto the raw message given to us to use sendResponse.
2) If we try to send a response or error more than once, we throw an exception.
3) If there aren't any listeners on the Java side to respond to the callback, we make sure to send a cancel response to avoid leaking observers. We could consider throwing here too, though I figured there may be valid situations where response listeners aren't attached. Also note that this still isn't perfect; the existence of listeners doesn't guarantee that any of those listeners will actually send a response. For debug builds, we might want to consider overriding finalize() in GeckoCallback to enforce that sent is true when it's destroyed.

Note that in our build system, classes in util/ cannot reference other o.m.g. classes. The weird GeckoCallback interface split here is used as a workaround.

Comment 3

[Brian Nicholson (:bnicholson)]

Attachment: Add GeckoCallback parameter to handleMessage, v2

Minor fix to use optString for the GUID.

Comment 4

[Wesley Johnston (:wesj)]

Comment on attachment 8403011 [details] [diff] [review]
Add only one observer for messages with callbacks

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

::: mobile/android/base/EventDispatcher.java
@@ +210,1 @@
>              wrapper.put("response", response);

Result and response are confusing. "success"? "error"?

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8403023 [details] [diff] [review]
Add GeckoCallback parameter to handleMessage, v2

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

Looks good; just one comment and some nits.

::: mobile/android/base/EventDispatcher.java
@@ +148,5 @@
>                  listeners = mGeckoThreadNativeListeners.get(type);
>              }
> +
> +            GeckoCallback callback = null;
> +            String guid = message.optString(GUID, null);

final

@@ +166,5 @@
>                  return;
> +            } else {
> +                // If there are no listeners, execute the callback immediately to prevent
> +                // Gecko-side observers from being leaked.
> +                callback.sendCancel();

I think we should call sendCancel in dispatchEvent(JSONObject) instead. If there's no native listener, we try dispatching to JSON listeners next. If there's no JSON listener either, we can then call sendCancel.

@@ +230,5 @@
>      }
> +
> +    private static class GeckoEventCallback implements GeckoCallback {
> +        private String guid;
> +        private String type;

final for guid and type

@@ +250,5 @@
> +        public void sendCancel() {
> +            sendHelper(RESULT_CANCEL, null);
> +        }
> +
> +        private void sendHelper(String result, Object response) {

final args

@@ +252,5 @@
> +        }
> +
> +        private void sendHelper(String result, Object response) {
> +            if (sent) {
> +                throw new IllegalStateException("Callback has already been executed for type=" + type + ", guid=" + guid);

Wrap line

@@ +262,5 @@
> +                final JSONObject wrapper = new JSONObject();
> +                wrapper.put(GUID, guid);
> +                wrapper.put("result", result);
> +                wrapper.put("response", response);
> +                GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent(type + ":Response", wrapper.toString()));

Wrap line

@@ +263,5 @@
> +                wrapper.put(GUID, guid);
> +                wrapper.put("result", result);
> +                wrapper.put("response", response);
> +                GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent(type + ":Response", wrapper.toString()));
> +            } catch (JSONException e) {

final

::: mobile/android/base/util/GeckoCallback.java
@@ +1,3 @@
> +package org.mozilla.gecko.util;
> +
> +public interface GeckoCallback {

I'd rather it be called EventCallback than GeckoCallback.

@@ +2,5 @@
> +
> +public interface GeckoCallback {
> +    public void sendResponse(Object response);
> +    public void sendError(Object response);
> +    public void sendCancel();

Please add some docs.

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: Add GeckoCallback parameter to handleMessage, v3

Thanks for the review; comments addressed.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8403524 [details] [diff] [review]
Add GeckoCallback parameter to handleMessage, v3

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

LGTM. Thanks!

Comment 8

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/fx-team/rev/5af035625dc0
https://hg.mozilla.org/integration/fx-team/rev/655f3cf12f45

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5af035625dc0
https://hg.mozilla.org/mozilla-central/rev/655f3cf12f45