Changeset 218149 in webkit

Timestamp:

Jun 12, 2017 3:48:11 PM (7 years ago)

Author:

Simon Fraser

Message:

[iOS WK2] Peeking on an image could result in a preview at the wrong size
​https://bugs.webkit.org/show_bug.cgi?id=173274
rdar://problem/30404627

Reviewed by Tim Horton.

There were a couple of issues with the way the page scale is communicated between the web process
and the UI process.

Generally, the page scale is computed by the web process, and sent to the UI process via
layer tree commits. UI-side scale is them communicated back to the web process via visibleContentRect
updates. When receiving a new scale, WebPage has a "scale was set by the UI process" flag to handle
the case where user interaction overrides the viewport-computed page scale. However, this flag would
get set erroneously in a couple of situations.

First, during page loading, layer flushing is suspended temporarily, so web process scale changes never
make it to the UI process. In that scenario, the UI process could send an old scale back to the web process,
setting the "scale was set by the UI process" when it really wasn't.

Secondly, web -> UI layer commit messages, and UI -> web updateVisibleContentRect messages can be in flight at the
same time, again causing a stale scale to reach the web process.

Fix this by only setting the "scale was set by the UI process" when we know the UI scale should be in sync, by comparing
commit IDs of sent and received scales.

• WebProcess/WebPage/WebPage.h:
• WebProcess/WebPage/ios/WebPageIOS.mm:

(WebKit::WebPage::scaleFromUIProcess):
(WebKit::WebPage::updateVisibleContentRects):

• WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.h:

(WebKit::RemoteLayerTreeDrawingArea::lastCommittedTransactionID):

Location:

trunk/Source/WebKit2

Files:

• ChangeLog (modified) (1 diff)
• WebProcess/WebPage/WebPage.h (modified) (1 diff)
• WebProcess/WebPage/ios/WebPageIOS.mm (modified) (5 diffs)
• WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.h (modified) (3 diffs)
• WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm (modified) (1 diff)

trunk/Source/WebKit2/ChangeLog

@@ +1 @@
+2017-06-12  Simon Fraser  <simon.fraser@apple.com>
+
+        [iOS WK2] Peeking on an image could result in a preview at the wrong size
+        https://bugs.webkit.org/show_bug.cgi?id=173274
+        rdar://problem/30404627
+
+        Reviewed by Tim Horton.
+
+        There were a couple of issues with the way the page scale is communicated between the web process
+        and the UI process.
+
+        Generally, the page scale is computed by the web process, and sent to the UI process via
+        layer tree commits. UI-side scale is them communicated back to the web process via visibleContentRect
+        updates. When receiving a new scale, WebPage has a "scale was set by the UI process" flag to handle
+        the case where user interaction overrides the viewport-computed page scale. However, this flag would
+        get set erroneously in a couple of situations.
+
+        First, during page loading, layer flushing is suspended temporarily, so web process scale changes never
+        make it to the UI process. In that scenario, the UI process could send an old scale back to the web process,
+        setting the "scale was set by the UI process" when it really wasn't.
+
+        Secondly, web -> UI layer commit messages, and UI -> web updateVisibleContentRect messages can be in flight at the
+        same time, again causing a stale scale to reach the web process.
+
+        Fix this by only setting the "scale was set by the UI process" when we know the UI scale should be in sync, by comparing
+        commit IDs of sent and received scales.
+
+        * WebProcess/WebPage/WebPage.h:
+        * WebProcess/WebPage/ios/WebPageIOS.mm:
+        (WebKit::WebPage::scaleFromUIProcess):
+        (WebKit::WebPage::updateVisibleContentRects):
+        * WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.h:
+        (WebKit::RemoteLayerTreeDrawingArea::lastCommittedTransactionID):
+
 2017-06-12  Alex Christensen  <achristensen@webkit.org>
 

trunk/Source/WebKit2/WebProcess/WebPage/WebPage.h

@@ -826 +826 @@
     void dynamicViewportSizeUpdate(const WebCore::FloatSize& minimumLayoutSize, const WebCore::FloatSize& maximumUnobscuredSize, const WebCore::FloatRect& targetExposedContentRect, const WebCore::FloatRect& targetUnobscuredRect, const WebCore::FloatRect& targetUnobscuredRectInScrollViewCoordinates, double scale, int32_t deviceOrientation, uint64_t dynamicViewportSizeUpdateID);
     void synchronizeDynamicViewportUpdate(double& newTargetScale, WebCore::FloatPoint& newScrollPosition, uint64_t& nextValidLayerTreeTransactionID);
