WebCore:
authordarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 6 Mar 2009 17:22:07 +0000 (17:22 +0000)
committerdarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 6 Mar 2009 17:22:07 +0000 (17:22 +0000)
2009-03-06  Darin Adler  <darin@apple.com>

        Reviewed by Darin Fisher.

        Bug 24422: REGRESSION: null-URL crash in FrameLoader setting location.hash on new window
        https://bugs.webkit.org/show_bug.cgi?id=24422
        rdar://problem/6402208

        Test: fast/dom/location-new-window-no-crash.html

        The issue here is empty (or null) URLs. I picked the "schedule navigation" bottleneck
        to add some checks for empty URLs. We could also put the empty URL checks at some
        other bottleneck level and add more assertions over time. I tried adding a few more
        assertions to functions like loadURL and hit them while running the regression tests,
        so it's probably going to be a bit tricky to clean this up throughout the loader.

        * loader/FrameLoader.cpp:
        (WebCore::ScheduledRedirection::ScheduledRedirection): Explicitly marked this struct
        immutable by making all its members const. Added assertions about the arguments,
        including that the URL is not empty. Initialized one uninitialized member in one of
        the constructors.
        (WebCore::FrameLoader::scheduleHTTPRedirection): Added an early exit to make this
        a no-op if passed an empty URL.
        (WebCore::FrameLoader::scheduleLocationChange): Ditto.
        (WebCore::FrameLoader::scheduleRefresh): Ditto.

LayoutTests:

2009-03-06  Darin Adler  <darin@apple.com>

        Reviewed by Darin Fisher.

        Bug 24422: REGRESSION: null-URL crash in FrameLoader setting location.hash on new window
        https://bugs.webkit.org/show_bug.cgi?id=24422
        rdar://problem/6402208

        The new test manipulates all the properties of the location object on a new window which
        has no location yet. I tested Firefox too and added comments about how its behavior differs
        from WebKit. At some point we may want to tweak our behavior to be a bit closer to theirs,
        or check IE's behavior or if HTML 5 or some other W3 specification has something to say
        about this, but for now the main purpose of the test is to verify we don't crash.

        * fast/dom/location-new-window-no-crash-expected.txt: Added.
        * fast/dom/location-new-window-no-crash.html: Added.
        * fast/dom/resources/location-new-window-no-crash.js: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/dom/location-new-window-no-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/location-new-window-no-crash.html [new file with mode: 0644]
LayoutTests/fast/dom/resources/location-new-window-no-crash.js [new file with mode: 0644]
WebCore/ChangeLog
WebCore/loader/FrameLoader.cpp

index 949ce3b..532ccb6 100644 (file)
@@ -1,5 +1,23 @@
 2009-03-06  Darin Adler  <darin@apple.com>
 
+        Reviewed by Darin Fisher.
+
+        Bug 24422: REGRESSION: null-URL crash in FrameLoader setting location.hash on new window
+        https://bugs.webkit.org/show_bug.cgi?id=24422
+        rdar://problem/6402208
+
+        The new test manipulates all the properties of the location object on a new window which
+        has no location yet. I tested Firefox too and added comments about how its behavior differs
+        from WebKit. At some point we may want to tweak our behavior to be a bit closer to theirs,
+        or check IE's behavior or if HTML 5 or some other W3 specification has something to say
+        about this, but for now the main purpose of the test is to verify we don't crash.
+
+        * fast/dom/location-new-window-no-crash-expected.txt: Added.
+        * fast/dom/location-new-window-no-crash.html: Added.
+        * fast/dom/resources/location-new-window-no-crash.js: Added.
+
+2009-03-06  Darin Adler  <darin@apple.com>
+
         * fast/dom/Window/window-properties-expected.txt: Updated for recent addition of canPlayType.
 
 2009-03-06  Hironori Bono  <hbono@chromium.org>
