Last Comment Bug 769021 - Leak nsTArray_base with "backface-visibility: hidden", border-radius
: Leak nsTArray_base with "backface-visibility: hidden", border-radius
Status: RESOLVED FIXED
[MemShrink:P3]
: mlk, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: mozilla18
Assigned To: Nick Cameron [:nrc]
:
Mentors:
Depends on:
Blocks: randomstyles
  Show dependency treegraph
 
Reported: 2012-06-27 12:38 PDT by Jesse Ruderman
Modified: 2012-09-04 02:55 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (233 bytes, text/html)
2012-06-27 12:38 PDT, Jesse Ruderman
no flags Details
leak log (591 bytes, text/plain)
2012-06-27 12:38 PDT, Jesse Ruderman
no flags Details
fix (8.23 KB, patch)
2012-08-30 23:20 PDT, Nick Cameron [:nrc]
khuey: review+
Details | Diff | Splinter Review
fix (8.15 KB, patch)
2012-09-02 23:57 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-06-27 12:38:07 PDT
Created attachment 637216 [details]
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 2012-06-27 12:38:28 PDT
Created attachment 637217 [details]
leak log
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-08-30 15:51:23 PDT
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 [:khuey] (khuey@mozilla.com) 2012-08-30 19:47:18 PDT
Tiny leak, almost impossible to hit, not very interesting to MemShrink.
Comment 4 Nick Cameron [:nrc] 2012-08-30 23:20:18 PDT
Created attachment 657171 [details] [diff] [review]
fix
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-08-31 13:30:07 PDT
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] 2012-09-02 14:45:44 PDT
Try push: https://tbpl.mozilla.org/?tree=Try&rev=4ffcb9da68f4
Comment 7 Nick Cameron [:nrc] 2012-09-02 23:57:38 PDT
Created attachment 657787 [details] [diff] [review]
fix

Addressed khuey's comments, carrying r+.

Thanks for the pointer to MOZ_DELETE!
Comment 9 Ed Morley [:emorley] 2012-09-03 02:41:03 PDT
Push backed out for burning the tree:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4fb15293efc
Comment 10 Nick Cameron [:nrc] 2012-09-03 03:37:45 PDT
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] 2012-09-03 03:50:14 PDT
backed out again: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f88af5665cf2

This one builds fine locally on Windows :-(
Comment 12 Nick Cameron [:nrc] 2012-09-03 18:03:50 PDT
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=61c0826e8903

(Remembered to qrefresh this time)
Comment 13 Ed Morley [:emorley] 2012-09-04 02:55:34 PDT
https://hg.mozilla.org/mozilla-central/rev/61c0826e8903

Note You need to log in before you can comment on or make changes to this bug.