Closed Bug 634210 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: jandem, Assigned: brendan)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [hardblocker][has patch][fixed-in-tracemonkey])

Attachments

(1 file, 3 obsolete files)

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
Blocks: 630996
blocking2.0: --- → ?
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.
(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
blocking2.0: ? → final+
Whiteboard: [hardblocker]
Assignee: general → brendan
Attached patch proposed fix (obsolete) — Splinter Review
Attachment #512566 - Flags: review?(jorendorff)
Whiteboard: [hardblocker] → [hardblocker][has patch]
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.
Attachment #512566 - Flags: review?(jorendorff)
(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
Whiteboard: [hardblocker][has patch] → [hardblocker]
Attached patch proposed fix, v2 (obsolete) — Splinter Review
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
Attachment #512566 - Attachment is obsolete: true
Attachment #512659 - Flags: review?(jorendorff)
Attachment #512659 - Attachment is obsolete: true
Attachment #512665 - Flags: review?(jorendorff)
Attachment #512659 - Flags: review?(jorendorff)
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
Whiteboard: [hardblocker] → [hardblocker][has patch]
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.
Attachment #512665 - Flags: review?(jorendorff) → review+
Attached patch patch to commitSplinter Review
Jason didn't pick any nits, so I did...

/be
Attachment #512665 - Attachment is obsolete: true
Attachment #512969 - Flags: review+
(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
Whiteboard: [hardblocker][has patch] → [hardblocker][has patch][fixed-in-tracemonkey]
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-634210-4.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.