diff --git a/LayoutTests/fast/dom/location-new-window-no-crash-expected.txt b/LayoutTests/fast/dom/location-new-window-no-crash-expected.txt
new file mode 100644 (file)
index 0000000..47de4b5
--- /dev/null
@@ -0,0 +1,39 @@
+CONSOLE MESSAGE: line 42: TypeError: Result of expression 'testWindow' [undefined] is not an object.
+Tests that manipulating location properties in a just-created window object does not crash.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+FAIL testWindow.location.toString() should be /. Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.href should be /. Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.protocol should be :. Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.host should be . Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.hostname should be . Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.port should be . Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.pathname should be /. Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.search should be . Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.hash should be . Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.href = 'data:text/plain,b' should be data:text/plain,b. Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.protocol = 'data' should be data. Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.host = 'c' should be c. Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.hostname = 'd' should be d. Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.port = 'e' should be e. Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.pathname = 'f' should be f. Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.search = 'g' should be g. Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.hash = 'h' should be h. Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.assign('data:text/plain,i') should be undefined. Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.replace('data:text/plain,j') should be undefined. Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.reload() should be undefined. Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.toString() should be /. Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.href should be /. Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.protocol should be :. Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.host should be . Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.hostname should be . Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.port should be . Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.pathname should be /. Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.search should be . Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL testWindow.location.hash should be . Threw exception TypeError: Result of expression 'testWindow' [undefined] is not an object.
+FAIL successfullyParsed should be true (of type boolean). Was undefined (of type undefined).
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/location-new-window-no-crash.html b/LayoutTests/fast/dom/location-new-window-no-crash.html
new file mode 100644 (file)
index 0000000..635eb6e
--- /dev/null
@@ -0,0 +1,13 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<link rel="stylesheet" href="../js/resources/js-test-style.css">
+<script src="../js/resources/js-test-pre.js"></script>
+</head>
+<body>
+<p id="description"></p>
+<div id="console"></div>
+<script src="resources/location-new-window-no-crash.js"></script>
+<script src="../js/resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/fast/dom/resources/location-new-window-no-crash.js b/LayoutTests/fast/dom/resources/location-new-window-no-crash.js
new file mode 100644 (file)
index 0000000..109cac3
--- /dev/null
@@ -0,0 +1,44 @@
+description("Tests that manipulating location properties in a just-created window object does not crash.");
+
+var testWindow = open("data:text/plain,a");
+
+// Note that the window does not navigate to the new URL right away, and that is a crucial element
+// of the test. We're checking behavior when the object was just created and is not yet at its new
+// location.
+
+shouldBe("testWindow.location.toString()", "'/'"); // Firefox returns about:blank
+shouldBe("testWindow.location.href", "'/'"); // Firefox returns about:blank
+shouldBe("testWindow.location.protocol", "':'"); // Firefox returns about:
+shouldBe("testWindow.location.host", "''"); // Firefox throws an exception
+shouldBe("testWindow.location.hostname", "''"); // Firefox throws an exception
+shouldBe("testWindow.location.port", "''");
+shouldBe("testWindow.location.pathname", "'/'"); // Firefox returns the empty string
+shouldBe("testWindow.location.search", "''");
+shouldBe("testWindow.location.hash", "''");
+
+shouldBe("testWindow.location.href = 'data:text/plain,b'", "'data:text/plain,b'");
+shouldBe("testWindow.location.protocol = 'data'", "'data'"); // Firefox throws an exception
+shouldBe("testWindow.location.host = 'c'", "'c'"); // Firefox throws an exception
+shouldBe("testWindow.location.hostname = 'd'", "'d'"); // Firefox throws an exception
+shouldBe("testWindow.location.port = 'e'", "'e'"); // Firefox throws an exception
+shouldBe("testWindow.location.pathname = 'f'", "'f'"); // Firefox throws an exception
+shouldBe("testWindow.location.search = 'g'", "'g'");
+shouldBe("testWindow.location.hash = 'h'", "'h'");
+
+shouldBe("testWindow.location.assign('data:text/plain,i')", "undefined"); // Firefox returns about:blank
+shouldBe("testWindow.location.replace('data:text/plain,j')", "undefined"); // Firefox returns about:blank
+shouldBe("testWindow.location.reload()", "undefined"); // Firefox returns about:blank
+
+shouldBe("testWindow.location.toString()", "'/'"); // Firefox returns about:blank
+shouldBe("testWindow.location.href", "'/'"); // Firefox returns about:blank
+shouldBe("testWindow.location.protocol", "':'"); // Firefox returns about:
+shouldBe("testWindow.location.host", "''"); // Firefox throws an exception
+shouldBe("testWindow.location.hostname", "''"); // Firefox throws an exception
+shouldBe("testWindow.location.port", "''");
+shouldBe("testWindow.location.pathname", "'/'"); // Firefox returns the empty string
+shouldBe("testWindow.location.search", "''");
+shouldBe("testWindow.location.hash", "''");
+
+testWindow.close();
+
+var successfullyParsed = true;
index bb39579..220aff8 100644 (file)
@@ -1,3 +1,29 @@
+2009-03-06  Darin Adler  <darin@apple.com>
+
+        Reviewed by Darin Fisher.
+
+        Bug 24422: REGRESSION: null-URL crash in FrameLoader setting location.hash on new window
+        https://bugs.webkit.org/show_bug.cgi?id=24422
+        rdar://problem/6402208
+
+        Test: fast/dom/location-new-window-no-crash.html
+
+        The issue here is empty (or null) URLs. I picked the "schedule navigation" bottleneck
+        to add some checks for empty URLs. We could also put the empty URL checks at some
+        other bottleneck level and add more assertions over time. I tried adding a few more
+        assertions to functions like loadURL and hit them while running the regression tests,
+        so it's probably going to be a bit tricky to clean this up throughout the loader.
+
+        * loader/FrameLoader.cpp:
+        (WebCore::ScheduledRedirection::ScheduledRedirection): Explicitly marked this struct
+        immutable by making all its members const. Added assertions about the arguments,
+        including that the URL is not empty. Initialized one uninitialized member in one of
+        the constructors.
+        (WebCore::FrameLoader::scheduleHTTPRedirection): Added an early exit to make this
+        a no-op if passed an empty URL.
+        (WebCore::FrameLoader::scheduleLocationChange): Ditto.
+        (WebCore::FrameLoader::scheduleRefresh): Ditto.
+
 2009-03-06  Gustavo Noronha Silva  <gns@gnome.org>
 
         Reviewed by Holger Freyther.
