WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
177639
Custom GetterSetterAccessCase does not use the correct slotBase when making call
https://bugs.webkit.org/show_bug.cgi?id=177639
Summary
Custom GetterSetterAccessCase does not use the correct slotBase when making call
Saam Barati
Reported
2017-09-28 19:47:18 PDT
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); ```
Attachments
jsc.cpp modification
(11.63 KB, patch)
2017-09-28 19:47 PDT
,
Saam Barati
no flags
Details
Formatted Diff
Diff
patch
(15.18 KB, patch)
2017-09-29 13:05 PDT
,
Saam Barati
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Saam Barati
Comment 1
2017-09-28 19:57:17 PDT
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
Comment 2
2017-09-29 09:46:02 PDT
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
Comment 3
2017-09-29 11:43:58 PDT
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
Comment 4
2017-09-29 13:05:20 PDT
Created
attachment 322224
[details]
patch
Build Bot
Comment 5
2017-09-29 13:07:33 PDT
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 6
2017-09-29 14:00:36 PDT
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
Comment 7
2017-09-29 14:06:15 PDT
(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 8
2017-09-29 16:48:14 PDT
Comment on
attachment 322224
[details]
patch Clearing flags on attachment: 322224 Committed
r222671
: <
http://trac.webkit.org/changeset/222671
>
WebKit Commit Bot
Comment 9
2017-09-29 16:48:16 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 10
2017-09-29 16:48:47 PDT
<
rdar://problem/34750577
>
Ryan Haddad
Comment 11
2017-10-02 09:17:46 PDT
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
Comment 12
2017-10-02 09:51:23 PDT
Will look into the assertion failure now.
Saam Barati
Comment 13
2017-10-02 10:09:27 PDT
(In reply to Saam Barati from
comment #12
)
> Will look into the assertion failure now.
Fixed in:
https://trac.webkit.org/changeset/222713/webkit
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug