Closed Bug 1420298 Opened 7 years ago Closed 7 years ago

Implement retained-vs-non-retained display list checker

Categories

(Core :: Web Painting, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: mozbugz, Assigned: mozbugz)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

"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.
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 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.
Attachment #8931534 - Flags: review?(matt.woodrow) → review+
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 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;
Pushed by gsquelart@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2948003d230a 'layout.display-list.retain.verify' to debug retained-dl - r=mattwoodrow
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1467514
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: