REGRESSION (r172771): Amazon product page becomes unresponsive after swiping back...

https://bugs.webkit.org/show_bug.cgi?id=136260
<rdar://problem/18107826>

Reviewed by Dan Bernstein.

Previously, when a swipe ended up performing a same-document navigation,
we would never get didFinishLoadForMainFrame nor didFirstVisuallyNonEmptyLayoutForMainFrame
nor would we even get didHitRenderTreeSizeThreshold in all cases, so we would never
remove the swipe snapshot. Previous implementations removed the snapshot on
didSameDocumentNavigation for the main frame if the navigation type was Replace or Pop,
so we will match that behavior.

Also, reinstate the watchdog that starts at swipe-end which would have prevented
this bug from forever breaking the view it was associated with.

Also, defend against removing the snapshot before the swipe has finished (before
we have even caused the navigation that we're watching for the effects of).

* UIProcess/API/mac/WKView.mm:
(-[WKView _didSameDocumentNavigationForMainFrame:]):
* UIProcess/API/mac/WKViewInternal.h:
* UIProcess/PageClient.h:
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::didSameDocumentNavigationForFrame):
* UIProcess/ios/PageClientImplIOS.h:
* UIProcess/ios/PageClientImplIOS.mm:
(WebKit::PageClientImpl::didSameDocumentNavigationForMainFrame):
* UIProcess/mac/PageClientImpl.h:
* UIProcess/mac/PageClientImpl.mm:
(WebKit::PageClientImpl::didSameDocumentNavigationForMainFrame):
Plumb main-frame same-document navigation notification from WebPageProxy
to ViewGestureControllerMac via PageClient and WKView.

* UIProcess/mac/ViewGestureController.h:
* UIProcess/mac/ViewGestureControllerMac.mm:
(WebKit::ViewGestureController::ViewGestureController):
(WebKit::ViewGestureController::beginSwipeGesture):
(WebKit::ViewGestureController::endSwipeGesture):
Keep track of whether a swipe is currently occurring. We can't use
activeGestureType for this because the swipe currently remains the "active"
gesture until the snapshot is removed.

Reintroduce the old swipeWatchdogTimer (and rename the shorter timer that starts
when we get a visually non-empty layout) so that we will always remove the
snapshot after 5 seconds, even if we haven't committed the load.
This could lead to flashing back to the old content if we fail to get a single
byte for 5 seconds, but that is a rare case and should eventually get additional
special treatment (dropping the tiles until we do get content, or some such).

(WebKit::ViewGestureController::didHitRenderTreeSizeThreshold):
If a swipe is still in progress, we haven't done our navigation and thus
don't care about render tree size changes.

(WebKit::ViewGestureController::didFirstVisuallyNonEmptyLayoutForMainFrame):
If a swipe is still in progress, we haven't done our navigation and thus
don't care about layouts.

Stop the 5 second overall watchdog if we start the 3 second after-visuallyNonEmptyLayout
watchdog. This means that the snapshot could stay up for a maximum of 8 seconds
for a very, very slow load.

(WebKit::ViewGestureController::didFinishLoadForMainFrame):
If a swipe is still in progress, we haven't done our navigation and thus
don't care about loads that complete.

(WebKit::ViewGestureController::didSameDocumentNavigationForMainFrame):
Remove the swipe snapshot after painting if we do replaceState or popState.

(WebKit::ViewGestureController::removeSwipeSnapshotAfterRepaint):
If a swipe is still in progress, we shouldn't remove the snapshot yet.

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

