Skip to content
Permalink
Browse files
BrandedStructure should keep its members alive.
https://bugs.webkit.org/show_bug.cgi?id=223495
rdar://75565765

Reviewed by Saam Barati.

JSTests:

* stress/BrandedStructure-should-keep-its-members-alive.js: Added.

Source/JavaScriptCore:

Normally, each type of JSCell would have its own structure (and therefore, its own
ClassInfo, MethodTable, etc), which would have handled visiting m_parentBrand.
Similarly, it would have its own destructor, which would deref m_brand.

However, the design of BrandedStructure is not like other JSCells.  As present,
we have chosen to go with having BrandedStructure look exactly like a regular
Structure, except that its isBrandedStructure flag is set to true.

This design has advantages because we do checks all over the system for whether
a cell is a Structure by simply comparing its structureID to structureStructure's
structureID.  By virtue of BrandedStructure having the same structure as Structure,
none of this code need to change.

The downside is that we need to enhance Structure's methods to check if it is
actually working on an instance of BrandedStructure, and do some additional work.

This patch fixes 2 bugs:

1. m_parentBrand was not visited by visitChildren().

   Structure::visitChildrenImpl() now calls BrandedStructure::visitAdditionalChildren()
   to handle this.

2. m_brand needs to be ref'ed.

   In Structure::setBrandTransition(), if the BrandedStructure is a dictionary,
   then its m_transitionPropertyName will be cleared.  m_transitionPropertyName
   was the only means by which the UniqueStringImpl pointed to by m_brand was
   ref'ed.  The fix is to make m_brand a RefPtr.

   Hence, it follows that we also need to deref m_brand on destruction.
   Structure's destructor now calls BrandedStructure::destruct() to handle this.

* runtime/BrandedStructure.h:
* runtime/Structure.cpp:
(JSC::Structure::~Structure):
(JSC::Structure::visitChildrenImpl):



Canonical link: https://commits.webkit.org/235547@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@274727 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
Mark Lam committed Mar 19, 2021
1 parent b40c42d commit 0ce363b1718d1c98f288349b575d3c25ab167c23
Showing 5 changed files with 93 additions and 2 deletions.
@@ -1 +1,11 @@
2021-03-19 Mark Lam <mark.lam@apple.com>

BrandedStructure should keep its members alive.
https://bugs.webkit.org/show_bug.cgi?id=223495
rdar://75565765

Reviewed by Saam Barati.

* stress/BrandedStructure-should-keep-its-members-alive.js: Added.

== Rolled over to ChangeLog-2021-03-18 ==
@@ -0,0 +1,17 @@
class U {
constructor() {
return Object;
}
}

for (let i = 0; i < 1000; i++) {
class C extends U {
#x;
#y() {}
constructor() {
super();
}
}
new C();
gc();
}
@@ -1,3 +1,49 @@
2021-03-19 Mark Lam <mark.lam@apple.com>

BrandedStructure should keep its members alive.
https://bugs.webkit.org/show_bug.cgi?id=223495
rdar://75565765

Reviewed by Saam Barati.

Normally, each type of JSCell would have its own structure (and therefore, its own
ClassInfo, MethodTable, etc), which would have handled visiting m_parentBrand.
Similarly, it would have its own destructor, which would deref m_brand.

However, the design of BrandedStructure is not like other JSCells. As present,
we have chosen to go with having BrandedStructure look exactly like a regular
Structure, except that its isBrandedStructure flag is set to true.

This design has advantages because we do checks all over the system for whether
a cell is a Structure by simply comparing its structureID to structureStructure's
structureID. By virtue of BrandedStructure having the same structure as Structure,
none of this code need to change.

The downside is that we need to enhance Structure's methods to check if it is
actually working on an instance of BrandedStructure, and do some additional work.

This patch fixes 2 bugs:

1. m_parentBrand was not visited by visitChildren().

Structure::visitChildrenImpl() now calls BrandedStructure::visitAdditionalChildren()
to handle this.

2. m_brand needs to be ref'ed.

In Structure::setBrandTransition(), if the BrandedStructure is a dictionary,
then its m_transitionPropertyName will be cleared. m_transitionPropertyName
was the only means by which the UniqueStringImpl pointed to by m_brand was
ref'ed. The fix is to make m_brand a RefPtr.

Hence, it follows that we also need to deref m_brand on destruction.
Structure's destructor now calls BrandedStructure::destruct() to handle this.

* runtime/BrandedStructure.h:
* runtime/Structure.cpp:
(JSC::Structure::~Structure):
(JSC::Structure::visitChildrenImpl):

2021-03-19 Sam Weinig <weinig@apple.com>

Add PropertyName parameter to custom setters to allow shared implementations to do late name lookup
@@ -60,13 +60,25 @@ class BrandedStructure final : public Structure {
return false;
}

private:
template<typename Visitor>
static void visitAdditionalChildren(JSCell* cell, Visitor& visitor)
{
BrandedStructure* thisObject = jsCast<BrandedStructure*>(cell);
visitor.append(thisObject->m_parentBrand);
}

private:
BrandedStructure(VM&, Structure*, UniquedStringImpl* brand, DeferredStructureTransitionWatchpointFire*);
BrandedStructure(VM&, BrandedStructure*, DeferredStructureTransitionWatchpointFire*);

static Structure* create(VM&, Structure*, UniquedStringImpl* brand, DeferredStructureTransitionWatchpointFire* = nullptr);

UniquedStringImpl* m_brand;
void destruct()
{
m_brand = nullptr;
}

RefPtr<UniquedStringImpl> m_brand;
WriteBarrier<BrandedStructure> m_parentBrand;

friend class Structure;
@@ -342,6 +342,9 @@ Structure::~Structure()
{
if (typeInfo().structureIsImmortal())
return;

if (isBrandedStructure())
static_cast<BrandedStructure*>(this)->destruct();
Heap::heap(this)->structureIDTable().deallocateID(this, m_blob.structureID());
}

@@ -1295,6 +1298,9 @@ void Structure::visitChildrenImpl(JSCell* cell, Visitor& visitor)
visitor.append(thisObject->m_propertyTableUnsafe);
else if (thisObject->m_propertyTableUnsafe)
thisObject->m_propertyTableUnsafe.clear();

if (thisObject->isBrandedStructure())
BrandedStructure::visitAdditionalChildren(cell, visitor);
}

DEFINE_VISIT_CHILDREN(Structure);

0 comments on commit 0ce363b

Please sign in to comment.