1420298 - Implement retained-vs-non-retained display list checker

Status: RESOLVED FIXED

(Core :: Web Painting, enhancement)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

"layout.display-list.build-twice" runs a non-retained display list builder after a retained one.

It would be useful to compare their results, to hopefully highlight bugs in the retained builder.

I will add "layout.display-list.retain.verify" to do that, and output the differing display lists.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1420298 'layout.display-list.retain.verify' to debug retained-dl -

Setting the 'layout.display-list.retain.verify' gfxPrefs implies
'layout.display-list.build-twice', and then compares the retained-built tree
to the non-retained one, and outputs differences&trees to the terminal.

Review commit: https://reviewboard.mozilla.org/r/202656/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202656/

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Sorry for the big-ish patch, but it's mostly new interconnected code. But if you'd prefer me to break it in more easily-reviewable pieces, just ask!

Try with layout.display-list.build-twice=true:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be90790c5378d45b1ab7f9c78ec23b663c449eb9
(Pick almost any mochitest log, and look for "****" to find checker results.)

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8931534 [details]
Bug 1420298 'layout.display-list.retain.verify' to debug retained-dl -

https://reviewboard.mozilla.org/r/202656/#review207986

Looks great, especially since we know it already found real bugs.

Do you think it's worth adding an NS_ASSERTION for when it fails? That won't crash, but it will cause test failures, so it'll be easier to find where we have problems. Depends on the rate of false positives I guess.

It's also a lot of new code into nsLayoutUtils. Think you could move the new class into nsLayoutDebugger (layout/base, but probably should be layout/painting), or a new file in layout/painting?

r=me with that.

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8931534 [details]
Bug 1420298 'layout.display-list.retain.verify' to debug retained-dl -

https://reviewboard.mozilla.org/r/202656/#review207986

Thank you for the quick review.

Re: NS_ASSERTION,
- The check is disabled by default (it's expensive!), so the assertion wouldn't appear unless we added tests/builds with the pref turned on;
- If enabled, I don't think it would be useful now or even acceptable, because of bug 1418840 which gets triggered during most mochitest runs (when changing sub-doc between each test). So we should wait until that one is fixed, and any other pending ones if any, to make sure we start from an all-green state.

Ok I will move the code elsewhere.

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8931534 [details]
Bug 1420298 'layout.display-list.retain.verify' to debug retained-dl -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202656/diff/1-2/

Comment 6

[Code Review Bot [:reviewbot]]

Comment on attachment 8931534 [details]
Bug 1420298 'layout.display-list.retain.verify' to debug retained-dl -

https://reviewboard.mozilla.org/r/202656/#review208324


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: layout/painting/DisplayListChecker.cpp:341
(Diff revision 2)
> +DisplayListChecker::DisplayListChecker(nsDisplayList* aList, const char* aName)
> +  : mBlueprint(MakeUnique<DisplayListBlueprint>(aList, aName))
> +{
> +}
> +
> +DisplayListChecker::~DisplayListChecker() {}

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

DisplayListChecker::~DisplayListChecker() {}
                    ^
                                          = default;

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8931534 [details]
Bug 1420298 'layout.display-list.retain.verify' to debug retained-dl -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202656/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2948003d230a
'layout.display-list.retain.verify' to debug retained-dl - r=mattwoodrow

Comment 9

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/2948003d230a