diff --git a/Source/WebKit2/ChangeLog b/Source/WebKit2/ChangeLog
index dcb006110a43d953266f9cd044a045df1e74e88c..37b93f4f00cfb2dbbdb90d01bfbbe042c9c25b0c 100644 (file)
--- a/Source/WebKit2/ChangeLog
+++ b/Source/WebKit2/ChangeLog
@@ -1,3 +1,77 @@
+2014-08-26  Tim Horton  <timothy_horton@apple.com>
+
+        REGRESSION (r172771): Amazon product page becomes unresponsive after swiping back to it
+        https://bugs.webkit.org/show_bug.cgi?id=136260
+        <rdar://problem/18107826>
+
+        Reviewed by Dan Bernstein.
+
+        Previously, when a swipe ended up performing a same-document navigation,
+        we would never get didFinishLoadForMainFrame nor didFirstVisuallyNonEmptyLayoutForMainFrame
+        nor would we even get didHitRenderTreeSizeThreshold in all cases, so we would never
+        remove the swipe snapshot. Previous implementations removed the snapshot on
+        didSameDocumentNavigation for the main frame if the navigation type was Replace or Pop,
+        so we will match that behavior.
+
+        Also, reinstate the watchdog that starts at swipe-end which would have prevented
+        this bug from forever breaking the view it was associated with.
+
+        Also, defend against removing the snapshot before the swipe has finished (before
+        we have even caused the navigation that we're watching for the effects of).
+
+        * UIProcess/API/mac/WKView.mm:
+        (-[WKView _didSameDocumentNavigationForMainFrame:]):
+        * UIProcess/API/mac/WKViewInternal.h:
+        * UIProcess/PageClient.h:
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::didSameDocumentNavigationForFrame):
+        * UIProcess/ios/PageClientImplIOS.h:
+        * UIProcess/ios/PageClientImplIOS.mm:
+        (WebKit::PageClientImpl::didSameDocumentNavigationForMainFrame):
+        * UIProcess/mac/PageClientImpl.h:
+        * UIProcess/mac/PageClientImpl.mm:
+        (WebKit::PageClientImpl::didSameDocumentNavigationForMainFrame):
+        Plumb main-frame same-document navigation notification from WebPageProxy
+        to ViewGestureControllerMac via PageClient and WKView.
+
+        * UIProcess/mac/ViewGestureController.h:
+        * UIProcess/mac/ViewGestureControllerMac.mm:
+        (WebKit::ViewGestureController::ViewGestureController):
+        (WebKit::ViewGestureController::beginSwipeGesture):
+        (WebKit::ViewGestureController::endSwipeGesture):
+        Keep track of whether a swipe is currently occurring. We can't use
+        activeGestureType for this because the swipe currently remains the "active"
+        gesture until the snapshot is removed.
+
+        Reintroduce the old swipeWatchdogTimer (and rename the shorter timer that starts
+        when we get a visually non-empty layout) so that we will always remove the
+        snapshot after 5 seconds, even if we haven't committed the load.
+        This could lead to flashing back to the old content if we fail to get a single
+        byte for 5 seconds, but that is a rare case and should eventually get additional
+        special treatment (dropping the tiles until we do get content, or some such).
+
+        (WebKit::ViewGestureController::didHitRenderTreeSizeThreshold):
+        If a swipe is still in progress, we haven't done our navigation and thus
+        don't care about render tree size changes.
+
+        (WebKit::ViewGestureController::didFirstVisuallyNonEmptyLayoutForMainFrame):
+        If a swipe is still in progress, we haven't done our navigation and thus
+        don't care about layouts.
+
+        Stop the 5 second overall watchdog if we start the 3 second after-visuallyNonEmptyLayout
+        watchdog. This means that the snapshot could stay up for a maximum of 8 seconds
+        for a very, very slow load.
+
+        (WebKit::ViewGestureController::didFinishLoadForMainFrame):
+        If a swipe is still in progress, we haven't done our navigation and thus
+        don't care about loads that complete.
+
+        (WebKit::ViewGestureController::didSameDocumentNavigationForMainFrame):
+        Remove the swipe snapshot after painting if we do replaceState or popState.
+
+        (WebKit::ViewGestureController::removeSwipeSnapshotAfterRepaint):
+        If a swipe is still in progress, we shouldn't remove the snapshot yet.
+
 2014-08-26  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [GTK] Translations are not initialized in the UI process

