Fixes a crash in JavaScriptDebugServer::returnEvent when debugging
authortimothy@apple.com <timothy@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 15 May 2008 01:59:19 +0000 (01:59 +0000)
committertimothy@apple.com <timothy@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 15 May 2008 01:59:19 +0000 (01:59 +0000)
code that contains an eval. This change makes stepping into eval
code work as expected.

http://bugs.webkit.org/show_bug.cgi?id=19038

Reviewed by Kevin McCullough.

Tested with: manual-tests/inspector/returnEvent-crash.html

* manual-tests/inspector/returnEvent-crash.html: Added.
* page/JavaScriptDebugServer.cpp:
(WebCore::JavaScriptDebugServer::sourceParsed): Adds #ifdefed
debugging code to prevent the sourceID and URL.
(WebCore::updateCurrentCallFrame): Added. A helper function that
is called from all 4 of the debugger hooks below. This function will
update and/or create JavaScriptCallFrames to match the exec state,
sourceID and lineNumber passed into it. Contains #ifdefed debugging
code that was helpful while fixing this bug.
(WebCore::JavaScriptDebugServer::callEvent): Call updateCurrentCallFrame
before pauseIfNeeded.
(WebCore::JavaScriptDebugServer::atStatement): Ditto.
(WebCore::JavaScriptDebugServer::returnEvent): Ditto.
(WebCore::JavaScriptDebugServer::exception): Ditto.

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

WebCore/ChangeLog
WebCore/manual-tests/inspector/returnEvent-crash.html [new file with mode: 0644]
WebCore/page/JavaScriptDebugServer.cpp

index df2e786..e5eb2a5 100644 (file)
@@ -1,3 +1,30 @@
+2008-05-14  Timothy Hatcher  <timothy@apple.com>
+
+        Fixes a crash in JavaScriptDebugServer::returnEvent when debugging
+        code that contains an eval. This change makes stepping into eval
+        code work as expected.
+
+        http://bugs.webkit.org/show_bug.cgi?id=19038
+
+        Reviewed by Kevin McCullough.
+
+        Tested with: manual-tests/inspector/returnEvent-crash.html
+
+        * manual-tests/inspector/returnEvent-crash.html: Added.
+        * page/JavaScriptDebugServer.cpp:
+        (WebCore::JavaScriptDebugServer::sourceParsed): Adds #ifdefed
+        debugging code to prevent the sourceID and URL.
+        (WebCore::updateCurrentCallFrame): Added. A helper function that
+        is called from all 4 of the debugger hooks below. This function will
+        update and/or create JavaScriptCallFrames to match the exec state,
+        sourceID and lineNumber passed into it. Contains #ifdefed debugging
+        code that was helpful while fixing this bug.
+        (WebCore::JavaScriptDebugServer::callEvent): Call updateCurrentCallFrame
+        before pauseIfNeeded.
+        (WebCore::JavaScriptDebugServer::atStatement): Ditto.
+        (WebCore::JavaScriptDebugServer::returnEvent): Ditto.
+        (WebCore::JavaScriptDebugServer::exception): Ditto.
+
 2008-05-14  Alp Toker  <alp@nuanti.com>
 
         GTK+ build fix for r33457. Add NetworkStateNotifier.cpp to the build.
 2008-05-14  Alp Toker  <alp@nuanti.com>
 
         GTK+ build fix for r33457. Add NetworkStateNotifier.cpp to the build.
diff --git a/WebCore/manual-tests/inspector/returnEvent-crash.html b/WebCore/manual-tests/inspector/returnEvent-crash.html
new file mode 100644 (file)
index 0000000..5dd1119
--- /dev/null
@@ -0,0 +1,17 @@
+<script>
+    function test2() {
+        var y = 6;
+        y += 3;
+        return y;
+    }
+
+    function test() {
+        var x = 5;
+        eval("test2()");
+        x += 6;
+        return x;
+    }
+</script>
+<p>Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=19038">Bug 19038: Crash in JavaScriptDebugServer::returnEvent when inspecting an attached Inspector</a>.</p>
+<p>To test, open and attach the Inspector's debugger, then click the button below. If you don't crash, you have passed the test.</p>
+<button onclick="test()">click me</button>
index b158bf5..ffa5672 100644 (file)
@@ -269,6 +269,10 @@ static Page* toPage(ExecState* exec)
     return window->impl()->frame()->page();
 }
 
     return window->impl()->frame()->page();
 }
 
