1901515 - Removing a DOM node with large text contents is slow

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[bartveneman]

Attachment: devtools-performance.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:126.0) Gecko/20100101 Firefox/126.0

Steps to reproduce:

• Render a DOM node on a page with a large string content (300kB of CSS, in my case)
• call node.remove()

Reduced test case: https://codepen.io/bartveneman/pen/RwmpGej?editors=1010

Actual results:

node.remove() takes more than 3 seconds to complete. All other browser interactions are blocked in the meantime.

Expected results:

The DOM node is removed instantly and the browser remains responsive to user input.

Comment 1

[BugBot [:suhaib / :marco]]

The Bugbug bot thinks this bug should belong to the 'Core::DOM: Core & HTML' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Mayank Bansal]

Can confirm.

Nightly: https://share.firefox.dev/3z726Is
Chrome: https://share.firefox.dev/3yOtQl5 (feels instant)

Comment 3

[Mayank Bansal]

Attachment: Reporters standalone testcase.html

Comment 4

[Mayank Bansal]

Bisection:

Bug 1865012 Part 4 - Remove PageValuesProperty copy in frame continuations. r=dholbert

It was added in bug 1804772. After Part 1, accessing FirstInFlow() is constant
time, so we don't need to duplicate PageValuesProperty in each frame
continuation.

Differential Revision: https://phabricator.services.mozilla.com/D197759

Comment 5

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1865012

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Looks similar to bug 1896875, but it's still slow on Nightly...

Comment 7

[Emilio Cobos Álvarez [:emilio]]

https://searchfox.org/mozilla-central/rev/aa9d148d5be3e7b606448f0b8da6e9f4fa43112f/layout/generic/nsBlockFrame.cpp#6920-6929 seems to be the root cause, we're deleting front to back there.

Comment 8

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

I'll take a look.

Comment 9

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Re comment 7:

nsContainerFrame::DeleteNextInFlowChild() actually collects the next-in-flows in an array, and delete them from back to front in this loop

However, in the testcase (attached in comment 3), the next-in-flows are not in the overflow container list, but in principal child list. The to-be-deleted Inline(code) has thousands of continuations, so does its child Text. The partial frame tree looks like

              Block(pre)(7)@7f744f2ee148 parent=7f744f2eda20 next=7f744f2ee210 (x=0, y=2010, w=59760, h=8686080) ink-overflow=(x=-180, y=-180, w=274269, h=8686440) scr-overflow=(x=0, y=0, w=274089, h=8686080) [content=7f744b2045e0] [cs=7f744f2e9808] <
                line@7f744f2ee598 count=1 state=inline,clean,prevmarginclean,not-impacted,not-wrapped,forced-break-after,clear-before:none,clear-after:none(x=0, y=0, w=0, h=870) <
                  Inline(code)(0)@7f744f2ee450 parent=7f744f2ee148 next=7f744f2ee6a8 next-in-flow=7f744f2ee6a8 (x=0, y=0, w=0, h=870) [content=7f744b204670] [cs=7f744f235a08] <
                    Text(0)"\n"@7f744f2ee4f8 parent=7f744f2ee450 next-in-flow=7f744f2ee5e8 (x=0, y=0, w=0, h=870) [content=7f744b205b00] [cs=7f744f235608:-moz-text] [run=7f744b1f2820][0,1,F] 
                  >
                >
                line@7f744f2ee750 count=1 state=inline,clean,prevmarginclean,not-impacted,not-wrapped,forced-break-after,clear-before:none,clear-after:none(x=0, y=870, w=7361, h=870) <
                  Inline(code)(0)@7f744f2ee6a8 parent=7f744f2ee148 next=7f744f2ee850 prev-in-flow=7f744f2ee450 next-in-flow=7f744f2ee850 (x=0, y=870, w=7361, h=870) [content=7f744b204670] [cs=7f744f235a08] <
                    Text(0)".position-unset {\n"@7f744f2ee5e8 parent=7f744f2ee6a8 prev-in-flow=7f744f2ee4f8 next-in-flow=7f744f2ee7a0 (x=0, y=0, w=7361, h=870) [content=7f744b205b00] [cs=7f744f235608:-moz-text] [run=7f744b1ab940][1,18,F] 
                  >
                >
                line@7f744f2ee8f8 count=1 state=inline,clean,prevmarginclean,not-impacted,not-wrapped,forced-break-after,clear-before:none,clear-after:none(x=0, y=1740, w=10392, h=870) <
                  Inline(code)(0)@7f744f2ee850 parent=7f744f2ee148 next=7f744f2ee9f8 prev-in-flow=7f744f2ee6a8 next-in-flow=7f744f2ee9f8 (x=0, y=1740, w=10392, h=870) [content=7f744b204670] [cs=7f744f235a08] <
                    Text(0)"\tposition: unset;\n"@7f744f2ee7a0 parent=7f744f2ee850 prev-in-flow=7f744f2ee5e8 next-in-flow=7f744f2ee948 (x=0, y=0, w=10392, h=870) [content=7f744b205b00] [cs=7f744f235608:-moz-text] [run=7f744b1aba10][19,18,F] 
                  >
                >

