Summary: | GC in the middle of JSObject::allocatePropertyStorage can cause badness | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Hahnenberg <mhahnenberg> | ||||||||||||||
Component: | JavaScriptCore | Assignee: | Mark Hahnenberg <mhahnenberg> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | barraclough, fpizlo, ggaren, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Mark Hahnenberg
2012-04-12 18:08:25 PDT
Created attachment 137007 [details]
Patch
Comment on attachment 137007 [details] Patch Attachment 137007 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12391710 Created attachment 137023 [details]
Patch
Comment on attachment 137023 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137023&action=review I think you want an interface like this: // Add property with structure transition newStorage = reallocPropertyStorage(m_propertyStorage.get(), ...); // Returns new storage, does not assign. newStructure = Structure::addPropertyTransition(...); // Returns new structure, does not assign. setPropertyStorage(newStorage, newStructure); // Assigns structure and storage "atomically". // Add property without structure transition newStorage = reallocPropertyStorage(m_propertyStorage.get(), ...); // Returns new storage, does not assign. setPropertyStorage(newStorage, m_structure.get()); // Assigns structure and storage "atomically". Since storage and structure need to be in sync, we should never set one without setting the other. > Source/JavaScriptCore/runtime/JSObject.h:661 > + if (structure()->shouldResizePropertyStorage()) Let's call this "shouldGrowPropertyStorage". "Resize" is imprecise, because we don't shrink. > Source/JavaScriptCore/runtime/JSObject.h:662 > + allocatePropertyStorage(globalData, structure()->propertyStorageCapacity(), structure()->suggestedPropertyStorageResizeAmount()); Let's call this "growPropertyStorage" or "reallocPropertyStorage" to help clarify that we're allocating for a second time. > Source/JavaScriptCore/runtime/JSObject.h:727 > + Structure* structure = Structure::addPropertyTransition(globalData, this->structure(), propertyName, attributes, specificFunction, offset); This addPropertyTransition will allocate a new structure, which can cause a GC. During that GC, our Structure's information about our backing store will be out of date (specifically, it will underestimate the size / capacity of our backing store). I'm not sure if that will definitely cause a problem, but it seems not so good. We never want our information about our backing store to be out of date. Created attachment 137104 [details]
Patch
Comment on attachment 137104 [details] Patch Attachment 137104 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12400123 Created attachment 137112 [details]
Patch
Comment on attachment 137112 [details] Patch Attachment 137112 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12403044 Created attachment 137121 [details]
Patch
Comment on attachment 137121 [details]
Patch
r=me
Did the test cases I mentioned pan out?
Comment on attachment 137121 [details] Patch Clearing flags on attachment: 137121 Committed r114255: <http://trac.webkit.org/changeset/114255> All reviewed patches have been landed. Closing bug. Reverted in r116494, this causes many failures if COLLECT_ON_EVERY_ALLOCATION is enabled. Rolling this change back in since the underlying bug that it revealed should have been fixed in http://trac.webkit.org/changeset/116565. Created attachment 142313 [details]
Patch
Resubmitting patch to make sure everything still builds. Committed r117343: <http://trac.webkit.org/changeset/117343> |