788368 - Port messages may be dropped if port is closed before other side connects

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

If you create a worker, post a message to the port, then close the port before the worker has finished initializing, the message will never be delivered to the worker.  It should be.

The problem is that the close of the port is done somewhat synchronously - when the worker initializes we only "connect" ports which are not yet closed.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: WIP patch

WIP patch - passes tests but consistently reports leaks of a BackstatePass etc.  These leaks might themselves be worth looking into as they might offer other insights into our memory usage.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: Allow pending messages to be delivered after one side closes

This new version is "rebased" against current m-c.  It doesn't seem to have any leaks and works as described.

It's not a high priority as our real code doesn't work around it yet - but the tests do (the workarounds are removed in the patch) and it might cause some pain as we make changes in the future, so I think it's worthwhile.

Try run at https://tbpl.mozilla.org/?tree=Try&rev=60a5c6d963af

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 687623 [details] [diff] [review]
Allow pending messages to be delivered after one side closes

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

Felipe, can you review this also?

::: toolkit/components/social/FrameWorker.jsm
@@ +234,5 @@
>        for (let port of worker.pendingPorts) {
> +        try {
> +          port._createWorkerAndEntangle(worker);
> +        }
> +        catch(e) {

nit: while making this change, might as well change this to: } catch(e) {

@@ -253,4 @@
>        for (let [portid, port] in Iterator(worker.ports)) {
> -        // port may have been closed as a side-effect from closing another port
> -        if (!port)
> -          continue;

Why should we stop checking for 'undefined' ports? If we |delete this.ports[portId]|, then |this.ports[portId] == undefined|.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

(In reply to Jared Wein [:jaws] from comment #3)
> @@ -253,4 @@
> >        for (let [portid, port] in Iterator(worker.ports)) {
> > -        // port may have been closed as a side-effect from closing another port
> > -        if (!port)
> > -          continue;
> 
> Why should we stop checking for 'undefined' ports? If we |delete
> this.ports[portId]|, then |this.ports[portId] == undefined|.

Right - but after |delete this.ports[portId]|, the Iterator will not return that slot.

I think this code was just a hang-over from before we started the landing effort, but if you can see a scenario where it would fail I'd be happy to add it back.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 687623 [details] [diff] [review]
Allow pending messages to be delivered after one side closes

>   close: function fw_AbstractPort_close() {
>-    if (!this._portid) {
>+    if (this._closed) {
>       return; // already closed.
>     }
>     this._postControlMessage("port-close");
>     // and clean myself up.
>     this._handler = null;
>     this._pendingMessagesIncoming = [];
>-    this._portid = null;
>+    this._closed = true;
>   }

Can we still set _portid here to null or is it necessary to keep it non-null for the message draining? It would be good if it's reset to null instead of an invalid value after the port is closed

Comment 6

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51b160a3672e

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/51b160a3672e