JSCell::classInfo() shouldn't have a bunch of mitigations for being called during...
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Jan 2017 18:55:55 +0000 (18:55 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Jan 2017 18:55:55 +0000 (18:55 +0000)
https://bugs.webkit.org/show_bug.cgi?id=167066

Reviewed by Keith Miller and Michael Saboff.
Source/JavaScriptCore:

This reduces the size of JSCell::classInfo() by half and removes some checks that
this function previously had to do in case it was called from destructors.

I changed all of the destructors so that they don't call JSCell::classInfo() and I
added an assertion to JSCell::classInfo() to catch cases where someone called it
from a destructor accidentally.

This means that we only have one place in destruction that needs to know the class:
the sweeper's call to the destructor.

One of the trickiest outcomes of this is the need to support inherits() tests in
JSObjectGetPrivate(), when it is called from the destructor callback on the object
being destructed. JSObjectGetPrivate() is undefined behavior anyway if you use it
on any dead-but-not-destructed object other than the one being destructed right
now. The purpose of the inherits() tests is to distinguish between different kinds
of CallbackObjects, which may have different kinds of base classes. I think that
this was always subtly wrong - for example, if the object being destructed is a
JSGlobalObject then it's not a DestructibleObject, is not in a destructor block,
but does not have an immortal Structure - so classInfo() is not valid. This fixes
the issue by having ~JSCallbackObject know its classInfo. It now stashes its
classInfo in VM so that JSObjectGetPrivate can use that classInfo if it detects
that it's being used on a currently-destructing object.

That was the only really weird part of this patch. The rest is mostly removing
illegal uses of jsCast<> in destructors. There were a few other genuine uses of
classInfo() but they were in code that already knew how to get its classInfo()
using other means:

- You can still say structure()->classInfo(), and I use this form in code that
  knows that its StructureIsImmortal.

- You can use this->classInfo() if it's overridden, like in subclasses of
  JSDestructibleObject.

* API/JSAPIWrapperObject.mm:
(JSAPIWrapperObjectHandleOwner::finalize):
* API/JSCallbackObject.h:
* API/JSCallbackObjectFunctions.h:
(JSC::JSCallbackObject<Parent>::~JSCallbackObject):
(JSC::JSCallbackObject<Parent>::init):
* API/JSObjectRef.cpp:
(classInfoPrivate):
(JSObjectGetPrivate):
(JSObjectSetPrivate):
* bytecode/EvalCodeBlock.cpp:
(JSC::EvalCodeBlock::destroy):
* bytecode/FunctionCodeBlock.cpp:
(JSC::FunctionCodeBlock::destroy):
* bytecode/ModuleProgramCodeBlock.cpp:
(JSC::ModuleProgramCodeBlock::destroy):
* bytecode/ProgramCodeBlock.cpp:
(JSC::ProgramCodeBlock::destroy):
* bytecode/UnlinkedEvalCodeBlock.cpp:
(JSC::UnlinkedEvalCodeBlock::destroy):
* bytecode/UnlinkedFunctionCodeBlock.cpp:
(JSC::UnlinkedFunctionCodeBlock::destroy):
* bytecode/UnlinkedFunctionExecutable.cpp:
(JSC::UnlinkedFunctionExecutable::destroy):
* bytecode/UnlinkedModuleProgramCodeBlock.cpp:
(JSC::UnlinkedModuleProgramCodeBlock::destroy):
* bytecode/UnlinkedProgramCodeBlock.cpp:
(JSC::UnlinkedProgramCodeBlock::destroy):
* heap/CodeBlockSet.cpp:
(JSC::CodeBlockSet::lastChanceToFinalize):
(JSC::CodeBlockSet::deleteUnmarkedAndUnreferenced):
* heap/MarkedAllocator.cpp:
(JSC::MarkedAllocator::allocateSlowCaseImpl):
* heap/MarkedBlock.cpp:
(JSC::MarkedBlock::Handle::sweep):
* jit/JITThunks.cpp:
(JSC::JITThunks::finalize):
* runtime/AbstractModuleRecord.cpp:
(JSC::AbstractModuleRecord::destroy):
* runtime/ExecutableBase.cpp:
(JSC::ExecutableBase::clearCode):
* runtime/JSCellInlines.h:
(JSC::JSCell::classInfo):
(JSC::JSCell::callDestructor):
* runtime/JSLock.h:
(JSC::JSLock::ownerThread):
* runtime/JSModuleNamespaceObject.cpp:
(JSC::JSModuleNamespaceObject::destroy):
* runtime/JSModuleRecord.cpp:
(JSC::JSModuleRecord::destroy):
* runtime/JSPropertyNameEnumerator.cpp:
(JSC::JSPropertyNameEnumerator::destroy):
* runtime/JSSegmentedVariableObject.h:
* runtime/SymbolTable.cpp:
(JSC::SymbolTable::destroy):
* runtime/VM.h:
* wasm/js/JSWebAssemblyCallee.cpp:
(JSC::JSWebAssemblyCallee::destroy):
* wasm/js/WebAssemblyModuleRecord.cpp:
(JSC::WebAssemblyModuleRecord::destroy):
* wasm/js/WebAssemblyToJSCallee.cpp:
(JSC::WebAssemblyToJSCallee::WebAssemblyToJSCallee):
(JSC::WebAssemblyToJSCallee::destroy):

Source/WebCore:

No new tests because no new behavior.

It's now necessary to avoid jsCast in destructors and finalizers. This was an easy
rule to introduce because this used to always be the rule.

* bindings/js/JSCSSValueCustom.cpp:
(WebCore::JSDeprecatedCSSOMValueOwner::finalize):
* bindings/js/JSDOMIterator.h:
(WebCore::IteratorTraits>::destroy):
* bindings/scripts/CodeGeneratorJS.pm:
(GenerateImplementation):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@210821 268f45cc-cd09-0410-ab3c-d52691b4dbfc

35 files changed:
Source/JavaScriptCore/API/JSAPIWrapperObject.mm
Source/JavaScriptCore/API/JSCallbackObject.h
Source/JavaScriptCore/API/JSCallbackObjectFunctions.h
Source/JavaScriptCore/API/JSObjectRef.cpp
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/EvalCodeBlock.cpp
Source/JavaScriptCore/bytecode/FunctionCodeBlock.cpp
Source/JavaScriptCore/bytecode/ModuleProgramCodeBlock.cpp
Source/JavaScriptCore/bytecode/ProgramCodeBlock.cpp
Source/JavaScriptCore/bytecode/UnlinkedEvalCodeBlock.cpp
Source/JavaScriptCore/bytecode/UnlinkedFunctionCodeBlock.cpp
Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp
Source/JavaScriptCore/bytecode/UnlinkedModuleProgramCodeBlock.cpp
Source/JavaScriptCore/bytecode/UnlinkedProgramCodeBlock.cpp
Source/JavaScriptCore/heap/CodeBlockSet.cpp
Source/JavaScriptCore/heap/MarkedAllocator.cpp
Source/JavaScriptCore/heap/MarkedBlock.cpp
Source/JavaScriptCore/jit/JITThunks.cpp
Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp
Source/JavaScriptCore/runtime/ExecutableBase.cpp
Source/JavaScriptCore/runtime/JSCellInlines.h
Source/JavaScriptCore/runtime/JSLock.h
Source/JavaScriptCore/runtime/JSModuleNamespaceObject.cpp
Source/JavaScriptCore/runtime/JSModuleRecord.cpp
Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.cpp
Source/JavaScriptCore/runtime/JSSegmentedVariableObject.h
Source/JavaScriptCore/runtime/SymbolTable.cpp
Source/JavaScriptCore/runtime/VM.h
Source/JavaScriptCore/wasm/js/JSWebAssemblyCallee.cpp
Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp
Source/JavaScriptCore/wasm/js/WebAssemblyToJSCallee.cpp
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSCSSValueCustom.cpp
Source/WebCore/bindings/js/JSDOMIterator.h
Source/WebCore/bindings/scripts/CodeGeneratorJS.pm

index 3f54f43..f301d3e 100644 (file)
@@ -48,7 +48,7 @@ static JSAPIWrapperObjectHandleOwner* jsAPIWrapperObjectHandleOwner()
 
 void JSAPIWrapperObjectHandleOwner::finalize(JSC::Handle<JSC::Unknown> handle, void*)
 {
-    JSC::JSAPIWrapperObject* wrapperObject = JSC::jsCast<JSC::JSAPIWrapperObject*>(handle.get().asCell());
+    JSC::JSAPIWrapperObject* wrapperObject = static_cast<JSC::JSAPIWrapperObject*>(handle.get().asCell());
     if (!wrapperObject->wrappedObject())
         return;
 
index 31f2166..43749e2 100644 (file)
@@ -232,6 +232,7 @@ private:
     static EncodedJSValue callbackGetter(ExecState*, EncodedJSValue, PropertyName);
 
     std::unique_ptr<JSCallbackObjectData> m_callbackObjectData;
+    const ClassInfo* m_classInfo;
 };
 
 } // namespace JSC
