Skip to content

Commit

Permalink
Potential crash when updating Interaction Regions layers
Browse files Browse the repository at this point in the history
<rdar://117358144>

Reviewed by David Kilzer.

When appending at the end of a sublayers array, we shouldn't look up
the `objectAtIndex` at this (out of bounds) position.

* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeInteractionRegionLayers.mm:
(WebKit::updateLayersForInteractionRegions):
Add a comment and an assertion about the `insertionPoint` range.
Check for the appending case and skip the objectAtIndex lookup.
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeNode.mm:
(WebKit::RemoteLayerTreeNode::repositionInteractionRegionsContainerIfNeeded):
Add a comment and an assertion about the `insertionPoint` range.
Check for the appending case and skip the objectAtIndex lookup.

* LayoutTests/interaction-region/layer-tree.html:
Make the test a bit more readable and make sure we exercise the layer
reuse, layer move and layer append code paths.
No expectations change needed.

Originally-landed-as: 272448.238@safari-7618-branch (a2d409b). rdar://124556170
Canonical link: https://commits.webkit.org/276602@main
  • Loading branch information
etiennesegonzac authored and JonWBedard committed Mar 23, 2024
1 parent 123b9c7 commit cbebffc
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 9 deletions.
9 changes: 6 additions & 3 deletions LayoutTests/interaction-region/layer-tree.html
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,16 @@
if (!window.internals)
return;

// Resizing - reuses the layer while maintaining its position in the sublayers array.
await UIHelper.animationFrame();
document.querySelector("iframe").srcdoc = "<div style='cursor:pointer;width:100%;height:200px'; onclick='click()'>tappable</div>";
document.getElementById("resized").style.backgroundColor = "blue";
document.getElementById("resized").style.width = "200px";

// Adding - appends a new layer.
await UIHelper.animationFrame();
document.getElementById("resized").style.width = "200px";
document.querySelector("iframe").srcdoc = "<div style='cursor:pointer;width:100%;height:200px'; onclick='click()'>tappable</div>";
document.getElementById("resized").style.backgroundColor = "blue";

// Removing - moves layers arround.
await UIHelper.animationFrame();
document.getElementById("to-hide").style.display = "none";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,10 @@ void updateLayersForInteractionRegions(RemoteLayerTreeNode& node)
if (applyBackgroundColorForDebugging)
applyBackgroundColorForDebuggingToLayer(regionLayer.get(), region);

if ([container.sublayers objectAtIndex:insertionPoint] != regionLayer) {
// Since we insert new layers as we go, insertionPoint is always <= container.sublayers.count.
ASSERT(insertionPoint <= container.sublayers.count);
bool shouldAppendLayer = insertionPoint == container.sublayers.count;
if (shouldAppendLayer || [container.sublayers objectAtIndex:insertionPoint] != regionLayer) {
[regionLayer removeFromSuperlayer];
[container insertSublayer:regionLayer.get() atIndex:insertionPoint];
}
Expand Down
12 changes: 7 additions & 5 deletions Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeNode.mm
Original file line number Diff line number Diff line change
Expand Up @@ -188,11 +188,13 @@
insertionPoint++;
}

if ([layer().sublayers objectAtIndex:insertionPoint] == m_interactionRegionsContainer)
return;

[m_interactionRegionsContainer removeFromSuperlayer];
[layer() insertSublayer:m_interactionRegionsContainer.get() atIndex:insertionPoint];
// We searched through the sublayers, so insertionPoint is always <= sublayers.count.
ASSERT(insertionPoint <= layer().sublayers.count);
bool shouldAppendLayer = insertionPoint == layer().sublayers.count;
if (shouldAppendLayer || [layer().sublayers objectAtIndex:insertionPoint] != m_interactionRegionsContainer) {
[m_interactionRegionsContainer removeFromSuperlayer];
[layer() insertSublayer:m_interactionRegionsContainer.get() atIndex:insertionPoint];
}
}

void RemoteLayerTreeNode::propagateInteractionRegionsChangeInHierarchy(InteractionRegionsInSubtree interactionRegionsInSubtree)
Expand Down

0 comments on commit cbebffc

Please sign in to comment.