Using Object.defineProperty to change a property from that property's setter fails for objects with 128 properties

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
--
major
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: djf, Assigned: bhackett)

Tracking

Trunk
mozilla12
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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".
(Reporter)

Comment 1

6 years ago
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.
Summary: Using Object.defineProperty to change a property from that property's setter fails after 110 invocations → Using Object.defineProperty to change a property from that property's setter fails for objects with many properties
(Reporter)

Comment 2

6 years ago
Created attachment 575084 [details]
Improved test case that demonstrates the bug
Attachment #575049 - Attachment is obsolete: true
(Reporter)

Comment 3

6 years ago
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.
Summary: Using Object.defineProperty to change a property from that property's setter fails for objects with many properties → Using Object.defineProperty to change a property from that property's setter fails for objects with 128 properties
(Reporter)

Comment 4

6 years ago
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?

Updated

6 years ago
Attachment #575095 - Attachment mime type: text/x-c → text/plain
(Reporter)

Comment 5

6 years ago
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...
(Reporter)

Comment 6

6 years ago
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.
(Reporter)

Comment 7

6 years ago
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...
Luke, Brian, this one looks like it's in code you guys are active in. Could you take a look?
(Assignee)

Comment 9

6 years ago
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 test.foo 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).
(Assignee)

Comment 10

6 years ago
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.
Assignee: general → bhackett1024
Attachment #585449 - Flags: review?(luke)

Comment 11

6 years ago
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
Attachment #585449 - Flags: review?(luke) → review+

Comment 12

6 years ago
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)?
(Assignee)

Comment 13

6 years ago
(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().
(Assignee)

Comment 14

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/faf5f8842fec
https://hg.mozilla.org/mozilla-central/rev/faf5f8842fec
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Depends on: 716232
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
Depends on: 722101
You need to log in before you can comment on or make changes to this bug.