<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[204057] trunk/Source/WebKit2</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/204057">204057</a></dd>
<dt>Author</dt> <dd>timothy_horton@apple.com</dd>
<dt>Date</dt> <dd>2016-08-02 18:22:25 -0700 (Tue, 02 Aug 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>REGRESSION (<a href="http://trac.webkit.org/projects/webkit/changeset/203385">r203385</a>): 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 <a href="http://trac.webkit.org/projects/webkit/changeset/203385">r203385</a>, 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).</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWebKit2ChangeLog">trunk/Source/WebKit2/ChangeLog</a></li>
<li><a href="#trunkSourceWebKit2UIProcessmacRemoteLayerTreeDrawingAreaProxyh">trunk/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h</a></li>
<li><a href="#trunkSourceWebKit2UIProcessmacRemoteLayerTreeDrawingAreaProxymm">trunk/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWebKit2ChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit2/ChangeLog (204056 => 204057)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit2/ChangeLog 2016-08-03 01:14:17 UTC (rev 204056)
+++ trunk/Source/WebKit2/ChangeLog 2016-08-03 01:22:25 UTC (rev 204057)
</span><span class="lines">@@ -1,3 +1,44 @@
</span><ins>+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).
+
</ins><span class="cx"> 2016-08-02 Enrica Casucci <enrica@apple.com>
</span><span class="cx">
</span><span class="cx"> Allow building with content filtering disabled.
</span></span></pre></div>
<a id="trunkSourceWebKit2UIProcessmacRemoteLayerTreeDrawingAreaProxyh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h (204056 => 204057)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h 2016-08-03 01:14:17 UTC (rev 204056)
+++ trunk/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h 2016-08-03 01:22:25 UTC (rev 204057)
</span><span class="lines">@@ -91,8 +91,8 @@
</span><span class="cx">
</span><span class="cx"> RemoteLayerTreeHost m_remoteLayerTreeHost;
</span><span class="cx"> bool m_isWaitingForDidUpdateGeometry { false };
</span><del>- enum DidUpdateMessageState { NotSent, Sent, MissedCommit };
- DidUpdateMessageState m_didUpdateMessageState { NotSent };
</del><ins>+ enum DidUpdateMessageState { DoesNotNeedDidUpdate, NeedsDidUpdate, MissedCommit };
+ DidUpdateMessageState m_didUpdateMessageState { DoesNotNeedDidUpdate };
</ins><span class="cx">
</span><span class="cx"> WebCore::IntSize m_lastSentSize;
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceWebKit2UIProcessmacRemoteLayerTreeDrawingAreaProxymm"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm (204056 => 204057)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm 2016-08-03 01:14:17 UTC (rev 204056)
+++ trunk/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm 2016-08-03 01:22:25 UTC (rev 204057)
</span><span class="lines">@@ -227,11 +227,11 @@
</span><span class="cx"> m_webPageProxy.layerTreeCommitComplete();
</span><span class="cx">
</span><span class="cx"> #if PLATFORM(IOS)
</span><del>- if (std::exchange(m_didUpdateMessageState, NotSent) == MissedCommit)
</del><ins>+ if (std::exchange(m_didUpdateMessageState, NeedsDidUpdate) == MissedCommit)
</ins><span class="cx"> didRefreshDisplay(monotonicallyIncreasingTime());
</span><span class="cx"> [m_displayLinkHandler schedule];
</span><span class="cx"> #else
</span><del>- m_didUpdateMessageState = NotSent;
</del><ins>+ m_didUpdateMessageState = NeedsDidUpdate;
</ins><span class="cx"> didRefreshDisplay(monotonicallyIncreasingTime());
</span><span class="cx"> #endif
</span><span class="cx">
</span><span class="lines">@@ -399,7 +399,7 @@
</span><span class="cx"> if (!m_webPageProxy.isValid())
</span><span class="cx"> return;
</span><span class="cx">
</span><del>- if (m_didUpdateMessageState != NotSent) {
</del><ins>+ if (m_didUpdateMessageState != NeedsDidUpdate) {
</ins><span class="cx"> m_didUpdateMessageState = MissedCommit;
</span><span class="cx"> #if PLATFORM(IOS)
</span><span class="cx"> [m_displayLinkHandler pause];
</span><span class="lines">@@ -407,7 +407,7 @@
</span><span class="cx"> return;
</span><span class="cx"> }
</span><span class="cx">
</span><del>- m_didUpdateMessageState = Sent;
</del><ins>+ m_didUpdateMessageState = DoesNotNeedDidUpdate;
</ins><span class="cx">
</span><span class="cx"> TraceScope tracingScope(RAFDidRefreshDisplayStart, RAFDidRefreshDisplayEnd);
</span><span class="cx">
</span><span class="lines">@@ -425,7 +425,7 @@
</span><span class="cx"> {
</span><span class="cx"> // We must send the didUpdate message before blocking on the next commit, otherwise
</span><span class="cx"> // we can be guaranteed that the next commit won't come until after the waitForAndDispatchImmediately times out.
</span><del>- if (m_didUpdateMessageState != Sent)
</del><ins>+ if (m_didUpdateMessageState != DoesNotNeedDidUpdate)
</ins><span class="cx"> didRefreshDisplay(monotonicallyIncreasingTime());
</span><span class="cx">
</span><span class="cx"> static std::chrono::milliseconds viewStateUpdateTimeout = [] {
</span></span></pre>
</div>
</div>
</body>
</html>