+#ifdef DEBUG_DEBUGGER_CALLBACKS
+static unsigned s_callDepth = 0;
+#endif
+
 bool JavaScriptDebugServer::sourceParsed(ExecState* exec, int sourceID, const UString& sourceURL, const UString& source, int startingLineNumber, int errorLine, const UString& errorMessage)
 {
     if (m_callingListeners)
 bool JavaScriptDebugServer::sourceParsed(ExecState* exec, int sourceID, const UString& sourceURL, const UString& source, int startingLineNumber, int errorLine, const UString& errorMessage)
 {
     if (m_callingListeners)
@@ -284,6 +288,13 @@ bool JavaScriptDebugServer::sourceParsed(ExecState* exec, int sourceID, const US
 
     bool isError = errorLine != -1;
 
 
     bool isError = errorLine != -1;
 
+#ifdef DEBUG_DEBUGGER_CALLBACKS
+    printf("source: ");
+    for(unsigned i = 0; i < s_callDepth; ++i)
+        printf(" ");
+    printf("%d: '%s' exec: %p (caller: %p) source: %d\n", s_callDepth, sourceURL.ascii(), exec, exec->callingExecState(), sourceID);
+#endif
+
     if (!m_listeners.isEmpty()) {
         if (isError)
             dispatchFailedToParseSource(m_listeners, source, startingLineNumber, sourceURL, errorLine, errorMessage);
     if (!m_listeners.isEmpty()) {
         if (isError)
             dispatchFailedToParseSource(m_listeners, source, startingLineNumber, sourceURL, errorLine, errorMessage);
@@ -426,17 +437,82 @@ void JavaScriptDebugServer::pauseIfNeeded(ExecState* exec, int sourceID, int lin
     m_paused = false;
 }
 
     m_paused = false;
 }
 
-bool JavaScriptDebugServer::callEvent(ExecState* exec, int sourceID, int lineNumber, JSObject*, const List&)
+static inline void updateCurrentCallFrame(RefPtr<JavaScriptCallFrame>& currentCallFrame, ExecState* exec, int sourceID, int lineNumber, ExecState*& pauseExecState)
 {
 {
-    if (m_paused)
-        return true;
+#ifdef DEBUG_DEBUGGER_CALLBACKS
+    const char* action = 0;
+#endif
 
 
-    if (m_currentCallFrame && m_currentCallFrame->execState() != exec->callingExecState()) {
-        m_currentCallFrame->invalidate();
-        m_currentCallFrame = 0;
+    if (currentCallFrame) {
+        if (currentCallFrame->execState() == exec) {
+            // Same call frame, just update the current line.
+            currentCallFrame->setLine(lineNumber);
+#ifdef DEBUG_DEBUGGER_CALLBACKS
+            action = "  same";
+#endif
+        } else if (currentCallFrame->execState() == exec->callingExecState()) {
+            // Create a new call frame, and make the caller the previous call frame.
+            currentCallFrame = JavaScriptCallFrame::create(exec, currentCallFrame, sourceID, lineNumber);
+#ifdef DEBUG_DEBUGGER_CALLBACKS
+            action = "  call";
+            ++s_callDepth;
+#endif
+        } else {
+#ifdef DEBUG_DEBUGGER_CALLBACKS
+            action = "return";
+#endif
+            // The current call frame isn't the same and it isn't the caller of a new call frame,
+            // so it might be a previous call frame (returning from a function). Or it is a stale call
+            // frame from the previous execution of global code. Walk up the caller chain until we find
+            // the current exec state. If the current exec state is found, the current call frame will be
+            // set to null (and a new one will be created below.)
+            while (currentCallFrame && currentCallFrame->execState() != exec) {
+                if (currentCallFrame->execState() == pauseExecState) {
+                    // The current call frame matches the pause exec state (used for step over.)
+                    // Since we are returning up the call stack, update the pause exec state to match.
+                    // This makes stepping over a return statement act like a step out.
+                    if (currentCallFrame->caller())
+                        pauseExecState = currentCallFrame->caller()->execState();
+                    else
+                        pauseExecState = 0;
+                }
+
+                // Invalidate the call frame since it's ExecState is stale now.
+                currentCallFrame->invalidate();
+                currentCallFrame = currentCallFrame->caller();
+
+#ifdef DEBUG_DEBUGGER_CALLBACKS
+                if (s_callDepth)
+                    --s_callDepth;
+#endif
+            }
+
+            if (currentCallFrame)
+                currentCallFrame->setLine(lineNumber);
+        }
     }
 
     }
 
-    m_currentCallFrame = JavaScriptCallFrame::create(exec, m_currentCallFrame, sourceID, lineNumber);
+    if (!currentCallFrame) {
+        // Create a new call frame with no caller, this is likely global code.
+        currentCallFrame = JavaScriptCallFrame::create(exec, 0, sourceID, lineNumber);
+#ifdef DEBUG_DEBUGGER_CALLBACKS
+        action = "   new";
+#endif
+    }
+
+#ifdef DEBUG_DEBUGGER_CALLBACKS
+    printf("%s: ", action);
+    for(unsigned i = 0; i < s_callDepth; ++i)
+        printf(" ");
+    printf("%d: at exec: %p (caller: %p, pause: %p) source: %d line: %d\n", s_callDepth, exec, exec->callingExecState(), pauseExecState, sourceID, lineNumber);
+#endif
+}
+
+bool JavaScriptDebugServer::callEvent(ExecState* exec, int sourceID, int lineNumber, JSObject*, const List&)
+{
+    if (m_paused)
+        return true;
+    updateCurrentCallFrame(m_currentCallFrame, exec, sourceID, lineNumber, m_pauseOnExecState);
     pauseIfNeeded(exec, sourceID, lineNumber);
     return true;
 }
     pauseIfNeeded(exec, sourceID, lineNumber);
     return true;
 }
@@ -445,10 +521,7 @@ bool JavaScriptDebugServer::atStatement(ExecState* exec, int sourceID, int first
 {
     if (m_paused)
         return true;
 {
     if (m_paused)
         return true;
-    if (m_currentCallFrame)
-        m_currentCallFrame->setLine(firstLine);
-    else
-        m_currentCallFrame = JavaScriptCallFrame::create(exec, 0, sourceID, firstLine);
+    updateCurrentCallFrame(m_currentCallFrame, exec, sourceID, firstLine, m_pauseOnExecState);
     pauseIfNeeded(exec, sourceID, firstLine);
     return true;
 }
     pauseIfNeeded(exec, sourceID, firstLine);
     return true;
 }
@@ -457,9 +530,8 @@ bool JavaScriptDebugServer::returnEvent(ExecState* exec, int sourceID, int lineN
 {
     if (m_paused)
         return true;
 {
     if (m_paused)
         return true;
+    updateCurrentCallFrame(m_currentCallFrame, exec, sourceID, lineNumber, m_pauseOnExecState);
     pauseIfNeeded(exec, sourceID, lineNumber);
     pauseIfNeeded(exec, sourceID, lineNumber);
-    m_currentCallFrame->invalidate();
-    m_currentCallFrame = m_currentCallFrame->caller();
     return true;
 }
 
     return true;
 }
 
@@ -467,10 +539,7 @@ bool JavaScriptDebugServer::exception(ExecState* exec, int sourceID, int lineNum
 {
     if (m_paused)
         return true;
 {
     if (m_paused)
         return true;
-    if (m_currentCallFrame)
-        m_currentCallFrame->setLine(lineNumber);
-    else
-        m_currentCallFrame = JavaScriptCallFrame::create(exec, 0, sourceID, lineNumber);
+    updateCurrentCallFrame(m_currentCallFrame, exec, sourceID, lineNumber, m_pauseOnExecState);
     if (m_pauseOnExceptions)
         m_pauseOnNextStatement = true;
     pauseIfNeeded(exec, sourceID, lineNumber);
     if (m_pauseOnExceptions)
         m_pauseOnNextStatement = true;
     pauseIfNeeded(exec, sourceID, lineNumber);