Last Comment Bug 703157 - Using Object.defineProperty to change a property from that property's setter fails for objects with 128 properties
: Using Object.defineProperty to change a property from that property's setter ...
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- major (vote)
: mozilla12
Assigned To: Brian Hackett (:bhackett)
: Jason Orendorff [:jorendorff]
Depends on: 716232 722101
  Show dependency treegraph
Reported: 2011-11-16 17:20 PST by David Flanagan [:djf]
Modified: 2012-03-13 18:17 PDT (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

An HTML file that demonstrates the bug (1.68 KB, text/html)
2011-11-16 17:20 PST, David Flanagan [:djf]
no flags Details
Improved test case that demonstrates the bug (1.60 KB, text/html)
2011-11-16 20:34 PST, David Flanagan [:djf]
no flags Details
pure JS (not html) test case (1.88 KB, text/plain)
2011-11-16 22:03 PST, David Flanagan [:djf]
no flags Details
improved test case (1.41 KB, text/plain)
2011-11-17 11:07 PST, David Flanagan [:djf]
no flags Details
patch (5bbbb0127380) (7.94 KB, patch)
2012-01-03 10:18 PST, Brian Hackett (:bhackett)
luke: review+
Details | Diff | Splinter Review

Description David Flanagan [:djf] 2011-11-16 17:20:31 PST
Created attachment 575049 [details]
An HTML file that demonstrates the bug

This is a weird one.  The attached .html file is just a script. (My hg tree is corrupted and I couldn't manage to build a current version of spidermonkey, so I'm demonstrating the bug in Nightly) The script defines a function named defineLazyProperty() which defines a getter and a setter for the specified object and property. The getter lazily computes the value of the property by invoking a function and then overwrites the property with the computed value as a data property. But if you set the property before accessing it, then the setter overwrites the property with the new value as a data property.

The bug is that if you call this defineLazyProperty() many times (110 in my testing) then the setter function breaks, and when you set the property before reading it, the setter function is called, but its call to Object.defineProperty fails and the property ends up getting set to undefined.

On my mac, in the 11/16 Nightly, the threshold is always 110 invocations so it is not completely non-deterministic. Running the code in an out-of-date version of spidermonkey gives me a different threshold. I've even seen the threshold change when I tweak the code.  So the threshold for demonstrating the bug may be different for you, too.  Change the value of the variable N if necessary.  With a high enough value the alert will say "undefined". With a low enough value it will say "33".
Comment 1 David Flanagan [:djf] 2011-11-16 20:25:14 PST
I've just realized that I don't have to call my defineLazyProperty() function 110 times to trigger this bug, I just have to define 110 properties on the global object and then call defineLazyProperty once.
Comment 2 David Flanagan [:djf] 2011-11-16 20:34:21 PST
Created attachment 575084 [details]
Improved test case that demonstrates the bug
Comment 3 David Flanagan [:djf] 2011-11-16 20:38:07 PST
The new attachment demonstrates that the bug occurs when I used my defineLazyProperty() on an object that already has 128 properties.  But if you change N to 127 or less, then the bug does not occur.  In the old demo I was using the global object which already has some properties.  In this case, I start with an empty object and add 128 plain ordinary properties to trigger the bug.
Comment 4 David Flanagan [:djf] 2011-11-16 22:03:33 PST
Created attachment 575095 [details]
pure JS (not html) test case

I got my mozilla-central tree repaired and built a current spidermonkey, to verify that I can reproduce this bug in the spidermonkey shell as well.  The attachment is basically the same code as in the HTML version, but you can specify the number of properties N on the command line:

   js definePropertyBug2.js 128

So how does an object change when it gets its 128th property?
Comment 5 David Flanagan [:djf] 2011-11-16 23:31:14 PST
gal suggests that the fact that this bug is triggered at 128 properties probably has something to do with jspropertytree.h: enum { MAX_HEIGHT = 128 };

And a little more digging turned up jsscope.cpp:670:

if (lastProp->entryCount() >= PropertyTree::MAX_HEIGHT) {
    if (!toDictionaryMode(cx))

Presumably, then, the bug is occuring within code wrapped by if (inDictionaryMode()).

I didn't see anything obviously amiss while single-stepping through the final obj_defineProperty call in from my test case, but I don't know a thing about that part of the code base...
Comment 6 David Flanagan [:djf] 2011-11-17 11:07:52 PST
Created attachment 575240 [details]
improved test case

I've added a simplified test case that doesn't require the defineLazyProperty() helper function and that puts the test object into dictionary mode just by deleting a property instead of adding 128 properties.  Comment out line 9 to make the bug go away.

It also demonstrates that inside the setter method, the call to Object.defineProperty appears to have succeeded because the correct value and
property descriptor are printed. So the bug doesn't strike (turning the property value to undefined) until after the setter returns.

Finally, it demonstrates that when the property is undefined, a call to getOwnPropertyDescriptor() on it returns a descriptor with no value property.
Comment 7 David Flanagan [:djf] 2011-11-17 11:37:43 PST
Please ignore the last paragraph of the last comment.  It is an artifact of JSON.stringify(), which doesn't serialize properties whose value is undefined.  The value property is there on the descriptor object.

Finally, this isn't in the most recent tests, but it was in the initial one...  A workaround to this bug is to delete the property before redefining it in the setter method. Maybe that will be a helpful hint to someone who understands what's going on...
Comment 8 David Mandelin [:dmandelin] 2011-11-17 12:11:36 PST
Luke, Brian, this one looks like it's in code you guys are active in. Could you take a look?
Comment 9 Brian Hackett (:bhackett) 2011-11-17 21:45:38 PST
The problem here seems to be an interaction between two object behaviors/quirks.  First, certain builtin properties can have both PropertyOp setter hooks *and* data slots.  In such cases, there is this funky bit of code at the end of the snippet from js_NativeSet below which updates the slot with the result of the setter:

    sample = cx->runtime->propertyRemovals;
    if (!shape->set(cx, obj, strict, vp))
        return false;

    JS_ASSERT_IF(!obj->inDictionaryMode(), shape->slot == slot);
    slot = shape->slot;

    if (obj->containsSlot(slot) &&
        (JS_LIKELY(cx->runtime->propertyRemovals == sample) ||
         obj->nativeContains(cx, *shape))) {
        if (!added) {
            AbortRecordingIfUnexpectedGlobalWrite(cx, obj, slot);
            if (!obj->methodWriteBarrier(cx, *shape, *vp))
                return false;
        obj->setSlot(slot, *vp);

This path is taken for properties with both PropertyOp hooks and those with scripted setters (as in the example).  Properties with scripted getters or setters can't have slots (I'm pretty sure), so the 'if' statement should never trip on such properties.  This is where the second object behavior/quirk comes in.  When reconfiguring or redefining an existing property on an object, the physical Shape representing that property can be modified in place *if* the object is a dictionary (and owns all of its properties).

If this happens, a shape with a scripted setter can be mutated to acquire a slot, the 'if' in the fragment above will hit and the now-slotful property will be incorrectly updated with the setter's value.  (You can see this in action; whatever value the 'set' lambda returns will incorrectly show up as the value of later on).

This testcase happens to work correctly in both cases on the JM branch (with ObjShrink changes), but that is only accidental.  I'm not sure yet whether to fix this by improving the test in js_NativeSet or by removing in-place modification of shapes for dictionary objects.  Will look at this more tomorrow.

I don't know if the getOwnPropertyDescriptor call is doing the right thing, but the internal structure of the object is OK even in the buggy case (the property has a slot, it just has an undefined value).
Comment 10 Brian Hackett (:bhackett) 2012-01-03 10:18:37 PST
Created attachment 585449 [details] [diff] [review]
patch (5bbbb0127380)

Patch to never modify shapes in place when existing properties are modified.  Instead, a new shape is generated and inserted in the original position, so that the rest of the VM can assume that shapes are immutable.
Comment 11 Luke Wagner [:luke] 2012-01-03 14:30:56 PST
Comment on attachment 585449 [details] [diff] [review]
patch (5bbbb0127380)

One thing existing callers may not expect is that 'shape' is no longer reachable from 'obj' so it could become garbage.  Conservative stack scanning should catch it for now, but perhaps some RootedVar's (in js_NativeSet, e.g.) are needed?

>+// Define a an accessor property with a setter that 

s/a an/an/

>-        if (!self->generateOwnShape(cx))
>+        if (!(shape = self->generateNewShape(cx, shape)))

Two things: can you rename generateNewShape to replaceWithNewEquivalentShape (or something else to indicate that this is more than just allocation) and also separate test from assignment:

  shape = self->replaceWithNewEquivalentShape(cx, shape);
  if (!shape)

>+    JS_ASSERT_IF(oldShape != lastProperty(),
>+                 inDictionaryMode() &&
>+                 !oldShape->isEmptyShape() &&
>+                 nativeLookup(cx, oldShape->maybePropid()) == oldShape);

I think you can replace the last two conjuncts with

  nativeLookup(cx, oldShape->propid()) == oldShape
Comment 12 Luke Wagner [:luke] 2012-01-03 14:51:07 PST
One last comment: could you update the just-landed new jsscope.h comment #2 (in the first numbered list) to indicate that dictionary shapes are only partially mutable (the next/prev links, but not flags, etc)?
Comment 13 Brian Hackett (:bhackett) 2012-01-04 07:34:04 PST
(In reply to Luke Wagner [:luke] from comment #11)
> Comment on attachment 585449 [details] [diff] [review]
> patch (5bbbb0127380)
> One thing existing callers may not expect is that 'shape' is no longer
> reachable from 'obj' so it could become garbage.  Conservative stack
> scanning should catch it for now, but perhaps some RootedVar's (in
> js_NativeSet, e.g.) are needed?

Bug 714647 adds a rooter I think.  With the dynamic analysis a crash will be triggered otherwise on any testcase where a GC thing is allocated under shape->set().
Comment 14 Brian Hackett (:bhackett) 2012-01-05 06:39:34 PST
Comment 15 Marco Bonardo [::mak] 2012-01-06 04:39:38 PST
Comment 16 Tony Mechelynck [:tonymec] 2012-02-11 23:37:21 PST
This patch causes SEGV crashes in the JS garbage collector (at every startup of chatZilla for me), see bug 716232 and in particular (after bisecting) bug 716232 comment #34

Note You need to log in before you can comment on or make changes to this bug.