800275 - n^2 algorithm used to iterate pending worker ports in FrameWorker.createSandbox

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#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

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

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]

Attachment: Proposed patch

Hi, this is my first contribution.
Please, review it :)

Comment 3

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

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]

Attachment: patch

Thank you very much for the review.
I made the changes in the attached patch.

Comment 5

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

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 [e-mail] as the requestee.

Comment 6

[Dão Gottwald [:dao]]

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

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

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]

Attachment: 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] [:mhammond]]

Comment on attachment 672161 [details] [diff] [review]
proposed patch

Looks good to me - thanks!

Comment 10

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

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]

Attachment: proposed patch

Please take a look and see if I've done it right.
I appreciate your feedback.

Comment 12

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

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

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

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]

> 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

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

Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f94888f5fde

Comment 16

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

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] [:mhammond]]

Oops, 802671 is only hit on worker creation - my faulty memory told me it was destruction.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8f94888f5fde

Comment 19

[:Gavin Sharp [email: [e-mail]]]

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: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ddb09617d2f1

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b0635185af6a