Bug 22393 – Segfault when caching property accesses to primitive cells.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Gavin Barraclough]

Test case cases segfault on ToT.

[Gavin Barraclough]

Created attachment 25335 [details]
Test file (1/2)

[Gavin Barraclough]

Created attachment 25336 [details]
Test file (2/2)

[Gavin Barraclough]

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).

[Gavin Barraclough]

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.

[Gavin Barraclough]

Hrrmmmm, the line "JSObject* slotBaseObject = asObject(slot.slotBase());" is starting to look fishy to me.

[Geoffrey Garen]

Created attachment 26114 [details]
patch

[Gavin Barraclough]

Comment on attachment 26114 [details]
patch

r=me

[Geoffrey Garen]

Committed revision 39374.