This asserts in interpreter/-m/-j: -- var o = Array; o.p = function() {}; o.watch('p', function() { }); for(var x in o) { o[x]; } delete o.p; o.p = function() {}; print(o.p); -- Assertion failure: isObject(), at ../jsvalue.h:602

Reduced further: -- var o = Array; o.watch('p', function() { }); for(var x in o) { }; delete o.p; o.p = function() {}; print(o.p); --

The first bad revision is: changeset: 58560:52d20032116a user: Jason Orendorff <jorendorff@mozilla.com> date: Thu Dec 09 12:04:35 2010 -0600 summary: Bug 614051 - TM: wrong behavior setting existing properties to joined function object values again. r=brendan.

security sensitive per jandem's request.

This one crashes with -j in a release build: -- var o = Array; o.watch('p', function() { }); for(var x in o) {}; delete o.p; o.p = function() {}; for(var i=0; i<20; i++) { print(o.p); } -- Stack trace: Program received signal EXC_BAD_ACCESS, Could not access memory. Reason: KERN_PROTECTION_FAILURE at address: 0x0000001c 0x00194fea in js::MethodReadBarrier () (gdb) bt #0 0x00194fea in js::MethodReadBarrier () #1 0x00382f08 in ?? () #2 0x001b326c in js::ExecuteTree () Previous frame inner to this frame (gdb could not unwind past this frame)

This seems relatively obscure, and seems not to be sg:critical, so that this late, I think it only needs to be a softblocker.

The branded shape and the slot holding the method are out of sync here. This is at least pretty scary.

var o = Array; o.p = function() {}; o.watch('p', function() { }); Object.keys(o); delete o.p; o.p = function() {}; print(o.p); The iteration seems necessary here (keys), I don't know why.

Object.keys triggers the resolve hook on Array. This simplified test case crashes too: var o = Array; o.p = function() {}; o.watch('p', function() { }); o.length; delete o.p; o.p = function() {}; print(o.p); Looks like we mess up the write barrier here. Watch sets a getter which makes shape no longer say yes to "isMethod", and resolving another method on that causes bad things. Pretty sketchy stuff.

No resolve hook needed. Just adding another function (which causes rebranding) will do it. var o = ({}); o.p = function() {}; o.watch('p', function() { }); o.q = function() {} delete o.p; o.p = function() {}; print(o.p);

This is the state right before we hit the methodReadBarrier in print(o.p). The object has clearly not the right method value in the slot: Breakpoint 4, JSObject::methodReadBarrier (this=0x100d0d068, cx=0x1008130e0, shape=@0x10111a5c8, vp=0x7fff5fbfede0) at jsobjinlines.h:165 165 if (!js_SetPropertyHelper(cx, this, shape.id, 0, vp, false)) (gdb) call js_DumpObject(this) object 0x100d0d068 class 0x100438ea0 Object flags: method_barrier inDictionaryMode hasPropertyTable properties: enumerate method(0x100d0bd80) "p": slot 2 enumerate method(0x100d0bd00) "q": slot 1 proto <Object at 0x100d030d8> parent <global object at 0x100d03048> slots: 0 = undefined 1 = <unnamed function (x.js:4) at 0x100d0bd00 (JSFunction at 0x100d0bd00)> 2 = undefined (gdb)