+    std::optional<float> scaleFromUIProcess(const VisibleContentRectUpdateInfo&) const;
     void updateVisibleContentRects(const VisibleContentRectUpdateInfo&, MonotonicTime oldestTimestamp);
     bool scaleWasSetByUIProcess() const { return m_scaleWasSetByUIProcess; }

trunk/Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm

@@ -3164 +3164 @@
 }
 
+std::optional<float> WebPage::scaleFromUIProcess(const VisibleContentRectUpdateInfo& visibleContentRectUpdateInfo) const
+{
+    auto transactionIDForLastScaleSentToUIProcess = downcast<RemoteLayerTreeDrawingArea>(*m_drawingArea).lastCommittedTransactionID();
+    auto transactionIDForLastScaleFromUIProcess = visibleContentRectUpdateInfo.lastLayerTreeTransactionID();
+    if (transactionIDForLastScaleSentToUIProcess != transactionIDForLastScaleFromUIProcess)
+        return std::nullopt;
+
+    float scaleFromUIProcess = visibleContentRectUpdateInfo.scale();
+    float currentScale = m_page->pageScaleFactor();
+
+    double scaleNoiseThreshold = 0.005;
+    if (!m_isInStableState && fabs(scaleFromUIProcess - currentScale) < scaleNoiseThreshold) {
+        // Tiny changes of scale during interactive zoom cause content to jump by one pixel, creating
+        // visual noise. We filter those useless updates.
+        scaleFromUIProcess = currentScale;
+    }
+    
+    scaleFromUIProcess = std::min<float>(m_viewportConfiguration.maximumScale(), std::max<float>(m_viewportConfiguration.minimumScale(), scaleFromUIProcess));
+    if (areEssentiallyEqualAsFloat(currentScale, scaleFromUIProcess))
+        return std::nullopt;
+
+    return scaleFromUIProcess;
+}
+
 void WebPage::updateVisibleContentRects(const VisibleContentRectUpdateInfo& visibleContentRectUpdateInfo, MonotonicTime oldestTimestamp)
 {
@@ -3175 +3199 @@
     m_isInStableState = visibleContentRectUpdateInfo.inStableState();
 
-    double scaleNoiseThreshold = 0.005;
-    float filteredScale = visibleContentRectUpdateInfo.scale();
-    float currentScale = m_page->pageScaleFactor();
-
-    if (!m_isInStableState && fabs(filteredScale - currentScale) < scaleNoiseThreshold) {
-        // Tiny changes of scale during interactive zoom cause content to jump by one pixel, creating
-        // visual noise. We filter those useless updates.
-        filteredScale = currentScale;
-    }
-
-    float boundedScale = std::min<float>(m_viewportConfiguration.maximumScale(), std::max<float>(m_viewportConfiguration.minimumScale(), filteredScale));
+    auto scaleFromUIProcess = this->scaleFromUIProcess(visibleContentRectUpdateInfo);
 
     // Skip progressively redrawing tiles if pinch-zooming while the system is under memory pressure.
-    if (boundedScale != currentScale && !m_isInStableState && MemoryPressureHandler::singleton().isUnderMemoryPressure())
+    if (scaleFromUIProcess && !m_isInStableState && MemoryPressureHandler::singleton().isUnderMemoryPressure())
         return;
 
@@ -3198 +3212 @@
     }
 
+    float scaleToUse = scaleFromUIProcess.value_or(m_page->pageScaleFactor());
     FloatRect exposedContentRect = visibleContentRectUpdateInfo.exposedContentRect();
-    FloatRect adjustedExposedContentRect = adjustExposedRectForBoundedScale(exposedContentRect, visibleContentRectUpdateInfo.scale(), boundedScale);
+    FloatRect adjustedExposedContentRect = adjustExposedRectForBoundedScale(exposedContentRect, visibleContentRectUpdateInfo.scale(), scaleToUse);
     m_drawingArea->setExposedContentRect(adjustedExposedContentRect);
 
@@ -3205 +3220 @@
 
     bool hasSetPageScale = false;
