Title: [248841] branches/safari-608-branch
Revision
248841
Author
bshaf...@apple.com
Date
2019-08-18 23:03:34 -0700 (Sun, 18 Aug 2019)

Log Message

Cherry-pick r248800. rdar://problem/54454996

    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):

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

Modified Paths

Added Paths

Diff

Modified: branches/safari-608-branch/JSTests/ChangeLog (248840 => 248841)


--- branches/safari-608-branch/JSTests/ChangeLog	2019-08-19 06:03:32 UTC (rev 248840)
+++ branches/safari-608-branch/JSTests/ChangeLog	2019-08-19 06:03:34 UTC (rev 248841)
@@ -1,3 +1,96 @@
+2019-08-18  Babak Shafiei  <bshaf...@apple.com>
+
+        Cherry-pick r248800. rdar://problem/54454996
+
+    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):
+    
+    
+    
+    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@248800 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+    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-13  Alan Coon  <alanc...@apple.com>
 
         Cherry-pick r248271. rdar://problem/54237771

Added: branches/safari-608-branch/JSTests/stress/codeblock-should-clear-watchpoints-on-destruction.js (0 => 248841)


--- branches/safari-608-branch/JSTests/stress/codeblock-should-clear-watchpoints-on-destruction.js	                        (rev 0)
+++ branches/safari-608-branch/JSTests/stress/codeblock-should-clear-watchpoints-on-destruction.js	2019-08-19 06:03:34 UTC (rev 248841)
@@ -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: branches/safari-608-branch/Source/_javascript_Core/ChangeLog (248840 => 248841)


--- branches/safari-608-branch/Source/_javascript_Core/ChangeLog	2019-08-19 06:03:32 UTC (rev 248840)
+++ branches/safari-608-branch/Source/_javascript_Core/ChangeLog	2019-08-19 06:03:34 UTC (rev 248841)
@@ -1,3 +1,157 @@
+2019-08-18  Babak Shafiei  <bshaf...@apple.com>
+
+        Cherry-pick r248800. rdar://problem/54454996
+
+    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):
+    
+    
+    
+    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@248800 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+    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-13  Alan Coon  <alanc...@apple.com>
 
         Cherry-pick r248271. rdar://problem/54237771

Modified: branches/safari-608-branch/Source/_javascript_Core/bytecode/CodeBlock.cpp (248840 => 248841)


--- branches/safari-608-branch/Source/_javascript_Core/bytecode/CodeBlock.cpp	2019-08-19 06:03:32 UTC (rev 248840)
+++ branches/safari-608-branch/Source/_javascript_Core/bytecode/CodeBlock.cpp	2019-08-19 06:03:34 UTC (rev 248841)
@@ -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

Reply via email to