1826517 - Simplify the devtools walker implementation

Status: RESOLVED FIXED

(DevTools :: Inspector, task)

Description

[Emilio Cobos Álvarez (:emilio)]

It can be much simpler than what it is.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1826517 - Simplify DevTools' walker. r=#devtools-reviewers

The tree devtools uses is the light dom + pseudo-elements + NAC, but sometimes
it wants to know stuff about the flat tree like assigned nodes. Previously it
was using a weird mix of the anonymous vs. non-anonymous walkers to get what it
wants, but that's needlessly complicated.

Instead, make InspectorUtils.getChildrenForNode do the right thing, and add
assigned nodes explicitly.

While _getChildren using a walker might seem like a good idea for performance,
realistically it was using InspectorUtils under the hood, and this is much
simpler.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Julian, so I'm down to a single test failure and I'm a bit confused about how my patch can cause it, is there any chance you could help out?

The test is devtools/client/inspector/test/browser_inspector_highlighter-iframes_01.js, and the issue is that the breadcrumb count is 3 instead of 9 (so we don't go "past" the document further up).

However the parts of the code I touched are doing the right thing it seems. The test is fixed if I comment out this return:

https://searchfox.org/mozilla-central/rev/55d5c4b9dffe5e59eb6b019c1a930ec9ada47e10/devtools/server/actors/inspector/walker.js#845

But that seems like a bug (plus I also see the issue that the markup view doesn't expand the root iframe).

Does this ring a bell? Do you know where should I look? Off-hand the behavior of the code I'm touching seems correct so I'm a bit baffled / confused about the behavior change. :/

I can keep looking, but reaching out in case this rings a bell to you or what not.

Comment 3

[Julian Descottes [:jdescottes]]

Not clearing the needinfo, but I'm looking at this now. The symptoms seem similar to issues we had in the past with shadow roots.

The client and server for the inspector need to always be in sync about which nodes have been communicated to the client or not, and if at any point the server misses to update the client about a part of the tree, it can lead to such issues. It's fragile and painful to maintain. The return you mention makes me think this could be related, ie we created the actor for a given node earlier but for some reason it was not communicated to the client.

Edit: Ok I can see that we create duplicate actors for the same node, probably something to investigate there.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Ah, I found it. rawParentNode was getting scoped to the document node before my patch but not after.

Comment 5

[Julian Descottes [:jdescottes]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

Ah, I found it. rawParentNode was getting scoped to the document node before my patch but not after.

Great, that sounds right! I could see that iframe node actors were created twice. Once for the target corresponding to the ownerDocument of the <iframe>, and once for the target of its contentDocument. But I didn't make it much further than this :)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c8535d8700e
Simplify DevTools' walker. r=smaug,devtools-reviewers,nchevobbe

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/81b06b888aca
Follow-up: fix tests with EFT disabled.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

I had to push comment 7 to fix tests with fission off and devtools.every-frame-target.enabled=false. The fix was trivial but I wonder, do we still care about that set-up or should the code be removed?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac865a674e98
Trivially fix macOS specific mochitest.

Comment 10

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/8c8535d8700e
https://hg.mozilla.org/mozilla-central/rev/81b06b888aca
https://hg.mozilla.org/mozilla-central/rev/ac865a674e98

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)

I had to push comment 7 to fix tests with fission off and devtools.every-frame-target.enabled=false. The fix was trivial but I wonder, do we still care about that set-up or should the code be removed?

we do still care about it because some configuration still don't use EFT , and as there's no way to test those directly, having this variant acts as a safe guard.
We reviewed this a few months ago, and we plan to re-evaluate the need for this later, but I don't think much changed since then

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Seems it's enabled everywhere here? I guess not in Thunderbird?

Comment 13

[Julian Descottes [:jdescottes]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)

Seems it's enabled everywhere here? I guess not in Thunderbird?

It's a bit more tricky than that. Most of our tests use DevTools opened against a webpage, which as you said always uses EFT in most platform. But the reason we want to keep this is we have other toolboxes which cannot use EFT today:

• Browser Toolbox
• WebExtension Toolbox
• etc...
  Regardless of the value for devtools.every-frame-target.enabled

So running the tests in non-eft gives us some confidence that we are not fully breaking those toolboxes.

Comment 14

[Julian Descottes [:jdescottes]]

== Change summary for alert #38070 (as of Wed, 12 Apr 2023 22:55:43 GMT) ==

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
13%	damp complicated.styleeditor.close.DAMP	windows10-64-shippable-qr	e10s fission stylo webrender-sw	10.85 -> 12.26

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
16%	damp inspector.mutations	windows10-64-shippable-qr	e10s fission stylo webrender-sw	764.27 -> 641.98
16%	damp inspector.mutations	windows10-64-shippable-qr	e10s fission stylo webrender	764.59 -> 642.53
12%	damp inspector.mutations	linux1804-64-shippable-qr	e10s fission stylo webrender	976.65 -> 860.18
12%	damp inspector.mutations	linux1804-64-shippable-qr	e10s fission stylo webrender-sw	973.49 -> 860.67
12%	damp custom.inspector.expandall.manychildren	linux1804-64-shippable-qr	e10s fission stylo webrender-sw	130.46 -> 115.34
...	...	...	...	...
2%	damp complicated.inspector.open.DAMP	windows10-64-shippable-qr	e10s fission stylo webrender-sw	375.86 -> 368.13

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=38070