diff --git a/Source/WebKit2/UIProcess/API/mac/WKView.mm b/Source/WebKit2/UIProcess/API/mac/WKView.mm
index bf317676a0a6fe531c8cdca4aa5c07992baceb8e..bbde7fe42161d20358c0e5eb9fcac7c30036058b 100644 (file)
--- a/Source/WebKit2/UIProcess/API/mac/WKView.mm
+++ b/Source/WebKit2/UIProcess/API/mac/WKView.mm
@@ -3610,6 +3610,12 @@ static NSString *pathWithUniqueFilenameForPath(NSString *path)
         _data->_gestureController->didFinishLoadForMainFrame();
 }
 
+- (void)_didSameDocumentNavigationForMainFrame:(SameDocumentNavigationType)type
+{
+    if (_data->_gestureController)
+        _data->_gestureController->didSameDocumentNavigationForMainFrame(type);
+}
+
 - (void)_removeNavigationGestureSnapshot
 {
     if (_data->_gestureController)

diff --git a/Source/WebKit2/UIProcess/API/mac/WKViewInternal.h b/Source/WebKit2/UIProcess/API/mac/WKViewInternal.h
index 840e5c0e8645b102603e92a710a94045a965c3a0..da7b613ba282d9642d6403975aaec2e73de76d85 100644 (file)
--- a/Source/WebKit2/UIProcess/API/mac/WKViewInternal.h
+++ b/Source/WebKit2/UIProcess/API/mac/WKViewInternal.h
@@ -26,6 +26,7 @@
 #import "WKViewPrivate.h"
 
 #import "PluginComplexTextInputState.h"
+#import "SameDocumentNavigationType.h"
 #import "WebFindOptions.h"
 #import <wtf/Forward.h>
 #import <wtf/RetainPtr.h>
@@ -109,6 +110,7 @@ struct WebPageConfiguration;
 
 - (void)_didFirstVisuallyNonEmptyLayoutForMainFrame;
 - (void)_didFinishLoadForMainFrame;
+- (void)_didSameDocumentNavigationForMainFrame:(WebKit::SameDocumentNavigationType)type;
 - (void)_removeNavigationGestureSnapshot;
 
 #if WK_API_ENABLED

diff --git a/Source/WebKit2/UIProcess/PageClient.h b/Source/WebKit2/UIProcess/PageClient.h
index 5438d7388b39942ab3a38fb5f02672eb6424d7ea..d781fcfc65c33af596a5eaf1dd183c52ec97b25e 100644 (file)
--- a/Source/WebKit2/UIProcess/PageClient.h
+++ b/Source/WebKit2/UIProcess/PageClient.h
@@ -305,6 +305,7 @@ public:
 
     virtual void didFirstVisuallyNonEmptyLayoutForMainFrame() = 0;
     virtual void didFinishLoadForMainFrame() = 0;
+    virtual void didSameDocumentNavigationForMainFrame(SameDocumentNavigationType) = 0;
 };
 
 } // namespace WebKit

diff --git a/Source/WebKit2/UIProcess/WebPageProxy.cpp b/Source/WebKit2/UIProcess/WebPageProxy.cpp
index 2c191491480ec691ec1c19c3493dbbcad0c50ecb..bc3703365233eda8ecf2d5030a2fa0e8b8048bc0 100644 (file)
--- a/Source/WebKit2/UIProcess/WebPageProxy.cpp
+++ b/Source/WebKit2/UIProcess/WebPageProxy.cpp
@@ -2714,14 +2714,20 @@ void WebPageProxy::didSameDocumentNavigationForFrame(uint64_t frameID, uint64_t
 
     auto transaction = m_pageLoadState.transaction();
 
-    if (frame->isMainFrame())
+    bool isMainFrame = frame->isMainFrame();
+    if (isMainFrame)
         m_pageLoadState.didSameDocumentNavigation(transaction, url);
 
     m_pageLoadState.clearPendingAPIRequestURL(transaction);
     frame->didSameDocumentNavigation(url);
 
     m_pageLoadState.commitChanges();
-    m_loaderClient->didSameDocumentNavigationForFrame(this, frame, navigationID, static_cast<SameDocumentNavigationType>(opaqueSameDocumentNavigationType), userData.get());
+
+    SameDocumentNavigationType navigationType = static_cast<SameDocumentNavigationType>(opaqueSameDocumentNavigationType);
+    m_loaderClient->didSameDocumentNavigationForFrame(this, frame, navigationID, navigationType, userData.get());
+
+    if (isMainFrame)
+        m_pageClient.didSameDocumentNavigationForMainFrame(navigationType);
 }
 
 void WebPageProxy::didReceiveTitleForFrame(uint64_t frameID, const String& title, IPC::MessageDecoder& decoder)