index fc8cb3c..a525f5b 100644 (file)
@@ -74,11 +74,17 @@ JSCallbackObject<Parent>::JSCallbackObject(VM& vm, JSClassRef jsClass, Structure
 template <class Parent>
 JSCallbackObject<Parent>::~JSCallbackObject()
 {
+    VM* vm = this->HeapCell::vm();
+    vm->currentlyDestructingCallbackObject = this;
+    ASSERT(m_classInfo);
+    vm->currentlyDestructingCallbackObjectClassInfo = m_classInfo;
     JSObjectRef thisRef = toRef(static_cast<JSObject*>(this));
     for (JSClassRef jsClass = classRef(); jsClass; jsClass = jsClass->parentClass) {
         if (JSObjectFinalizeCallback finalize = jsClass->finalize)
             finalize(thisRef);
     }
+    vm->currentlyDestructingCallbackObject = nullptr;
+    vm->currentlyDestructingCallbackObjectClassInfo = nullptr;
 }
     
 template <class Parent>
@@ -117,6 +123,8 @@ void JSCallbackObject<Parent>::init(ExecState* exec)
         JSObjectInitializeCallback initialize = initRoutines[i];
         initialize(toRef(exec), toRef(this));
     }
+    
+    m_classInfo = this->classInfo();
 }
 
 template <class Parent>
index bc96eb1..5b3d37d 100644 (file)
@@ -380,21 +380,38 @@ bool JSObjectDeleteProperty(JSContextRef ctx, JSObjectRef object, JSStringRef pr
     return result;
 }
 
