635195 - Assertion failure: !wp->setter, at jsdbgapi.cpp:781

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following code asserts on TM tip:

this.__defineSetter__("x", Function);
this.watch("x", eval("with ({}) { for (var x in {}); } (function() { return delete x; })"));
TrySomething( "x = \"hi\"", true );
function TrySomething( expression, throwing ) {
      eval( expression );
}


The first bad revision is:
changeset:   62178:589bb166be02
user:        Jason Orendorff
date:        Tue Feb 08 16:09:33 2011 -0600
summary:     Bug 627984 - Tighten up assertions in JSObject::methodReadBarrier. r=brendan.

Comment 1

[kosver]

Attachment: removes the assert

I've looked into the code and I think the assert on line 781 in jsdbgapi.cpp isn't necessary. It will assert if the shape is false and there is a setter defined. That's the case when you delete a defined setter in the watch function. 

When you delete the assert, the code will just complete the watchpoint and the just deleted setter will not be called. (Like it should be, I think)

So the included patch only removes the assert

reduced testcode:
-----------------
this.__defineSetter__("x", Function);
this.watch("x", function() { delete x; });
x = "hi";

Comment 2

[Jason Orendorff [:jorendorff]]

Thanks for the patch. I think this is basically correct but I want to look at all the code a little more before committing the fix. The worst case if setter is completely wrong is a crash during GC.

Taking.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 513517 [details] [diff] [review]
removes the assert

Yep, this is right. I'll add the test to it, then ask for approval to land for FF4.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: same as before, but with a test

Comment 5

[Jason Orendorff [:jorendorff]]

Seeking approval since this is basically zero-risk (it removes a DEBUG-mode assertion) and assertions are bad.

Comment 6

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/8101f728882a

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/8101f728882a

Comment 8

[Christian Holler (:decoder)]

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