Last Comment Bug 800275 - n^2 algorithm used to iterate pending worker ports in FrameWorker.createSandbox
: n^2 algorithm used to iterate pending worker ports in FrameWorker.createSandbox
Status: RESOLVED FIXED
[good first bug][mentor=jaws][lang=js...
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 19
Assigned To: Nanci Bonfim
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-11 00:22 PDT by (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
Modified: 2012-10-30 11:50 PDT (History)
4 users (show)
jaws: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
+
fixed
fixed


Attachments
Proposed patch (1.62 KB, patch)
2012-10-12 11:17 PDT, Nanci Bonfim
jaws: feedback+
Details | Diff | Review
patch (1.63 KB, patch)
2012-10-12 17:00 PDT, Nanci Bonfim
dao+bmo: review-
Details | Diff | Review
proposed patch (1.55 KB, patch)
2012-10-16 21:39 PDT, Nanci Bonfim
jaws: review+
markh: feedback+
Details | Diff | Review
proposed patch (2.88 KB, patch)
2012-10-17 18:15 PDT, Nanci Bonfim
jaws: review+
gavin.sharp: approval‑mozilla‑aurora+
gavin.sharp: approval‑mozilla‑beta+
Details | Diff | Review

Description (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-11 00:22:23 PDT
See https://mxr.mozilla.org/mozilla-central/source/toolkit/components/social/FrameWorker.jsm#196

> 195       while (pending.length) {
> 196         let port = pending.shift();
> 197         if (port._portid) { // may have already been closed!
> 198           try {
> 199             port._createWorkerAndEntangle(worker);
> 200           }
> 201           catch(e) {
> 202             Cu.reportError("FrameWorker: Failed to create worker port: " + e + "\n" + e.stack);
> 203           }
> 204         }
> 205       }

There isn't a need to remove elements from the array at each pass through the array. The array can just be cleared after the loop has finished.
Comment 1 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-11 00:57:56 PDT
A similar construct can be found here as well, https://mxr.mozilla.org/mozilla-central/source/toolkit/components/social/FrameWorker.jsm#359

> 359     while (this._pendingMessagesOutgoing.length) {
> 360       this._dopost(this._pendingMessagesOutgoing.shift());
> 361     }
Comment 2 Nanci Bonfim 2012-10-12 11:17:28 PDT
Created attachment 670878 [details] [diff] [review]
Proposed patch

Hi, this is my first contribution.
Please, review it :)
Comment 3 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-12 12:49:00 PDT
Comment on attachment 670878 [details] [diff] [review]
Proposed patch

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

::: toolkit/components/social/FrameWorker.jsm
@@ +191,4 @@
>        // so finally we are ready to roll - dequeue all the pending connects
>        worker.loaded = true;
>  
> +      let pending = worker.pendingPorts;        

I don't think we need this line anymore.

@@ +191,5 @@
>        // so finally we are ready to roll - dequeue all the pending connects
>        worker.loaded = true;
>  
> +      let pending = worker.pendingPorts;        
> +      for (let i = 0; i < pending.length; i++) {

for each (let port in worker.pendingPorts) {

@@ +202,4 @@
>            }
>          }
>        }
> +      let pending = [];

This line should just be |worker.pendingPorts.splice(0, worker.pendingPorts.length);|. The usage of |let| here will cause an error since the variable was already declared above.

@@ +361,1 @@
>      }

for each (let message in this._pendingMessagesOutgoing)
  this._dopost(message);

@@ +361,2 @@
>      }
> +    this._pendingMessagesOutgoing = [];

this._pendingMessagesOutgoing.splice(0, this._pendingMessagesOutgoing.length);
Comment 4 Nanci Bonfim 2012-10-12 17:00:55 PDT
Created attachment 670990 [details] [diff] [review]
patch

Thank you very much for the review.
I made the changes in the attached patch.
Comment 5 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-15 22:24:59 PDT
Comment on attachment 670990 [details] [diff] [review]
patch

