915880 - Add onclose event handlers in the MozInterAppMessagePort

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gene Lian (I already quit Mozilla)]

Please see [1] for the motivation. When it comes to the inter-app communication, it could be necessary and useful for a message port to know if the other end is started or closed, so that the port can decide if it needs to keep posting or receiving messages.

For example, an Music App can post its now-playing song info to the Lock Screen App and show it on the screen. However, the Lock Screen App still needs a dynamic way to know if the Music App is still alive or not to update its displaying info. Otherwise, the Lock Screen App will misunderstand the port on the Music App as it doesn't post any more messages, but it is actually dead.

Unfortunately, the current W3C MessagePort [2] doesn't define these two event handlers. If we want to align our MozInterAppMessagePort to the W3C MessagePort in the future, we should either propose to add them in the formal W3C spec or we can just add them in our own MozInterAppMessagePort. However, the latter will cause the incompatibility with the W3C spec in the future.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=876397#c134
[2] http://www.w3.org/TR/2010/WD-webmessaging-20101118/#message-ports

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

So you'd want to get close event when? When the other side is closed or when your side is closed?
I guess the first one. Sounds like a reasonable feature request.

I guess start event would make sense too.

Both these need of course feedback from the spec editor, so file a spec bug please.
https://www.w3.org/Bugs/Public/enter_bug.cgi?product=WebAppsWG
or
https://www.w3.org/Bugs/Public/enter_bug.cgi?product=WHATWG

> [2] http://www.w3.org/TR/2010/WD-webmessaging-20101118/#message-ports
TR is for trash ;) up-to-date specs tend to be in dev.w3.org or whatwg.org

Comment 2

[Gene Lian (I already quit Mozilla)]

(In reply to Olli Pettay [:smaug] from comment #1)
> So you'd want to get close event when? When the other side is closed or when
> your side is closed?
> I guess the first one. Sounds like a reasonable feature request.

Yes, for the other side. :)

> Both these need of course feedback from the spec editor, so file a spec bug
> please.
> https://www.w3.org/Bugs/Public/enter_bug.cgi?product=WebAppsWG
> or
> https://www.w3.org/Bugs/Public/enter_bug.cgi?product=WHATWG

Firing a bug at [1]. It's my first time doing that. Please let me know if I did anything wrong. Thanks!

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=23327

Comment 3

[Jim Porter (:squib)]

Hm, I see that the W3 bug was WONTFIXed. This is a pretty unfortunate outcome for my use-case, since the system app needs to know when the music app is closed, and I can't think of another clean way to do this. I can't listen for the "unload" event in the music app because by that time, the MessagePort is already closed so I can't send a message. If we could fix that, I wouldn't really need an onclose event, although that would still be the simpler way to handle my use-case.

Thoughts?

Comment 4

[Gene Lian (I already quit Mozilla)]

