Bug 22393 - Segfault when caching property accesses to primitive cells.
Summary: Segfault when caching property accesses to primitive cells.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P1 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-20 21:30 PST by Gavin Barraclough
Modified: 2008-12-18 09:45 PST (History)
0 users

See Also:


Attachments
Test file (1/2) (368 bytes, text/plain)
2008-11-20 21:31 PST, Gavin Barraclough
no flags Details
Test file (2/2) (1.48 KB, text/plain)
2008-11-20 21:32 PST, Gavin Barraclough
no flags Details
patch (9.40 KB, patch)
2008-12-17 23:13 PST, Geoffrey Garen
barraclough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2008-11-20 21:30:19 PST
Test case cases segfault on ToT.
Comment 1 Gavin Barraclough 2008-11-20 21:31:23 PST
Created attachment 25335 [details]
Test file (1/2)
Comment 2 Gavin Barraclough 2008-11-20 21:32:03 PST
Created attachment 25336 [details]
Test file (2/2)
Comment 3 Gavin Barraclough 2008-11-20 21:39:08 PST
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).
Comment 4 Gavin Barraclough 2008-11-20 21:44:57 PST
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.

Comment 5 Gavin Barraclough 2008-11-20 22:30:19 PST
Hrrmmmm, the line "JSObject* slotBaseObject = asObject(slot.slotBase());" is starting to look fishy to me.
Comment 6 Geoffrey Garen 2008-12-17 23:13:46 PST
Created attachment 26114 [details]
patch
Comment 7 Gavin Barraclough 2008-12-17 23:21:49 PST
Comment on attachment 26114 [details]
patch

r=me
Comment 8 Geoffrey Garen 2008-12-18 09:45:56 PST
Committed revision 39374.