312138 - Array.sort swallows JS exceptions (ecma_3/Array/15.4.4.11-01.js)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Consider the following example:

var N = 0;
var array = [4,3,2,1];

try {
    array.sort(function() {
        throw ++N;
    }); 
} catch (e) {
    print("e="+e+" N="+N);
}

When executed in jsshell it prints "e=5 N=5" instead of expected "e=1 N=1" as
Array.prototype.sort implementation swallows all JS exceptions except the last
generated during sort execution.

A similar problem is with aborting too-long running script. Try to execute 

[0,1,2].sort(function() { for (;;) continue; })

in the browser, it would ask to terminate the script exactly the number of times
the comparison function is called.

Comment 1

[Igor Bukanov]

Attachment: Fix

The patch adds error propagation to HeapSort so it would return immediately
after a failed compare call.

Comment 2

[Bob Clary [:bc] (inactive)]

Checking in 15.4.4.11-01.js;
/cvsroot/mozilla/js/tests/ecma_3/Array/15.4.4.11-01.js,v  <--  15.4.4.11-01.js
initial revision: 1.1
done

Comment 3

[Brendan Eich [:brendan]]

Igor, are you willing to take this bug, request review on the patch, and get it checked in?  That would be a help.  The CVS trunk is open for 1.9alpha changes, and only one review stamp of approval is required for SpiderMonkey changes.  Let me know if you'd rather someone else take this.

/be

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> Igor, are you willing to take this bug, request review on the patch, and get it
> checked in?

I will ask for review some time next week or so.

Comment 5

[Igor Bukanov]

Attachment: Updated fix to reflect trunk changes

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 201253 [details] [diff] [review]
Updated fix to reflect trunk changes

I'll get to this by end of tomorrow for sure.  Higher priority stuff in the short run, bear with me.

/be

Comment 7

[Igor Bukanov]

Attachment: Updated fix to reflect trunk changes

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 203129 [details] [diff] [review]
Updated fix to reflect trunk changes

No need for goto done and goto quit if those labels are affixed to return statements -- just return true or false at the goto site.  r=me with that change.

/be

Comment 9

[Igor Bukanov]

Attachment: Patch to commit

Patch with goto->return changes to reflect above comment.

Comment 10

[Igor Bukanov]

I committed the last patch

Comment 11

[Bob Clary [:bc] (inactive)]

verified fixed cvs trunk builds 2005-11-23 windows/linux

Comment 12

[Bob Clary [:bc] (inactive)]

fixed by Bug 336373 on the 1.8.1 branch. 
verified fixed 1.8.1 with windows/macppc/linux 20060707