We are delete aDeletedFrame, i.e. Inline(code) front to back repeatedly in this loop. We are also updating the first continuation caches in both the to-be-removed inline frames and their child text frames, which is unnecessary.

nsBlockFrame::DoRemoveFrame() is quite complex as it deletes continuations while updating/removing lines. At first glance, it doesn’t seem straightforward to rewrite it to delete continuations and lines from back to front. Perhaps its time to introduce a flag in SetPrevContinuation and SetPrevNextInFlow to allow stalled cache when we are removing all continuations?

Comment 10

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1901515 Part 1 - Revise documentation in nsIFrame and nsSplittableFrame. r?#layout

Remove obsolete documentation about "width" being "inline-sizes," because we've
renamed methods to use inline size such as MarkIntrinsicISizesDirty(), GetPrefISize`, etc.

Remove public: access specifiers because the sections is already public.

In nsSplittableFrame, move the warnings to SetPrevContinuation() and
SetPrevInFlow() because we update the caches in these two methods, not in
SetNextContinuation() and SetNextInFlow().

Comment 11

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1901515 Part 2 - Use MOZ_ASSERT_UNREACHABLE() instead of MOZ_ASSERT(false). r?#layout

Comment 12

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1901515 Part 3 - Add methods to set previous continuation without updating first continuation cache. r?#layout

The correctness assumption for this patch is that we destroy all frame
continuations when removing an element, so any stale first continuation cache
during frame destruction is acceptable.

Comment 13

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1901515 Part 4 - Add a perf test that removes a child with lengthy text. r?#layout

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca817ef0bc6e
Part 1 - Revise documentation in nsIFrame and nsSplittableFrame. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/e70f430e9826
Part 2 - Use MOZ_ASSERT_UNREACHABLE() instead of MOZ_ASSERT(false). r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/8ea9b9da6922
Part 3 - Add methods to set previous continuation without updating first continuation cache. r=layout-reviewers,dholbert
https://hg.mozilla.org/integration/autoland/rev/f1a42b843f2d
Part 4 - Add a perf test that removes a child with lengthy text. r=perftest-reviewers,dholbert,sparky

Comment 15

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/ca817ef0bc6e
https://hg.mozilla.org/mozilla-central/rev/e70f430e9826
https://hg.mozilla.org/mozilla-central/rev/8ea9b9da6922
https://hg.mozilla.org/mozilla-central/rev/f1a42b843f2d

Comment 16

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:TYLin, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox128 to wontfix.

For more information, please visit BugBot documentation.

Comment 17

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Let's let this ride the train because we've shipped this regression for several releases and the patch stack is not trivial.

Comment 18

[Monica Chiorean]

I was able to reproduce the issue on Win11x64 using FF build 128.0a1(20240606154042).
Verified as fixed on Win11x64 using FF builds 129.0b8 and 130.0a1.