Bug 177639 – Custom GetterSetterAccessCase does not use the correct slotBase when making call

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

Created attachment 322157 [details]
jsc.cpp modification

Maybe there is a bug, but here is a test I added for poly proto. If you run this w/ my jsc.cpp changes, this test crashes:
```
function assert(b, m) {
    if (!b)
        throw new Error("Bad:" + m);
}

class Class { };
function makePolyProtoObject() {
    return new Class;
}

let items = [
    makePolyProtoObject(),
    makePolyProtoObject(),
    makePolyProtoObject(),
    makePolyProtoObject(),
];

let customGetterSetter = createCustomTestGetterSetter();
items.forEach((x) => {
    x.__proto__ = customGetterSetter;
    assert(x.__proto__ === customGetterSetter);
});

function validate(x, valueResult, accessorResult) {
    assert(x.customValue === valueResult);

    assert(x.customAccessor === accessorResult);

    let o = {};
    x.customValue = o;
    assert(o.result === valueResult);

    o = {};
    x.customAccessor = o;
    assert(o.result === accessorResult);

    assert(x.randomProp === 42 || x.randomProp === undefined);
}
noInline(validate);


let start = Date.now();
for (let i = 0; i < 10000; ++i) {
    for (let i = 0; i < items.length; ++i) {
        validate(items[i], customGetterSetter, items[i]);
    }
    print(i);
}

customGetterSetter.randomProp = 42;

for (let i = 0; i < 10000; ++i) {
    for (let i = 0; i < items.length; ++i) {
        validate(items[i], customGetterSetter, items[i]);
    }
}

items.forEach((x) => {
    Reflect.setPrototypeOf(x, {
        get customValue() { return 42; },
        get customAccessor() { return 22; },
        set customValue(x) { x.result = 42; },
        set customAccessor(x) { x.result = 22; },
    });
});

for (let i = 0; i < 10000; ++i) {
    for (let i = 0; i < items.length; ++i) {
        validate(items[i], 42, 22);
    }
}

if (true)
    print(Date.now() - start);
```

[Saam Barati]

Yeah, this code seems really weird:

```
ObjectPropertyConditionSet generateConditionsForPrototypePropertyHitCustom(
    VM& vm, JSCell* owner, ExecState* exec, Structure* headStructure, JSObject* prototype,
    UniquedStringImpl* uid)
{
    return generateConditions(
        vm, exec->lexicalGlobalObject(), headStructure, prototype,
        [&] (Vector<ObjectPropertyCondition>& conditions, JSObject* object) -> bool {
            if (object == prototype)
                return true;
            ObjectPropertyCondition result =
                generateCondition(vm, owner, object, uid, PropertyCondition::Absence);
            if (!result)
                return false;
            conditions.append(result);
            return true;
        });
}
```

Note, if we have some object o whose __proto__ points to an object with a customValueGetter, we'll end up in this situation. We'll produce an m_conditionSet.isEmpty(). This causes AccessCase to use the base for the access as the slot holder. Clearly that's wrong in this type of program.

[Saam Barati]

I spoke to Fil, this code on OPC set generation is actually fine. The bug is here I believe, inside access case:

        if (!m_conditionSet.isEmpty()) {
            jit.move(
                CCallHelpers::TrustedImmPtr(alternateBase()),
                scratchGPR);
            baseForAccessGPR = scratchGPR;
        } else
            baseForAccessGPR = baseForGetGPR;

In the above program, you will have an empty set, however, we really do want the alternate base.

[Saam Barati]

I still think the cleanest fix it to remove generateConditionsForPrototypePropertyHitCustom and just use generateConditionsForPrototypePropertyHit. We already do this for gets. We only do generateConditionsForPrototypePropertyHitCustom for puts, weirdly enough.

If we decide not to do this, what we need to do is then pipe through a notion of a prototype access vs non prototype access that doesn't rely on m_conditionSet.isEmpty(). Then, if we're a custom getter/setter *and not* a prototype access, we simply use the access's base. If we're a custom getter/setter *and* a prototype access, then we call GetterSetterAccessCase's alternateBase() function.

I think the latter is probably prone to bugs and breakage, which is why I'm more in favor of the first way. And since we already do the first way for custom getters, I doubt it'll be a regression to also do it for custom setters.

[Saam Barati]

Created attachment 322224 [details]
patch

