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

RESOLVED FIXED in Firefox 22

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jaws, Assigned: Nanci Bonfim)

Tracking

Trunk
Firefox 22
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=jaws][lang=js][qa-])

Attachments

(1 attachment)

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);
>   }
> }
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
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.
(Assignee)

Comment 3

5 years ago
A WeakMap isn't a iterable object, is this right? What do you think I should do?
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);
>   }
> }
In the example above
> port.close();
should be replaced with
> portRef.close();
Sigh... also
> var portRef = wm.get(portId)
should be replaced with
> var portRef = wm.get(arr[i]);
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

4 years ago
i am new, how should i proceed to fix this bug?
(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.
(Assignee)

Comment 10

4 years ago
Created attachment 717640 [details] [diff] [review]
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.
Attachment #717640 - Flags: review?(jAwS)
Attachment #717640 - Flags: review?(jAwS) → review+
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
Assignee: nobody → nancibonfim
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/d181c1929487

Thanks for the patch!
Keywords: checkin-needed
(Assignee)

Comment 13

4 years ago
You are welcome :)
Thank you all, I am going to see another tasks to work on.
https://hg.mozilla.org/mozilla-central/rev/d181c1929487
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Does this need QA testing before release?
Flags: needinfo?(jaws)
Nope, this is covered by other tests. Thanks for checking!
Flags: needinfo?(jaws)
Whiteboard: [good first bug][mentor=jaws][lang=js] → [good first bug][mentor=jaws][lang=js][qa-]
You need to log in before you can comment on or make changes to this bug.