-    if (boundedScale != currentScale) {
+    if (scaleFromUIProcess) {
         m_scaleWasSetByUIProcess = true;
         m_hasStablePageScaleFactor = m_isInStableState;
@@ -3211 +3226 @@
         m_dynamicSizeUpdateHistory.clear();
 
-        m_page->setPageScaleFactor(boundedScale, scrollPosition, m_isInStableState);
+        m_page->setPageScaleFactor(scaleFromUIProcess.value(), scrollPosition, m_isInStableState);
         hasSetPageScale = true;
-        send(Messages::WebPageProxy::PageScaleFactorDidChange(boundedScale));
-    }
-
+        send(Messages::WebPageProxy::PageScaleFactorDidChange(scaleFromUIProcess.value()));
+    }
+    
     if (!hasSetPageScale && m_isInStableState) {
-        m_page->setPageScaleFactor(boundedScale, scrollPosition, true);
+        m_page->setPageScaleFactor(scaleToUse, scrollPosition, true);
         hasSetPageScale = true;
     }

trunk/Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.h

@@ -52 +52 @@
 
     uint64_t nextTransactionID() const { return m_currentTransactionID + 1; }
+    uint64_t lastCommittedTransactionID() const { return m_currentTransactionID; }
 
     WeakPtr<RemoteLayerTreeDrawingArea> createWeakPtr() { return m_weakPtrFactory.createWeakPtr(); }
-    
+
 private:
     // DrawingArea
@@ -146 +147 @@
 
     WebCore::Timer m_layerFlushTimer;
-    bool m_isFlushingSuspended;
-    bool m_hasDeferredFlush;
-    bool m_isThrottlingLayerFlushes;
-    bool m_isLayerFlushThrottlingTemporarilyDisabledForInteraction;
-    bool m_isInitialThrottledLayerFlush;
+    bool m_isFlushingSuspended { false };
+    bool m_hasDeferredFlush { false };
+    bool m_isThrottlingLayerFlushes { false };
+    bool m_isLayerFlushThrottlingTemporarilyDisabledForInteraction { false };
+    bool m_isInitialThrottledLayerFlush { false };
 
-    bool m_waitingForBackingStoreSwap;
-    bool m_hadFlushDeferredWhileWaitingForBackingStoreSwap;
+    bool m_waitingForBackingStoreSwap { false };
+    bool m_hadFlushDeferredWhileWaitingForBackingStoreSwap { false };
     bool m_nextFlushIsForImmediatePaint { false };
 
@@ -160 +161 @@
 
     HashSet<RemoteLayerTreeDisplayRefreshMonitor*> m_displayRefreshMonitors;
-    HashSet<RemoteLayerTreeDisplayRefreshMonitor*>* m_displayRefreshMonitorsToNotify;
+    HashSet<RemoteLayerTreeDisplayRefreshMonitor*>* m_displayRefreshMonitorsToNotify { nullptr };
 
-    uint64_t m_currentTransactionID;
+    uint64_t m_currentTransactionID { 0 };
     Vector<RemoteLayerTreeTransaction::TransactionCallbackID> m_pendingCallbackIDs;
 
     WebCore::LayoutMilestones m_pendingNewlyReachedLayoutMilestones { 0 };
 
-    WebCore::GraphicsLayer* m_contentLayer;
-    WebCore::GraphicsLayer* m_viewOverlayRootLayer;
+    WebCore::GraphicsLayer* m_contentLayer { nullptr };
+    WebCore::GraphicsLayer* m_viewOverlayRootLayer { nullptr };
     
     WeakPtrFactory<RemoteLayerTreeDrawingArea> m_weakPtrFactory;

trunk/Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm

@@ -61 +61 @@
     , m_rootLayer(GraphicsLayer::create(graphicsLayerFactory(), *this))
     , m_layerFlushTimer(*this, &RemoteLayerTreeDrawingArea::flushLayers)
-    , m_isFlushingSuspended(false)
-    , m_hasDeferredFlush(false)
-    , m_isThrottlingLayerFlushes(false)
-    , m_isLayerFlushThrottlingTemporarilyDisabledForInteraction(false)
-    , m_isInitialThrottledLayerFlush(false)
-    , m_waitingForBackingStoreSwap(false)
-    , m_hadFlushDeferredWhileWaitingForBackingStoreSwap(false)
-    , m_displayRefreshMonitorsToNotify(nullptr)
-    , m_currentTransactionID(0)
-    , m_contentLayer(nullptr)
-    , m_viewOverlayRootLayer(nullptr)
     , m_weakPtrFactory(this)
 {