Created attachment 238389 [details] Test case for bug reproducing Under certain conditions JSC fails to look in prototype chain. I've attached test case to reproduce such behavior. It uses JSIL libraries, but example is pretty simple: function clone(kvp){ return kvp.MemberwiseClone(); } function runMain() { var kvp = JSIL.CreateInstanceOfType(System.Collections.Generic.KeyValuePair$b2.Of($jsilcore.System.Int32, $jsilcore.System.String).__Type__); for (var i=0; i<40; i++) { clone(kvp); } kvp = JSIL.CreateInstanceOfType(System.Collections.Generic.KeyValuePair$b2.Of($jsilcore.System.Int32, $jsilcore.System.Int32).__Type__) for (var i=0; i<2; i++) { clone(kvp); } } JSIL calls runMain(), that call clone function 42 times. On last call, kvp.MemberwiseClone call fails with: "'undefined' is not a function". Nothing has been changed in object on which it was called between 41st an 42nd call. Note: previous 40 calls was on another object. If we lower first cycle upper bound from 40 to lower value, problem will be not reproducible. until recent changes in Webkit (before Safari 7.1/8.0 release) problem was reproducible even with 8 as upper bound in first cycle. Problem is also not reproducible if clone function will be rewritten in next form: function clone(kvp){ return kvp['MemberwiseClone'](); }
<rdar://problem/18416918>
Created attachment 242005 [details] Fixes inline cache fast path accessing nonexistent getters.
Attachment 242005 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSCJSValue.h:266: The parameter name "propertyName" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 242005 [details] Fixes inline cache fast path accessing nonexistent getters. View in context: https://bugs.webkit.org/attachment.cgi?id=242005&action=review > Source/JavaScriptCore/runtime/JSCJSValueInlines.h:713 > + // If this is a primitive, we'll need to synthesize the prototype - > + // and if it's a string there are special properties to check first. > + JSObject* object; > + if (UNLIKELY(!isObject())) { > + if (isCell() && asString(*this)->getStringPropertySlot(exec, propertyName, slot)) > + return true; > + object = synthesizePrototype(exec); > + } else > + object = asObject(asCell()); > + > + if (object->getPropertySlot(exec, propertyName, slot)) > + return true; Looks exactly like JSValue::get() except for the return type. Maybe that one should wrap this?
Comment on attachment 242005 [details] Fixes inline cache fast path accessing nonexistent getters. View in context: https://bugs.webkit.org/attachment.cgi?id=242005&action=review > Source/JavaScriptCore/jit/JITOperations.cpp:152 > if (accessType == static_cast<AccessType>(stubInfo->accessType)) I think you can remove this line of code now. Perhaps this was a previous attempt to guard against the property type changing? In any case, it seems trivially true in the current state of the code, since we are testing something we just loaded against the place we loaded it from.
What about the other cases in which we perform a property access optimization only after doing a get()? For example, operationPutByIdStrictBuildList calls put() and then calls buildPutByIdList(), passing it the slot from prior to the property access.
(In reply to comment #6) > What about the other cases in which we perform a property access > optimization only after doing a get()? For example, > operationPutByIdStrictBuildList calls put() and then calls > buildPutByIdList(), passing it the slot from prior to the property access. If possible, I'd like to fix puts in a separate patch and keep this one small.
Created attachment 242086 [details] Fixes inline cache fast path accessing nonexistent getters. Fixes get not being a reference to getPropertySlot.
Comment on attachment 242086 [details] Fixes inline cache fast path accessing nonexistent getters. View in context: https://bugs.webkit.org/attachment.cgi?id=242086&action=review Fix looks okay to me, but I'll let someone else do final review. Some drive-by comments: > Source/JavaScriptCore/ChangeLog:5 > + https://bugs.webkit.org/show_bug.cgi?id=136961 Please include the radar link here as well. > Source/JavaScriptCore/ChangeLog:23 > + * tests/stress/badproperty.js: Added test case for bug. badproperty.js seems like a very generic name for this test. :) > Source/JavaScriptCore/runtime/JSCJSValueInlines.h:686 > + bool hasResult = getPropertySlot(exec, propertyName, slot); > + return hasResult ? slot.getValue(exec, propertyName) : jsUndefined(); I don't think the local variable here adds anything. My suggestion: if (getPropertySlot(exec, propertyName, slot)) return slot.getValue(exec, propertyName); return jsUndefined(); > Source/JavaScriptCore/runtime/JSCJSValueInlines.h:703 > if (object->getPropertySlot(exec, propertyName, slot)) > - return slot.getValue(exec, propertyName); > - return jsUndefined(); > + return true; > + return false; This would look nicer: return object->getPropertySlot(exec, propertyName, slot);
Created attachment 242324 [details] Fixes inline cache fast path accessing nonexistent getters. Responds to kling's comments.
Comment on attachment 242324 [details] Fixes inline cache fast path accessing nonexistent getters. Clearing flags on attachment: 242324 Committed r176676: <http://trac.webkit.org/changeset/176676>
All reviewed patches have been landed. Closing bug.