Skip to content

Commit

Permalink
Ignore willCommitLayerTree and commitLayerTreeNotTriggered when reord…
Browse files Browse the repository at this point in the history
…ered after a commitLayerTree

https://bugs.webkit.org/show_bug.cgi?id=259632
rdar://110498860

Reviewed by Matt Woodrow.

The normal sequence of RemoteLayerTreeDrawingAreaProxy messages should normally be:
willCommit(1) commit(1) notTriggered(next commit 2) willCommit(2) commit(2) ...

However this could be disrupted by RemoteLayerTreeDrawingAreaProxy::waitForDidUpdateActivityState, which
waits for the first commit message and runs it immediately! This could lead to handling them in this order:
willCommit(1) commit(1) **commit(2)** notTriggered(next commit 2) willCommit(2) ...
The main issue is that the notTriggered(next commit 2) handler pauses further display refresh callbacks,
which would normally have been restarted by the later-sent commit(2), so the rendering doesn't get updated
anymore. In some applications like Mail, this may result in blank email bodies.

The solution here is to detect such re-orderings, and produce the same effects as if they had run in the
expected order. Details below:

* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.messages.in:
Add `nextCommitTransactionID` to CommitLayerTreeNotTriggered, to know which commit they should
precede (and hence which commit should come after).

* Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:
(WebKit::RemoteLayerTreeDrawingArea::displayDidRefresh):
Send the next commit transaction ID with CommitLayerTreeNotTriggered.

* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.h:
Record the last processed CommitLayerTree transaction ID.

* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm:
(WebKit::RemoteLayerTreeDrawingAreaProxy::commitLayerTreeNotTriggered):
If this message's next-to-commit transaction ID is lower than or equal to the last-received committed
transaction ID, this means that the commit was reordered and handled before this not-triggered, so nothing
should be done here; especially, display-refresh callbacks should not be paused, and the scrolling update
should happen in the upcoming not-triggered that corresponds to the last-received commit.
This should be rare, so a log message is appropriate to expose this event.

(WebKit::RemoteLayerTreeDrawingAreaProxy::willCommitLayerTree):
Similar to the message above, if the will-commit transaction ID is not smaller than the last-received
committed transaction ID, it means that this message was reordered and can be ignored. Its effect would
have already been handled by the commit message (see below.)

(WebKit::RemoteLayerTreeDrawingAreaProxy::commitLayerTreeTransaction):
Update the last processed transaction ID.
Also, if the pending transaction ID is lower than this commit's transaction ID, it means this message is
being preemptively handled sooner, so the will-commit effect is simulated by updating the pending
transaction ID now -- the already sent will-commit will be ignored later on.

Canonical link: https://commits.webkit.org/266421@main
  • Loading branch information
squelart committed Jul 31, 2023
1 parent e50b54c commit 3fe5ba4
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ class RemoteLayerTreeDrawingAreaProxy : public DrawingAreaProxy {
// Message handlers
virtual void setPreferredFramesPerSecond(WebCore::FramesPerSecond) { }
void willCommitLayerTree(TransactionID);
void commitLayerTreeNotTriggered();
void commitLayerTreeNotTriggered(TransactionID);
void commitLayerTree(IPC::Connection&, const Vector<std::pair<RemoteLayerTreeTransaction, RemoteScrollingCoordinatorTransaction>>&);
void commitLayerTreeTransaction(IPC::Connection&, const RemoteLayerTreeTransaction&, const RemoteScrollingCoordinatorTransaction&);
virtual void didCommitLayerTree(IPC::Connection&, const RemoteLayerTreeTransaction&, const RemoteScrollingCoordinatorTransaction&) { }
Expand Down Expand Up @@ -141,6 +141,7 @@ class RemoteLayerTreeDrawingAreaProxy : public DrawingAreaProxy {
RetainPtr<CALayer> m_exposedRectIndicatorLayer;

TransactionID m_pendingLayerTreeTransactionID;
TransactionID m_lastLayerTreeTransactionID;
#if ASSERT_ENABLED
TransactionID m_lastVisibleTransactionID;
#endif
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
messages -> RemoteLayerTreeDrawingAreaProxy : DrawingAreaProxy NotRefCounted {
void SetPreferredFramesPerSecond(unsigned preferredFramesPerSecond)
void WillCommitLayerTree(WebKit::TransactionID transactionID)
void CommitLayerTreeNotTriggered()
void CommitLayerTreeNotTriggered(WebKit::TransactionID nextCommitTransactionID)
void CommitLayerTree(Vector<std::pair<WebKit::RemoteLayerTreeTransaction, WebKit::RemoteScrollingCoordinatorTransaction>> transactions)
void AsyncSetLayerContents(WebCore::PlatformLayerIdentifier layer, WebKit::ImageBufferBackendHandle handle, WebCore::RenderingResourceIdentifier identifier)
}
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,13 @@
}, m_identifier);
}

void RemoteLayerTreeDrawingAreaProxy::commitLayerTreeNotTriggered()
void RemoteLayerTreeDrawingAreaProxy::commitLayerTreeNotTriggered(TransactionID nextCommitTransactionID)
{
if (nextCommitTransactionID <= m_lastLayerTreeTransactionID) {
LOG_WITH_STREAM(RemoteLayerTree, stream << "RemoteLayerTreeDrawingAreaProxy::commitLayerTreeNotTriggered nextCommitTransactionID=" << nextCommitTransactionID << ") already obsoleted by m_lastLayerTreeTransactionID=" << m_lastLayerTreeTransactionID);
return;
}

m_commitLayerTreeMessageState = Idle;
pauseDisplayRefreshCallbacks();
#if ENABLE(ASYNC_SCROLLING)
Expand All @@ -148,6 +153,9 @@

void RemoteLayerTreeDrawingAreaProxy::willCommitLayerTree(TransactionID transactionID)
{
if (transactionID <= m_lastLayerTreeTransactionID)
return;

m_pendingLayerTreeTransactionID = transactionID;
}

Expand Down Expand Up @@ -181,6 +189,10 @@
LOG_WITH_STREAM(RemoteLayerTree, stream << "RemoteLayerTreeDrawingAreaProxy::commitLayerTree transaction:" << layerTreeTransaction.description());
LOG_WITH_STREAM(RemoteLayerTree, stream << "RemoteLayerTreeDrawingAreaProxy::commitLayerTree scrolling tree:" << scrollingTreeTransaction.description());

m_lastLayerTreeTransactionID = layerTreeTransaction.transactionID();
if (m_pendingLayerTreeTransactionID < m_lastLayerTreeTransactionID)
m_pendingLayerTreeTransactionID = m_lastLayerTreeTransactionID;

bool didUpdateEditorState { false };
if (layerTreeTransaction.isMainFrameProcessTransaction()) {
ASSERT(layerTreeTransaction.transactionID() == m_lastVisibleTransactionID.next());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@
} else if (wasWaitingForBackingStoreSwap && m_updateRenderingTimer.isActive())
m_deferredRenderingUpdateWhileWaitingForBackingStoreSwap = true;
else
send(Messages::RemoteLayerTreeDrawingAreaProxy::CommitLayerTreeNotTriggered());
send(Messages::RemoteLayerTreeDrawingAreaProxy::CommitLayerTreeNotTriggered(nextTransactionID()));
}

auto RemoteLayerTreeDrawingArea::rootLayerInfoWithFrameIdentifier(WebCore::FrameIdentifier frameID) -> RootLayerInfo*
Expand Down

0 comments on commit 3fe5ba4

Please sign in to comment.