1431209 - Certain nsDisplayListBuilder regions are never cleared when display list is retained

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P2)

Description

[Miko Mynttinen]

nsDisplayListBuilder::mWindowOpaqueRegion and nsDisplayListBuilder::mWindowExcludeGlassRegion keep on accumulating regions with retained display lists, possibly causing high memory usage and unexpected behavior on Windows 7 systems.

Comment 1

[Miko Mynttinen]

Attachment: Bug 1431209 - Part 1: Add WeakFrameRegion and use it for WindowDraggingRegions

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

Comment 2

[Miko Mynttinen]

Attachment: Bug 1431209 - Part 2: Use WeakFrameRegion for mWindowExcludeGlassRegion

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

Comment 3

[Miko Mynttinen]

Attachment: Bug 1431209 - Part 3: Clear mWindowOpaqueRegion before building a display list

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

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8945497 [details]
Bug 1431209 - Part 1: Add WeakFrameRegion and use it for WindowDraggingRegions

https://reviewboard.mozilla.org/r/215644/#review221292


Static analysis found 6 defects in this patch.
 - 6 defects found by clang-tidy

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


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/painting/nsDisplayList.h:1746
(Diff revision 1)
> +    nsTArray<WeakFrame> mFrames;
> +    nsTArray<pixman_box32_t> mRects;
> +
> +    void Add(nsIFrame* aFrame, const nsRect& aRect)
> +    {
> +      mFrames.AppendElement(WeakFrame(aFrame));

Error: Variable of type 'weakframe' only valid on the heap [clang-tidy: mozilla-scope]

::: layout/painting/nsDisplayList.h:1746
(Diff revision 1)
> +    nsTArray<WeakFrame> mFrames;
> +    nsTArray<pixman_box32_t> mRects;
> +
> +    void Add(nsIFrame* aFrame, const nsRect& aRect)
> +    {
> +      mFrames.AppendElement(WeakFrame(aFrame));

Error: Variable of type 'weakframe' only valid on the heap [clang-tidy: mozilla-scope]

::: layout/painting/nsDisplayList.h:1746
(Diff revision 1)
> +    nsTArray<WeakFrame> mFrames;
> +    nsTArray<pixman_box32_t> mRects;
> +
> +    void Add(nsIFrame* aFrame, const nsRect& aRect)
> +    {
> +      mFrames.AppendElement(WeakFrame(aFrame));

Error: Variable of type 'weakframe' only valid on the heap [clang-tidy: mozilla-scope]

::: layout/painting/nsDisplayList.h:1752
(Diff revision 1)
> +      mRects.AppendElement(nsRegion::RectToBox(aRect));
> +    }
> +
> +    void Add(nsIFrame* aFrame, const mozilla::gfx::IntRect& aRect)
> +    {
> +      mFrames.AppendElement(WeakFrame(aFrame));

Error: Variable of type 'weakframe' only valid on the heap [clang-tidy: mozilla-scope]

::: layout/painting/nsDisplayList.h:1752
(Diff revision 1)
> +      mRects.AppendElement(nsRegion::RectToBox(aRect));
> +    }
> +
> +    void Add(nsIFrame* aFrame, const mozilla::gfx::IntRect& aRect)
> +    {
> +      mFrames.AppendElement(WeakFrame(aFrame));

Error: Variable of type 'weakframe' only valid on the heap [clang-tidy: mozilla-scope]

::: layout/painting/nsDisplayList.h:1752
(Diff revision 1)
> +      mRects.AppendElement(nsRegion::RectToBox(aRect));
> +    }
> +
> +    void Add(nsIFrame* aFrame, const mozilla::gfx::IntRect& aRect)
> +    {
> +      mFrames.AppendElement(WeakFrame(aFrame));

Error: Variable of type 'weakframe' only valid on the heap [clang-tidy: mozilla-scope]

Comment 5

[Miko Mynttinen]

Comment on attachment 8945497 [details]
Bug 1431209 - Part 1: Add WeakFrameRegion and use it for WindowDraggingRegions

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

Comment 6

[Miko Mynttinen]

Comment on attachment 8945498 [details]
Bug 1431209 - Part 2: Use WeakFrameRegion for mWindowExcludeGlassRegion

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

Comment 7

[Miko Mynttinen]

Comment on attachment 8945499 [details]
Bug 1431209 - Part 3: Clear mWindowOpaqueRegion before building a display list

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

Comment 8

[Miko Mynttinen]

Updated the patches to use std::vector instead of nsTArray.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8945497 [details]
Bug 1431209 - Part 1: Add WeakFrameRegion and use it for WindowDraggingRegions

https://reviewboard.mozilla.org/r/215644/#review221704

::: layout/painting/nsDisplayList.h:1737
(Diff revision 2)
>  
> +  /**
> +   * Represents a region composed of frame/rect pairs.
> +   * WeakFrames are used to track whether a rect still belongs to the region.
> +   * Modified frames and rects are removed and re-added to the region if needed.
> +   * nsDisplayLayerEventRegions::FrameRects implements the same functionality

Should we make this a template class with T instead of WeakFrame, so that event regions can use it too?

::: layout/painting/nsDisplayList.cpp:2001
(Diff revision 2)
> -      // we can remove them all at the end in one go.
> -      mWindowDraggingFrames[i] = mWindowDraggingFrames[length - 1];
> -      mWindowDraggingRects[i] = mWindowDraggingRects[length - 1];
> +    if (!frame.IsAlive() || frame->IsFrameModified()) {
> +      // To avoid O(n) shifts in the array, move the last element of the array
> +      // to the current position and decrease the array length. Moving WeakFrame
> +      // inside of the array causes a new WeakFrame to be created and registered
> +      // with PresShell. We could avoid this by, for example, using a wrapper
> +      // class for WeakFrame, or by storing raw  WeakFrame pointers.

Can't we just fix WeakFrame::Init to be smarter?

if (mFrame && aFrame && mFrame->PresShell() == aFrame->PresShell()) {
  // This WeakFrame is already registered, don't need to do anything else.
  mFrame = aFrame;
} else {
  Clear();
  mFrame = aFrame;
  shell->AddWeakFrame(this);
}

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8945498 [details]
Bug 1431209 - Part 2: Use WeakFrameRegion for mWindowExcludeGlassRegion

https://reviewboard.mozilla.org/r/215646/#review221706

Comment 11

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8945499 [details]
Bug 1431209 - Part 3: Clear mWindowOpaqueRegion before building a display list

https://reviewboard.mozilla.org/r/215648/#review221708

Comment 12

[Miko Mynttinen]

Comment on attachment 8945497 [details]
Bug 1431209 - Part 1: Add WeakFrameRegion and use it for WindowDraggingRegions

https://reviewboard.mozilla.org/r/215644/#review221704

> Should we make this a template class with T instead of WeakFrame, so that event regions can use it too?

Dropping this issue because we will hopefully get rid of nsDisplayLayerEventRegions.

> Can't we just fix WeakFrame::Init to be smarter?
> 
> if (mFrame && aFrame && mFrame->PresShell() == aFrame->PresShell()) {
>   // This WeakFrame is already registered, don't need to do anything else.
>   mFrame = aFrame;
> } else {
>   Clear();
>   mFrame = aFrame;
>   shell->AddWeakFrame(this);
> }

I'll change this in a separate bug.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e344c29015b0
Part 1: Add WeakFrameRegion and use it for WindowDraggingRegions r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/f71093a719c9
Part 2: Use WeakFrameRegion for mWindowExcludeGlassRegion r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/334052f09a38
Part 3: Clear mWindowOpaqueRegion before building a display list r=mattwoodrow

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/e344c29015b0
https://hg.mozilla.org/mozilla-central/rev/f71093a719c9
https://hg.mozilla.org/mozilla-central/rev/334052f09a38