<rdar://problem/8636239> and https://bugs.webkit.org/show_bug.cgi?id=53785
authorbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 4 Feb 2011 20:38:44 +0000 (20:38 +0000)
committerbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 4 Feb 2011 20:38:44 +0000 (20:38 +0000)
WebKit2: Pages with dynamically inserted iframes can add extraneous back/forward items.

Reviewed by Anders Carlsson.

Source/WebKit2:

WebCore doesn't gracefully handle the case where the decidePolicyForNavigationAction callback
does not occur synchronously. Let's make it synchronous.

Change WebPageProxy to handle this message reply synchronously:
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::WebPageProxy):
(WebKit::WebPageProxy::receivedPolicyDecision):
(WebKit::WebPageProxy::decidePolicyForNavigationAction):
* UIProcess/WebPageProxy.h:
* UIProcess/WebPageProxy.messages.in: Make the navigation policy action message be synchronous.

* Platform/CoreIPC/HandleMessage.h:
(CoreIPC::callMemberFunction): Add a 6-argument varient

* WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
(WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction): Send the message synchronously.

LayoutTests:

* fast/loader/dynamic-iframe-extra-back-forward-item-expected.txt: Added.
* fast/loader/dynamic-iframe-extra-back-forward-item.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/loader/dynamic-iframe-extra-back-forward-item-expected.txt [new file with mode: 0644]
LayoutTests/fast/loader/dynamic-iframe-extra-back-forward-item.html [new file with mode: 0644]
Source/WebKit2/ChangeLog
Source/WebKit2/Platform/CoreIPC/HandleMessage.h
Source/WebKit2/UIProcess/WebPageProxy.cpp
Source/WebKit2/UIProcess/WebPageProxy.h
Source/WebKit2/UIProcess/WebPageProxy.messages.in
Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp

index 2d91157659dea1ada3a484a14e9c38d0467e90af..6f2379b190429cbbdff7a384cfea81a6546f4456 100644 (file)
@@ -1,3 +1,13 @@
+2011-02-04  Brady Eidson  <beidson@apple.com>
+
+        Reviewed by Anders Carlsson.
+
+        <rdar://problem/8636239> and https://bugs.webkit.org/show_bug.cgi?id=53785
+        WebKit2: Pages with dynamically inserted iframes can add extraneous back/forward items.
+
+        * fast/loader/dynamic-iframe-extra-back-forward-item-expected.txt: Added.
+        * fast/loader/dynamic-iframe-extra-back-forward-item.html: Added.
+
 2011-02-04  Dimitri Glazkov  <dglazkov@chromium.org>
 
         Add platform/mac-leopard baselines for tests added in r77639. The text rendering is
diff --git a/LayoutTests/fast/loader/dynamic-iframe-extra-back-forward-item-expected.txt b/LayoutTests/fast/loader/dynamic-iframe-extra-back-forward-item-expected.txt
new file mode 100644 (file)
index 0000000..1f9b19f
--- /dev/null
@@ -0,0 +1,8 @@
+<rdar://problem/8636239> and https://bugs.webkit.org/show_bug.cgi?id=53785 - Extraneous back/forward entries with dynamically appended iframes.
+This page appends an iframe after onload. That iframe should *not* result in a new back/forward entry in the session history.
+
+
+============== Back Forward List ==============
+curr->  (file test):fast/loader/dynamic-iframe-extra-back-forward-item.html  **nav target**
+            (file test):fast/loader/resources/notify-done.html (in frame "<!--framePath //<!--frame0-->-->")
+===============================================
diff --git a/LayoutTests/fast/loader/dynamic-iframe-extra-back-forward-item.html b/LayoutTests/fast/loader/dynamic-iframe-extra-back-forward-item.html
new file mode 100644 (file)
index 0000000..3573a81
--- /dev/null
@@ -0,0 +1,18 @@
+<script>
+if (window.layoutTestController) {
+   layoutTestController.dumpAsText();
+   layoutTestController.dumpBackForwardList();
+   layoutTestController.waitUntilDone();
+}
+
+function loaded()
+{
+    var f = document.createElement("iframe");
+    document.body.appendChild(f);
+    f.setAttribute('src', 'resources/notify-done.html');
+}    
+</script>
+<body onload="setTimeout('loaded()', 0);">
+&lt;rdar://problem/8636239&gt; and https://bugs.webkit.org/show_bug.cgi?id=53785 - Extraneous back/forward entries with dynamically appended iframes.<br>
+This page appends an iframe after onload.  That iframe should *not* result in a new back/forward entry in the session history.<br>
+</body>
index ccea1633517926e3293d2b81854b675e6a2a4b04..347fd805a05ec252e462c0d21dbf222c659a8a54 100644 (file)
@@ -1,3 +1,27 @@
+2011-02-04  Brady Eidson  <beidson@apple.com>
+
+        Reviewed by Anders Carlsson.
+
+        <rdar://problem/8636239> and https://bugs.webkit.org/show_bug.cgi?id=53785
+        WebKit2: Pages with dynamically inserted iframes can add extraneous back/forward items.
+
+        WebCore doesn't gracefully handle the case where the decidePolicyForNavigationAction callback
+        does not occur synchronously. Let's make it synchronous.
+
+        Change WebPageProxy to handle this message reply synchronously:
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::WebPageProxy):
+        (WebKit::WebPageProxy::receivedPolicyDecision):
+        (WebKit::WebPageProxy::decidePolicyForNavigationAction):
+        * UIProcess/WebPageProxy.h:
+        * UIProcess/WebPageProxy.messages.in: Make the navigation policy action message be synchronous.
+
+        * Platform/CoreIPC/HandleMessage.h:
+        (CoreIPC::callMemberFunction): Add a 6-argument varient 
+
+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
+        (WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction): Send the message synchronously.
+
 2011-02-04  Steve Falkenburg  <sfalken@apple.com>
 
         Windows build fix.
