Treat all synchronous loads equally in FrameLoader::loadSubframe
authoraroben@apple.com <aroben@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 Jan 2010 19:29:51 +0000 (19:29 +0000)
committeraroben@apple.com <aroben@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 Jan 2010 19:29:51 +0000 (19:29 +0000)
Only loads of the empty URL or about:blank were being treated as
synchronous loads. But other loads can be synchronous (e.g., when we
receive a null ResourceRequest from requestFromDelegate or when a
policy decision of "ignore" is made). We now treat those loads the
same way we treated empty URLs and about:blank.

Fixes <rdar://problem/7533333> <http://webkit.org/b/33533>
window.onload never fires if page contains an <iframe> with a bad
scheme or whose load is cancelled by returning null from resource load
delegate's willSendRequest

Tests: fast/loader/onload-bad-scheme-for-frame.html
       fast/loader/onload-policy-ignore-for-frame.html
       fast/loader/onload-willSendRequest-null-for-frame.html

Reviewed by Brady Eidson.

WebCore:

* loader/FrameLoader.cpp:
(WebCore::FrameLoader::loadSubframe):
  - Detect synchronous loads by checking the subframe's loader's
    state, rather than by checking its URL
  - Removed unnecessary call to completed(), since checkCompleted()
    will call completed() if needed (the call to completed() was added
    first and wasn't removed when the call to checkCompleted() was
    added in r8316)
  - Added more comments about the strange thing this function does
    with the subframe's loader

WebKitTools:

Add LayoutTestController API to force
-webView:resource:willSendRequest:: to return null

* DumpRenderTree/LayoutTestController.cpp:
(LayoutTestController::LayoutTestController): Initialize new member.
(setWillSendRequestReturnsNullCallback): Call through to
LayoutTestController.
(LayoutTestController::staticFunctions): Added new function.

* DumpRenderTree/LayoutTestController.h: Added
m_willSendRequestReturnsNull.
(LayoutTestController::willSendRequestReturnsNull):
(LayoutTestController::setWillSendRequestReturnsNull):
Added standard accessors.

* DumpRenderTree/mac/ResourceLoadDelegate.mm:
(-[ResourceLoadDelegate webView:resource:willSendRequest:redirectResponse:fromDataSource:]):
* DumpRenderTree/win/ResourceLoadDelegate.cpp:
(ResourceLoadDelegate::willSendRequest):
Return null if LayoutTestController says to.

LayoutTests:

Add tests that show that onload still fires in various situations with
subframes

* fast/loader/onload-bad-scheme-for-frame-expected.txt: Added.
* fast/loader/onload-bad-scheme-for-frame.html: Added.
Tests that onload still fires when a subframe with an unrecognized
scheme is in the page.

* fast/loader/onload-policy-ignore-for-frame-expected.txt: Added.
* fast/loader/onload-policy-ignore-for-frame.html: Added.
Tests that onload still fires when the policy delegate says to ignore
the initial load of a subframe.

* fast/loader/onload-willSendRequest-null-for-frame-expected.txt: Added.
* fast/loader/onload-willSendRequest-null-for-frame.html: Added.
Tests that onload still fires when the resource load delegate returns
null from willSendRequest for the initial load of a subframe.

* http/tests/loading/bad-scheme-subframe-expected.txt: Updated result
to show that we now fire onload for the main frame in this case. (This
is essentially the same as onload-bad-scheme-for-frame.html, but it
still seems useful to have that new, more explicit, test.)

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

15 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/loader/onload-bad-scheme-for-frame-expected.txt [new file with mode: 0644]
LayoutTests/fast/loader/onload-bad-scheme-for-frame.html [new file with mode: 0644]
LayoutTests/fast/loader/onload-policy-ignore-for-frame-expected.txt [new file with mode: 0644]
LayoutTests/fast/loader/onload-policy-ignore-for-frame.html [new file with mode: 0644]
LayoutTests/fast/loader/onload-willSendRequest-null-for-frame-expected.txt [new file with mode: 0644]
LayoutTests/fast/loader/onload-willSendRequest-null-for-frame.html [new file with mode: 0644]
LayoutTests/http/tests/loading/bad-scheme-subframe-expected.txt
WebCore/ChangeLog
WebCore/loader/FrameLoader.cpp
WebKitTools/ChangeLog
WebKitTools/DumpRenderTree/LayoutTestController.cpp
WebKitTools/DumpRenderTree/LayoutTestController.h
WebKitTools/DumpRenderTree/mac/ResourceLoadDelegate.mm
WebKitTools/DumpRenderTree/win/ResourceLoadDelegate.cpp

