Closed Bug 1449821 Opened 2 years ago Closed 2 years ago

Child process EventDispatcher doesn't support multiple callbacks for one event

Categories

(GeckoView :: General, enhancement, P1)

All
Android
enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(4 files)

EventDispatcher has this feature where callbacks for a single event can be invoked multiple times. E.g. `callback.sendSuccess()` can be called multiple times to deliver multiple results. We're using this feature in bug 1441279 to enable performing multiple selection actions at once.

However, currently this feature is broken when using EventDispatcher from a child process, because we unregister the child message listener as soon as a single callback is received. So we need a way to unregister the listener regardless of the number of times a callback is received. This would also fix the potential memory leak right now when we fail to unregister the listener if a callback is never called.
Priority: -- → P1
Comment on attachment 8963629 [details]
Bug 1449821 - 4. Use one child message listener per EventDispatcher;

https://reviewboard.mozilla.org/r/232518/#review238270

::: mobile/android/modules/geckoview/Messaging.jsm:83
(Diff revision 1)
>      };
>  
>      if (callback) {
> -      forwardData.uuid = UUIDGen.generateUUID().toString();
> -      mm.addMessageListener("GeckoView:MessagingReply", function listener(msg) {
> -        if (msg.data.uuid !== forwardData.uuid) {
> +      const uuid = UUIDGen.generateUUID().toString();
> +      this._replies.set(uuid, {
> +        callback: callback,

You can just do:

{ callback, finalizer }
Comment on attachment 8963627 [details]
Bug 1449821 - 2. Add finalizer support to EventDispatcher;

https://reviewboard.mozilla.org/r/232514/#review238272
Attachment #8963627 - Flags: review?(snorp) → review+
Comment on attachment 8963626 [details]
Bug 1449821 - 1. Remove legacy Messaging.jsm APIs;

https://reviewboard.mozilla.org/r/232512/#review239210

::: mobile/android/modules/geckoview/Messaging.jsm:96
(Diff revision 1)
> -  /**
>     * Sends a request to Java.
>     *
>     * @param msg Message to send; must be an object with a "type" property
>     */
>    sendRequest: function(msg, callback) {

aMsg, aCallback
Attachment #8963626 - Flags: review?(esawin) → review+
Comment on attachment 8963628 [details]
Bug 1449821 - 3. Add finalizer support in Messaging.jsm;

https://reviewboard.mozilla.org/r/232516/#review239220

Reusable callbacks are an odd pattern, requiring finalize isn't great either.
Maybe it's better to just pass a "listener" to the calls that require such behavior with the callback working as the finalizer?

::: mobile/android/modules/geckoview/Messaging.jsm:60
(Diff revision 1)
> -   * @param event    Name of event to dispatch.
> -   * @param data     Optional object containing data for the event.
> -   * @param callback Optional callback implementing nsIAndroidEventCallback.
> +   * @param event     Name of event to dispatch.
> +   * @param data      Optional object containing data for the event.
> +   * @param callback  Optional callback implementing nsIAndroidEventCallback.
> +   * @param finalizer Optional finalizer implementing nsIAndroidEventFinalizer.
>     */
> -  dispatch: function(event, data, callback) {
> +  dispatch: function(event, data, callback, finalizer) {

Missing argument prefixes, this seems to be inconsistent across the file.
Attachment #8963628 - Flags: review?(esawin) → review+
Comment on attachment 8963629 [details]
Bug 1449821 - 4. Use one child message listener per EventDispatcher;

https://reviewboard.mozilla.org/r/232518/#review239232
Attachment #8963629 - Flags: review?(esawin) → review+
(In reply to Eugen Sawin [:esawin] from comment #8)
> Comment on attachment 8963628 [details]
> Bug 1449821 - 3. Add finalizer support in Messaging.jsm;
> 
> https://reviewboard.mozilla.org/r/232516/#review239220
> 
> Reusable callbacks are an odd pattern, requiring finalize isn't great either.
> Maybe it's better to just pass a "listener" to the calls that require such
> behavior with the callback working as the finalizer?

I see what you're saying. I think I was aiming for keeping compatibility with the existing interface. Also, if the callback becomes a finalizer, the event listener must remember to use the callback, and I think it'd be hard to enforce that.
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6f120441cdd0
1. Remove legacy Messaging.jsm APIs; r=esawin
https://hg.mozilla.org/integration/autoland/rev/3427c2b0cf4b
2. Add finalizer support to EventDispatcher; r=snorp
https://hg.mozilla.org/integration/autoland/rev/a6ccb53865ed
3. Add finalizer support in Messaging.jsm; r=esawin
https://hg.mozilla.org/integration/autoland/rev/7ddea90ba7db
4. Use one child message listener per EventDispatcher; r=esawin
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 61 → mozilla61
You need to log in before you can comment on or make changes to this bug.