[webkit-reviews] review denied: [Bug 83839] GC in the middle of JSObject::allocatePropertyStorage can cause badness : [Attachment 137023] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 83839: GC in the middle of JSObject::allocatePropertyStorage can cause
badness
https://bugs.webkit.org/show_bug.cgi?id=83839

Attachment 137023: Patch
https://bugs.webkit.org/attachment.cgi?id=137023&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.