Thanks for uploading a new version of the patch. Please make sure to flag me for review when uploading to guarantee that I see it. You can do this by setting review to "?" and then entering jaws@mozilla.com as the requestee.
Comment 6 Dão Gottwald [:dao] 2012-10-15 23:17:18 PDT
Comment on attachment 670990 [details] [diff] [review]
patch

>-      let pending = worker.pendingPorts;
>-      while (pending.length) {
>-        let port = pending.shift();
>+      
>+      for each (let port in worker.pendingPorts) {
>         if (port._portid) { // may have already been closed!
>           try {
>             port._createWorkerAndEntangle(worker);

Don't use 'for each' on arrays, use for..of instead.

>           }
>         }
>       }
>+      worker.pendingPorts.splice(0, worker.pendingPorts.length);

This can be simplified to:

worker.pendingPorts.length = 0

or:

worker.pendingPorts = []
Comment 7 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-16 11:24:22 PDT
Comment on attachment 670990 [details] [diff] [review]
patch

Thanks for the driveby review Dao. I'll clear my review request until these points are addressed.
Comment 8 Nanci Bonfim 2012-10-16 21:39:04 PDT
Created attachment 672161 [details] [diff] [review]
proposed patch

Hi, thanks again for the feedback. 
I found an explanation for the for..of in the docs and uploaded a new version of the patch.
Comment 9 Mark Hammond [:markh] 2012-10-16 21:43:00 PDT
Comment on attachment 672161 [details] [diff] [review]
proposed patch

Looks good to me - thanks!
Comment 10 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-16 21:59:04 PDT
Comment on attachment 672161 [details] [diff] [review]
proposed patch

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

Thanks Nanci, this looks great! Can you set up your Mercurial instance to include your name and email address, as well as a summary for the changes made in this patch? See https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in for details on how to do this.

::: toolkit/components/social/FrameWorker.jsm
@@ +190,4 @@
>  
>        // so finally we are ready to roll - dequeue all the pending connects
>        worker.loaded = true;
> +    

The whitespace characters on this line can be removed.
Comment 11 Nanci Bonfim 2012-10-17 18:15:56 PDT
Created attachment 672626 [details] [diff] [review]
proposed patch

Please take a look and see if I've done it right.
I appreciate your feedback.
Comment 12 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-17 18:32:48 PDT
Comment on attachment 672626 [details] [diff] [review]
proposed patch

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

Yes, this looks perfect. Would you like to work on bug 800278 next?
Comment 13 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-17 18:35:21 PDT
I will check this in tomorrow to the mozilla-inbound repository. From there it will get merged to mozilla-central within a day or two where it will show up in our Nightly builds: http://nightly.mozilla.org

In less than six weeks, mozilla-central will become Aurora. Six weeks later it becomes Beta, then in another six weeks it will become Release.
Comment 14 Nanci Bonfim 2012-10-17 18:55:14 PDT
> Yes, this looks perfect. Would you like to work on bug 800278 next?

Yes, I will work on it tomorrow.

Thanks for the explanation and the bug suggestion.
Comment 15 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-10-18 12:54:42 PDT
Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f94888f5fde
Comment 16 Mark Hammond [:markh] 2012-10-18 15:07:12 PDT
We shouldn't hit this in the normal case, but it is probably a contributing factor in bug 802671 - proposing for 17 as it would reduce the impact of any other "port leaks" we have.
Comment 17 Mark Hammond [:markh] 2012-10-18 15:48:26 PDT
Oops, 802671 is only hit on worker creation - my faulty memory told me it was destruction.
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-10-18 18:37:26 PDT
https://hg.mozilla.org/mozilla-central/rev/8f94888f5fde
Comment 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-29 18:05:06 PDT
Comment on attachment 672626 [details] [diff] [review]
proposed patch

[Triage Comment]
let's get this on 17/18 for the first social release
Comment 20 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-29 23:39:50 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/ddb09617d2f1
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-10-30 11:50:57 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/b0635185af6a

Note You need to log in before you can comment on or make changes to this bug.