index 534e825741e546d9ed4b3ee13090d4876f41b908..4868fc0907ca540607a429c93330e9d207722f44 100644 (file)
@@ -159,6 +159,11 @@ void callMemberFunction(const Arguments5<P1, P2, P3, P4, P5>& args, Arguments2<R
     (object->*function)(args.argument1, args.argument2, args.argument3, args.argument4, args.argument5, replyArgs.argument1, replyArgs.argument2);
 }
 
+template<typename C, typename MF, typename P1, typename P2, typename P3, typename P4, typename P5, typename P6, typename R1, typename R2>
+void callMemberFunction(const Arguments6<P1, P2, P3, P4, P5, P6>& args, Arguments2<R1, R2>& replyArgs, C* object, MF function)
+{
+    (object->*function)(args.argument1, args.argument2, args.argument3, args.argument4, args.argument5, args.argument6, replyArgs.argument1, replyArgs.argument2);
+}
     
 template<typename C, typename MF, typename P1, typename P2, typename P3, typename P4, typename R1, typename R2, typename R3>
 void callMemberFunction(const Arguments4<P1, P2, P3, P4>& args, Arguments3<R1, R2, R3>& replyArgs, C* object, MF function)
index 4a1d0b5ce88340630b0f9897e11cbdb33fc91497..f977e61248e49652d0d119d6da0f6d87f7a73d9f 100644 (file)
@@ -118,6 +118,9 @@ WebPageProxy::WebPageProxy(PageClient* pageClient, WebContext* context, WebPageG
     , m_syncMimeTypePolicyActionIsValid(false)
     , m_syncMimeTypePolicyAction(PolicyUse)
     , m_syncMimeTypePolicyDownloadID(0)
+    , m_inDecidePolicyForNavigationAction(false)
+    , m_syncNavigationActionPolicyActionIsValid(false)
+    , m_syncNavigationActionPolicyAction(PolicyUse)
     , m_processingWheelEvent(false)
     , m_processingMouseMoveEvent(false)
     , m_pageID(pageID)
@@ -771,6 +774,14 @@ void WebPageProxy::receivedPolicyDecision(PolicyAction action, WebFrameProxy* fr
         return;
     }
 
+    // If we received a policy decision while in decidePolicyForNavigationAction the decision will 
+    // be sent back to the web process by decidePolicyForNavigationAction. 
+    if (m_inDecidePolicyForNavigationAction) {
+        m_syncNavigationActionPolicyActionIsValid = true;
+        m_syncNavigationActionPolicyAction = action;
+        return;
+    }
+    
     process()->send(Messages::WebPage::DidReceivePolicyDecision(frame->frameID(), listenerID, action, downloadID), m_pageID);
 }
 
@@ -1455,8 +1466,7 @@ void WebPageProxy::frameDidBecomeFrameSet(uint64_t frameID, bool value)
 }
 
 // PolicyClient
