Title: [274727] trunk
- Revision
- 274727
- Author
- [email protected]
- Date
- 2021-03-19 10:48:47 -0700 (Fri, 19 Mar 2021)
Log Message
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/_javascript_Core:
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):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (274726 => 274727)
--- trunk/JSTests/ChangeLog 2021-03-19 17:48:11 UTC (rev 274726)
+++ trunk/JSTests/ChangeLog 2021-03-19 17:48:47 UTC (rev 274727)
@@ -1 +1,11 @@
+2021-03-19 Mark Lam <[email protected]>
+
+ 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 ==
Added: trunk/JSTests/stress/BrandedStructure-should-keep-its-members-alive.js (0 => 274727)
--- trunk/JSTests/stress/BrandedStructure-should-keep-its-members-alive.js (rev 0)
+++ trunk/JSTests/stress/BrandedStructure-should-keep-its-members-alive.js 2021-03-19 17:48:47 UTC (rev 274727)
@@ -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();
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (274726 => 274727)
--- trunk/Source/_javascript_Core/ChangeLog 2021-03-19 17:48:11 UTC (rev 274726)
+++ trunk/Source/_javascript_Core/ChangeLog 2021-03-19 17:48:47 UTC (rev 274727)
@@ -1,3 +1,49 @@
+2021-03-19 Mark Lam <[email protected]>
+
+ 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 <[email protected]>
Add PropertyName parameter to custom setters to allow shared implementations to do late name lookup
Modified: trunk/Source/_javascript_Core/runtime/BrandedStructure.h (274726 => 274727)
--- trunk/Source/_javascript_Core/runtime/BrandedStructure.h 2021-03-19 17:48:11 UTC (rev 274726)
+++ trunk/Source/_javascript_Core/runtime/BrandedStructure.h 2021-03-19 17:48:47 UTC (rev 274727)
@@ -60,13 +60,25 @@
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;
Modified: trunk/Source/_javascript_Core/runtime/Structure.cpp (274726 => 274727)
--- trunk/Source/_javascript_Core/runtime/Structure.cpp 2021-03-19 17:48:11 UTC (rev 274726)
+++ trunk/Source/_javascript_Core/runtime/Structure.cpp 2021-03-19 17:48:47 UTC (rev 274727)
@@ -342,6 +342,9 @@
{
if (typeInfo().structureIsImmortal())
return;
+
+ if (isBrandedStructure())
+ static_cast<BrandedStructure*>(this)->destruct();
Heap::heap(this)->structureIDTable().deallocateID(this, m_blob.structureID());
}
@@ -1295,6 +1298,9 @@
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);
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