(In reply to Jim Porter (:squib) from comment #3)
> I can't
> listen for the "unload" event in the music app because by that time, the
> MessagePort is already closed so I can't send a message. If we could fix
> that, I wouldn't really need an onclose event

The current design is when the system gets "child-process-shutdown", it will try to disable all the message ports that belong to that child process. After that, calling postMessage() will not be able to deliver the message to the other end.

I'm wanting to make sure you're saying "MessagePort is already closed after unload event". Does that mean MessagePort.postMessage() doesn't work like the above-mentioned reason? Or it actually means you cannot access MessagePort at all? If it's the first case, then we can remove the "child-process-shutdown" logic to fix your problem.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/InterAppCommService.js#851

Comment 5

[Gene Lian (I already quit Mozilla)]

Attachment: Patch (WIP)

Comment 6

[Gene Lian (I already quit Mozilla)]

Hi Jim,

I just uploaded a WIP patch. If the root cause is listening for the "child-process-shutdown", then this patch can solve your problem. Could you please give it a try or just give me a link to your codes which adds the logic of listening for "unload" event? Thanks!

Comment 7

[Gene Lian (I already quit Mozilla)]

Attachment: IAC-unload-event.patch

Hi Jim,

I just hacked some Gaia codes by myself. Eventually, I found out the music app can still deliver its message before the central receives 'child-process-shutdown' to close its port, and the system app can successfully receive that.

The 'onload' event seems to be working well to me, which means we don't need any Gecko changes to fix. Could you please take a look on the Gaia patch? Thanks!

P.S.1. After some thoughts, I think removing the 'child-process-shutdown' listening is a bad idea (the previous patch I uploaded). I hope to keep that.

P.S.2. If you randomly encounter the issue again, that's due to the bug 915898 which is still under way to fix by Fernando.

Comment 8

[Jim Porter (:squib)]

Ah, I'd forgotten about bug 915898. Everything works now!

While I still think an onclose event would be nice, it's not something I really need anymore. Thanks for the help, Gene!

Comment 9

[Jim Porter (:squib)]

Hmm, I should have know it was too good to be true. Using the 'unload' event in the music app fails to handle the case where the music app crashes or is killed by the OOM killer. In these cases, I can use the 'appterminated' event to respond, but that also works for closing the app normally, so the 'unload' event isn't very useful.

However, the 'appterminated' event doesn't help when the app is opened as an activity. Assuming the activity closes normally, we're ok, since we can send a message to the system app, but if the activity crashes, there's nothing we can do.

Comment 10

[Gene Lian (I already quit Mozilla)]

Hi Olli/Nikhil/Bent,

Can we add onclose event for MozInterAppMessagePort specific? We're kind of stucked because there is no better solution than the onclose event when it comes to inter-app communication. The props and cons we used to talk about are summarized as below:

1. Publisher listens for 'unload' event and calls postMessage() to notify subscriber it's closing.
   - Pros: the current postMessage/onmessage is enough to work.
   - Cons: 'unload' only reflects manual kill instead of OOM'ed kill or crash.

2. Subscriber listens for 'appterminated' event to know if publisher's app is closing.
   - Pros: 'appterminated' can also work for OOM'ed kill and crash.
   - Cons: it doesn't work for the activity crash. Also, only System App can listen for it.

3. Subscriber listens for 'onclose' event triggered by either publisher's port.close() or port is killed.
   - Pros: system watches the aliveness of ports and it solves the cons of #1 and #2.
   - Cons: W3C peer doesn't like port to be garbage collection observable [1].

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=23327

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

GC-observability should not be exposed to the web platform. That leads easily to code which works on one browser engine implementation but not on others.
If we have something like appterminated, which isn't about ports, but apps, and may be dispatched
whenever an app dies (it is killed, or crashed or shutdown), that sounds like a better option.

How long are we keeping MozInterAppMessagePort objects alive? As long as the other side is alive?
Or do we kill it when the last reference to it in the current process goes away and GC/CC runs?

Comment 12

[Jim Porter (:squib)]

(In reply to Olli Pettay [:smaug] from comment #11)
> If we have something like appterminated, which isn't about ports, but apps,
> and may be dispatched
> whenever an app dies (it is killed, or crashed or shutdown), that sounds
> like a better option.

The problem with appterminated is that to use it, you 1) have to be the system app, and 2) have to send a message over the port to tell the system app which app is sending messages.

> How long are we keeping MozInterAppMessagePort objects alive? As long as the
> other side is alive?

That's what I'd prefer. I really just want to know when the connection is broken.

Comment 13

[Gene Lian (I already quit Mozilla)]

