Bug 83839

Summary: GC in the middle of JSObject::allocatePropertyStorage can cause badness
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg@apple.com>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg@apple.com>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough@apple.com, fpizlo@apple.com, ggaren@apple.com, webkit.review.bot@gmail.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description From 2012-04-12 18:08:25 PST
Currently, when we're in the middle of JSObject::allocatePropertyStorage, our Structure has inconsistent information. The Structure has just had a property added to it and it thinks it is bigger in both size and capacity, but our actual property backing store is still the same size as before. This can lead the garbage collector to do bad things based on this inaccurate information. We should instead check to see if we need to allocate more space first and do so if necessary, and then add the new property to our structure.
------- Comment #1 From 2012-04-12 18:13:49 PST -------
Created an attachment (id=137007) [details]
Patch
------- Comment #2 From 2012-04-12 18:38:47 PST -------
(From update of attachment 137007 [details])
Attachment 137007 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12391710
------- Comment #3 From 2012-04-12 18:54:47 PST -------
Created an attachment (id=137023) [details]
Patch
------- Comment #4 From 2012-04-12 21:52:56 PST -------
(From update of attachment 137023 [details])
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.
------- Comment #5 From 2012-04-13 11:02:41 PST -------
Created an attachment (id=137104) [details]
Patch
------- Comment #6 From 2012-04-13 11:24:40 PST -------
(From update of attachment 137104 [details])
Attachment 137104 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12400123
------- Comment #7 From 2012-04-13 11:32:50 PST -------
Created an attachment (id=137112) [details]
Patch
------- Comment #8 From 2012-04-13 11:55:57 PST -------
(From update of attachment 137112 [details])
Attachment 137112 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12403044
------- Comment #9 From 2012-04-13 12:04:39 PST -------
Created an attachment (id=137121) [details]
Patch
------- Comment #10 From 2012-04-13 14:47:23 PST -------
(From update of attachment 137121 [details])
r=me

Did the test cases I mentioned pan out?
------- Comment #11 From 2012-04-16 08:10:26 PST -------
(From update of attachment 137121 [details])
Clearing flags on attachment: 137121

Committed r114255: <http://trac.webkit.org/changeset/114255>
------- Comment #12 From 2012-04-16 08:10:31 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #13 From 2012-04-16 15:27:42 PST -------
<rdar://problem/10884040>
------- Comment #14 From 2012-05-08 21:45:52 PST -------
Reverted in r116494, this causes many failures if COLLECT_ON_EVERY_ALLOCATION is enabled.
------- Comment #15 From 2012-05-09 15:56:30 PST -------
Rolling this change back in since the underlying bug that it revealed should have been fixed in http://trac.webkit.org/changeset/116565.
------- Comment #16 From 2012-05-16 11:48:19 PST -------
Created an attachment (id=142313) [details]
Patch
------- Comment #17 From 2012-05-16 11:48:42 PST -------
Resubmitting patch to make sure everything still builds.
------- Comment #18 From 2012-05-16 14:20:55 PST -------
Committed r117343: <http://trac.webkit.org/changeset/117343>