769021 - Leak nsTArray_base with "backface-visibility: hidden", border-radius

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

1. Run a debug build of Firefox with XPCOM_MEM_LEAK_LOG=2
2. Load the testcase.
3. Quit Firefox.

Comment 1

[Jesse Ruderman]

Attachment: leak log

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The MaskLayerImageKey allocated at http://hg.mozilla.org/mozilla-central/annotate/ee7a3bddfe5f/layout/base/FrameLayerBuilder.cpp#l3270 is leaked if we take the early return at http://hg.mozilla.org/mozilla-central/annotate/ee7a3bddfe5f/layout/base/FrameLayerBuilder.cpp#l3289, which this testcase triggers.

As part of this bug we should add MOZ_COUNT_CTOR/DTOR macros to MaskLayerImageKey and PixelRoundedRect.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Tiny leak, almost impossible to hit, not very interesting to MemShrink.

Comment 4

[Nick Cameron [:nrc]]

Attachment: fix

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 657171 [details] [diff] [review]
fix

Review of attachment 657171 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/FrameLayerBuilder.cpp
@@ +3274,2 @@
>    // copy and transform the rounded rects
> +  for (size_t i = 0; i < newData.mRoundedClipRects.Length(); ++i) {

Why did you switch from uint32_t to size_t?  nsTArray::Length returns a uint32_t.

::: layout/base/MaskLayerImageCache.h
@@ +114,5 @@
>      // Indices into mRadii are the NS_CORNER_* constants in nsStyleConsts.h
>      gfxFloat mRadii[8];
> +
> +  private:
> +    PixelRoundedRect() { NS_ERROR("Don't call me!"); }

No need to do this, you can make it a compile time error.

http://whereswalden.com/2011/11/09/introducing-moz_delete-a-macro-improving-upon-the-deliberately-unimplemented-method-idiom/

@@ +209,5 @@
>      MaskLayerImageEntry(const MaskLayerImageEntry& aOther)
>        : mKey(aOther.mKey.get())
>      {
>        NS_ERROR("ALLOW_MEMMOVE == true, should never be called");
>      }

You should use MOZ_DELETE here too.

Comment 6

[Nick Cameron [:nrc]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=4ffcb9da68f4

Comment 7

[Nick Cameron [:nrc]]

Attachment: fix

Addressed khuey's comments, carrying r+.

Thanks for the pointer to MOZ_DELETE!

Comment 8

[Nick Cameron [:nrc]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=03cb77f8d75d

Comment 9

[Ed Morley [:emorley]]

Push backed out for burning the tree:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4fb15293efc

Comment 10

[Nick Cameron [:nrc]]

Try again, fixed the missing symbol (was lazy and only did incremental build, missed the linking error, sorry): https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f88af5665cf2

Thanks for the backout edmorley!

Comment 11

[Nick Cameron [:nrc]]

backed out again: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f88af5665cf2

This one builds fine locally on Windows :-(

Comment 12

[Nick Cameron [:nrc]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=61c0826e8903

(Remembered to qrefresh this time)

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/61c0826e8903