Skip to content
Permalink
Browse files
Web Inspector: Getting style data via WebCore::InspectorCSSAgent::get…
…MatchedStylesForNode for a detached node crashes

https://bugs.webkit.org/show_bug.cgi?id=240608
rdar://93473674

Reviewed by Devin Rousso.

Added test case to inspector/css/node-styles-refreshed.html

Previously it was possible under at least two different circumstances that
`WebCore::InspectorCSSAgent::getMatchedStylesForNode` could be called with the NodeId of a detached node, which means
that `computedStyle` for that Node will be `nullptr`, and calls to `Style::Scope::forNode` will fail.

The first failure state is timing based. If the Inspector frontend makes a request for updated styles for a node, the
node may already have been detached by some other means (webpage JS, for example). In this case by the time
`getMatchedStylesForNode` is invoked the `computedStyle` is no longer available.

The second failure state is easier to get into. In a DOM tree with elements A, B, and C, each a child of the
previous such that A is the parent of B and B is the parent of C, imagine C is the selected DOM node. Now if
we right click and delete A from the DOM tree, we will have removed all three elements from the tree (both the
frontend representation as well as the actual tree in the backend). Previously C remained the selected node in the
tree, which means that we continue to show the Styles sidebar panel, where the user could then attempt to edit the style
of the node. These style changes trigger the need to refresh the frontend's style information, at which point we will
call `getMatchedStylesForNode` with a detached node id.

The fix to the underlying problem is to check in the backend that a node is actually attached before determining its
style information. Additionally, we should always update the selection of a TreeOutline when the current selection is
part of the subtree being removed from the tree. This prevents the user being able to easily get into the state in the
first place as well as prevents folks from getting confused about what the open sidebars are showing information for,
since visually no node _appears_ selected.

* Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:
(WebCore::InspectorCSSAgent::getMatchedStylesForNode):
(WebCore::InspectorCSSAgent::getComputedStyleForNode):
- Guard the functions that will eventually use `computedStyle` or `Style::Scope::forNode`, both of which only work for
"connected" (in Web Inspector parlance "attached") nodes. Note there are a few other uses in our code where
`computedStyle` is used without this check, but we do explicitly check the result of computed styles in those cases,
which also works since `computedStyle` returns early if the node is not connected.

(WebCore::InspectorCSSAgent::buildObjectForRule):
- Add an assertion that we never call buildObjectForRule with a disconnected Element.

* Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:
(WI.TreeOutline.prototype.removeChildAtIndex):
- Fix selection updating to account for the fact the selected node may be part of the descendants tree of the removed
item, in which case we would still want to update the selection to something else.

* LayoutTests/inspector/css/node-styles-refreshed-expected.txt:
* LayoutTests/inspector/css/node-styles-refreshed.html:
- Add test case to try getting styles of a detached node, and update other test cases with a new helper function instead
of relying on a "global" `nodeStyles`.

Canonical link: https://commits.webkit.org/250769@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@294512 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
patrickangle committed May 19, 2022
1 parent 75993ef commit 4c354a6ba2286680e337d91f3a09ec60ee5bf5bf
Showing 4 changed files with 73 additions and 19 deletions.
@@ -14,3 +14,10 @@ PASS: Adding '.blah' class shouldn't cause a significant change.
-- Running test case: NodeStylesRefreshed.IrrelevantClassRemoved
PASS: Removing '.blah' class shouldn't cause a significant change.

-- Running test case: NodeStylesRefreshed.DisconnectedNode
Refreshing styles before removal.
PASS: Refreshing styles of an unchanged node should not be a significant change.
Removing node from DOM tree.
Refreshing styles after removal.
PASS: Refreshing styles of a now detached node should be a significant change.

@@ -2,14 +2,32 @@
<head>
<script src="../../http/tests/inspector/resources/inspector-test.js"></script>
<script>
function removeRemovableNode()
{
window.__removableNode = document.getElementById("removable-node");
window.__removableNode.remove();
}

