Last Comment Bug 688685 - Property cache handling of property adds is unsound when the JSClass has an addProperty hook and the Shape has no setter
: Property cache handling of property adds is unsound when the JSClass has an a...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: mozilla9
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
:
Mentors:
Depends on:
Blocks: 660233
  Show dependency treegraph
 
Reported: 2011-09-22 20:38 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2011-11-04 03:05 PDT (History)
8 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed fix (2.50 KB, patch)
2011-09-22 21:08 PDT, Boris Zbarsky [:bz] (still a bit busy)
bhackett1024: review+
Details | Diff | Splinter Review
test. (3.09 KB, patch)
2011-11-01 11:50 PDT, Boris Zbarsky [:bz] (still a bit busy)
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2011-09-22 20:38:46 PDT
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
                  break;
            }
         }
         // 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....
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-09-22 20:57:55 PDT
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.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2011-09-22 21:08:09 PDT
Created attachment 561963 [details] [diff] [review]
Proposed fix

I have no idea how to write shell tests for this....
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2011-09-22 22:10:10 PDT
Brian, thanks!

http://hg.mozilla.org/integration/mozilla-inbound/rev/4309aaa4b59b
Comment 4 Ed Morley [:emorley] 2011-09-23 04:38:00 PDT
https://hg.mozilla.org/mozilla-central/rev/4309aaa4b59b
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-11-01 11:50:03 PDT
Created attachment 571085 [details] [diff] [review]
test.

Is there any way I can make the test assert that we in fact hit the propcache?
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-03 17:23:21 PDT
Comment on attachment 571085 [details] [diff] [review]
test.

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?
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2011-11-03 22:33:43 PDT
Test pushed as https://hg.mozilla.org/integration/mozilla-inbound/rev/4f57f89653df
Comment 8 Ed Morley [:emorley] 2011-11-04 03:05:25 PDT
https://hg.mozilla.org/mozilla-central/rev/4f57f89653df

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