Bug 69102

Summary: Structure transitions involving many (> 64) properties sometimes cause structure corruption
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 68990    
Attachments:
Description Flags
the patch darin: review+

Description Filip Pizlo 2011-09-29 13:24:55 PDT
This involves two problems:

1) Some transitions, such as function despecify, lead the new structure to "forget" that it should have been a dictionary.

2) Flattening a dictionary with many (> 64) fields overflows Structure::m_offset, making it impossible to rematerialize the property map.
Comment 1 Filip Pizlo 2011-09-29 13:27:22 PDT
Created attachment 109196 [details]
the patch

I tried to reduce the fail to a test case, but failed.  The chain of structure transitions that causes this bug to manifest is too chaotic for my small mind.
Comment 2 Darin Adler 2011-09-29 13:36:18 PDT
Comment on attachment 109196 [details]
the patch

Unfortunate that we can’t figure out how to test. Given we did signed char before, I’m surprised you went right to int rather than considering short. Not sure how we’re doing on total size of Structure.
Comment 3 Geoffrey Garen 2011-09-29 13:37:23 PDT
If you really want to support an arbitrary size, shouldn't m_offset be size_t?
Comment 4 Filip Pizlo 2011-09-29 13:38:26 PDT
(In reply to comment #2)
> (From update of attachment 109196 [details])
> Unfortunate that we can’t figure out how to test. Given we did signed char before, I’m surprised you went right to int rather than considering short. Not sure how we’re doing on total size of Structure.

I went to int because I didn't want to see this bug ever again.  I considered unsigned, but that would make the change bigger (the code uses -1 as a marker).
Comment 5 Filip Pizlo 2011-09-29 13:41:32 PDT
(In reply to comment #3)
> If you really want to support an arbitrary size, shouldn't m_offset be size_t?

I could imagine code that wants > 2^15 properties.  I've seen Java code out there that pushes right up to that limit.  (Java has a 2^16 hard limit on fields, and I've seen code generators that push that limit by splitting the code into multiple classes.)  If someone wanted to set >2^31 fields, then we'd probably fall over and die for other reasons.

I didn't want to use an unsigned type because that would require making this a bigger change.  We use -1 as a marker.  And anyway, the difference between dying at 2^31 and 2^32 is not so great.
Comment 6 Filip Pizlo 2011-09-29 13:45:08 PDT
Landed in r96354.