index 72ccddc..fe69011 100644 (file)
 
 2010-01-14  Adam Roben  <aroben@apple.com>
 
+        Add tests that show that onload still fires in various situations with
+        subframes
+
+        Tests for <rdar://problem/7533333> <http://webkit.org/b/33533>
+        window.onload never fires if page contains an <iframe> with a bad
+        scheme or whose load is cancelled by returning null from resource load
+        delegate's willSendRequest
+
+        Reviewed by Brady Eidson.
+
+        * fast/loader/onload-bad-scheme-for-frame-expected.txt: Added.
+        * fast/loader/onload-bad-scheme-for-frame.html: Added.
+        Tests that onload still fires when a subframe with an unrecognized
+        scheme is in the page.
+
+        * fast/loader/onload-policy-ignore-for-frame-expected.txt: Added.
+        * fast/loader/onload-policy-ignore-for-frame.html: Added.
+        Tests that onload still fires when the policy delegate says to ignore
+        the initial load of a subframe.
+
+        * fast/loader/onload-willSendRequest-null-for-frame-expected.txt: Added.
+        * fast/loader/onload-willSendRequest-null-for-frame.html: Added.
+        Tests that onload still fires when the resource load delegate returns
+        null from willSendRequest for the initial load of a subframe.
+
+        * http/tests/loading/bad-scheme-subframe-expected.txt: Updated result
+        to show that we now fire onload for the main frame in this case. (This
+        is essentially the same as onload-bad-scheme-for-frame.html, but it
+        still seems useful to have that new, more explicit, test.)
+
+2010-01-14  Adam Roben  <aroben@apple.com>
+
         Fix a typo in editing/selection/inactive-selection.html
 
         Fixes <http://webkit.org/b/33676> Exception when opening
