Bug 69102 - Structure transitions involving many (> 64) properties sometimes cause structure corruption
Summary: Structure transitions involving many (> 64) properties sometimes cause struct...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 68990
  Show dependency treegraph
 
Reported: 2011-09-29 13:24 PDT by Filip Pizlo
Modified: 2011-09-30 05:49 PDT (History)
2 users (show)

See Also:


Attachments
the patch (2.48 KB, patch)
2011-09-29 13:27 PDT, Filip Pizlo
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.