index 20e8912..794d26b 100644 (file)
@@ -144,15 +144,16 @@ struct FormSubmission {
 
 struct ScheduledRedirection {
     enum Type { redirection, locationChange, historyNavigation, locationChangeDuringLoad };
-    Type type;
-    double delay;
-    String url;
-    String referrer;
-    int historySteps;
-    bool lockHistory;
-    bool lockBackForwardList;
-    bool wasUserGesture;
-    bool wasRefresh;
+
+    const Type type;
+    const double delay;
+    const String url;
+    const String referrer;
+    const int historySteps;
+    const bool lockHistory;
+    const bool lockBackForwardList;
+    const bool wasUserGesture;
+    const bool wasRefresh;
 
     ScheduledRedirection(double delay, const String& url, bool lockHistory, bool lockBackForwardList, bool wasUserGesture, bool refresh)
         : type(redirection)
@@ -164,6 +165,7 @@ struct ScheduledRedirection {
         , wasUserGesture(wasUserGesture)
         , wasRefresh(refresh)
     {
+        ASSERT(!url.isEmpty());
     }
 
     ScheduledRedirection(Type locationChangeType, const String& url, const String& referrer, bool lockHistory, bool lockBackForwardList, bool wasUserGesture, bool refresh)
@@ -177,6 +179,8 @@ struct ScheduledRedirection {
         , wasUserGesture(wasUserGesture)
         , wasRefresh(refresh)
     {
+        ASSERT(locationChangeType == locationChange || locationChangeType == locationChangeDuringLoad);
+        ASSERT(!url.isEmpty());
     }
 
     explicit ScheduledRedirection(int historyNavigationSteps)
@@ -184,6 +188,7 @@ struct ScheduledRedirection {
         , delay(0)
         , historySteps(historyNavigationSteps)
         , lockHistory(false)
+        , lockBackForwardList(false)
         , wasUserGesture(false)
         , wasRefresh(false)
     {
@@ -372,7 +377,6 @@ void FrameLoader::changeLocation(const String& url, const String& referrer, bool
     changeLocation(completeURL(url), referrer, lockHistory, lockBackForwardList, userGesture, refresh);
 }
 
-
 void FrameLoader::changeLocation(const KURL& url, const String& referrer, bool lockHistory, bool lockBackForwardList, bool userGesture, bool refresh)
 {
     RefPtr<Frame> protect(m_frame);
@@ -1313,6 +1317,9 @@ void FrameLoader::scheduleHTTPRedirection(double delay, const String& url)
     if (!m_frame->page())
         return;
 
+    if (url.isEmpty())
+        return;
+
     // We want a new history item if the refresh timeout is > 1 second.
     if (!m_scheduledRedirection || delay <= m_scheduledRedirection->delay)
         scheduleRedirection(new ScheduledRedirection(delay, url, true, delay <= 1, false, false));
@@ -1323,6 +1330,9 @@ void FrameLoader::scheduleLocationChange(const String& url, const String& referr
     if (!m_frame->page())
         return;
 
+    if (url.isEmpty())
+        return;
+
     // If the URL we're going to navigate to is the same as the current one, except for the
     // fragment part, we don't need to schedule the location change.
     KURL parsedURL(url);
@@ -1354,6 +1364,9 @@ void FrameLoader::scheduleRefresh(bool wasUserGesture)
     if (!m_frame->page())
         return;
 
+    if (m_URL.isEmpty())
+        return;
+
     ScheduledRedirection::Type type = ScheduledRedirection::locationChange;
     scheduleRedirection(new ScheduledRedirection(type, m_URL.string(), m_outgoingReferrer, true, true, wasUserGesture, true));
 }
@@ -1465,13 +1478,14 @@ void FrameLoader::redirectionTimerFired(Timer<FrameLoader>*)
 void FrameLoader::loadURLIntoChildFrame(const KURL& url, const String& referer, Frame* childFrame)
 {
     ASSERT(childFrame);
+
     HistoryItem* parentItem = currentHistoryItem();
     FrameLoadType loadType = this->loadType();
     FrameLoadType childLoadType = FrameLoadTypeRedirectWithLockedBackForwardList;
 
     KURL workingURL = url;
     
-    // If we're moving in the backforward list, we might want to replace the content
+    // If we're moving in the back/forward list, we might want to replace the content
     // of this child frame with whatever was there at that point.
     if (parentItem && parentItem->children().size() && isBackForwardLoadType(loadType)) {
         HistoryItem* childItem = parentItem->childItemWithName(childFrame->tree()->name());