We call the watch point in SetMethod after we have updated the shape and then write that value into the slot. The watch point returns undefined and that value is written. It might be possible to trick the property cache into calling the wrong function with this: (gdb) s obj_watch_handler (cx=0x1008130e0, obj=0x100d0d068, id={asBits = 4299215808}, old={asBits = 18444773748872577024, debugView = {payload47 = 0, tag = JSVAL_TAG_UNDEFINED}, s = {payload = {i32 = 0, u32 = 0, why = JS_ARRAY_HOLE, word = 18444773748872577024}}, asDouble = -nan(0x9000000000000), asPtr = 0xfff9000000000000}, nvp=0x7fff5fbfed90, closure=0x100d0e058) at ../jsobj.cpp:1310 1310 callable = (JSObject *) closure; (gdb) n 1312 callbacks = JS_GetSecurityCallbacks(cx); (gdb) n 1313 if (callbacks && callbacks->findObjectPrincipals) { (gdb) n 1332 key.obj = obj; (gdb) n 1333 key.id = id; (gdb) n 1334 if (!js_StartResolving(cx, &key, JSRESFLAG_WATCH, &entry)) (gdb) n 1336 if (!entry) (gdb) n 1338 generation = cx->resolvingTable->generation; (gdb) n 1340 argv[0] = IdToValue(id); (gdb) n 1341 argv[1] = Valueify(old); (gdb) n 1342 argv[2] = Valueify(*nvp); (gdb) n 1343 ok = ExternalInvoke(cx, obj, ObjectOrNullValue(callable), 3, argv, Valueify(nvp)); (gdb) p *nvp $63 = { asBits = 18445477440623000960, debugView = { payload47 = 4308647296, tag = JSVAL_TAG_OBJECT }, s = { payload = { i32 = 13680000, u32 = 13680000, why = 13680000, word = 18445477440623000960 } }, asDouble = -nan(0xb800100d0bd80), asPtr = 0xfffb800100d0bd80 } (gdb) n 1344 js_StopResolving(cx, &key, JSRESFLAG_WATCH, entry, generation); (gdb) p *nvp $64 = { asBits = 18444773748872577024, debugView = { payload47 = 0, tag = JSVAL_TAG_UNDEFINED }, s = { payload = { i32 = 0, u32 = 0, why = JS_ARRAY_HOLE, word = 18444773748872577024 } }, asDouble = -nan(0x9000000000000), asPtr = 0xfff9000000000000 } (gdb) (gdb) l 5662 getter = CastAsPropertyOp(funobj); 5663 } 5664 } 5665 5666 shape = obj->putProperty(cx, id, getter, setter, SHAPE_INVALID_SLOT, 5667 attrs, flags, shortid); 5668 if (!shape) 5669 return JS_FALSE; 5670 5671 if (defineHow & JSDNP_CACHE_RESULT) (gdb) n 5668 if (!shape) (gdb) n 5671 if (defineHow & JSDNP_CACHE_RESULT) (gdb) n 5672 TRACE_1(AddProperty, obj); (gdb) l 5667 attrs, flags, shortid); 5668 if (!shape) 5669 return JS_FALSE; 5670 5671 if (defineHow & JSDNP_CACHE_RESULT) 5672 TRACE_1(AddProperty, obj); 5673 5674 /* 5675 * Initialize the new property value (passed to setter) to undefined. 5676 * Note that we store before calling addProperty, to match the order (gdb) n 5679 if (obj->containsSlot(shape->slot)) (gdb) n 5680 obj->nativeSetSlot(shape->slot, UndefinedValue()); (gdb) n 5683 if (!CallAddPropertyHook(cx, clasp, obj, shape, vp)) { (gdb) p *vp $54 = { data = { asBits = 18445477440623000960, debugView = { payload47 = 4308647296, tag = JSVAL_TAG_OBJECT }, s = { payload = { i32 = 13680000, u32 = 13680000, why = 13680000, word = 18445477440623000960 } }, asDouble = -nan(0xb800100d0bd80), asPtr = 0xfffb800100d0bd80 } } (gdb) call js_DumpValue(*vp) <unnamed function (x.js:6) at 0x100d0bd80 (JSFunction at 0x100d0bd80)> (gdb) s CallAddPropertyHook (cx=0x1008130e0, clasp=0x100438ea0, obj=0x100d0d068, shape=0x10111a5c8, vp=0x7fff5fbfed90) at ../jsobj.cpp:4576 4576 if (clasp->addProperty != PropertyStub) { (gdb) n 4586 return true; (gdb) fin Run till exit from #0 CallAddPropertyHook (cx=0x1008130e0, clasp=0x100438ea0, obj=0x100d0d068, shape=0x10111a5c8, vp=0x7fff5fbfed90) at ../jsobj.cpp:4586 0x0000000100106c62 in js_SetPropertyHelper (cx=0x1008130e0, obj=0x100d0d068, id={asBits = 4299215808}, defineHow=5, vp=0x7fff5fbfed90, strict=0) at ../jsobj.cpp:5683 5683 if (!CallAddPropertyHook(cx, clasp, obj, shape, vp)) { Value returned is $55 = true (gdb) n 5687 added = true; (gdb) p *vp $56 = { data = { asBits = 18445477440623000960, debugView = { payload47 = 4308647296, tag = JSVAL_TAG_OBJECT }, s = { payload = { i32 = 13680000, u32 = 13680000, why = 13680000, word = 18445477440623000960 } }, asDouble = -nan(0xb800100d0bd80), asPtr = 0xfffb800100d0bd80 } } (gdb) n 5690 if (defineHow & JSDNP_CACHE_RESULT) (gdb) n 5691 JS_PROPERTY_CACHE(cx).fill(cx, obj, 0, 0, obj, shape, added); (gdb) n 5693 return js_NativeSet(cx, obj, shape, added, vp);

There is a bunch of bugs here actually. In js_NativeSet we never trigger the method write barrier if a property is added (added=true), because we think that can't cause a shape change, but that's not true for watchpoint setters which fire for additions as well since they are persistent. So in other words watch makes our write barrier more expensive in general. Super lame. There is a second bug. The slot is initialized to undefined when we call the setter, so even if we would call the method write barrier it would see undefined (old) == undefined (new) and not brand. I think this is all pretty seriously broken and all sorts of unsafe. I vote for a hard blocker. This probably wants jorendorff or brendan, though I think I can fix it if push comes to shove.

What a mess.

OK. My first instinct here was to trigger the method *read* barrier in UpdateWatchpointShape. If a method property would be watched, the barrier would convert it an ordinary data property. Then you would watch that. But that would go seriously against the grain, because UpdateWatchpointShape gets called at a point where the method property has been added, but before the slot is filled. So for now watchpoints need to know about the method barrier. (Which after all is only consistent. They know about everything else.) I have a sort of patch, but unfortunately the assertions I'm trying to add keep failing, and I'm out of time for the day.

Elaborating a little on comment 16: There are three bugs I know very clearly how to fix. 1. js_watch_set passes the property's old slot value (the one we're about to overwrite) to wp->handler without checking to see if the property is a method. If it's a method, the old value is an uncloned function object that must not be exposed to script! The fix is for js_watch_set to trip the read barrier before calling wp->handler. 2. js_watch_set can get called at an unfortunate time. The order of adding a data property, such as a method, is like this: - add the shape to the object - call the setter - store the value in the slot So suppose we watch a property, then delete it, and then use JSOP_INITMETHOD to create it again. Here's what happens: - add the (watched method) shape to the object - call js_watch_set, which calls wp->handler - store the uncloned function object in the slot Note js_watch_set is called *after* the METHOD property is added to obj but *before* the slot is assigned. So the slot value is undefined. To my mind, this state is a violation of the method invariant. We do not want to be calling back into the interpreter with the object in that state. The fix, again, is for js_watch_set to trip the method barrier before calling wp->handler. 3. The watchpoint handler could delete the property and re-add it as a method property again. Then whatever value it returns must not be stored in the slot of that method property; it would violate the method invariant. So after the handler returns, we probably need to call methodWriteBarrier. (This one I'm not quite sure about.) There are also at least 2 other issues: 4. Bug 631723 - Deleted watchpoints can make obj->addProperty/putProperty return a shape not in obj. 5. If wp->handler deletes the property, wp->shape is not set to NULL. I think I have seen wp->shape be wrong (that is, != obj->nativeLookup(propid)) in other circumstances, too, which would be bad. This needs another look, possibly in a follow-up bug.

6. js_NativeSet skips the slot write if the before and after shape of the property are different. This means that unwatching from a watchpoint can cause the slot write to be skipped: var obj = {m: 'o'}; var other = {a: 1}; obj.watch("m", function (id, oldval, newval) { delete other.a; obj.unwatch('m'); return newval; }); obj.m = 'n'; // since the watchpoint unconditionally returns newval, // this value should not be lost assertEq(obj.m, 'n'); // FAILS Similarly, scripts can detect the transition to dictionary mode (which re-creates all Shapes) this way. var N = 129; var obj = {m: "fail"}; var other = {a: 1}; obj.watch("m", function (id, oldval, newval) { for (var j = 0; j < N; j++) obj['x' + j] = 0; delete other.a; return newval; }); obj.m = 'ok'; assertEq(obj.m, 'ok'); // FAILS when N >= 128

To clarify a little on comment 17 item 2,

Kindly disregard comment 19. My clarification wasn't going to be correct anyway. (If item 2 isn't clear, wait for the patch -- there's a test.) I found another bug. 7. A method is ostensibly an ordinary data property--but getting its value the first time will trigger a watchpoint on that property. var obj = {m: function () {}}; obj.watch("m", function () { throw 'FAIL'; }); var f = obj.m; // FAIL Patch coming!

This patch applies on top of the patches in bug 631723 and bug 631305. This patch does not address comment 17 item 5 or comment 18 item 6. I will file follow-up bugs for those.

Wow, what a mess indeed. Sorry I forgot about watchpoints when doing the method stuff. Could you fix sevearl of these bugs before the js_watch_set setter is invoked, in js_UpdateWatchpointsForShape? Joined methods are an optimization, so deoptimizing as soon as there's sign of a watchpoint seems fine, and it looks from this patch like it would be simpler. /be

Brendan and I talked about this on IRC yesterday or the day before. Two reasons not to do it that way: first, js_UpdateWatchpointsForShape is called at a weird time; the method property exists but the slot isn't populated yet. No big deal; I could populate the slot from shape->methodValue(), then trip the read barrier. Second, and more importantly, js_UpdateWatchpointsForShape's relevant caller (js_SetPropertyHelper) doesn't expect it to trip the read barrier, so it will unconditionally write the noncloned function to the slot. To avoid that, the caller would have to check whether js_UpdateWatchpointsForShape changed the shape. That adds a branch to the no-watchpoint path, and it complicates code outside jsdbgapi.cpp. So I put the added complexity in js_watch_set, which seemed more consistent with the design.

Jason's right, the ancient watchpoint design, which predates user-defined setters, tries to modularize watchpoints by putting their overhead behind a setter wrapper. This hasn't aged particularly well. His preferred approach would add a few if (JS_UNLIKELY(obj->watched())) { // obj->flags & WATCHED . . . } clauses. Sounds worth filing and doing after fx4. /be

Comment on attachment 510780 [details] [diff] [review] v1 >+ * property added. We must write the slot ourselves--however we Nit: spaces around --, should rewrap nicely. >+ //JS_ASSERT_IF(shape, wp->shape == shape); >+ //JS_ASSERT_IF(!shape, !wp->setter); Uncomment (fix if needed?) or remove. >+ ok = (shape->hasSetterValue() >+ ? ExternalInvoke(cx, ObjectValue(*obj), >+ ObjectValue(*CastAsObject(wp->setter)), >+ 1, vp, vp) >+ : CallJSPropertyOpSetter(cx, wp->setter, obj, userid, vp)); Super-duper-nit: no need to parenthesize the whole ?: expression. >+ * It is not ordinarily the setter's job to call >+ * methodWriteBarrier; js_watch_set alone must do so, because Would wrap better if written /* * It is not the setter's job to call methodWriteBarrier, * but js_watch_set must do so, because the caller will be * fooled into not doing it: shape does *not* have the * default setter and therefore seems not to be a method. */ Great work, r=me. Thanks for diving on this grenade. /be

I will land this as soon as bug 631723 gets review. (In reply to comment #25) > >+ //JS_ASSERT_IF(shape, wp->shape == shape); > >+ //JS_ASSERT_IF(!shape, !wp->setter); > > Uncomment (fix if needed?) or remove. The second assertion passes the whole test suite, so I'll uncomment it. The first assertion can be tripped, and not only that, I found another assertion in js_watch_set that can also be tripped. Filed bug 633637. I'm removing the trippable assertions from this patch for now.

cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/589bb166be02

Given gal's analysis and several agreements that the code was a mess, is [sg:critical?] more accurate a severity rating?

The issues pointed out in comment 0 and ultimately fixed in this bug are correctness bugs, so this bug isn't sg:anything per se. However several of the comments here, particularly comment 17 item 4, point to bug 631723, which is [sg:critical?]. So I'm not sure this should be opened. (Given all that, I'm really not sure what should be done to this bug exactly.)

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