-
-void WebPageProxy::decidePolicyForNavigationAction(uint64_t frameID, uint32_t opaqueNavigationType, uint32_t opaqueModifiers, int32_t opaqueMouseButton, const String& url, uint64_t listenerID)
+void WebPageProxy::decidePolicyForNavigationAction(uint64_t frameID, uint32_t opaqueNavigationType, uint32_t opaqueModifiers, int32_t opaqueMouseButton, const String& url, uint64_t listenerID, bool& receivedPolicyAction, uint64_t& policyAction)
 {
     if (url != pendingAPIRequestURL())
         clearPendingAPIRequestURL();
@@ -1469,8 +1479,22 @@ void WebPageProxy::decidePolicyForNavigationAction(uint64_t frameID, uint32_t op
     WebMouseEvent::Button mouseButton = static_cast<WebMouseEvent::Button>(opaqueMouseButton);
     
     RefPtr<WebFramePolicyListenerProxy> listener = frame->setUpPolicyListenerProxy(listenerID);
+
+    ASSERT(!m_inDecidePolicyForNavigationAction);
+
+    m_inDecidePolicyForNavigationAction = true;
+    m_syncNavigationActionPolicyActionIsValid = false;
+    
     if (!m_policyClient.decidePolicyForNavigationAction(this, navigationType, modifiers, mouseButton, url, frame, listener.get()))
         listener->use();
+
+    m_inDecidePolicyForNavigationAction = false;
+
+    // Check if we received a policy decision already. If we did, we can just pass it back.
+    if (m_syncNavigationActionPolicyActionIsValid) {
+        receivedPolicyAction = true;
+        policyAction = m_syncNavigationActionPolicyAction;
+    }
 }
 
 void WebPageProxy::decidePolicyForNewWindowAction(uint64_t frameID, uint32_t opaqueNavigationType, uint32_t opaqueModifiers, int32_t opaqueMouseButton, const String& url, uint64_t listenerID)
index bce079529ce151cee1e4e56cd637454dcdccfa77..41e73d00b9c02cf10d84299bded5a9e69cd41e0c 100644 (file)
@@ -424,7 +424,7 @@ private:
     void didFinishProgress();
     void didReceiveAccessibilityPageToken(const CoreIPC::DataReference&);
     
-    void decidePolicyForNavigationAction(uint64_t frameID, uint32_t navigationType, uint32_t modifiers, int32_t mouseButton, const String& url, uint64_t listenerID);
+    void decidePolicyForNavigationAction(uint64_t frameID, uint32_t navigationType, uint32_t modifiers, int32_t mouseButton, const String& url, uint64_t listenerID, bool& receivedPolicyAction, uint64_t& policyAction);
     void decidePolicyForNewWindowAction(uint64_t frameID, uint32_t navigationType, uint32_t modifiers, int32_t mouseButton, const String& url, uint64_t listenerID);
     void decidePolicyForMIMEType(uint64_t frameID, const String& MIMEType, const String& url, uint64_t listenerID, bool& receivedPolicyAction, uint64_t& policyAction, uint64_t& downloadID);
 
@@ -642,6 +642,10 @@ private:
     WebCore::PolicyAction m_syncMimeTypePolicyAction;
     uint64_t m_syncMimeTypePolicyDownloadID;
 
+    bool m_inDecidePolicyForNavigationAction;
+    bool m_syncNavigationActionPolicyActionIsValid;
+    WebCore::PolicyAction m_syncNavigationActionPolicyAction;
+    
     Deque<NativeWebKeyboardEvent> m_keyEventQueue;
     bool m_processingWheelEvent;
     OwnPtr<WebWheelEvent> m_nextWheelEvent;
index f640383c3065fd1922d0c95e83393a9051afcfbd..ee87acafedfe25d702703e60e428848570fd7ccf 100644 (file)
@@ -67,7 +67,7 @@ messages -> WebPageProxy {
 
     # Policy messages
     DecidePolicyForMIMEType(uint64_t frameID, WTF::String MIMEType, WTF::String url, uint64_t listenerID) -> (bool receivedPolicyAction, uint64_t policyAction, uint64_t downloadID)
-    DecidePolicyForNavigationAction(uint64_t frameID, uint32_t navigationType, uint32_t modifiers, int32_t mouseButton, WTF::String url, uint64_t listenerID)
+    DecidePolicyForNavigationAction(uint64_t frameID, uint32_t navigationType, uint32_t modifiers, int32_t mouseButton, WTF::String url, uint64_t listenerID) -> (bool receivedPolicyAction, uint64_t policyAction)
     DecidePolicyForNewWindowAction(uint64_t frameID, uint32_t navigationType, uint32_t modifiers, int32_t mouseButton, WTF::String url, uint64_t listenerID)
 
     # Progress messages
index e733325c332c386432e4bb0e125241b0a27515b2..e3a41b8110deddb8e160af1c81593427eb18835d 100644 (file)
@@ -679,8 +679,15 @@ void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(FramePolicyFu
     uint32_t navigationType = static_cast<uint32_t>(navigationAction.type());
     uint32_t modifiers = modifiersForNavigationAction(navigationAction);
     int32_t mouseButton = mouseButtonForNavigationAction(navigationAction);
+    
+    bool receivedPolicyAction;
+    uint64_t policyAction;
+    if (!webPage->sendSync(Messages::WebPageProxy::DecidePolicyForNavigationAction(m_frame->frameID(), navigationType, modifiers, mouseButton, url, listenerID), Messages::WebPageProxy::DecidePolicyForNavigationAction::Reply(receivedPolicyAction, policyAction)))
+        return;
 
-    webPage->send(Messages::WebPageProxy::DecidePolicyForNavigationAction(m_frame->frameID(), navigationType, modifiers, mouseButton, url, listenerID));
+    // We call this synchronously because WebCore cannot gracefully handle a frame load without a synchronous navigation policy reply.
+    if (receivedPolicyAction)
+        m_frame->didReceivePolicyDecision(listenerID, static_cast<PolicyAction>(policyAction), 0);
 }
 
 void WebFrameLoaderClient::cancelPolicyCheck()