LayoutTests:
authorggaren <ggaren@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 25 Jan 2007 20:21:34 +0000 (20:21 +0000)
committerggaren <ggaren@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 25 Jan 2007 20:21:34 +0000 (20:21 +0000)
        Reviewed by Geoffrey Garen.

        Added tests for http://bugs.webkit.org/show_bug.cgi?id=12342
        REGRESSION: destroying a frame from its own script causes various crashes

        Thanks to Alexey Proskuryakov for the original versions of these tests.

        * fast/dom/exception-no-frame-inline-script-crash-expected.txt: Added.
        * fast/dom/exception-no-frame-inline-script-crash.html: Added.
        * fast/dom/exception-no-frame-timeout-crash-expected.txt: Added.
        * fast/dom/exception-no-frame-timeout-crash.html: Added.

WebCore:

        Reviewed and landed by Geoffrey Garen.

        - fix http://bugs.webkit.org/show_bug.cgi?id=12342
          REGRESSION: destroying a frame from its own script causes various crashes

        * bindings/js/kjs_window.cpp: (KJS::ScheduledAction::execute):
        Use RefPtr for the frame and the interpreter object so they don't get deleted
        out from underneath us.

        * bindings/js/kjs_proxy.cpp: (WebCore::KJSProxy::evaluate):
        Add a check for a page of 0. This was the only call site for the
        addMessageToConsole function that did not have a check for a page of 0.

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

LayoutTests/ChangeLog
LayoutTests/fast/dom/exception-no-frame-inline-script-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/exception-no-frame-inline-script-crash.html [new file with mode: 0644]
LayoutTests/fast/dom/exception-no-frame-timeout-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/exception-no-frame-timeout-crash.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/bindings/js/kjs_proxy.cpp
WebCore/bindings/js/kjs_window.cpp

index eeb3654..ea639bf 100644 (file)
@@ -1,3 +1,17 @@
+2007-01-25  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Geoffrey Garen.
+        
+        Added tests for http://bugs.webkit.org/show_bug.cgi?id=12342
+        REGRESSION: destroying a frame from its own script causes various crashes
+        
+        Thanks to Alexey Proskuryakov for the original versions of these tests.
+
+        * fast/dom/exception-no-frame-inline-script-crash-expected.txt: Added.
+        * fast/dom/exception-no-frame-inline-script-crash.html: Added.
+        * fast/dom/exception-no-frame-timeout-crash-expected.txt: Added.
+        * fast/dom/exception-no-frame-timeout-crash.html: Added.
+
 2007-01-25  Beth Dakin  <bdakin@apple.com>
 
         Test for <rdar://problem/4921692> not processing comments inside a 
diff --git a/LayoutTests/fast/dom/exception-no-frame-inline-script-crash-expected.txt b/LayoutTests/fast/dom/exception-no-frame-inline-script-crash-expected.txt
new file mode 100644 (file)
index 0000000..22ff8ef
--- /dev/null
@@ -0,0 +1,5 @@
+This test checks for a crash when throwing an exception under the following conditions: (1) The throwing script's document has no frame; (2) The script is inline in the document.
+
+PASS: You didn't crash.
+
+
diff --git a/LayoutTests/fast/dom/exception-no-frame-inline-script-crash.html b/LayoutTests/fast/dom/exception-no-frame-inline-script-crash.html
new file mode 100644 (file)
index 0000000..8061241
--- /dev/null
@@ -0,0 +1,30 @@
+<p>This test checks for a crash when throwing an exception under the following
+conditions: (1) The throwing script's document has no frame; (2) The script is
+inline in the document.
+</p>
+
+<hr>
+
+<p>PASS: You didn't crash.
+</p>
+
+<iframe id="iframe" 
+        src='data:text/html,
+                <script>
+                if (window.layoutTestController)
+                    layoutTestController.dumpAsText();
+
+                function crash()
+                {
+                    /* Ensure we have no frame when the exception is thrown. */
+                    var iframe = parent.document.getElementById("iframe");
+                    iframe.parentNode.removeChild(iframe);
+                    
+                    /* Throw an exception. */
+                    throw "crash";
+                }
+                
+                crash();
+                </script>
+            '
+></iframe>
diff --git a/LayoutTests/fast/dom/exception-no-frame-timeout-crash-expected.txt b/LayoutTests/fast/dom/exception-no-frame-timeout-crash-expected.txt
new file mode 100644 (file)
index 0000000..4ad3405
--- /dev/null
@@ -0,0 +1,5 @@
+This test checks for a crash when throwing an exception under the following conditions: (1) The throwing script's document has no frame; (2) The script is run from a timeout.
+
+PASS: You didn't crash.
+
+
diff --git a/LayoutTests/fast/dom/exception-no-frame-timeout-crash.html b/LayoutTests/fast/dom/exception-no-frame-timeout-crash.html
new file mode 100644 (file)
index 0000000..f1e2d5a
--- /dev/null
@@ -0,0 +1,30 @@
+<p>This test checks for a crash when throwing an exception under the following
+conditions: (1) The throwing script's document has no frame; (2) The script is run 
+from a timeout.
+</p>
+
+<hr>
+
+<p>PASS: You didn't crash.
+</p>
+
+<iframe id="iframe" 
+        src='data:text/html,
+                <script>
+                if (window.layoutTestController)
+                    layoutTestController.dumpAsText();
+
+                function crash()
+                {
+                    /* Ensure we have no frame when the exception is thrown. */
+                    var iframe = parent.document.getElementById("iframe");
+                    iframe.parentNode.removeChild(iframe);
+                    
+                    /* Throw an exception. */
+                    throw "crash";
+                }
+                
+                setTimeout(crash, 0);
+                </script>
+            '
+></iframe>
index 34ab0d3..c0c22bf 100644 (file)
@@ -1,3 +1,18 @@
+2007-01-25  Darin Adler  <darin@apple.com>
+
+        Reviewed and landed by Geoffrey Garen.
+
+        - fix http://bugs.webkit.org/show_bug.cgi?id=12342
+          REGRESSION: destroying a frame from its own script causes various crashes
+
+        * bindings/js/kjs_window.cpp: (KJS::ScheduledAction::execute):
+        Use RefPtr for the frame and the interpreter object so they don't get deleted
+        out from underneath us.
+
+        * bindings/js/kjs_proxy.cpp: (WebCore::KJSProxy::evaluate):
+        Add a check for a page of 0. This was the only call site for the
+        addMessageToConsole function that did not have a check for a page of 0.
+
 2007-01-25 Dirk Mueller  <mueller@kde.org>
 
         Reviewed and merged by Beth.
