-- Object.preventExtensions(this); delete Function; Object.getOwnPropertyNames(this); -- This asserts without any jit flags: $ ./js test.js Assertion failure: !cx->isExceptionPending(), at ../jscntxtinlines.h:704

With this patch we get: test.js:3: TypeError: Object.getOwnPropertyNames(this) is not extensible I don't know if this makes sense, but at least the patch shows where the exception occurs. global_enumerate calls JS_EnumerateStandardClasses, which calls js_InitFunctionAndObjectClasses.

Old regression (from bug 352604). Jan, thanks for patching -- just need some braces around the "then" clause since the "if" condition is multiline. The assertion is pointing to a bug where someone forgot to check and propagate failure, so "this makes sense" (comment 1). /be

Comment on attachment 511972 [details] [diff] [review] Check defineProperty result >diff --git a/js/src/tests/js1_8_5/regress/jstests.list b/js/src/tests/js1_8_5/regress/jstests.list >+script regress-633741.js >diff --git a/js/src/tests/js1_8_5/regress/regress-633741.js b/js/src/tests/js1_8_5/regress/regress-633741.js >+Object.preventExtensions(this); >+delete Function; >+ >+try { >+ /* Don't assert. */ >+ Object.getOwnPropertyNames(this); >+} catch(e) { >+} >+reportCompare(0, 0, "ok"); The test matches what ES5 requires, but it's kind of a bit goofy how it does it. |Object.getOwnPropertyNames(this)| shouldn't throw at all per ES5! So how about if you add a |reportCompare(true, false, "this shouldn't have thrown");| inside the catch-block, then mark the test as failing? It's still going to test for not asserting. (I think -- please verify that a "fails" test doesn't treat a fatal assertion as an acceptable failure condition. Intuition from the crash-whole-browser scenario suggests it doesn't, but best to be certain.) It'll just do one better and check for per-spec behavior as well. The patch doesn't really implement ES5's semantics, but that's a bit more involved certainly than this spot-fix. In the meantime we definitely shouldn't assert, or drop an exception on the floor, or whatever, so this is an improvement even if it's not fully correct. Anyway, if you could post a patch with these changes, plus the bracing, I'd appreciate it. For a trivial spot-fix like this there's no reason we can't get it landed before release if we have the time.

Thanks for the reviews. This does not throw in the browser so I marked it as fails-if(xulRuntime.shell). I also confirmed that reverting the fix still makes the test fail; this is much nicer indeed.

Comment on attachment 512141 [details] [diff] [review] Patch Excellent. I poked jorendorff about landing this, doubt I have time.

(In reply to comment #7) > http://hg.mozilla.org/tracemonkey/rev/d3dd12467800 - assertEq(e instanceof TypeError, true, "expected TypeError, got: " + e); + reportCompare(e instanceof TypeError, true, "expected TypeError, got: " + e); I learned from jorendorff that reportCompare takes (expect, actual) -- opposite order from assertEq. rs=me on fix. /be

Brendan, I think that's bug 621432, right?

(In reply to comment #9) > Brendan, I think that's bug 621432, right? I saw it here: http://hg.mozilla.org/tracemonkey/rev/902e844c46bd which is not this bug's cset, nor the one for bug 621432. Waldo, which bug number is the right one for 902e844c46bd ? /be

Oops, my bad -- that was a followup to bug 621432. Sorry about that!

FYI, bug 642772 fixed regress-633741.js.

A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-633741.js.