function test()
{
let nodeStyles = null;
let suite = InspectorTest.createAsyncSuite("NodeStylesRefreshed");

async function nodeStylesForSelector(selector) {
let documentNode = await WI.domManager.requestDocument();
let nodeId = await documentNode.querySelector(selector);
let node = WI.domManager.nodeForId(nodeId);

let nodeStyles = WI.cssManager.stylesForNode(node);
await nodeStyles.refreshIfNeeded();

return nodeStyles;
}

suite.addTestCase({
name: "NodeStylesRefreshed.ClassAdded",
async test() {
let nodeStyles = await nodeStylesForSelector("body");

nodeStyles.singleFireEventListener(WI.DOMNodeStyles.Event.Refreshed, (event) => {
InspectorTest.expectTrue(event.data.significantChange, `Adding '.baz' class should cause a significant change.`);
});
@@ -22,6 +40,8 @@
suite.addTestCase({
name: "NodeStylesRefreshed.ClassRemoved",
async test() {
let nodeStyles = await nodeStylesForSelector("body");

nodeStyles.singleFireEventListener(WI.DOMNodeStyles.Event.Refreshed, (event) => {
InspectorTest.expectTrue(event.data.significantChange, `Removing '.foo' class should cause a significant change.`);
});
@@ -34,6 +54,8 @@
suite.addTestCase({
name: "NodeStylesRefreshed.IrrelevantClassAdded",
async test() {
let nodeStyles = await nodeStylesForSelector("body");

nodeStyles.singleFireEventListener(WI.DOMNodeStyles.Event.Refreshed, (event) => {
InspectorTest.expectFalse(event.data.significantChange, `Adding '.blah' class shouldn't cause a significant change.`);
});
@@ -46,6 +68,8 @@
suite.addTestCase({
name: "NodeStylesRefreshed.IrrelevantClassRemoved",
async test() {
let nodeStyles = await nodeStylesForSelector("body");

nodeStyles.singleFireEventListener(WI.DOMNodeStyles.Event.Refreshed, (event) => {
InspectorTest.expectFalse(event.data.significantChange, `Removing '.blah' class shouldn't cause a significant change.`);
});
@@ -55,26 +79,39 @@
}
});

WI.domManager.requestDocument((documentNode) => {
documentNode.querySelector("body", (contentNodeId) => {
if (contentNodeId) {
let domNode = WI.domManager.nodeForId(contentNodeId);
nodeStyles = WI.cssManager.stylesForNode(domNode);

nodeStyles.refreshIfNeeded().then(function() {
suite.runTestCasesAndFinish();
});
} else {
InspectorTest.fail("DOM node not found.");
InspectorTest.completeTest();
}
});
suite.addTestCase({
name: "NodeStylesRefreshed.DisconnectedNode",
description: "Ensure that changing the style of a disconnected node does not crash.",
async test() {
let nodeStyles = await nodeStylesForSelector("#removable-node");

nodeStyles.singleFireEventListener(WI.DOMNodeStyles.Event.Refreshed, (event) => {
InspectorTest.expectFalse(event.data.significantChange, `Refreshing styles of an unchanged node should not be a significant change.`);
});

InspectorTest.log("Refreshing styles before removal.")
await nodeStyles.refresh();

nodeStyles.singleFireEventListener(WI.DOMNodeStyles.Event.Refreshed, (event) => {
InspectorTest.expectTrue(event.data.significantChange, `Refreshing styles of a now detached node should be a significant change.`);
});

InspectorTest.log("Removing node from DOM tree.")
InspectorTest.evaluateInPage(`removeRemovableNode();`);

InspectorTest.log("Refreshing styles after removal.")
await nodeStyles.refresh();
}
});

suite.runTestCasesAndFinish();

}
</script>
</head>
<body onLoad="runTest()" class="foo bar">
<p>Testing that WI.DOMNodeStyles.Event.Refreshed event dispatches with correct significantChange flag.</p>
<div id="removable-node" class="foo bar"></div>
<style>
.foo {font-size: 12px;}
.bar {background: lightyellow;}
@@ -491,6 +491,9 @@ Protocol::ErrorStringOr<std::tuple<RefPtr<JSON::ArrayOf<Protocol::CSS::RuleMatch
if (!element)
return makeUnexpected(errorString);

if (!element->isConnected())
return makeUnexpected("Element for given nodeId was not connected to DOM tree."_s);

Element* originalElement = element;
PseudoId elementPseudoId = element->pseudoId();
if (elementPseudoId != PseudoId::None) {
@@ -570,6 +573,9 @@ Protocol::ErrorStringOr<Ref<JSON::ArrayOf<Protocol::CSS::CSSComputedStylePropert
if (!element)
return makeUnexpected(errorString);

if (!element->isConnected())
return makeUnexpected("Element for given nodeId was not connected to DOM tree."_s);

auto computedStyleInfo = CSSComputedStyleDeclaration::create(*element, true);
auto inspectorStyle = InspectorStyle::create(InspectorCSSId(), WTFMove(computedStyleInfo), nullptr);
return inspectorStyle->buildArrayForComputedStyle();
@@ -1096,6 +1102,8 @@ Protocol::CSS::StyleSheetOrigin InspectorCSSAgent::detectOrigin(CSSStyleSheet* p

RefPtr<Protocol::CSS::CSSRule> InspectorCSSAgent::buildObjectForRule(const StyleRule* styleRule, Style::Resolver& styleResolver, Element& element)
{
ASSERT(element.isConnected());

if (!styleRule)
return nullptr;

@@ -316,13 +316,15 @@ WI.TreeOutline = class TreeOutline extends WI.Object
let child = this.children[childIndex];
let parent = child.parent;

if (child.deselect(suppressOnDeselect) && !suppressSelectSibling) {
let childOrDescendantWasSelected = child.deselect(suppressOnDeselect) || child.selfOrDescendant((descendant) => descendant.selected);
if (childOrDescendantWasSelected && !suppressSelectSibling) {
const omitFocus = true;
if (child.previousSibling)
child.previousSibling.select(true, false);
child.previousSibling.select(omitFocus);
else if (child.nextSibling)
child.nextSibling.select(true, false);
child.nextSibling.select(omitFocus);
else
parent.select(true, false);
parent.select(omitFocus);
}

let treeOutline = child.treeOutline;

0 comments on commit 4c354a6

Please sign in to comment.