Closed Bug 788368 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox Graveyard :: SocialAPI, defect)

17 Branch
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 20

People

(Reporter: markh, Assigned: markh)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch WIP patch (obsolete) — Splinter Review
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.
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
Assignee: nobody → mhammond
Attachment #664356 - Attachment is obsolete: true
Attachment #687623 - Flags: review?(jaws)
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|.
Attachment #687623 - Flags: review?(jaws)
Attachment #687623 - Flags: review?(felipc)
Attachment #687623 - Flags: feedback+
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86_64 → All
Version: unspecified → 17 Branch
(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 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
Attachment #687623 - Flags: review?(felipc) → review+
https://hg.mozilla.org/mozilla-central/rev/51b160a3672e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.