[Build Bot]

Attachment 322224 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jsc.cpp:1076:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 322224 [details]
patch

r=me

I don't see any downside to this approach.

Still, I think it's super weird, and a design booby trap, that we use "!m_conditionSet.isEmtpy()" to mean "this is a prototype access". Would be nice to clean that up in the long run. I'm not particularly interested in restoring the optimization to skip a condition in this case -- I just want to make the design more obvious and explicit.

You know something is not obvious when it takes three paragraphs and an IRC conversation to explain :P.

[Saam Barati]

(In reply to Geoffrey Garen from comment #6)
> Comment on attachment 322224 [details]
> patch
> 
> r=me
> 
> I don't see any downside to this approach.
> 
> Still, I think it's super weird, and a design booby trap, that we use
> "!m_conditionSet.isEmtpy()" to mean "this is a prototype access". Would be
> nice to clean that up in the long run. I'm not particularly interested in
> restoring the optimization to skip a condition in this case -- I just want
> to make the design more obvious and explicit.
> 
> You know something is not obvious when it takes three paragraphs and an IRC
> conversation to explain :P.

I agree. At some point after the initial poly proto patch lands, I'm going to come up with a hybrid data structure between OPC set, and the data structure used for poly proto accesses. When doing that patch, I'll take the time to clean up the naming and clarity of things.

[WebKit Commit Bot]

Comment on attachment 322224 [details]
patch

Clearing flags on attachment: 322224

Committed r222671: <http://trac.webkit.org/changeset/222671>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34750577>

[Ryan Haddad]

The test added with this change is hitting an assertion failure on debug JSC bots:

stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: ASSERTION FAILED: !heap.vm()->isInitializingObject()
stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: /Volumes/Data/slave/highsierra-debug/build/Source/JavaScriptCore/runtime/JSCellInlines.h(163) : void *JSC::tryAllocateCellHelper(JSC::Heap &, JSC::GCDeferralContext *, size_t) [T = JSC::CustomGetterSetter, mode = JSC::AllocationFailureMode::ShouldAssertOnFailure, deferralContextArgPresence = JSC::GCDeferralContextArgPresense::DoesNotHaveArg]
stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: 1   0x1068e8efd WTFCrash
stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: 2   0x1039d9216 void* JSC::tryAllocateCellHelper<JSC::CustomGetterSetter, (JSC::AllocationFailureMode)0, (JSC::GCDeferralContextArgPresense)1>(JSC::Heap&, JSC::GCDeferralContext*, unsigned long)
stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: 3   0x1039d8f94 void* JSC::allocateCell<JSC::CustomGetterSetter>(JSC::Heap&, unsigned long)
stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: 4   0x1039d388e JSC::CustomGetterSetter::create(JSC::VM&, long long (*)(JSC::ExecState*, long long, JSC::PropertyName), bool (*)(JSC::ExecState*, long long, long long))
stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: 5   0x1039d3738 JSTestCustomGetterSetter::finishCreation(JSC::VM&)
stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: 6   0x103a2348a JSTestCustomGetterSetter::create(JSC::VM&, JSC::JSGlobalObject*, JSC::Structure*)
stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: 7   0x1039fbb19 functionCreateCustomTestGetterSetter(JSC::ExecState*)
stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: 8   0x34745cc01028
stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: 9   0x105576b89 llint_entry
stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: 10  0x10556ecf7 vmEntryToJavaScript
stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: 11  0x10665c21e JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: 12  0x1065fd046 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*)
stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: 13  0x10687b537 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: 14  0x103a23f87 runWithOptions(GlobalObject*, CommandLine&)
stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: 15  0x1039ea8e4 jscmain(int, char**)::$_3::operator()(JSC::VM&, GlobalObject*) const
stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: 16  0x1039d886f int runJSC<jscmain(int, char**)::$_3>(CommandLine, bool, jscmain(int, char**)::$_3 const&)
stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: 17  0x1039d735f jscmain(int, char**)
stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: 18  0x1039d72be main
stress/custom-get-set-inline-caching-one-level-up-proto-chain.js.default: 19  0x7fff6ceb6145 start

https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/14

[Saam Barati]

Will look into the assertion failure now.

[Saam Barati]

(In reply to Saam Barati from comment #12)
> Will look into the assertion failure now.

Fixed in:
https://trac.webkit.org/changeset/222713/webkit