Test case cases segfault on ToT.
Created attachment 25335 [details] Test file (1/2)
Created attachment 25336 [details] Test file (2/2)
The bug can be circumvented by adding an isObject(baseValue) check in tryCTICacheGetByID. I have not tested whether the problem also exists in the non-JIT code path. When this is fixed, cti_op_get_by_id_proto_list should also be updated (this currently is preventing polymorphic caching of prototype accesses, since allowing this caused a test failure in an existing Layout test).
Comments from Darin's original review of the polymorphic prototype list caching code: > > + if (baseValue->isObject() && > > + slot.isCacheable() && > > + !asCell(baseValue)->structure()->isDictionary() && > > + slot.slotBase() == asCell(baseValue)->structure()->prototypeForLookup(callFrame)) { > > We normally try to format these with the operators at the starts of lines > instead of the ends -- I think it makes things read a little bit clearer. > > Those two places where you call asCell, I think I'd prefer asObject, because > you've already established it's an object, and it's possible that we might some > day implement an operation like structure() slightly more efficiently for > JSObject than a JSCell. OK, it's not a realistic possibility, but still I like > types to be as specific as possible in case that helps with optimization later. > > > + JSCell* baseCell = asCell(baseValue); > > Same thought here. > > I also think that if the values are worth putting into local variables for the > code, maybe it's worth using a nested if and using the local variables in the > if conditional too. This may all no longer be relevant, assuming caching of accesses to cell primitives is fixed, since the check at the top of this can then be !JSImmediate::isImmediate(baseValue). Repasted here incase these comments provide useful guidance in refactoring the code, post bug fix. > > + // Heavy access to a prototype is a good indication that it's not being > > + // used as a dictionary. > > Great comment, but I don't understand how the single call here indicates "heavy > use". You might want to word it slightly differently to make that clearer. Not *strictly* related to this bug as such ;-) , but a comment from Darin in this area of the code.
Hrrmmmm, the line "JSObject* slotBaseObject = asObject(slot.slotBase());" is starting to look fishy to me.
Created attachment 26114 [details] patch
Comment on attachment 26114 [details] patch r=me
Committed revision 39374.