Title: [248800] trunk
- Revision
- 248800
- Author
- mark....@apple.com
- Date
- 2019-08-16 15:49:26 -0700 (Fri, 16 Aug 2019)
Log Message
CodeBlock destructor should clear all of its watchpoints.
https://bugs.webkit.org/show_bug.cgi?id=200792
<rdar://problem/53947800>
Reviewed by Yusuke Suzuki.
JSTests:
* stress/codeblock-should-clear-watchpoints-on-destruction.js: Added.
Source/_javascript_Core:
We need to clear the watchpoints explicitly (just like we do in CodeBlock::jettison())
because the JITCode may outlive the CodeBlock for a while. For example, the JITCode
is ref'd in Interpreter::execute(JSC::CallFrameClosure&) like so:
JSValue result = closure.functionExecutable->generatedJITCodeForCall()->execute(&vm, closure.protoCallFrame);
The call to generatedJITCodeForCall() returns a Ref<JITCode> with the underlying
JITCode ref'd. Hence, while the interpreter frame is still on the stack, the
executing JITCode instance will have a non-zero refCount, and be kept alive even
though its CodeBlock may have already been destructed.
Note: the Interpreter execute() methods aren't the only ones who would ref the JITCode:
ExecutableBase also holds a RefPtr<JITCode> m_jitCodeForCall and RefPtr<JITCode>
m_jitCodeForConstruct. But a CodeBlock will be uninstalled before it gets destructed.
Hence, the uninstallation will deref the JITCode before we get to the CodeBlock
destructor. That said, we should be aware that a JITCode's refCount is not always
1 after the JIT installs it into the CodeBlock, and it should not be assumed to be so.
For this patch, I also audited all Watchpoint subclasses to ensure that we are
clearing all the relevant watchpoints in the CodeBlock destructor. Here is the
list of audited Watchpoints:
CodeBlockJettisoningWatchpoint
AdaptiveStructureWatchpoint
AdaptiveInferredPropertyValueWatchpoint
- these are held in the DFG::CommonData, and is tied to JITCode's life cycle.
- they need to be cleared eagerly in CodeBlock's destructor.
LLIntPrototypeLoadAdaptiveStructureWatchpoint
- stored in m_llintGetByIdWatchpointMap in the CodeBlock.
- this will be automatically cleared on CodeBlock destruction.
The following does not reference CodeBlock:
FunctionRareData::AllocationProfileClearingWatchpoint
- stored in FunctionRareData and will be cleared automatically on
FunctionRareData destruction.
- only references the owner FunctionRareData.
ObjectToStringAdaptiveStructureWatchpoint
ObjectToStringAdaptiveInferredPropertyValueWatchpoint
- stored in StructureRareData and will be cleared automatically on
StructureRareData destruction.
ObjectPropertyChangeAdaptiveWatchpoint
- stored in JSGlobalObject, and will be cleared automatically on
JSGlobalObject destruction.
- only references the owner JSGlobalObject.
StructureStubClearingWatchpoint
- stored in WatchpointsOnStructureStubInfo and will be cleared automatically
on WatchpointsOnStructureStubInfo destruction.
PropertyWatchpoint
StructureWatchpoint
- embedded in AdaptiveInferredPropertyValueWatchpointBase, which is extended
as AdaptiveInferredPropertyValueWatchpoint, ObjectPropertyChangeAdaptiveWatchpoint,
and ObjectToStringAdaptiveInferredPropertyValueWatchpoint.
- life cycle is handled by those 3 subclasses.
* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::~CodeBlock):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (248799 => 248800)
--- trunk/JSTests/ChangeLog 2019-08-16 22:14:26 UTC (rev 248799)
+++ trunk/JSTests/ChangeLog 2019-08-16 22:49:26 UTC (rev 248800)
@@ -1,3 +1,13 @@
+2019-08-16 Mark Lam <mark....@apple.com>
+
+ CodeBlock destructor should clear all of its watchpoints.
+ https://bugs.webkit.org/show_bug.cgi?id=200792
+ <rdar://problem/53947800>
+
+ Reviewed by Yusuke Suzuki.
+
+ * stress/codeblock-should-clear-watchpoints-on-destruction.js: Added.
+
2019-08-16 Justin Michaud <justin_mich...@apple.com>
Fix InBounds speculation of typed array PutByVal and add extra step to integer range optimization to search for equality relationships on the RHS value
Added: trunk/JSTests/stress/codeblock-should-clear-watchpoints-on-destruction.js (0 => 248800)
--- trunk/JSTests/stress/codeblock-should-clear-watchpoints-on-destruction.js (rev 0)
+++ trunk/JSTests/stress/codeblock-should-clear-watchpoints-on-destruction.js 2019-08-16 22:49:26 UTC (rev 248800)
@@ -0,0 +1,49 @@
+//@ runFTLNoCJIT("--thresholdForFTLOptimizeAfterWarmUp=1000")
+
+// This test should not crash.
+
+let source;
+for (__v1 of 'gu') {
+ let __v3 = new RegExp(source, __v1);
+ let __v0 = 'Over many a quaint and curious volume of forgotten lore,'.replace(__v3, (...__v0) => {
+ try {
+ try {
+ try {
+ for (let __v0 = 27; __v0 < 1000; ++__v0) {}
+ } finally {
+ return __v4;
+ }
+ } finally {
+ gc();
+ }
+ } catch (__v3) {
+ try {
+ } finally {
+ ({}).__proto__[__v0] = __v3;
+ for (__v1 of 'gu') {
+ let __v3 = new RegExp(source, __v1);
+ let __v0 = 'Over many a quaint and curious volume of forgotten lore,'.replace(__v3, (...__v0) => {
+ try {
+ try {
+ } finally {(((((((((((((((((((((((((((((((((((((('blahblahblahblah' + __v0) + __v0) + __v0) + __v0) + __v0 instanceof __v0) + __v0) + __v0 === __v0) + __v0) + __v0) + __v0) + __v0) + __v0) + __v0 != __v0) + __v1) + __v0) + __v0) + __v0) + __v0) + __v0) + __v0) + __v0) + __v0) + __v0) + __v0) + __v0) + __v0) + __v0) + __v0) + __v0) + __v0) + __v0 === __v0) + __v5) + __v0) + __v0) + __v0) + __v0) + __v0) + __v0) + __v2 + __v0;
+ }
+ } catch (__v3) {
+ try {
+ eval('tag`Hello\n${v}world`');
+ } finally {
+ try {
+ } finally {
+ try {
+ eval('tag`Hello\n${v}world`');
+ } finally {
+ return;
+ }
+ }
+ }
+ }
+ });
+ }
+ }
+ }
+ });
+}
\ No newline at end of file
Modified: trunk/Source/_javascript_Core/ChangeLog (248799 => 248800)
--- trunk/Source/_javascript_Core/ChangeLog 2019-08-16 22:14:26 UTC (rev 248799)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-08-16 22:49:26 UTC (rev 248800)
@@ -1,3 +1,74 @@
+2019-08-16 Mark Lam <mark....@apple.com>
+
+ CodeBlock destructor should clear all of its watchpoints.
+ https://bugs.webkit.org/show_bug.cgi?id=200792
+ <rdar://problem/53947800>
+
+ Reviewed by Yusuke Suzuki.
+
+ We need to clear the watchpoints explicitly (just like we do in CodeBlock::jettison())
+ because the JITCode may outlive the CodeBlock for a while. For example, the JITCode
+ is ref'd in Interpreter::execute(JSC::CallFrameClosure&) like so:
+
+ JSValue result = closure.functionExecutable->generatedJITCodeForCall()->execute(&vm, closure.protoCallFrame);
+
+ The call to generatedJITCodeForCall() returns a Ref<JITCode> with the underlying
+ JITCode ref'd. Hence, while the interpreter frame is still on the stack, the
+ executing JITCode instance will have a non-zero refCount, and be kept alive even
+ though its CodeBlock may have already been destructed.
+
+ Note: the Interpreter execute() methods aren't the only ones who would ref the JITCode:
+ ExecutableBase also holds a RefPtr<JITCode> m_jitCodeForCall and RefPtr<JITCode>
+ m_jitCodeForConstruct. But a CodeBlock will be uninstalled before it gets destructed.
+ Hence, the uninstallation will deref the JITCode before we get to the CodeBlock
+ destructor. That said, we should be aware that a JITCode's refCount is not always
+ 1 after the JIT installs it into the CodeBlock, and it should not be assumed to be so.
+
+ For this patch, I also audited all Watchpoint subclasses to ensure that we are
+ clearing all the relevant watchpoints in the CodeBlock destructor. Here is the
+ list of audited Watchpoints:
+
+ CodeBlockJettisoningWatchpoint
+ AdaptiveStructureWatchpoint
+ AdaptiveInferredPropertyValueWatchpoint
+ - these are held in the DFG::CommonData, and is tied to JITCode's life cycle.
+ - they need to be cleared eagerly in CodeBlock's destructor.
+
+ LLIntPrototypeLoadAdaptiveStructureWatchpoint
+ - stored in m_llintGetByIdWatchpointMap in the CodeBlock.
+ - this will be automatically cleared on CodeBlock destruction.
+
+ The following does not reference CodeBlock:
+
+ FunctionRareData::AllocationProfileClearingWatchpoint
+ - stored in FunctionRareData and will be cleared automatically on
+ FunctionRareData destruction.
+ - only references the owner FunctionRareData.
+
+ ObjectToStringAdaptiveStructureWatchpoint
+ ObjectToStringAdaptiveInferredPropertyValueWatchpoint
+ - stored in StructureRareData and will be cleared automatically on
+ StructureRareData destruction.
+
+ ObjectPropertyChangeAdaptiveWatchpoint
+ - stored in JSGlobalObject, and will be cleared automatically on
+ JSGlobalObject destruction.
+ - only references the owner JSGlobalObject.
+
+ StructureStubClearingWatchpoint
+ - stored in WatchpointsOnStructureStubInfo and will be cleared automatically
+ on WatchpointsOnStructureStubInfo destruction.
+
+ PropertyWatchpoint
+ StructureWatchpoint
+ - embedded in AdaptiveInferredPropertyValueWatchpointBase, which is extended
+ as AdaptiveInferredPropertyValueWatchpoint, ObjectPropertyChangeAdaptiveWatchpoint,
+ and ObjectToStringAdaptiveInferredPropertyValueWatchpoint.
+ - life cycle is handled by those 3 subclasses.
+
+ * bytecode/CodeBlock.cpp:
+ (JSC::CodeBlock::~CodeBlock):
+
2019-08-16 Justin Michaud <justin_mich...@apple.com>
Fix InBounds speculation of typed array PutByVal and add extra step to integer range optimization to search for equality relationships on the RHS value
Modified: trunk/Source/_javascript_Core/bytecode/CodeBlock.cpp (248799 => 248800)
--- trunk/Source/_javascript_Core/bytecode/CodeBlock.cpp 2019-08-16 22:14:26 UTC (rev 248799)
+++ trunk/Source/_javascript_Core/bytecode/CodeBlock.cpp 2019-08-16 22:49:26 UTC (rev 248800)
@@ -811,6 +811,26 @@
{
VM& vm = *m_vm;
+#if ENABLE(DFG_JIT)
+ // The JITCode (and its corresponding DFG::CommonData) may outlive the CodeBlock by
+ // a short amount of time after the CodeBlock is destructed. For example, the
+ // Interpreter::execute methods will ref JITCode before invoking it. This can
+ // result in the JITCode having a non-zero refCount when its owner CodeBlock is
+ // destructed.
+ //
+ // Hence, we cannot rely on DFG::CommonData destruction to clear these now invalid
+ // watchpoints in a timely manner. We'll ensure they are cleared here eagerly.
+ //
+ // We only need to do this for a DFG/FTL CodeBlock because only these will have a
+ // DFG:CommonData. Hence, the LLInt and Baseline will not have any of these watchpoints.
+ //
+ // Note also that the LLIntPrototypeLoadAdaptiveStructureWatchpoint is also related
+ // to the CodeBlock. However, its lifecycle is tied directly to the CodeBlock, and
+ // will be automatically cleared when the CodeBlock destructs.
+
+ if (JITCode::isOptimizingJIT(jitType()))
+ jitCode()->dfgCommon()->clearWatchpoints();
+#endif
vm.heap.codeBlockSet().remove(this);
if (UNLIKELY(vm.m_perBytecodeProfiler))
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