diff --git a/LayoutTests/fast/loader/onload-bad-scheme-for-frame-expected.txt b/LayoutTests/fast/loader/onload-bad-scheme-for-frame-expected.txt
new file mode 100644 (file)
index 0000000..ee370ad
--- /dev/null
@@ -0,0 +1,4 @@
+Test for window.onload never fires if page contains an <iframe> with a bad scheme or whose load is cancelled by returning null from resource load delegate's willSendRequest. If the test passes, you should see the word "PASSED" below.
+
+PASSED
+
diff --git a/LayoutTests/fast/loader/onload-bad-scheme-for-frame.html b/LayoutTests/fast/loader/onload-bad-scheme-for-frame.html
new file mode 100644 (file)
index 0000000..d470f73
--- /dev/null
@@ -0,0 +1,26 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <title>Test for Bug 33533: window.onload never fires if page contains an &lt;iframe> with a bad scheme or whose load is cancelled by returning null from resource load delegate's willSendRequest</title>
+    <script>
+        if (window.layoutTestController)
+            layoutTestController.dumpAsText();
+
+        window.onload = function() {
+            var result = document.getElementById("result");
+            if (!window.layoutTestController) {
+                result.innerText = "This test can only be run in DumpRenderTree.";
+                return;
+            }
+            result.innerText = "PASSED";
+        };
+    </script>
+</head>
+<body>
+    <p>Test for <a href="http://webkit.org/b/33533">window.onload never fires if page contains an
+    &lt;iframe> with a bad scheme or whose load is cancelled by returning null from resource load
+    delegate's willSendRequest</a>. If the test passes, you should see the word "PASSED" below.</p>
+    <div id=result>FAILED</div>
+    <iframe src="this-is-definitely-a-bad-uri-scheme:"></iframe>
+</body>
+</html>
diff --git a/LayoutTests/fast/loader/onload-policy-ignore-for-frame-expected.txt b/LayoutTests/fast/loader/onload-policy-ignore-for-frame-expected.txt
new file mode 100644 (file)
index 0000000..a22fad4
--- /dev/null
@@ -0,0 +1,5 @@
+Policy delegate: attempt to load http://www.example.com/ with navigation type 'other'
+Test for window.onload never fires if page contains an <iframe> with a bad scheme or whose load is cancelled by returning null from resource load delegate's willSendRequest. If the test passes, you should see the word "PASSED" below.
+
+PASSED
+
diff --git a/LayoutTests/fast/loader/onload-policy-ignore-for-frame.html b/LayoutTests/fast/loader/onload-policy-ignore-for-frame.html
new file mode 100644 (file)
index 0000000..5289b0c
--- /dev/null
@@ -0,0 +1,28 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <title>Test for Bug 33533: window.onload never fires if page contains an &lt;iframe> with a bad scheme or whose load is cancelled by returning null from resource load delegate's willSendRequest</title>
+    <script>
+        if (window.layoutTestController) {
+            layoutTestController.dumpAsText();
+            layoutTestController.setCustomPolicyDelegate(true);
+        }
+
+        window.onload = function() {
+            var result = document.getElementById("result");
+            if (!window.layoutTestController) {
+                result.innerText = "This test can only be run in DumpRenderTree.";
+                return;
+            }
+            result.innerText = "PASSED";
+        };
+    </script>
+</head>
+<body>
+    <p>Test for <a href="http://webkit.org/b/33533">window.onload never fires if page contains an
+    &lt;iframe> with a bad scheme or whose load is cancelled by returning null from resource load
+    delegate's willSendRequest</a>. If the test passes, you should see the word "PASSED" below.</p>
+    <div id=result>FAILED</div>
+    <iframe src="http://www.example.com/"></iframe>
+</body>
+</html>
diff --git a/LayoutTests/fast/loader/onload-willSendRequest-null-for-frame-expected.txt b/LayoutTests/fast/loader/onload-willSendRequest-null-for-frame-expected.txt
new file mode 100644 (file)
index 0000000..ee370ad
--- /dev/null
@@ -0,0 +1,4 @@
+Test for window.onload never fires if page contains an <iframe> with a bad scheme or whose load is cancelled by returning null from resource load delegate's willSendRequest. If the test passes, you should see the word "PASSED" below.
+
+PASSED
+
diff --git a/LayoutTests/fast/loader/onload-willSendRequest-null-for-frame.html b/LayoutTests/fast/loader/onload-willSendRequest-null-for-frame.html
new file mode 100644 (file)
index 0000000..d6f74c4
--- /dev/null
@@ -0,0 +1,28 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <title>Test for Bug 33533: window.onload never fires if page contains an &lt;iframe> with a bad scheme or whose load is cancelled by returning null from resource load delegate's willSendRequest</title>
+    <script>
+        if (window.layoutTestController) {
+            layoutTestController.dumpAsText();
+            layoutTestController.setWillSendRequestReturnsNull(true);
+        }
+
+        window.onload = function() {
+            var result = document.getElementById("result");
+            if (!window.layoutTestController) {
+                result.innerText = "This test can only be run in DumpRenderTree.";
+                return;
+            }
+            result.innerText = "PASSED";
+        };
+    </script>
+</head>
+<body>
+    <p>Test for <a href="http://webkit.org/b/33533">window.onload never fires if page contains an
+    &lt;iframe> with a bad scheme or whose load is cancelled by returning null from resource load
+    delegate's willSendRequest</a>. If the test passes, you should see the word "PASSED" below.</p>
+    <div id=result>FAILED</div>
+    <iframe src="http://www.example.com/"></iframe>
+</body>
+</html>
index f1e6368..957f1c7 100644 (file)
@@ -1,6 +1,7 @@
 main frame - didStartProvisionalLoadForFrame
 main frame - didCommitLoadForFrame
 main frame - didFinishDocumentLoadForFrame
+main frame - didHandleOnloadEventsForFrame
 main frame - didFinishLoadForFrame
 This is a test of load callbacks. It is only useful inside the regression test tool.
 
