634210 - Assertion failure: shape.writable(), at ../jsobjinlines.h:154

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

This snippet:
--
function f() {}
f.p = function() {};
Object.freeze(f);
f.p;
--
Triggers this assert introduced by bug 627984:

Assertion failure: shape.writable(), at ../jsobjinlines.h:154

Comment 1

[Jeff Walden [:Waldo]]

Needs to be loaded as a file to fail.  (We need to fix this.  Yesterday.  :-( )

Basic summary: changing a function-valued property's [[Writable]] attribute should also trigger the method write barrier.  Or the MWB needs to account for its property being non-writable.

Comment 2

[Brendan Eich [:brendan]]

(In reply to comment #1)
> Needs to be loaded as a file to fail.  (We need to fix this.  Yesterday.  :-( )

Fix this how?

> Basic summary: changing a function-valued property's [[Writable]] attribute
> should also trigger the method write barrier.

s/write/read/

I can fix this.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 512566 [details] [diff] [review]
proposed fix

This patch fixes Object.seal but not Object.defineProperty:

var a = [];
for (var i = 0; i < 2; i++) {
    a[i] = {m: function () {}};
    Object.defineProperty(a[i], "m", {writable: false});
}
assertEq(a[0].m === a[1].m, false);

(The same test with {configurable: false} instead of writable also fails.)

>+    if ((attrs & JSPROP_READONLY) && shape->isMethod()) {
>+        JS_ASSERT(obj->hasMethodBarrier());
>+        JS_ASSERT(shape->isDataDescriptor());
>+        JS_ASSERT(shape->writable());
>+
>+        JSObject *funobj = &shape->methodObject();
>+#ifdef DEBUG
>+        JSFunction *fun = funobj->getFunctionPrivate();
>+        JS_ASSERT(fun == funobj);
>+        JS_ASSERT(FUN_NULL_CLOSURE(fun));
>+#endif

I think all these assertions are redundant with the methodReadBarrier call.

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> Comment on attachment 512566 [details] [diff] [review]
> proposed fix
> 
> This patch fixes Object.seal

(freeze, rather)

> but not Object.defineProperty:
> 
> var a = [];
> for (var i = 0; i < 2; i++) {
>     a[i] = {m: function () {}};
>     Object.defineProperty(a[i], "m", {writable: false});
> }
> assertEq(a[0].m === a[1].m, false);
> 
> (The same test with {configurable: false} instead of writable also fails.)
> 
> >+    if ((attrs & JSPROP_READONLY) && shape->isMethod()) {
> >+        JS_ASSERT(obj->hasMethodBarrier());
> >+        JS_ASSERT(shape->isDataDescriptor());
> >+        JS_ASSERT(shape->writable());
> >+
> >+        JSObject *funobj = &shape->methodObject();
> >+#ifdef DEBUG
> >+        JSFunction *fun = funobj->getFunctionPrivate();
> >+        JS_ASSERT(fun == funobj);
> >+        JS_ASSERT(FUN_NULL_CLOSURE(fun));
> >+#endif
> 
> I think all these assertions are redundant with the methodReadBarrier call.

Too right! Thanks,

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: proposed fix, v2

Whew. As noted on IRC, the method barrier does not belong in jsscope.cpp code, as that code should not be JSObject-as-receiver-based. Trying to interpose the read barrier there can easily loop (due to the read barrier writing by calling back to js_SetPropertyHelper) in a recursive runaway.

So there are two places I can see in jsobj.cpp that need the read barrier:

* for Object.defineProperty, js_DefineNativeProperty, which already gets the existing shape (if any) being overwritten;

* for Object.freeze, js_ChangeNativePropertyAttrs.

Let me know if I am missing another place.

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: proposed fix, v2a (another test, using seal to show write-back from read barrier works)

Comment 8

[Brendan Eich [:brendan]]

Cc'ing billm -- we were discussing shape rooting. I believe all the read barrier calls here take a shape that's in a live object (caller-roots).

/be

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 512665 [details] [diff] [review]
proposed fix, v2a (another test, using seal to show write-back from read barrier works)

OK, looks good.

Comment 10

[Brendan Eich [:brendan]]

Attachment: patch to commit

Jason didn't pick any nits, so I did...

/be

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #10)
> Created attachment 512969 [details] [diff] [review]
> patch to commit
> 
> Jason didn't pick any nits, so I did...
> 
> /be

http://hg.mozilla.org/tracemonkey/rev/90be6dccf2c6

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/90be6dccf2c6

Comment 13

[Christian Holler (:decoder)]

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