800278 - FrameWorker.ports should use a Map instead of object in FrameWorker.jsm

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

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

See https://mxr.mozilla.org/mozilla-central/source/toolkit/components/social/FrameWorker.jsm#69

All uses of FrameWorker.ports seem better suited if the ports member was a Map. This would be particularly useful in this spot, where the syntax of iterating a Map is much nicer than the current solution:

> 210     // closing the port also removes it from this.ports via port-close
> 211     for (let [portid, port] in Iterator(this.ports)) {
> 212       // port may have been closed as a side-effect from closing another port
> 213       if (!port)
> 214         continue;
> 215       try {
> 216         port.close();
> 217       } catch (ex) {
> 218         Cu.reportError("FrameWorker: failed to close port. " + ex);
> 219       }
> 220     }

Once this.ports is changed to a Map, this code can be changed to (I'm impartial to naming portId or leaving it unnamed):
> for (let [, port] of this.ports) {
>   // port may have been closed as a side-effect from closing another port
>   if (!port)
>     continue;
>   try {
>     port.close();
>   } catch (ex) {
>     Cu.reportError("FrameWorker: failed to close port. " + ex);
>   }
> }

Comment 1

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

The documentation for iterating Map isn't easy to find yet. You can find some good examples here, https://bugzilla.mozilla.org/show_bug.cgi?id=725909#c12

Comment 2

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

Actually, a WeakMap might be a better choice here so 'temporary' ports have a better chance of automatically getting cleaned up instead of waiting for the worker to terminate.

Comment 3

[Nanci Bonfim]

A WeakMap isn't a iterable object, is this right? What do you think I should do?

Comment 4

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

Yeah, you're right that a WeakMap is not enumerable (https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/WeakMap#Why_WeakMap.3F confirms this).

If there is still a desire to use a WeakMap, we could keep an array of portIds for each time a port is added to the WeakMap, and then use this array to enumerate the WeakMap. This is possible since the port IDs are Numbers and the presence of them in the array won't increase the reference count of their associated ports.

For example:

> var wm = new WeakMap();
> var arr = [];
> ...
> wm[data.portId] = port;
> arr.push(data.portId);
> ...
> for (var i = 0; i < arr.length;) {
>   var portRef = wm.get(portId);
>   if (portRef) {
>     try {
>       port.close();
>     } catch (ex) {
>       Cu.reportError("FrameWorker: failed to close port. " + ex);
>     }
>     i++;
>   } else {
>     arr.splice(i, 1);
>   }
> }

Comment 5

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

In the example above
> port.close();
should be replaced with
> portRef.close();

Comment 6

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

Sigh... also
> var portRef = wm.get(portId)
should be replaced with
> var portRef = wm.get(arr[i]);

Comment 7

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

Hi Nanci, I talked with Felipe (CC'd) about this and he pointed out to me that WeakMaps need an object as the key. Let's just go ahead and use a Map for this.

Comment 8

[Nikhil Johny]

i am new, how should i proceed to fix this bug?

Comment 9

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

(In reply to Nikhil Johny from comment #8)
> i am new, how should i proceed to fix this bug?

It's great you want to help!  You will need to get your own copy of Firefox building - https://developer.mozilla.org/en/docs/Developer_Guide/Build_Instructions is a good place to start.

You will then need to modify the Frameworker.jsm file as mentioned in the first comment, and ensure all the social stuff works with the changes - mainly by running the social tests.  You can then upload the patch to this bug where it will be reviewed etc and finally be integrated.

Let us know if you get stuck - #socialdev on the mozilla IRC servers is probably the easiest.

Comment 10

[Nanci Bonfim]

Attachment: Proposed patch

Hi, I am so sorry to have to stop working on this and since nobody proposed a patch I did it today. Please review it.

Comment 11

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

I've pushed your patch to our tryserver. If the builds come back without any issues then we can add the checkin-needed keyword to this bug.

You can view the progress of your build at the following URL:
  https://tbpl.mozilla.org/?tree=Try&rev=57b66aca1260

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d181c1929487

Thanks for the patch!

Comment 13

[Nanci Bonfim]

You are welcome :)
Thank you all, I am going to see another tasks to work on.

Comment 14

[Ed Morley [:emorley]]

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

Comment 15

[u279076]

Does this need QA testing before release?

Comment 16

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

Nope, this is covered by other tests. Thanks for checking!