index 28cdf9a..e36bb95 100644 (file)
@@ -1,3 +1,35 @@
+2010-01-14  Adam Roben  <aroben@apple.com>
+
+        Treat all synchronous loads equally in FrameLoader::loadSubframe
+
+        Only loads of the empty URL or about:blank were being treated as
+        synchronous loads. But other loads can be synchronous (e.g., when we
+        receive a null ResourceRequest from requestFromDelegate or when a
+        policy decision of "ignore" is made). We now treat those loads the
+        same way we treated empty URLs and about:blank.
+
+        Fixes <rdar://problem/7533333> <http://webkit.org/b/33533>
+        window.onload never fires if page contains an <iframe> with a bad
+        scheme or whose load is cancelled by returning null from resource load
+        delegate's willSendRequest
+
+        Tests: fast/loader/onload-bad-scheme-for-frame.html
+               fast/loader/onload-policy-ignore-for-frame.html
+               fast/loader/onload-willSendRequest-null-for-frame.html
+
+        Reviewed by Brady Eidson.
+
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::loadSubframe):
+          - Detect synchronous loads by checking the subframe's loader's
+            state, rather than by checking its URL
+          - Removed unnecessary call to completed(), since checkCompleted()
+            will call completed() if needed (the call to completed() was added
+            first and wasn't removed when the call to checkCompleted() was
+            added in r8316)
+          - Added more comments about the strange thing this function does
+            with the subframe's loader
+
 2010-01-14  Diego Gonzalez  <diego.gonzalez@openbossa.org>
 
         Reviewed by Kenneth Christiansen.
index 314ca33..26545f4 100644 (file)
@@ -403,6 +403,12 @@ Frame* FrameLoader::loadSubframe(HTMLFrameOwnerElement* ownerElement, const KURL
         return 0;
     }
     
+    // All new frames will have m_isComplete set to true at this point due to synchronously loading
+    // an empty document in FrameLoader::init(). But many frames will now be starting an
+    // asynchronous load of url, so we set m_isComplete to false and then check if the load is
+    // actually completed below. (Note that we set m_isComplete to false even for synchronous
+    // loads, so that checkCompleted() below won't bail early.)
+    // FIXME: Can we remove this entirely? m_isComplete normally gets set to false when a load is committed.
     frame->loader()->m_isComplete = false;
    
     RenderObject* renderer = ownerElement->renderer();
@@ -412,16 +418,17 @@ Frame* FrameLoader::loadSubframe(HTMLFrameOwnerElement* ownerElement, const KURL
     
     checkCallImplicitClose();
     
+    // Some loads are performed synchronously (e.g., about:blank and loads
+    // cancelled by returning a null ResourceRequest from requestFromDelegate).
     // In these cases, the synchronous load would have finished
     // before we could connect the signals, so make sure to send the 
-    // completed() signal for the child by hand
+    // completed() signal for the child by hand and mark the load as being
+    // complete.
     // FIXME: In this case the Frame will have finished loading before 
     // it's being added to the child list. It would be a good idea to
     // create the child first, then invoke the loader separately.
-    if (url.isEmpty() || url == blankURL()) {
-        frame->loader()->completed();
+    if (frame->loader()->state() == FrameStateComplete)
         frame->loader()->checkCompleted();
-    }
 
     return frame.get();
 }
index 7e8a2f3..b10d8b6 100644 (file)
@@ -1,3 +1,33 @@
+2010-01-14  Adam Roben  <aroben@apple.com>
+
+        Add LayoutTestController API to force
+        -webView:resource:willSendRequest:: to return null
+
+        Enables tests for <rdar://problem/7533333> <http://webkit.org/b/33533>
+        window.onload never fires if page contains an <iframe> with a bad
+        scheme or whose load is cancelled by returning null from resource load
+        delegate's willSendRequest
+
+        Reviewed by Brady Eidson.
+
+        * DumpRenderTree/LayoutTestController.cpp:
+        (LayoutTestController::LayoutTestController): Initialize new member.
+        (setWillSendRequestReturnsNullCallback): Call through to
+        LayoutTestController.
+        (LayoutTestController::staticFunctions): Added new function.
+
+        * DumpRenderTree/LayoutTestController.h: Added
+        m_willSendRequestReturnsNull.
+        (LayoutTestController::willSendRequestReturnsNull):
+        (LayoutTestController::setWillSendRequestReturnsNull):
+        Added standard accessors.
+
+        * DumpRenderTree/mac/ResourceLoadDelegate.mm:
+        (-[ResourceLoadDelegate webView:resource:willSendRequest:redirectResponse:fromDataSource:]):
+        * DumpRenderTree/win/ResourceLoadDelegate.cpp:
+        (ResourceLoadDelegate::willSendRequest):
+        Return null if LayoutTestController says to.
+
 2010-01-14  Kevin Ollivier  <kevino@theolliviers.com>
 
         [wx] Build fix after removal of XBM support.