diff --git a/Source/WebKit2/UIProcess/ios/PageClientImplIOS.h b/Source/WebKit2/UIProcess/ios/PageClientImplIOS.h
index 09d51868e7bd3efd58473101d62ce752138ca560..136c6f9251d3585e0b7cf2716442a89ef22103f1 100644 (file)
--- a/Source/WebKit2/UIProcess/ios/PageClientImplIOS.h
+++ b/Source/WebKit2/UIProcess/ios/PageClientImplIOS.h
@@ -177,6 +177,7 @@ private:
 
     virtual void didFirstVisuallyNonEmptyLayoutForMainFrame() override;
     virtual void didFinishLoadForMainFrame() override;
+    virtual void didSameDocumentNavigationForMainFrame(SameDocumentNavigationType) override;
 
     WKContentView *m_contentView;
     WKWebView *m_webView;

diff --git a/Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm b/Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm
index a750fd6d957933131eb1430de1cd21158702dbd5..d4497ecbf24849bd59355b25a2d8c177a9c2428b 100644 (file)
--- a/Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm
+++ b/Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm
@@ -690,6 +690,10 @@ void PageClientImpl::didFinishLoadForMainFrame()
 {
 }
 
+void PageClientImpl::didSameDocumentNavigationForMainFrame(SameDocumentNavigationType)
+{
+}
+
 } // namespace WebKit
 
 #endif // PLATFORM(IOS)

diff --git a/Source/WebKit2/UIProcess/mac/PageClientImpl.h b/Source/WebKit2/UIProcess/mac/PageClientImpl.h
index c8a539645719ff5ecf734ec30e5786b9ad906d9e..75eb214b0fe6aa3b0d2881615aa6bdaca4f9521b 100644 (file)
--- a/Source/WebKit2/UIProcess/mac/PageClientImpl.h
+++ b/Source/WebKit2/UIProcess/mac/PageClientImpl.h
@@ -181,6 +181,7 @@ private:
 
     virtual void didFirstVisuallyNonEmptyLayoutForMainFrame() override;
     virtual void didFinishLoadForMainFrame() override;
+    virtual void didSameDocumentNavigationForMainFrame(SameDocumentNavigationType) override;
     virtual void removeNavigationGestureSnapshot() override;
 
     WKView *m_wkView;

diff --git a/Source/WebKit2/UIProcess/mac/PageClientImpl.mm b/Source/WebKit2/UIProcess/mac/PageClientImpl.mm
index 6fe61b38dfcb41da08f0bb2a05da37d05205d395..3c96e7eb674e8097b9bdaab5aadbd17125b5cada 100644 (file)
--- a/Source/WebKit2/UIProcess/mac/PageClientImpl.mm
+++ b/Source/WebKit2/UIProcess/mac/PageClientImpl.mm
@@ -732,6 +732,11 @@ void PageClientImpl::didFinishLoadForMainFrame()
     [m_wkView _didFinishLoadForMainFrame];
 }
 
