Closed Bug 800278 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 22

People

(Reporter: jaws, Assigned: nancibonfim)

Details

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

Attachments

(1 file)

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.
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.
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.
Attached patch Proposed patchSplinter Review
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
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
Closed: 7 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-]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.