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

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jaws, Assigned: Nanci Bonfim)

Tracking

Trunk
Firefox 19
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox17 fixed, firefox18+ fixed, firefox19 fixed)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

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

Comment 2

5 years ago
Created attachment 670878 [details] [diff] [review]
Proposed patch

Hi, this is my first contribution.
Please, review it :)
Attachment #670878 - Flags: review?(jaws)
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);
Attachment #670878 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 4

5 years ago
Created attachment 670990 [details] [diff] [review]
patch

Thank you very much for the review.
I made the changes in the attached patch.
Attachment #670878 - Attachment is obsolete: true
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.
Attachment #670990 - Flags: review?(jaws)
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 = []
Attachment #670990 - Flags: review-
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.
Attachment #670990 - Flags: review?(jaws)
(Assignee)

Comment 8

5 years ago
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.
Attachment #670990 - Attachment is obsolete: true
Attachment #672161 - Flags: review?
Comment on attachment 672161 [details] [diff] [review]
proposed patch

Looks good to me - thanks!
Attachment #672161 - Flags: review?(jaws)
Attachment #672161 - Flags: review?
Attachment #672161 - Flags: feedback+
Assignee: nobody → nancibonfim
Status: NEW → ASSIGNED
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.
Attachment #672161 - Flags: review?(jaws) → review+
(Assignee)

Comment 11

5 years ago
Created attachment 672626 [details] [diff] [review]
proposed patch

Please take a look and see if I've done it right.
I appreciate your feedback.
Attachment #672626 - Flags: review?

Updated

5 years ago
Attachment #672626 - Flags: review? → review?(jaws)
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?
Attachment #672626 - Flags: review?(jaws) → review+
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.
(Assignee)

Comment 14

5 years ago
> 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.
Attachment #672161 - Attachment is obsolete: true
Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f94888f5fde
Flags: in-testsuite-
Whiteboard: [good first bug][mentor=jaws][lang=js] → [good first bug][mentor=jaws][lang=js][qa-]
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.
tracking-firefox17: --- → ?
Whiteboard: [good first bug][mentor=jaws][lang=js][qa-] → [good first bug][mentor=jaws][lang=js][qa-]]Fx17]
Oops, 802671 is only hit on worker creation - my faulty memory told me it was destruction.
tracking-firefox17: ? → ---
Whiteboard: [good first bug][mentor=jaws][lang=js][qa-]]Fx17] → [good first bug][mentor=jaws][lang=js][qa-]
https://hg.mozilla.org/mozilla-central/rev/8f94888f5fde
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Comment on attachment 672626 [details] [diff] [review]
proposed patch

[Triage Comment]
let's get this on 17/18 for the first social release
Attachment #672626 - Flags: approval-mozilla-beta+
Attachment #672626 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-beta/rev/ddb09617d2f1
status-firefox17: --- → fixed
tracking-firefox18: --- → +
https://hg.mozilla.org/releases/mozilla-aurora/rev/b0635185af6a
status-firefox18: --- → fixed
status-firefox19: --- → fixed
You need to log in before you can comment on or make changes to this bug.