index 572d610..fe93984 100644 (file)
@@ -66,6 +66,7 @@ LayoutTestController::LayoutTestController(const std::string& testPathOrURL, con
     , m_testRepaint(false)
     , m_testRepaintSweepHorizontally(false)
     , m_waitToDump(false)
+    , m_willSendRequestReturnsNull(false)
     , m_willSendRequestReturnsNullOnRedirect(false)
     , m_windowIsKey(true)
     , m_alwaysAcceptCookies(false)
@@ -911,6 +912,18 @@ static JSValueRef setUserStyleSheetLocationCallback(JSContextRef context, JSObje
     return JSValueMakeUndefined(context);
 }
 
+static JSValueRef setWillSendRequestReturnsNullCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
+{
+    // Has cross-platform implementation
+    if (argumentCount < 1)
+        return JSValueMakeUndefined(context);
+
+    LayoutTestController* controller = static_cast<LayoutTestController*>(JSObjectGetPrivate(thisObject));
+    controller->setWillSendRequestReturnsNull(JSValueToBoolean(context, arguments[0]));
+
+    return JSValueMakeUndefined(context);
+}
+
 static JSValueRef setWillSendRequestReturnsNullOnRedirectCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
 {
     // Has cross-platform implementation
@@ -1307,6 +1320,7 @@ JSStaticFunction* LayoutTestController::staticFunctions()
         { "setUseDashboardCompatibilityMode", setUseDashboardCompatibilityModeCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },
         { "setUserStyleSheetEnabled", setUserStyleSheetEnabledCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },
         { "setUserStyleSheetLocation", setUserStyleSheetLocationCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },
+        { "setWillSendRequestReturnsNull", setWillSendRequestReturnsNullCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },
         { "setWillSendRequestReturnsNullOnRedirect", setWillSendRequestReturnsNullOnRedirectCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },
         { "setWindowIsKey", setWindowIsKeyCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },
         { "showWebInspector", showWebInspectorCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },
index 7e18dce..51e929a 100644 (file)
@@ -176,6 +176,9 @@ public:
     void setWaitToDump(bool waitToDump);
     void waitToDumpWatchdogTimerFired();
 
+    bool willSendRequestReturnsNull() const { return m_willSendRequestReturnsNull; }
+    void setWillSendRequestReturnsNull(bool returnsNull) { m_willSendRequestReturnsNull = returnsNull; }
+
     bool willSendRequestReturnsNullOnRedirect() const { return m_willSendRequestReturnsNullOnRedirect; }
     void setWillSendRequestReturnsNullOnRedirect(bool returnsNull) { m_willSendRequestReturnsNullOnRedirect = returnsNull; }
 
@@ -249,6 +252,7 @@ private:
     bool m_testRepaint;
     bool m_testRepaintSweepHorizontally;
     bool m_waitToDump; // True if waitUntilDone() has been called, but notifyDone() has not yet been called.
+    bool m_willSendRequestReturnsNull;
     bool m_willSendRequestReturnsNullOnRedirect;
     bool m_windowIsKey;
     bool m_alwaysAcceptCookies;
index 3aaba59..6f82e01 100644 (file)
         printf("%s\n", [string UTF8String]);
     }
 
+    if (!done && gLayoutTestController->willSendRequestReturnsNull())
+        return nil;
+
     if (!done && gLayoutTestController->willSendRequestReturnsNullOnRedirect() && redirectResponse) {
         printf("Returning null for this redirect\n");
         return nil;
index 0edf69b..19bf84a 100644 (file)
@@ -243,6 +243,11 @@ HRESULT STDMETHODCALLTYPE ResourceLoadDelegate::willSendRequest(
             descriptionSuitableForTestResult(redirectResponse).c_str());
     }
 
+    if (!done && gLayoutTestController->willSendRequestReturnsNull()) {
+        *newRequest = 0;
+        return S_OK;
+    }
+
     if (!done && gLayoutTestController->willSendRequestReturnsNullOnRedirect() && redirectResponse) {
         printf("Returning null for this redirect\n");
         *newRequest = 0;