Bug 83839 - GC in the middle of JSObject::allocatePropertyStorage can cause badness
: GC in the middle of JSObject::allocatePropertyStorage can cause badness
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Mark Hahnenberg
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-12 18:08 PDT by Mark Hahnenberg
Modified: 2012-05-16 14:20 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.82 KB, patch)
2012-04-12 18:13 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (6.82 KB, patch)
2012-04-12 18:54 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (12.85 KB, patch)
2012-04-13 11:02 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (13.37 KB, patch)
2012-04-13 11:32 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (13.82 KB, patch)
2012-04-13 12:04 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (13.15 KB, patch)
2012-05-16 11:48 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2012-04-12 18:08:25 PDT
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 Mark Hahnenberg 2012-04-12 18:13:49 PDT
Created attachment 137007 [details]
Patch
Comment 2 Build Bot 2012-04-12 18:38:47 PDT
Comment on attachment 137007 [details]
Patch

Attachment 137007 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12391710
Comment 3 Mark Hahnenberg 2012-04-12 18:54:47 PDT
Created attachment 137023 [details]
Patch
Comment 4 Geoffrey Garen 2012-04-12 21:52:56 PDT
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.
Comment 5 Mark Hahnenberg 2012-04-13 11:02:41 PDT
Created attachment 137104 [details]
Patch
Comment 6 Build Bot 2012-04-13 11:24:40 PDT
Comment on attachment 137104 [details]
Patch

Attachment 137104 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12400123
Comment 7 Mark Hahnenberg 2012-04-13 11:32:50 PDT
Created attachment 137112 [details]
Patch
Comment 8 Build Bot 2012-04-13 11:55:57 PDT
Comment on attachment 137112 [details]
Patch

Attachment 137112 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12403044
Comment 9 Mark Hahnenberg 2012-04-13 12:04:39 PDT
Created attachment 137121 [details]
Patch
Comment 10 Geoffrey Garen 2012-04-13 14:47:23 PDT
Comment on attachment 137121 [details]
Patch

r=me

Did the test cases I mentioned pan out?
Comment 11 WebKit Review Bot 2012-04-16 08:10:26 PDT
Comment on attachment 137121 [details]
Patch

Clearing flags on attachment: 137121

Committed r114255: <http://trac.webkit.org/changeset/114255>
Comment 12 WebKit Review Bot 2012-04-16 08:10:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Mark Hahnenberg 2012-04-16 15:27:42 PDT
<rdar://problem/10884040>
Comment 14 Gavin Barraclough 2012-05-08 21:45:52 PDT
Reverted in r116494, this causes many failures if COLLECT_ON_EVERY_ALLOCATION is enabled.
Comment 15 Mark Hahnenberg 2012-05-09 15:56:30 PDT
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 Mark Hahnenberg 2012-05-16 11:48:19 PDT
Created attachment 142313 [details]
Patch
Comment 17 Mark Hahnenberg 2012-05-16 11:48:42 PDT
Resubmitting patch to make sure everything still builds.
Comment 18 Mark Hahnenberg 2012-05-16 14:20:55 PDT
Committed r117343: <http://trac.webkit.org/changeset/117343>