(In reply to Olli Pettay [:smaug] from comment #11)
> GC-observability should not be exposed to the web platform. That leads
> easily to code which works on one browser engine implementation but not on
> others.
> If we have something like appterminated, which isn't about ports, but apps,
> and may be dispatched
> whenever an app dies (it is killed, or crashed or shutdown), that sounds
> like a better option.

As Jim mentioned, 'appterminated' can only be applied in the System App. If the IAC is going to be used between non-System Apps, 'appterminated' doesn't work, unfortunately.

> 
> How long are we keeping MozInterAppMessagePort objects alive? As long as the
> other side is alive?
> Or do we kill it when the last reference to it in the current process goes
> away and GC/CC runs?

Ideally, the MozInterAppMessagePort should live with the window creating it. It shouldn't be GC'ed when there is no reference, because it has to be kept alive to receive the message from the other end.

Supposing portA and portB are a pair. If the window of creating portA is closed, we're hoping portB can have a way to know portA is no longer valid because portA's window is terminated. From the point of view of implementation, tt seems not a matter of the GC-observability of port, the aliveness of window instead.

Adding onclose event handler still sounds the cleanest solution to me. I don't have other ideas better than this, though...

Comment 14

[Gene Lian (I already quit Mozilla)]

How about putting the ondisconnect event handler in the app level instead of the port level? Please refer to [1] to see the following proposal:

1. For publisher:

  partial interface Application {
    Promise connect(DOMString keyword, jsval rules);
    Promise getConnections();

    EventHandler ondisconnect;
  };

2. For subscriber:

  interface InterAppConnectionRequest {
    DOMString keyword;
    InterAppMessagePort port;

    EventHandler ondisconnect;
  };

Note that the ondisconnect for publisher will return the index of MozInterAppMessagePort because connect(...) actually returns an array of MozInterAppMessagePorts where each of them corresponds to one subscriber.

[1] https://wiki.mozilla.org/WebAPI/Inter_App_Communication_Alt_proposal

Comment 15

[Gene Lian (I already quit Mozilla)]

Or another alternative (maybe better) is to improve the 'onload' event to reflect not only manual close but also OOM'ed close to tackle the cons of solution #1 at comment #10. Does that make sense?

Comment 16

[Gene Lian (I already quit Mozilla)]

[1] is reopen for adding onclose event. Thanks for Ehsan's help, some discussions happen at [2].

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=23327
[2] http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2013-October/040939.html

Comment 17

[(no longer active)]

(In reply to comment #16)
> [1] is reopen for adding onclose event. Thanks for Ehsan's help, some
> discussions happen at [2].

FWIW, I need to think of a more detailed proposal and post it on the whatwg mailing list.  This is on my todo list for next week.

Comment 18

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Clearing ni? while Ehsan clears this up.

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

There's some experimentation going on as part of the NGA work that requires IAC ports to have an .onclose() event.

Given that it is quite clear now that we won't ever expose the IAC API to web content, I think it is safe to add this event to the API. We want to keep the API available to FxOS certified apps only and eventually get rid of it, as soon as we have a better alternative like navigator.connect or Foreign Fetch.

Ehsan, Olli is this ok for you?

Comment 20

[(no longer active)]

Fine by me!

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Sounds ok

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Comment 23

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8691512 [details] [diff] [review]
v1

Vivien, let me know if this works for you, please. Thanks!

I'd like to write some tests before asking for review.

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Now with tests.

Ehsan, could you take a look at the WebIDL change, please? Fabrice, could you review the dom/apps ones, please? Thank you!

Comment 25

[Fernando Jiménez Moreno [:ferjm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec81c12da815

Comment 26

[(no longer active)]

Comment on attachment 8692014 [details] [diff] [review]
v1

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

r=me on InterAppMessagePort.webidl.

Comment 27

[[:fabrice] Fabrice Desré]

Comment on attachment 8692014 [details] [diff] [review]
v1

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

r=me but let's rename this bug since this patch only fixes the onclose case.

::: dom/apps/InterAppCommService.jsm
@@ +885,5 @@
>      }
> +
> +    let receiver = identity.isPublisher ?
> +                   identity.pair.subscriber :
> +                   identity.pair.publisher;

nit: I find that more readable:
let receiver = identity.isPublisher ? identity.pair.subscriber
                                    : identity.pair.publisher;

::: dom/apps/InterAppMessagePort.js
@@ +271,2 @@
>        default:
> +        dump("WARNING - Invalid InterAppMessagePort message type\n");

nit: maybe also add the invalid message received

Comment 28

[Fernando Jiménez Moreno [:ferjm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27e64c57dc23

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2cefe6f9f0a0218a159cefb1880ebaf88fb0f1c
Bug 915880 - Add onclose event handlers in the MozInterAppMessagePort. r=ehsan,fabrice

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2cefe6f9f0a