Closed Bug 688685 Opened 9 years ago Closed 9 years ago

Property cache handling of property adds is unsound when the JSClass has an addProperty hook and the Shape has no setter


(Core :: JavaScript Engine, defect, P1)






(Reporter: bzbarsky, Assigned: bzbarsky)




(2 files)

In the JSOP_SETNAME/JSOP_SETPROP case in jsinterp.cpp, the logic is sorta like this:

        if (cache->testForSet(cx, regs.pc, obj, &entry, &obj2, &atom)) {
            if (!entry->adding()) {
            } else {
                if (shape->previous() == obj->lastProperty() &&
                    entry->vshape() == rt->protoHazardShape &&
                    shape->hasDefaultSetter()) {
                  // Just stick it in the slot
         // Go through either js_SetPropertyHelper if no setter hook or
         // obj->setProperty if there is one

The key part is that if we have a propcached property add, the next time around we will skip calling addProperty if the shape has a default setter.

This is causing the test fail observed in bug 660233 comment 43: for the second and later media element we don't call addProperty, so don't end up preserving the wrapper and a GC ends up wiping out the expando.

Fixing this in interp is pretty easy, but what changes do we need to the JITs and ICs?

And one more thing: the fact that we have to do this on every property add on DOM nodes kinda sucks.  After the first add the class addProperty is a no-op anyway....
OK, I'm assuming things handle setters soundly and looking for places that check shape->hasDefaultSetter but not addProperty.

PolyIC.cpp seems to do something with checking addProperty hooks, so I'm going to hope it's OK.  Would love a double-check on that.

StubCalls has the same issue as interp, easily fixable.

I can't figure out what MonoIC is doing.

Compiler.cpp only checks shape->hasDefaultSetter for setgname, so should be ok for my purposes.

The jsobj.cpp uses are ok as far as I can tell.

The tracer looks like it sends property _adds_ through TraceRecorder::addDataProperty which does check for addProperty and bails.  So that should be safe too.

So maybe the bug is just limited to jsinterp/stubcalls.
Attached patch Proposed fixSplinter Review
I have no idea how to write shell tests for this....
Attachment #561963 - Flags: review?(bhackett1024)
Assignee: general → bzbarsky
Priority: -- → P1
Whiteboard: [need review]
Attachment #561963 - Flags: review?(bhackett1024) → review+
Brian, thanks!
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla9
Closed: 9 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Attached patch test.Splinter Review
Is there any way I can make the test assert that we in fact hit the propcache?
Attachment #571085 - Flags: review?(jwalden+bmo)
Comment on attachment 571085 [details] [diff] [review]

Review of attachment 571085 [details] [diff] [review]:

I think, if you had JS_PROPERTY_CACHE_METERING defined, you could check stats and verify it.  But that's not the default even in debug builds.  Beyond that, or manually probing the cache in ways tied very specifically to the bytecode of the exec'd script, I'm not aware of anything.  jorendorff might know, possibly...

::: js/src/jsapi-tests/testAddPropertyPropcache.cpp
@@ +34,5 @@
> +    jsvalRoot proto(cx);
> +    JSObject *obj = JS_NewObject(cx, NULL, NULL, NULL);
> +    CHECK(obj);
> +    proto = OBJECT_TO_JSVAL(obj);
> +    JS_InitClass(cx, global, obj, &addPropertyClass, NULL, 0, NULL, NULL, NULL,

CHECK the result of this is != NULL.

@@ +38,5 @@
> +    JS_InitClass(cx, global, obj, &addPropertyClass, NULL, 0, NULL, NULL, NULL,
> +                 NULL);
> +
> +    jsvalRoot arr(cx);
> +    obj = JS_NewArrayObject(cx, 0, NULL);

I think we have an API to create an array from a pointer and length, if you ever want to do this again.  But it's test code, so who cares about that slightly-cleaner way now?
Attachment #571085 - Flags: review?(jwalden+bmo) → review+
Test pushed as
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.