+void PageClientImpl::didSameDocumentNavigationForMainFrame(SameDocumentNavigationType type)
+{
+    [m_wkView _didSameDocumentNavigationForMainFrame:type];
+}
+
 void PageClientImpl::removeNavigationGestureSnapshot()
 {
     [m_wkView _removeNavigationGestureSnapshot];

diff --git a/Source/WebKit2/UIProcess/mac/ViewGestureController.h b/Source/WebKit2/UIProcess/mac/ViewGestureController.h
index 2d0d2dfcef326b1a13dc449788660de0eecdec56..8435754c4b89e9f5b9c0b6baf488967f5777768b 100644 (file)
--- a/Source/WebKit2/UIProcess/mac/ViewGestureController.h
+++ b/Source/WebKit2/UIProcess/mac/ViewGestureController.h
@@ -27,6 +27,7 @@
 #define ViewGestureController_h
 
 #include "MessageReceiver.h"
+#include "SameDocumentNavigationType.h"
 #include "WeakObjCPtr.h"
 #include <WebCore/FloatRect.h>
 #include <wtf/RetainPtr.h>
@@ -107,6 +108,7 @@ public:
 
     void didFirstVisuallyNonEmptyLayoutForMainFrame();
     void didFinishLoadForMainFrame();
+    void didSameDocumentNavigationForMainFrame(SameDocumentNavigationType);
 #else
     void installSwipeHandler(UIView *gestureRecognizerView, UIView *swipingView);
     void setAlternateBackForwardListSourceView(WKWebView *);
@@ -158,6 +160,8 @@ private:
 #endif
 
 #if PLATFORM(MAC)
+    RunLoop::Timer<ViewGestureController> m_swipeWatchdogAfterFirstVisuallyNonEmptyLayoutTimer;
+
     double m_magnification;
     WebCore::FloatPoint m_magnificationOrigin;
 
@@ -190,6 +194,7 @@ private:
     bool m_swipeWaitingForVisuallyNonEmptyLayout;
     bool m_swipeWaitingForRenderTreeSizeThreshold;
     bool m_swipeWaitingForRepaint;
+    bool m_swipeInProgress;
 #else    
     UIView *m_liveSwipeView;
     RetainPtr<UIView> m_liveSwipeViewClippingView;

diff --git a/Source/WebKit2/UIProcess/mac/ViewGestureControllerMac.mm b/Source/WebKit2/UIProcess/mac/ViewGestureControllerMac.mm
index a63f3fce31bbe018763cd9a415c696525a90fe4a..6c7ecaa753d2daf0494643c7fe8ece10cadeea4a 100644 (file)
--- a/Source/WebKit2/UIProcess/mac/ViewGestureControllerMac.mm
+++ b/Source/WebKit2/UIProcess/mac/ViewGestureControllerMac.mm
@@ -77,7 +77,8 @@ static const CGFloat minimumHorizontalSwipeDistance = 15;
 static const float minimumScrollEventRatioForSwipe = 0.5;
 
 static const float swipeSnapshotRemovalRenderTreeSizeTargetFraction = 0.5;
-static const std::chrono::seconds swipeSnapshotRemovalWatchdogDuration = 3_s;
+static const std::chrono::seconds swipeSnapshotRemovalWatchdogDuration = 5_s;
+static const std::chrono::seconds swipeSnapshotRemovalWatchdogAfterFirstVisuallyNonEmptyLayoutDuration = 3_s;
 
 @interface WKSwipeCancellationTracker : NSObject {
 @private
@@ -98,6 +99,7 @@ ViewGestureController::ViewGestureController(WebPageProxy& webPageProxy)
     : m_webPageProxy(webPageProxy)
     , m_activeGestureType(ViewGestureType::None)
     , m_swipeWatchdogTimer(RunLoop::main(), this, &ViewGestureController::swipeSnapshotWatchdogTimerFired)
+    , m_swipeWatchdogAfterFirstVisuallyNonEmptyLayoutTimer(RunLoop::main(), this, &ViewGestureController::swipeSnapshotWatchdogTimerFired)
     , m_lastMagnificationGestureWasSmartMagnification(false)
     , m_visibleContentRectIsValid(false)
     , m_frameHandlesMagnificationGesture(false)
@@ -108,6 +110,7 @@ ViewGestureController::ViewGestureController(WebPageProxy& webPageProxy)
     , m_swipeWaitingForVisuallyNonEmptyLayout(false)
     , m_swipeWaitingForRenderTreeSizeThreshold(false)
     , m_swipeWaitingForRepaint(false)
+    , m_swipeInProgress(false)
 {
     m_webPageProxy.process().addMessageReceiver(Messages::ViewGestureController::messageReceiverName(), m_webPageProxy.pageID(), *this);
 }
@@ -505,6 +508,7 @@ void ViewGestureController::beginSwipeGesture(WebBackForwardListItem* targetItem
     m_webPageProxy.navigationGestureDidBegin();
 
     m_activeGestureType = ViewGestureType::Swipe;
+    m_swipeInProgress = true;
 
     CALayer *rootContentLayer = m_webPageProxy.acceleratedCompositingRootLayer();
 
@@ -627,6 +631,8 @@ void ViewGestureController::endSwipeGesture(WebBackForwardListItem* targetItem,
 
     m_swipeCancellationTracker = nullptr;
 
+    m_swipeInProgress = false;
+
     CALayer *rootLayer = m_webPageProxy.acceleratedCompositingRootLayer();
 
     [rootLayer setShadowOpacity:0];
@@ -649,11 +655,13 @@ void ViewGestureController::endSwipeGesture(WebBackForwardListItem* targetItem,
 
     m_webPageProxy.navigationGestureDidEnd(true, *targetItem);
     m_webPageProxy.goToBackForwardItem(targetItem);
+
+    m_swipeWatchdogTimer.startOneShot(swipeSnapshotRemovalWatchdogDuration.count());
 }
 
 void ViewGestureController::didHitRenderTreeSizeThreshold()
 {
-    if (m_activeGestureType != ViewGestureType::Swipe)
+    if (m_activeGestureType != ViewGestureType::Swipe || m_swipeInProgress)
         return;
 
     m_swipeWaitingForRenderTreeSizeThreshold = false;
@@ -664,20 +672,27 @@ void ViewGestureController::didHitRenderTreeSizeThreshold()
 
 void ViewGestureController::didFirstVisuallyNonEmptyLayoutForMainFrame()
 {
-    if (m_activeGestureType != ViewGestureType::Swipe)
+    if (m_activeGestureType != ViewGestureType::Swipe || m_swipeInProgress)
         return;
 
     m_swipeWaitingForVisuallyNonEmptyLayout = false;
 
     if (!m_swipeWaitingForRenderTreeSizeThreshold)
         removeSwipeSnapshotAfterRepaint();
-    else
-        m_swipeWatchdogTimer.startOneShot(swipeSnapshotRemovalWatchdogDuration.count());
+    else {
+        m_swipeWatchdogAfterFirstVisuallyNonEmptyLayoutTimer.startOneShot(swipeSnapshotRemovalWatchdogAfterFirstVisuallyNonEmptyLayoutDuration.count());
+        m_swipeWatchdogTimer.stop();
+    }
 }
 
 void ViewGestureController::didFinishLoadForMainFrame()
 {
-    if (m_activeGestureType != ViewGestureType::Swipe)
+    removeSwipeSnapshotAfterRepaint();
+}
+
+void ViewGestureController::didSameDocumentNavigationForMainFrame(SameDocumentNavigationType type)
+{
+    if (type != SameDocumentNavigationSessionStateReplace && type != SameDocumentNavigationSessionStatePop)
         return;
 
     removeSwipeSnapshotAfterRepaint();
@@ -690,7 +705,7 @@ void ViewGestureController::swipeSnapshotWatchdogTimerFired()
 
 void ViewGestureController::removeSwipeSnapshotAfterRepaint()
 {
-    if (m_activeGestureType != ViewGestureType::Swipe)
+    if (m_activeGestureType != ViewGestureType::Swipe || m_swipeInProgress)
         return;
 
     if (m_swipeWaitingForRepaint)
@@ -709,6 +724,7 @@ void ViewGestureController::removeSwipeSnapshot()
     m_swipeWaitingForRepaint = false;
 
     m_swipeWatchdogTimer.stop();
+    m_swipeWatchdogAfterFirstVisuallyNonEmptyLayoutTimer.stop();
 
     if (m_activeGestureType != ViewGestureType::Swipe)
         return;