index e063cf4..59d98f3 100644 (file)
@@ -1,7 +1,7 @@
 /*
- *  This file is part of the KDE libraries
  *  Copyright (C) 1999-2001 Harri Porten (porten@kde.org)
  *  Copyright (C) 2001 Peter Kelly (pmk@post.com)
+ *  Copyright (C) 2006, 2007 Apple Inc. All rights reserved.
  *
  *  This library is free software; you can redistribute it and/or
  *  modify it under the terms of the GNU Lesser General Public
 #include "kjs_proxy.h"
 
 #include "Chrome.h"
-#include "kjs_events.h"
-#include "kjs_window.h"
 #include "Frame.h"
 #include "FrameLoader.h"
 #include "JSDOMWindow.h"
 #include "Page.h"
+#include "kjs_events.h"
+#include "kjs_window.h"
 
 #ifdef SVG_SUPPORT
 #include "JSSVGLazyEventListener.h"
@@ -45,37 +45,38 @@ KJSProxy::KJSProxy(Frame* frame)
 
 JSValue* KJSProxy::evaluate(const String& filename, int baseLine, const String& str, Node* n) 
 {
-  // evaluate code. Returns the JS return value or 0
-  // if there was none, an error occured or the type couldn't be converted.
+    // evaluate code. Returns the JS return value or 0
+    // if there was none, an error occured or the type couldn't be converted.
 
-  initScriptIfNeeded();
-  // inlineCode is true for <a href="javascript:doSomething()">
-  // and false for <script>doSomething()</script>. Check if it has the
-  // expected value in all cases.
-  // See smart window.open policy for where this is used.
-  bool inlineCode = filename.isNull();
+    initScriptIfNeeded();
+    // inlineCode is true for <a href="javascript:doSomething()">
+    // and false for <script>doSomething()</script>. Check if it has the
+    // expected value in all cases.
+    // See smart window.open policy for where this is used.
+    bool inlineCode = filename.isNull();
 
-  m_script->setInlineCode(inlineCode);
+    m_script->setInlineCode(inlineCode);
 
-  JSLock lock;
+    JSLock lock;
 
-  JSValue* thisNode = n ? Window::retrieve(m_frame) : toJS(m_script->globalExec(), n);
+    JSValue* thisNode = n ? Window::retrieve(m_frame) : toJS(m_script->globalExec(), n);
   
-  m_script->startTimeoutCheck();
-  Completion comp = m_script->evaluate(filename, baseLine, reinterpret_cast<const KJS::UChar*>(str.characters()), str.length(), thisNode);
-  m_script->stopTimeoutCheck();
+    m_script->startTimeoutCheck();
+    Completion comp = m_script->evaluate(filename, baseLine, reinterpret_cast<const KJS::UChar*>(str.characters()), str.length(), thisNode);
+    m_script->stopTimeoutCheck();
   
-  if (comp.complType() == Normal || comp.complType() == ReturnValue)
-    return comp.value();
-
-  if (comp.complType() == Throw) {
-    UString errorMessage = comp.value()->toString(m_script->globalExec());
-    int lineNumber = comp.value()->toObject(m_script->globalExec())->get(m_script->globalExec(), "line")->toInt32(m_script->globalExec());
-    UString sourceURL = comp.value()->toObject(m_script->globalExec())->get(m_script->globalExec(), "sourceURL")->toString(m_script->globalExec());
-    m_frame->page()->chrome()->addMessageToConsole(errorMessage, lineNumber, sourceURL);
-  }
-
-  return 0;
+    if (comp.complType() == Normal || comp.complType() == ReturnValue)
+        return comp.value();
+
+    if (comp.complType() == Throw) {
+        UString errorMessage = comp.value()->toString(m_script->globalExec());
+        int lineNumber = comp.value()->toObject(m_script->globalExec())->get(m_script->globalExec(), "line")->toInt32(m_script->globalExec());
+        UString sourceURL = comp.value()->toObject(m_script->globalExec())->get(m_script->globalExec(), "sourceURL")->toString(m_script->globalExec());
+        if (Page* page = m_frame->page())
+            page->chrome()->addMessageToConsole(errorMessage, lineNumber, sourceURL);
+    }
+
+    return 0;
 }
 
 void KJSProxy::clear() {
index 3e3f541..b8544dd 100644 (file)
@@ -1221,7 +1221,7 @@ bool Window::isSafeScript(const ScriptInterpreter *origin, const ScriptInterpret
     String message = String::format("Unsafe JavaScript attempt to access frame with URL %s from frame with URL %s. Domains must match.\n", 
                   targetDocument->URL().latin1(), originDocument->URL().latin1());
     if (Page* page = targetFrame->page())
-        page->chrome()->addMessageToConsole(message, 1, String()); //fixme: provide a real line number and sourceurl
+        page->chrome()->addMessageToConsole(message, 1, String()); // FIXME: provide a real line number and source URL.
 
     return false;
 }
@@ -1819,22 +1819,27 @@ void Window::updateLayout() const
 
 ////////////////////// ScheduledAction ////////////////////////
 
-void ScheduledAction::execute(Window *window)
+void ScheduledAction::execute(Windowwindow)
 {
-    if (!window->m_frame || !window->m_frame->scriptProxy())
+    RefPtr<Frame> frame = window->m_frame;
+    if (!frame)
+        return;
+
+    KJSProxy* scriptProxy = frame->scriptProxy();
+    if (!scriptProxy)
         return;
 
-    ScriptInterpreter *interpreter = window->m_frame->scriptProxy()->interpreter();
+    RefPtr<ScriptInterpreter> interpreter = scriptProxy->interpreter();
 
     interpreter->setProcessingTimerCallback(true);
-  
+
     if (JSValue* func = m_func.get()) {
-        if (func->isObject() && static_cast<JSObject *>(func)->implementsCall()) {
-            ExecState *exec = interpreter->globalExec();
+        if (func->isObject() && static_cast<JSObject*>(func)->implementsCall()) {
+            ExecStateexec = interpreter->globalExec();
             ASSERT(window == interpreter->globalObject());
             JSLock lock;
             interpreter->startTimeoutCheck();
-            static_cast<JSObject *>(func)->call(exec, window, m_args);
+            static_cast<JSObject*>(func)->call(exec, window, m_args);
             interpreter->stopTimeoutCheck();
             if (exec->hadException()) {
                 JSObject* exception = exec->exception()->toObject(exec);
@@ -1843,17 +1848,19 @@ void ScheduledAction::execute(Window *window)
                 int lineNumber = exception->get(exec, "line")->toInt32(exec);
                 if (Interpreter::shouldPrintExceptions())
                     printf("(timer):%s\n", message.utf8().data());
-                if (Page* page = window->m_frame->page())
+                if (Page* page = frame->page())
                     page->chrome()->addMessageToConsole(message, lineNumber, String());
             }
         }
     } else
-        window->m_frame->loader()->executeScript(0, m_code);
-  
+        frame->loader()->executeScript(0, m_code);
+
     // Update our document's rendering following the execution of the timeout callback.
-    // FIXME: Why? Why not other documents, for example?
-    Document *doc = window->m_frame->document();
-    if (doc)
+    // FIXME: Why not use updateDocumentsRendering to update rendering of all documents?
+    // FIXME: Is this really the right point to do the update? We need a place that works
+    // for all possible entry points that might possibly execute script, but this seems
+    // to be a bit too low-level.
+    if (Document* doc = frame->document())
         doc->updateRendering();
   
     interpreter->setProcessingTimerCallback(false);