Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
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...
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)
: Jason Orendorff [:jorendorff]
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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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
         // 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!
Comment 4 Ed Morley [:emorley] 2011-09-23 04:38:00 PDT
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-11-01 11:50:03 PDT
Created attachment 571085 [details] [diff] [review]

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]

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
Comment 8 Ed Morley [:emorley] 2011-11-04 03:05:25 PDT

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