REGRESSION (r203385): Frequent RELEASE_ASSERT in WebKit::RemoteLayerTreeDrawingArea...

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

Reviewed by Simon Fraser.

* UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h:
* UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:
(WebKit::RemoteLayerTreeDrawingAreaProxy::commitLayerTree):
(WebKit::RemoteLayerTreeDrawingAreaProxy::didRefreshDisplay):
(WebKit::RemoteLayerTreeDrawingAreaProxy::waitForDidUpdateViewState):
If the UI process sends a didUpdate message while the Web process is in
the middle of flushing on a background thread, the drawing area will
allow another commit to start on the main thread, which then (rightfully)
causes the RELEASE_ASSERT.

This is normally not a problem, because didRefreshDisplay (which sends the didUpdate)
bails if m_didUpdateMessageState is anything other than NotSent, and m_didUpdateMessageState
is only NotSent if the Web process has sent a commit (and thus will not commit again until
it gets a didUpdate). This is the fundamental mechanism that avoids multiple commits being
in flight at once.

In r203385, I added a path where didRefreshDisplay could be called
before the first commit arrived (by way of
_applicationWillEnterForeground -> viewStateDidChange -> waitForDidUpdateViewState).

This caused trouble because m_didUpdateMessageState is initialized to NotSent,
which means that we could end up sending a didUpdate immediately, before the first
commit arrives - even worse, while the first commit is being flushed on a background thread,
leading the aforementioned RELEASE_ASSERT to fire.

Instead, initialize it to Sent (which I've renamed to DoesNotNeedDidUpdate), so that
we won't send a didUpdate until after the first commit arrives (at which point
the two processes are in agreement about the order of things).

It's not currently possible to API test this for multiple reasons, though it is fairly
easy to write a test app that reproduces reliably (by simulating suspend/resume notifications
inside the didFinishNavigation: callback).

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

diff --git a/Source/WebKit2/ChangeLog b/Source/WebKit2/ChangeLog
index 9c23339..67d708c 100644 (file)
--- a/Source/WebKit2/ChangeLog
+++ b/Source/WebKit2/ChangeLog
@@ -1,3 +1,44 @@
+2016-08-02  Tim Horton  <timothy_horton@apple.com>
+
+        REGRESSION (r203385): Frequent RELEASE_ASSERT in WebKit::RemoteLayerTreeDrawingArea::flushLayers()
+        https://bugs.webkit.org/show_bug.cgi?id=160481
+        <rdar://problem/27534205>
+
+        Reviewed by Simon Fraser.
+
+        * UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h:
+        * UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:
+        (WebKit::RemoteLayerTreeDrawingAreaProxy::commitLayerTree):
+        (WebKit::RemoteLayerTreeDrawingAreaProxy::didRefreshDisplay):
+        (WebKit::RemoteLayerTreeDrawingAreaProxy::waitForDidUpdateViewState):
+        If the UI process sends a didUpdate message while the Web process is in
+        the middle of flushing on a background thread, the drawing area will
+        allow another commit to start on the main thread, which then (rightfully)
+        causes the RELEASE_ASSERT.
+
+        This is normally not a problem, because didRefreshDisplay (which sends the didUpdate)
+        bails if m_didUpdateMessageState is anything other than NotSent, and m_didUpdateMessageState
+        is only NotSent if the Web process has sent a commit (and thus will not commit again until
+        it gets a didUpdate). This is the fundamental mechanism that avoids multiple commits being
+        in flight at once.
+
+        In r203385, I added a path where didRefreshDisplay could be called
+        before the first commit arrived (by way of
+        _applicationWillEnterForeground -> viewStateDidChange -> waitForDidUpdateViewState).
+
+        This caused trouble because m_didUpdateMessageState is initialized to NotSent,
+        which means that we could end up sending a didUpdate immediately, before the first
+        commit arrives - even worse, while the first commit is being flushed on a background thread,
+        leading the aforementioned RELEASE_ASSERT to fire.
+
+        Instead, initialize it to Sent (which I've renamed to DoesNotNeedDidUpdate), so that
+        we won't send a didUpdate until after the first commit arrives (at which point
+        the two processes are in agreement about the order of things).
+
+        It's not currently possible to API test this for multiple reasons, though it is fairly
+        easy to write a test app that reproduces reliably (by simulating suspend/resume notifications
+        inside the didFinishNavigation: callback).
+
 2016-08-02  Enrica Casucci  <enrica@apple.com>
 
         Allow building with content filtering disabled.

diff --git a/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h b/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h
index 0ede909..eb0c2c3 100644 (file)
--- a/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h
+++ b/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h
@@ -91,8 +91,8 @@ private:
 
     RemoteLayerTreeHost m_remoteLayerTreeHost;
     bool m_isWaitingForDidUpdateGeometry { false };
-    enum DidUpdateMessageState { NotSent, Sent, MissedCommit };
-    DidUpdateMessageState m_didUpdateMessageState { NotSent };
+    enum DidUpdateMessageState { DoesNotNeedDidUpdate, NeedsDidUpdate, MissedCommit };
+    DidUpdateMessageState m_didUpdateMessageState { DoesNotNeedDidUpdate };
 
     WebCore::IntSize m_lastSentSize;
 

diff --git a/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm b/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm
index 875877f..a668352 100644 (file)
--- a/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm
+++ b/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm
@@ -227,11 +227,11 @@ void RemoteLayerTreeDrawingAreaProxy::commitLayerTree(const RemoteLayerTreeTrans
     m_webPageProxy.layerTreeCommitComplete();
 
 #if PLATFORM(IOS)
-    if (std::exchange(m_didUpdateMessageState, NotSent) == MissedCommit)
+    if (std::exchange(m_didUpdateMessageState, NeedsDidUpdate) == MissedCommit)
         didRefreshDisplay(monotonicallyIncreasingTime());
     [m_displayLinkHandler schedule];
 #else
-    m_didUpdateMessageState = NotSent;
+    m_didUpdateMessageState = NeedsDidUpdate;
     didRefreshDisplay(monotonicallyIncreasingTime());
 #endif
 
@@ -399,7 +399,7 @@ void RemoteLayerTreeDrawingAreaProxy::didRefreshDisplay(double)
     if (!m_webPageProxy.isValid())
         return;
 
-    if (m_didUpdateMessageState != NotSent) {
+    if (m_didUpdateMessageState != NeedsDidUpdate) {
         m_didUpdateMessageState = MissedCommit;
 #if PLATFORM(IOS)
         [m_displayLinkHandler pause];
@@ -407,7 +407,7 @@ void RemoteLayerTreeDrawingAreaProxy::didRefreshDisplay(double)
         return;
     }
     
-    m_didUpdateMessageState = Sent;
+    m_didUpdateMessageState = DoesNotNeedDidUpdate;
 
     TraceScope tracingScope(RAFDidRefreshDisplayStart, RAFDidRefreshDisplayEnd);
 
@@ -425,7 +425,7 @@ void RemoteLayerTreeDrawingAreaProxy::waitForDidUpdateViewState()
 {
     // We must send the didUpdate message before blocking on the next commit, otherwise
     // we can be guaranteed that the next commit won't come until after the waitForAndDispatchImmediately times out.
-    if (m_didUpdateMessageState != Sent)
+    if (m_didUpdateMessageState != DoesNotNeedDidUpdate)
         didRefreshDisplay(monotonicallyIncreasingTime());
 
     static std::chrono::milliseconds viewStateUpdateTimeout = [] {