+// API objects have private properties, which may get accessed during destruction. This
+// helper lets us get the ClassInfo of an API object from a function that may get called
+// during destruction.
+static const ClassInfo* classInfoPrivate(JSObject* jsObject)
+{
+    VM* vm = jsObject->vm();
+    
+    if (vm->currentlyDestructingCallbackObject != jsObject)
+        return jsObject->classInfo();
+
+    return vm->currentlyDestructingCallbackObjectClassInfo;
+}
+
 void* JSObjectGetPrivate(JSObjectRef object)
 {
     JSObject* jsObject = uncheckedToJS(object);
 
+    const ClassInfo* classInfo = classInfoPrivate(jsObject);
+    
     // Get wrapped object if proxied
-    if (jsObject->inherits(JSProxy::info()))
-        jsObject = jsCast<JSProxy*>(jsObject)->target();
+    if (classInfo->isSubClassOf(JSProxy::info())) {
+        jsObject = static_cast<JSProxy*>(jsObject)->target();
+        classInfo = jsObject->classInfo();
+    }
 
-    if (jsObject->inherits(JSCallbackObject<JSGlobalObject>::info()))
-        return jsCast<JSCallbackObject<JSGlobalObject>*>(jsObject)->getPrivate();
-    if (jsObject->inherits(JSCallbackObject<JSDestructibleObject>::info()))
-        return jsCast<JSCallbackObject<JSDestructibleObject>*>(jsObject)->getPrivate();
+    if (classInfo->isSubClassOf(JSCallbackObject<JSGlobalObject>::info()))
+        return static_cast<JSCallbackObject<JSGlobalObject>*>(jsObject)->getPrivate();
+    if (classInfo->isSubClassOf(JSCallbackObject<JSDestructibleObject>::info()))
+        return static_cast<JSCallbackObject<JSDestructibleObject>*>(jsObject)->getPrivate();
 #if JSC_OBJC_API_ENABLED
-    if (jsObject->inherits(JSCallbackObject<JSAPIWrapperObject>::info()))
-        return jsCast<JSCallbackObject<JSAPIWrapperObject>*>(jsObject)->getPrivate();
+    if (classInfo->isSubClassOf(JSCallbackObject<JSAPIWrapperObject>::info()))
+        return static_cast<JSCallbackObject<JSAPIWrapperObject>*>(jsObject)->getPrivate();
 #endif
     
     return 0;
@@ -404,20 +421,24 @@ bool JSObjectSetPrivate(JSObjectRef object, void* data)
 {
     JSObject* jsObject = uncheckedToJS(object);
 
+    const ClassInfo* classInfo = classInfoPrivate(jsObject);
+    
     // Get wrapped object if proxied
-    if (jsObject->inherits(JSProxy::info()))
+    if (classInfo->isSubClassOf(JSProxy::info())) {
         jsObject = jsCast<JSProxy*>(jsObject)->target();
+        classInfo = jsObject->classInfo();
+    }
 
-    if (jsObject->inherits(JSCallbackObject<JSGlobalObject>::info())) {
+    if (classInfo->isSubClassOf(JSCallbackObject<JSGlobalObject>::info())) {
         jsCast<JSCallbackObject<JSGlobalObject>*>(jsObject)->setPrivate(data);
         return true;
     }
-    if (jsObject->inherits(JSCallbackObject<JSDestructibleObject>::info())) {
+    if (classInfo->isSubClassOf(JSCallbackObject<JSDestructibleObject>::info())) {
         jsCast<JSCallbackObject<JSDestructibleObject>*>(jsObject)->setPrivate(data);
         return true;
     }
 #if JSC_OBJC_API_ENABLED
-    if (jsObject->inherits(JSCallbackObject<JSAPIWrapperObject>::info())) {
+    if (classInfo->isSubClassOf(JSCallbackObject<JSAPIWrapperObject>::info())) {
         jsCast<JSCallbackObject<JSAPIWrapperObject>*>(jsObject)->setPrivate(data);
         return true;
     }
index b908ab2..7193754 100644 (file)
@@ -1,3 +1,108 @@
+2017-01-16  Filip Pizlo  <fpizlo@apple.com>
+
+        JSCell::classInfo() shouldn't have a bunch of mitigations for being called during destruction
+        https://bugs.webkit.org/show_bug.cgi?id=167066
+
+        Reviewed by Keith Miller and Michael Saboff.
+        
+        This reduces the size of JSCell::classInfo() by half and removes some checks that
+        this function previously had to do in case it was called from destructors.
+        
+        I changed all of the destructors so that they don't call JSCell::classInfo() and I
+        added an assertion to JSCell::classInfo() to catch cases where someone called it
+        from a destructor accidentally.
+        
+        This means that we only have one place in destruction that needs to know the class:
+        the sweeper's call to the destructor.
+        
+        One of the trickiest outcomes of this is the need to support inherits() tests in
+        JSObjectGetPrivate(), when it is called from the destructor callback on the object
+        being destructed. JSObjectGetPrivate() is undefined behavior anyway if you use it
+        on any dead-but-not-destructed object other than the one being destructed right
+        now. The purpose of the inherits() tests is to distinguish between different kinds
+        of CallbackObjects, which may have different kinds of base classes. I think that
+        this was always subtly wrong - for example, if the object being destructed is a
+        JSGlobalObject then it's not a DestructibleObject, is not in a destructor block,
+        but does not have an immortal Structure - so classInfo() is not valid. This fixes
+        the issue by having ~JSCallbackObject know its classInfo. It now stashes its
+        classInfo in VM so that JSObjectGetPrivate can use that classInfo if it detects
+        that it's being used on a currently-destructing object.
+        
+        That was the only really weird part of this patch. The rest is mostly removing
+        illegal uses of jsCast<> in destructors. There were a few other genuine uses of
+        classInfo() but they were in code that already knew how to get its classInfo()
+        using other means:
+        
+        - You can still say structure()->classInfo(), and I use this form in code that
+          knows that its StructureIsImmortal.
+        
+        - You can use this->classInfo() if it's overridden, like in subclasses of
+          JSDestructibleObject.
+
+        * API/JSAPIWrapperObject.mm:
+        (JSAPIWrapperObjectHandleOwner::finalize):
+        * API/JSCallbackObject.h:
+        * API/JSCallbackObjectFunctions.h:
+        (JSC::JSCallbackObject<Parent>::~JSCallbackObject):
+        (JSC::JSCallbackObject<Parent>::init):
+        * API/JSObjectRef.cpp:
+        (classInfoPrivate):
+        (JSObjectGetPrivate):
+        (JSObjectSetPrivate):
+        * bytecode/EvalCodeBlock.cpp:
+        (JSC::EvalCodeBlock::destroy):
+        * bytecode/FunctionCodeBlock.cpp:
+        (JSC::FunctionCodeBlock::destroy):
+        * bytecode/ModuleProgramCodeBlock.cpp:
+        (JSC::ModuleProgramCodeBlock::destroy):
+        * bytecode/ProgramCodeBlock.cpp:
+        (JSC::ProgramCodeBlock::destroy):
+        * bytecode/UnlinkedEvalCodeBlock.cpp:
+        (JSC::UnlinkedEvalCodeBlock::destroy):
+        * bytecode/UnlinkedFunctionCodeBlock.cpp:
+        (JSC::UnlinkedFunctionCodeBlock::destroy):
+        * bytecode/UnlinkedFunctionExecutable.cpp:
+        (JSC::UnlinkedFunctionExecutable::destroy):
+        * bytecode/UnlinkedModuleProgramCodeBlock.cpp:
+        (JSC::UnlinkedModuleProgramCodeBlock::destroy):
+        * bytecode/UnlinkedProgramCodeBlock.cpp:
+        (JSC::UnlinkedProgramCodeBlock::destroy):
+        * heap/CodeBlockSet.cpp:
+        (JSC::CodeBlockSet::lastChanceToFinalize):
+        (JSC::CodeBlockSet::deleteUnmarkedAndUnreferenced):
+        * heap/MarkedAllocator.cpp:
+        (JSC::MarkedAllocator::allocateSlowCaseImpl):
+        * heap/MarkedBlock.cpp:
+        (JSC::MarkedBlock::Handle::sweep):
+        * jit/JITThunks.cpp:
+        (JSC::JITThunks::finalize):
+        * runtime/AbstractModuleRecord.cpp:
+        (JSC::AbstractModuleRecord::destroy):
+        * runtime/ExecutableBase.cpp:
+        (JSC::ExecutableBase::clearCode):
+        * runtime/JSCellInlines.h:
+        (JSC::JSCell::classInfo):
+        (JSC::JSCell::callDestructor):
+        * runtime/JSLock.h:
+        (JSC::JSLock::ownerThread):
+        * runtime/JSModuleNamespaceObject.cpp:
+        (JSC::JSModuleNamespaceObject::destroy):
+        * runtime/JSModuleRecord.cpp:
+        (JSC::JSModuleRecord::destroy):
+        * runtime/JSPropertyNameEnumerator.cpp:
+        (JSC::JSPropertyNameEnumerator::destroy):
+        * runtime/JSSegmentedVariableObject.h:
+        * runtime/SymbolTable.cpp:
+        (JSC::SymbolTable::destroy):
+        * runtime/VM.h:
+        * wasm/js/JSWebAssemblyCallee.cpp:
+        (JSC::JSWebAssemblyCallee::destroy):
+        * wasm/js/WebAssemblyModuleRecord.cpp:
+        (JSC::WebAssemblyModuleRecord::destroy):
+        * wasm/js/WebAssemblyToJSCallee.cpp:
+        (JSC::WebAssemblyToJSCallee::WebAssemblyToJSCallee):
+        (JSC::WebAssemblyToJSCallee::destroy):
+
 2017-01-16  Joseph Pecoraro  <pecoraro@apple.com>
 
         Remove the REQUEST_ANIMATION_FRAME flag
index c2cccbc..5232a0e 100644 (file)
@@ -39,7 +39,7 @@ const ClassInfo EvalCodeBlock::s_info = {
 
 void EvalCodeBlock::destroy(JSCell* cell)
 {
-    jsCast<EvalCodeBlock*>(cell)->~EvalCodeBlock();
+    static_cast<EvalCodeBlock*>(cell)->~EvalCodeBlock();
 }
 
 } // namespace JSC
index 609674a..56eadc6 100644 (file)
@@ -39,7 +39,7 @@ const ClassInfo FunctionCodeBlock::s_info = {
 
 void FunctionCodeBlock::destroy(JSCell* cell)
 {
-    jsCast<FunctionCodeBlock*>(cell)->~FunctionCodeBlock();
+    static_cast<FunctionCodeBlock*>(cell)->~FunctionCodeBlock();
 }
 
 } // namespace JSC
index 6073366..3d54c3a 100644 (file)
@@ -39,7 +39,7 @@ const ClassInfo ModuleProgramCodeBlock::s_info = {
 
 void ModuleProgramCodeBlock::destroy(JSCell* cell)
 {
-    jsCast<ModuleProgramCodeBlock*>(cell)->~ModuleProgramCodeBlock();
+    static_cast<ModuleProgramCodeBlock*>(cell)->~ModuleProgramCodeBlock();
 }
 
 } // namespace JSC
index bee5105..b4fac57 100644 (file)
@@ -39,7 +39,7 @@ const ClassInfo ProgramCodeBlock::s_info = {
 
 void ProgramCodeBlock::destroy(JSCell* cell)
 {
-    jsCast<ProgramCodeBlock*>(cell)->~ProgramCodeBlock();
+    static_cast<ProgramCodeBlock*>(cell)->~ProgramCodeBlock();
 }
 
 } // namespace JSC
index 75c43fb..07f9916 100644 (file)
@@ -34,7 +34,7 @@ const ClassInfo UnlinkedEvalCodeBlock::s_info = { "UnlinkedEvalCodeBlock", &Base
 
 void UnlinkedEvalCodeBlock::destroy(JSCell* cell)
 {
-    jsCast<UnlinkedEvalCodeBlock*>(cell)->~UnlinkedEvalCodeBlock();
+    static_cast<UnlinkedEvalCodeBlock*>(cell)->~UnlinkedEvalCodeBlock();
 }
 
 }
index 64d14cd..151d560 100644 (file)
@@ -34,7 +34,7 @@ const ClassInfo UnlinkedFunctionCodeBlock::s_info = { "UnlinkedFunctionCodeBlock
 
 void UnlinkedFunctionCodeBlock::destroy(JSCell* cell)
 {
-    jsCast<UnlinkedFunctionCodeBlock*>(cell)->~UnlinkedFunctionCodeBlock();
+    static_cast<UnlinkedFunctionCodeBlock*>(cell)->~UnlinkedFunctionCodeBlock();
 }
 
 }
index c11ab95..066bc67 100644 (file)
@@ -119,7 +119,7 @@ UnlinkedFunctionExecutable::UnlinkedFunctionExecutable(VM* vm, Structure* struct
 
 void UnlinkedFunctionExecutable::destroy(JSCell* cell)
 {
-    jsCast<UnlinkedFunctionExecutable*>(cell)->~UnlinkedFunctionExecutable();
+    static_cast<UnlinkedFunctionExecutable*>(cell)->~UnlinkedFunctionExecutable();
 }
 
 void UnlinkedFunctionExecutable::visitChildren(JSCell* cell, SlotVisitor& visitor)
index 789887e..00f36c0 100644 (file)
@@ -42,7 +42,7 @@ void UnlinkedModuleProgramCodeBlock::visitChildren(JSCell* cell, SlotVisitor& vi
 
 void UnlinkedModuleProgramCodeBlock::destroy(JSCell* cell)
 {
-    jsCast<UnlinkedModuleProgramCodeBlock*>(cell)->~UnlinkedModuleProgramCodeBlock();
+    static_cast<UnlinkedModuleProgramCodeBlock*>(cell)->~UnlinkedModuleProgramCodeBlock();
 }
 
 }
index 69346b3..95df299 100644 (file)
@@ -42,7 +42,7 @@ void UnlinkedProgramCodeBlock::visitChildren(JSCell* cell, SlotVisitor& visitor)
 
 void UnlinkedProgramCodeBlock::destroy(JSCell* cell)
 {
-    jsCast<UnlinkedProgramCodeBlock*>(cell)->~UnlinkedProgramCodeBlock();
+    static_cast<UnlinkedProgramCodeBlock*>(cell)->~UnlinkedProgramCodeBlock();
 }
 
 }
index b0b928f..8819a45 100644 (file)
@@ -65,10 +65,10 @@ void CodeBlockSet::lastChanceToFinalize()
 {
     LockHolder locker(&m_lock);
     for (CodeBlock* codeBlock : m_newCodeBlocks)
-        codeBlock->classInfo()->methodTable.destroy(codeBlock);
+        codeBlock->structure()->classInfo()->methodTable.destroy(codeBlock);
 
     for (CodeBlock* codeBlock : m_oldCodeBlocks)
-        codeBlock->classInfo()->methodTable.destroy(codeBlock);
+        codeBlock->structure()->classInfo()->methodTable.destroy(codeBlock);
 }
 
 void CodeBlockSet::deleteUnmarkedAndUnreferenced(CollectionScope scope)
@@ -83,7 +83,7 @@ void CodeBlockSet::deleteUnmarkedAndUnreferenced(CollectionScope scope)
             unmarked.append(codeBlock);
         }
         for (CodeBlock* codeBlock : unmarked) {
-            codeBlock->classInfo()->methodTable.destroy(codeBlock);
+            codeBlock->structure()->classInfo()->methodTable.destroy(codeBlock);
             set.remove(codeBlock);
         }
         unmarked.resize(0);
index 0280cd7..101760b 100644 (file)
@@ -211,7 +211,7 @@ void* MarkedAllocator::allocateSlowCaseImpl(GCDeferralContext* deferralContext,
     
     didConsumeFreeList();
     
-    AllocatingScope healpingHeap(*m_heap);
+    AllocatingScope helpingHeap(*m_heap);
 
     m_heap->collectIfNecessaryOrDefer(deferralContext);
     
index 87b763f..262d501 100644 (file)
@@ -26,6 +26,7 @@
 #include "config.h"
 #include "MarkedBlock.h"
 
+#include "HelpingGCScope.h"
 #include "JSCell.h"
 #include "JSDestructibleObject.h"
 #include "JSCInlines.h"
@@ -195,6 +196,9 @@ FreeList MarkedBlock::Handle::specializedSweep()
 
 FreeList MarkedBlock::Handle::sweep(SweepMode sweepMode)
 {
+    // FIXME: Maybe HelpingGCScope should just be called SweepScope?
+    HelpingGCScope helpingGCScope(*heap());
+    
     m_allocator->setIsUnswept(NoLockingNecessary, this, false);
     
     m_weakSet.sweep();
index 40c12ce..60c3cc7 100644 (file)
@@ -84,7 +84,7 @@ MacroAssemblerCodeRef JITThunks::ctiStub(VM* vm, ThunkGenerator generator)
 
 void JITThunks::finalize(Handle<Unknown> handle, void*)
 {
-    auto* nativeExecutable = jsCast<NativeExecutable*>(handle.get().asCell());
+    auto* nativeExecutable = static_cast<NativeExecutable*>(handle.get().asCell());
     weakRemove(*m_hostFunctionStubMap, std::make_tuple(nativeExecutable->function(), nativeExecutable->constructor(), nativeExecutable->name()), nativeExecutable);
 }
 
index a2caa45..015e3e3 100644 (file)
@@ -46,7 +46,7 @@ AbstractModuleRecord::AbstractModuleRecord(VM& vm, Structure* structure, const I
 
 void AbstractModuleRecord::destroy(JSCell* cell)
 {
-    AbstractModuleRecord* thisObject = jsCast<AbstractModuleRecord*>(cell);
+    AbstractModuleRecord* thisObject = static_cast<AbstractModuleRecord*>(cell);
     thisObject->AbstractModuleRecord::~AbstractModuleRecord();
 }
 
index d31fc48..73f1ac5 100644 (file)
@@ -60,36 +60,36 @@ void ExecutableBase::clearCode()
     m_numParametersForCall = NUM_PARAMETERS_NOT_COMPILED;
     m_numParametersForConstruct = NUM_PARAMETERS_NOT_COMPILED;
 
-    if (classInfo() == FunctionExecutable::info()) {
-        FunctionExecutable* executable = jsCast<FunctionExecutable*>(this);
+    if (structure()->classInfo() == FunctionExecutable::info()) {
+        FunctionExecutable* executable = static_cast<FunctionExecutable*>(this);
         executable->m_codeBlockForCall.clear();
         executable->m_codeBlockForConstruct.clear();
         return;
     }
 
-    if (classInfo() == EvalExecutable::info()) {
-        EvalExecutable* executable = jsCast<EvalExecutable*>(this);
+    if (structure()->classInfo() == EvalExecutable::info()) {
+        EvalExecutable* executable = static_cast<EvalExecutable*>(this);
         executable->m_evalCodeBlock.clear();
         executable->m_unlinkedEvalCodeBlock.clear();
         return;
     }
     
-    if (classInfo() == ProgramExecutable::info()) {
-        ProgramExecutable* executable = jsCast<ProgramExecutable*>(this);
+    if (structure()->classInfo() == ProgramExecutable::info()) {
+        ProgramExecutable* executable = static_cast<ProgramExecutable*>(this);
         executable->m_programCodeBlock.clear();
         executable->m_unlinkedProgramCodeBlock.clear();
         return;
     }
 
-    if (classInfo() == ModuleProgramExecutable::info()) {
-        ModuleProgramExecutable* executable = jsCast<ModuleProgramExecutable*>(this);
+    if (structure()->classInfo() == ModuleProgramExecutable::info()) {
+        ModuleProgramExecutable* executable = static_cast<ModuleProgramExecutable*>(this);
         executable->m_moduleProgramCodeBlock.clear();
         executable->m_unlinkedModuleProgramCodeBlock.clear();
         executable->m_moduleEnvironmentSymbolTable.clear();
         return;
     }
     
-    ASSERT(classInfo() == NativeExecutable::info());
+    ASSERT(structure()->classInfo() == NativeExecutable::info());
 }
 
 void ExecutableBase::dump(PrintStream& out) const
index 2ffe851..b7f181b 100644 (file)
@@ -267,17 +267,13 @@ inline bool JSCell::canUseFastGetOwnProperty(const Structure& structure)
 
 ALWAYS_INLINE const ClassInfo* JSCell::classInfo() const
 {
-    if (isLargeAllocation()) {
-        LargeAllocation& allocation = largeAllocation();
-        if (allocation.attributes().destruction == NeedsDestruction
-            && !(inlineTypeFlags() & StructureIsImmortal))
-            return static_cast<const JSDestructibleObject*>(this)->classInfo();
-        return structure(*allocation.vm())->classInfo();
-    }
-    MarkedBlock& block = markedBlock();
-    if (block.needsDestruction() && !(inlineTypeFlags() & StructureIsImmortal))
-        return static_cast<const JSDestructibleObject*>(this)->classInfo();
-    return structure(*block.vm())->classInfo();
+    VM* vm;
+    if (isLargeAllocation())
+        vm = largeAllocation().vm();
+    else
+        vm = markedBlock().vm();
+    ASSERT(vm->heap.mutatorState() == MutatorState::Running || vm->apiLock().ownerThread() != std::this_thread::get_id());
+    return structure(*vm)->classInfo();
 }
 
 inline bool JSCell::toBoolean(ExecState* exec) const
@@ -307,7 +303,7 @@ inline void JSCell::callDestructor(VM& vm)
         MethodTable::DestroyFunctionPtr destroy = classInfo->methodTable.destroy;
         destroy(this);
     } else
-        jsCast<JSDestructibleObject*>(this)->classInfo()->methodTable.destroy(this);
+        static_cast<JSDestructibleObject*>(this)->classInfo()->methodTable.destroy(this);
     zap();
 }
 
index 75ee783..31171b4 100644 (file)
@@ -99,6 +99,7 @@ public:
         ASSERT(m_hasExclusiveThread);
         return m_ownerThreadID;
     }
+    std::thread::id ownerThread() const { return m_ownerThreadID; }
     JS_EXPORT_PRIVATE void setExclusiveThread(std::thread::id);
     JS_EXPORT_PRIVATE bool currentThreadIsHoldingLock();
 
index d968c5c..b91ee0f 100644 (file)
@@ -83,7 +83,7 @@ void JSModuleNamespaceObject::finishCreation(ExecState* exec, JSGlobalObject* gl
 
 void JSModuleNamespaceObject::destroy(JSCell* cell)
 {
-    JSModuleNamespaceObject* thisObject = jsCast<JSModuleNamespaceObject*>(cell);
+    JSModuleNamespaceObject* thisObject = static_cast<JSModuleNamespaceObject*>(cell);
     thisObject->JSModuleNamespaceObject::~JSModuleNamespaceObject();
 }
 
index b786ff2..9f73523 100644 (file)
@@ -59,7 +59,7 @@ JSModuleRecord::JSModuleRecord(VM& vm, Structure* structure, const Identifier& m
 
 void JSModuleRecord::destroy(JSCell* cell)
 {
-    JSModuleRecord* thisObject = jsCast<JSModuleRecord*>(cell);
+    JSModuleRecord* thisObject = static_cast<JSModuleRecord*>(cell);
     thisObject->JSModuleRecord::~JSModuleRecord();
 }
 
index 67efdf7..dfd9651 100644 (file)
@@ -83,7 +83,7 @@ void JSPropertyNameEnumerator::finishCreation(VM& vm, uint32_t indexedLength, ui
 
 void JSPropertyNameEnumerator::destroy(JSCell* cell)
 {
-    jsCast<JSPropertyNameEnumerator*>(cell)->JSPropertyNameEnumerator::~JSPropertyNameEnumerator();
+    static_cast<JSPropertyNameEnumerator*>(cell)->JSPropertyNameEnumerator::~JSPropertyNameEnumerator();
 }
 
 void JSPropertyNameEnumerator::visitChildren(JSCell* cell, SlotVisitor& visitor)
index 60a7ac1..d5c3195 100644 (file)
@@ -47,6 +47,8 @@ class LLIntOffsetsExtractor;
 // JSSegmentedVariableObject has its own GC tracing functionality, since it knows the
 // exact dimensions of the variables array at all times.
 
+// Except for JSGlobalObject, subclasses of this don't call the destructor and leak memory.
+
 class JSSegmentedVariableObject : public JSSymbolTableObject {
     friend class JIT;
     friend class LLIntOffsetsExtractor;
index 8101944..55c6dd2 100644 (file)
@@ -49,7 +49,7 @@ SymbolTableEntry& SymbolTableEntry::copySlow(const SymbolTableEntry& other)
 
 void SymbolTable::destroy(JSCell* cell)
 {
-    SymbolTable* thisObject = jsCast<SymbolTable*>(cell);
+    SymbolTable* thisObject = static_cast<SymbolTable*>(cell);
     thisObject->SymbolTable::~SymbolTable();
 }
 
index 2a82fe0..b5f1499 100644 (file)
@@ -363,6 +363,9 @@ public:
     std::once_flag m_wasmSignatureInformationOnceFlag;
     std::unique_ptr<Wasm::SignatureInformation> m_wasmSignatureInformation;
 #endif
+    
+    JSCell* currentlyDestructingCallbackObject;
+    const ClassInfo* currentlyDestructingCallbackObjectClassInfo;
 
     AtomicStringTable* m_atomicStringTable;
     WTF::SymbolRegistry m_symbolRegistry;
index 4db9269..c27712a 100644 (file)
@@ -47,7 +47,7 @@ void JSWebAssemblyCallee::finishCreation(VM& vm, Wasm::Entrypoint&& entrypoint)
 
 void JSWebAssemblyCallee::destroy(JSCell* cell)
 {
-    JSWebAssemblyCallee* thisObject = jsCast<JSWebAssemblyCallee*>(cell);
+    JSWebAssemblyCallee* thisObject = static_cast<JSWebAssemblyCallee*>(cell);
     thisObject->JSWebAssemblyCallee::~JSWebAssemblyCallee();
 }
 
index 9eff4c4..b695f41 100644 (file)
@@ -64,7 +64,7 @@ WebAssemblyModuleRecord::WebAssemblyModuleRecord(VM& vm, Structure* structure, c
 
 void WebAssemblyModuleRecord::destroy(JSCell* cell)
 {
-    WebAssemblyModuleRecord* thisObject = jsCast<WebAssemblyModuleRecord*>(cell);
+    WebAssemblyModuleRecord* thisObject = static_cast<WebAssemblyModuleRecord*>(cell);
     thisObject->WebAssemblyModuleRecord::~WebAssemblyModuleRecord();
 }
 
index 24ea801..4e891a0 100644 (file)
@@ -48,7 +48,8 @@ Structure* WebAssemblyToJSCallee::createStructure(VM& vm, JSGlobalObject* global
 
 WebAssemblyToJSCallee::WebAssemblyToJSCallee(VM& vm, Structure* structure)
     : Base(vm, structure)
-{ }
+{
+}
 
 void WebAssemblyToJSCallee::finishCreation(VM& vm)
 {
@@ -57,7 +58,7 @@ void WebAssemblyToJSCallee::finishCreation(VM& vm)
 
 void WebAssemblyToJSCallee::destroy(JSCell* cell)
 {
-    WebAssemblyToJSCallee* thisObject = jsCast<WebAssemblyToJSCallee*>(cell);
+    WebAssemblyToJSCallee* thisObject = static_cast<WebAssemblyToJSCallee*>(cell);
     thisObject->WebAssemblyToJSCallee::~WebAssemblyToJSCallee();
 }
 
index e3cc91a..fc17f32 100644 (file)
@@ -1,3 +1,22 @@
+2017-01-16  Filip Pizlo  <fpizlo@apple.com>
+
+        JSCell::classInfo() shouldn't have a bunch of mitigations for being called during destruction
+        https://bugs.webkit.org/show_bug.cgi?id=167066
+
+        Reviewed by Keith Miller and Michael Saboff.
+
+        No new tests because no new behavior.
+        
+        It's now necessary to avoid jsCast in destructors and finalizers. This was an easy
+        rule to introduce because this used to always be the rule.
+
+        * bindings/js/JSCSSValueCustom.cpp:
+        (WebCore::JSDeprecatedCSSOMValueOwner::finalize):
+        * bindings/js/JSDOMIterator.h:
+        (WebCore::IteratorTraits>::destroy):
+        * bindings/scripts/CodeGeneratorJS.pm:
+        (GenerateImplementation):
+
 2017-01-17  Miguel Gomez  <magomez@igalia.com>
 
         REGRESSION(r208997): [GLX] Google maps labels broken when using glXCreateContextAttribsARB
index 01e3374..13e2370 100644 (file)
@@ -50,7 +50,7 @@ bool JSDeprecatedCSSOMValueOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Un
 
 void JSDeprecatedCSSOMValueOwner::finalize(JSC::Handle<JSC::Unknown> handle, void* context)
 {
-    JSDeprecatedCSSOMValue* jsCSSValue = jsCast<JSDeprecatedCSSOMValue*>(handle.slot()->asCell());
+    JSDeprecatedCSSOMValue* jsCSSValue = static_cast<JSDeprecatedCSSOMValue*>(handle.slot()->asCell());
     DOMWrapperWorld& world = *static_cast<DOMWrapperWorld*>(context);
     world.m_deprecatedCSSOMValueRoots.remove(&jsCSSValue->wrapped());
     uncacheWrapper(world, &jsCSSValue->wrapped(), jsCSSValue);
index 3c5fe0e..7c1d5ce 100644 (file)
@@ -225,7 +225,7 @@ template<typename JSIterator> JSC::JSValue iteratorForEach(JSC::ExecState& state
 template<typename JSWrapper, typename IteratorTraits>
 void JSDOMIterator<JSWrapper, IteratorTraits>::destroy(JSCell* cell)
 {
-    JSDOMIterator<JSWrapper, IteratorTraits>* thisObject = JSC::jsCast<JSDOMIterator<JSWrapper, IteratorTraits>*>(cell);
+    JSDOMIterator<JSWrapper, IteratorTraits>* thisObject = static_cast<JSDOMIterator<JSWrapper, IteratorTraits>*>(cell);
     thisObject->JSDOMIterator<JSWrapper, IteratorTraits>::~JSDOMIterator();
 }
 
index b05e4fd..71b26d4 100644 (file)
@@ -4243,7 +4243,7 @@ END
     if (ShouldGenerateWrapperOwnerCode($hasParent, $interface) && !$interface->extendedAttributes->{JSCustomFinalize}) {
         push(@implContent, "void JS${interfaceName}Owner::finalize(JSC::Handle<JSC::Unknown> handle, void* context)\n");
         push(@implContent, "{\n");
-        push(@implContent, "    auto* js${interfaceName} = jsCast<JS${interfaceName}*>(handle.slot()->asCell());\n");
+        push(@implContent, "    auto* js${interfaceName} = static_cast<JS${interfaceName}*>(handle.slot()->asCell());\n");
         push(@implContent, "    auto& world = *static_cast<DOMWrapperWorld*>(context);\n");
         push(@implContent, "    uncacheWrapper(world, &js${interfaceName}->wrapped(), js${interfaceName});\n");
         push(@implContent, "}\n\n");