Attachment 358634 Details for Bug 193246 – [PATCH] Proposed Fix

[patch] [PATCH] Proposed Fix

debugger-1.patch (text/plain), 9.64 KB, created by Joseph Pecoraro on 2019-01-08 13:52:53 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Joseph Pecoraro

Created: 2019-01-08 13:52:53 PST

Size: 9.64 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 3a64ed79d70..59c8f89a0b3 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2019-01-08  Joseph Pecoraro  <pecoraro@apple.com>
>+
>+        ASSERT when paused in debugger and console evaluation causes exception
>+        https://bugs.webkit.org/show_bug.cgi?id=193246
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * inspector/debugger/evaluateOnCallFrame-CommandLineAPI.html:
>+        Correct a typo.
>+
>+        * inspector/debugger/evaluateOnCallFrame-exception-expected.txt: Added.
>+        * inspector/debugger/evaluateOnCallFrame-exception.html: Added.
>+        New test that would have asserted before.
>+
> 2019-01-07  Dean Jackson  <dino@apple.com>
> 
>         Turn on Pointer Events by default for iOS
>diff --git a/LayoutTests/inspector/debugger/evaluateOnCallFrame-CommandLineAPI.html b/LayoutTests/inspector/debugger/evaluateOnCallFrame-CommandLineAPI.html
>index dbc19e98da5..c80eb018e6c 100644
>--- a/LayoutTests/inspector/debugger/evaluateOnCallFrame-CommandLineAPI.html
>+++ b/LayoutTests/inspector/debugger/evaluateOnCallFrame-CommandLineAPI.html
>@@ -76,7 +76,7 @@ function test()
> 
>     suite.addTestCase({
>         name: "ValidateCallFrames",
>-        description: "Test6 evaluate can access CommandLineAPI methods.",
>+        description: "Test evaluate can access CommandLineAPI methods.",
>         test(resolve, reject) {
>             let targetData = WI.debuggerManager.dataForTarget(WI.debuggerManager.activeCallFrame.target);
>             InspectorTest.expectThat(WI.debuggerManager.activeCallFrame.functionName === "bar", "Strict call frame is `bar`.");
>diff --git a/LayoutTests/inspector/debugger/evaluateOnCallFrame-exception-expected.txt b/LayoutTests/inspector/debugger/evaluateOnCallFrame-exception-expected.txt
>new file mode 100644
>index 00000000000..5ac00f0a726
>--- /dev/null
>+++ b/LayoutTests/inspector/debugger/evaluateOnCallFrame-exception-expected.txt
>@@ -0,0 +1,9 @@
>+Tests exceptions with Debugger.evaluateOnCallFrame.
>+
>+
>+== Running test suite: Debugger.evaluateOnCallFrame.Exception
>+-- Running test case: Debugger.evaluateOnCallFrame.Exception
>+PASS: Should be a runtime error.
>+
>+-- Running test case: Complete
>+
>diff --git a/LayoutTests/inspector/debugger/evaluateOnCallFrame-exception.html b/LayoutTests/inspector/debugger/evaluateOnCallFrame-exception.html
>new file mode 100644
>index 00000000000..71d6ede13ba
>--- /dev/null
>+++ b/LayoutTests/inspector/debugger/evaluateOnCallFrame-exception.html
>@@ -0,0 +1,53 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<script src="../../http/tests/inspector/resources/inspector-test.js"></script>
>+<script>
>+function triggerPause() {
>+    debugger;
>+}
>+
>+function test()
>+{
>+    const objectGroup = "test";
>+    const includeCommandLineAPI = true;
>+    const returnByValue = true;
>+
>+    InspectorTest.debug();
>+    let suite = InspectorTest.createAsyncSuite("Debugger.evaluateOnCallFrame.Exception");
>+
>+    suite.addTestCase({
>+        name: "Debugger.evaluateOnCallFrame.Exception",
>+        description: "Test exception from evaluateOnCallFrame.",
>+        test(resolve, reject) {
>+            let callFrame = WI.debuggerManager.activeCallFrame;
>+            let targetData = WI.debuggerManager.dataForTarget(callFrame.target);
>+            let callFrameId = callFrame.id;
>+            let expression = "{}.x";
>+            DebuggerAgent.evaluateOnCallFrame.invoke({callFrameId, expression, objectGroup, includeCommandLineAPI, returnByValue}, (error, resultValue, wasThrown) => {
>+                InspectorTest.assert(!error, "Should not be a protocol error.");
>+                InspectorTest.expectTrue(wasThrown, "Should be a runtime error.");
>+                resolve();
>+            });
>+        }
>+    });
>+
>+    suite.addTestCase({
>+        name: "Complete",
>+        test(resolve, reject) {
>+            WI.debuggerManager.resume();
>+            resolve();
>+        }
>+    })
>+
>+    InspectorTest.evaluateInPage("triggerPause()");
>+    WI.debuggerManager.singleFireEventListener(WI.DebuggerManager.Event.Paused, (event) => {
>+        suite.runTestCasesAndFinish();
>+    });
>+}
>+</script>
>+</head>
>+<body onload="runTest()">
>+<p>Tests exceptions with Debugger.evaluateOnCallFrame.</p>
>+</body>
>+</html>
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index a9e3bd281f0..5429ad82c2b 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,26 @@
>+2019-01-08  Joseph Pecoraro  <pecoraro@apple.com>
>+
>+        ASSERT when paused in debugger and console evaluation causes exception
>+        https://bugs.webkit.org/show_bug.cgi?id=193246
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * runtime/VM.cpp:
>+        (JSC::VM::throwException):
>+        Improve assertion to allow for the debugger's evaluate on call frame condition.
>+
>+        * runtime/JSGlobalObject.h:
>+        (JSC::JSGlobalObject::evalDebuggerCallFrame const):
>+        (JSC::JSGlobalObject::setEvalDebuggerCallFrame):
>+        Debugger call frame only used by assertions.
>+
>+        * debugger/DebuggerCallFrame.cpp:
>+        (JSC::DebuggerCallFrame::evaluateWithScopeExtension):
>+        * debugger/DebuggerEvalEnabler.h:
>+        (JSC::DebuggerEvalEnabler::DebuggerEvalEnabler):
>+        (JSC::DebuggerEvalEnabler::~DebuggerEvalEnabler):
>+        When evaluating on a call frame, set a debug GlobalObject state.
>+
> 2019-01-04  Joseph Pecoraro  <pecoraro@apple.com>
> 
>         Web Inspector: Always buffer some console messages without requiring developerExtrasEnabled be enabled at log time
>diff --git a/Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp b/Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp
>index 334d542c6e6..34bb1dd1467 100644
>--- a/Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp
>+++ b/Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp
>@@ -232,7 +232,7 @@ JSValue DebuggerCallFrame::evaluateWithScopeExtension(const String& script, JSOb
>     if (!codeBlock)
>         return jsUndefined();
>     
>-    DebuggerEvalEnabler evalEnabler(callFrame);
>+    DebuggerEvalEnabler evalEnabler(callFrame, DebuggerEvalEnabler::Mode::EvalOnDebuggerCallFrame);
> 
>     EvalContextType evalContextType;
>     
>diff --git a/Source/JavaScriptCore/debugger/DebuggerEvalEnabler.h b/Source/JavaScriptCore/debugger/DebuggerEvalEnabler.h
>index 0e4c4cfac49..8c5124dbbd8 100644
>--- a/Source/JavaScriptCore/debugger/DebuggerEvalEnabler.h
>+++ b/Source/JavaScriptCore/debugger/DebuggerEvalEnabler.h
>@@ -32,29 +32,50 @@ namespace JSC {
> 
> class DebuggerEvalEnabler {
> public:
>-    explicit DebuggerEvalEnabler(const ExecState* exec)
>+    enum class Mode {
>+        EvalOnCurrentFrame,
>+        EvalOnDebuggerCallFrame,
>+    };
>+
>+    DebuggerEvalEnabler(const ExecState* exec, Mode mode = Mode::EvalOnCurrentFrame)
>         : m_exec(exec)
>-        , m_evalWasDisabled(false)
>+#if !ASSERT_DISABLED
>+        , m_mode(mode)
>+#endif
>+        
>     {
>+        UNUSED_PARAM(mode);
>         if (exec) {
>             JSGlobalObject* globalObject = exec->lexicalGlobalObject();
>             m_evalWasDisabled = !globalObject->evalEnabled();
>             if (m_evalWasDisabled)
>                 globalObject->setEvalEnabled(true, globalObject->evalDisabledErrorMessage());
>+#if !ASSERT_DISABLED
>+            if (m_mode == Mode::EvalOnDebuggerCallFrame)
>+                globalObject->setEvalDebuggerCallFrame(exec);
>+#endif
>         }
>     }
> 
>     ~DebuggerEvalEnabler()
>     {
>-        if (m_evalWasDisabled) {
>+        if (m_exec) {
>             JSGlobalObject* globalObject = m_exec->lexicalGlobalObject();
>-            globalObject->setEvalEnabled(false, globalObject->evalDisabledErrorMessage());
>+            if (m_evalWasDisabled)
>+                globalObject->setEvalEnabled(false, globalObject->evalDisabledErrorMessage());
>+#if !ASSERT_DISABLED
>+            if (m_mode == Mode::EvalOnDebuggerCallFrame)
>+                globalObject->setEvalDebuggerCallFrame(nullptr);
>+#endif
>         }
>     }
> 
> private:
>     const ExecState* m_exec;
>-    bool m_evalWasDisabled;
>+    bool m_evalWasDisabled { false };
>+#if !ASSERT_DISABLED
>+    DebuggerEvalEnabler::Mode m_mode;
>+#endif
> };
> 
> } // namespace JSC
>diff --git a/Source/JavaScriptCore/runtime/JSGlobalObject.h b/Source/JavaScriptCore/runtime/JSGlobalObject.h
>index 357ca8bf62f..e5f87de468c 100644
>--- a/Source/JavaScriptCore/runtime/JSGlobalObject.h
>+++ b/Source/JavaScriptCore/runtime/JSGlobalObject.h
>@@ -490,6 +490,10 @@ public:
>     RuntimeFlags m_runtimeFlags;
>     ConsoleClient* m_consoleClient { nullptr };
> 
>+#if !ASSERT_DISABLED
>+    const ExecState* m_evalDebuggerCallFrame { nullptr };
>+#endif
>+
>     static JS_EXPORT_PRIVATE const GlobalObjectMethodTable s_globalObjectMethodTable;
>     const GlobalObjectMethodTable* m_globalObjectMethodTable;
> 
>@@ -895,6 +899,11 @@ public:
>         m_webAssemblyDisabledErrorMessage = errorMessage;
>     }
> 
>+#if !ASSERT_DISABLED
>+    const ExecState* evalDebuggerCallFrame() const { return m_evalDebuggerCallFrame; }
>+    void setEvalDebuggerCallFrame(const ExecState* callFrame) { m_evalDebuggerCallFrame = callFrame; }
>+#endif
>+
>     void resetPrototype(VM&, JSValue prototype);
> 
>     VM& vm() const { return m_vm; }
>diff --git a/Source/JavaScriptCore/runtime/VM.cpp b/Source/JavaScriptCore/runtime/VM.cpp
>index 190e57187d5..722c212d57d 100644
>--- a/Source/JavaScriptCore/runtime/VM.cpp
>+++ b/Source/JavaScriptCore/runtime/VM.cpp
>@@ -830,7 +830,7 @@ void VM::clearSourceProviderCaches()
> 
> void VM::throwException(ExecState* exec, Exception* exception)
> {
>-    ASSERT(exec == topCallFrame || exec->isGlobalExec());
>+    ASSERT(exec == topCallFrame || exec->isGlobalExec() || exec == exec->lexicalGlobalObject()->evalDebuggerCallFrame());
>     CallFrame* throwOriginFrame = exec->isGlobalExec() ? exec : topJSCallFrame();
> 
>     if (Options::breakOnThrow()) {

Flags:

mark.lam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193246: 358634 | 358636