|nsRuleNode::SetStyleClipPathToCSSValue| can leak |basicShape| in various |NS_NOTREACHED| scenarios [2,3,4]. This probably isn't a huge deal, but would still be a leak in non-debug builds where |NS_NOTREACHED| is a no-op. [1] http://hg.mozilla.org/mozilla-central/annotate/e4cfacb76830/layout/style/nsRuleNode.cpp#l8781 [2] http://hg.mozilla.org/mozilla-central/annotate/e4cfacb76830/layout/style/nsRuleNode.cpp#l8835 [3] http://hg.mozilla.org/mozilla-central/annotate/e4cfacb76830/layout/style/nsRuleNode.cpp#l8842 [4] http://hg.mozilla.org/mozilla-central/annotate/e4cfacb76830/layout/style/nsRuleNode.cpp#l8847 [5] http://hg.mozilla.org/mozilla-central/annotate/e4cfacb76830/xpcom/glue/nsDebug.h#l142

Hey! I'd like to work on this as my first bug. Would simply deallocating basicShape in each instance of NS_NOTREACHED solve the problem?

Thanks! So, nsStyleBasicShape is a refcounted object[1], which means we shouldn't delete it directly -- we should let its refcounting methods take care of that. I think we can just change the type of "basicShape" to be "nsRefPtr<nsStyleBasicShape>" instead of "nsStyleBasicShape*". And then it will automatically get freed when it goes out of scope. (And if we passed it to SetBasicShape -- as we do in the normal case -- then that will have internally incremented the reference count, and it won't be freed after all, which is good.) [1] because of http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.h?rev=7938cd20bee3#3030

[tangent: technically, using a nsRefPtr here may cause a teeny slowdown in the normal case, due to the extra AddRef/Release. The nsRefPtr will increment the count to 1, and SetBasicShape will increment it to 2, and then it'll go back to 1 when basicShape goes out of scope. We *could* avoid this by making a new version of SetBasicShape() which takes an already_AddRefed<nsStyleBasicShape>, but mccr8 tells me the perf difference should be negligible, so it's probably not worth the maintenance burden.]

Change the nsStyleBasicShape pointer to an nsRefPtr for automatic memory management

Comment on attachment 8631362 [details] [diff] [review] Change the nsStyleBasicShape pointer to an nsRefPtr >+++ b/layout/style/nsRuleNode.cpp >@@ -8894,17 +8894,17 @@ nsRuleNode::SetStyleClipPathToCSSValue(n > { > MOZ_ASSERT(aValue->GetUnit() != eCSSUnit_ListDep || > aValue->GetUnit() != eCSSUnit_List, > "expected a basic shape or reference box"); > > const nsCSSValueList* cur = aValue->GetListValue(); > > uint8_t sizingBox = NS_STYLE_CLIP_SHAPE_SIZING_NOBOX; >- nsStyleBasicShape* basicShape = nullptr; >+ nsRefPtr<nsStyleBasicShape> basicShape = nullptr; I think this one-line change should be all you need -- I think the rest of this patch isn't necessary. (nsRefPtr has an operator that lets it cast to its wrapped type automatically, so the "SetBasicShape(basicShape)" call should still Just Work without any changes needed there. Generally, Mozilla code uses nsRefPtr for local variables & member-variables, but not in function parameters or return values*, to avoid unnecessary reference-count increment/decrement churn.)

Oh I see. Thanks for the comment! Will keep that in mind for the next bug I work on.

Comment on attachment 8631411 [details] [diff] [review] Change the nsStyleBasicShape pointer to an nsRefPtr This patch file seems to be empty. :)

Sorry (In reply to Daniel Holbert [:dholbert] from comment #8) > Comment on attachment 8631411 [details] [diff] [review] > Change the nsStyleBasicShape pointer to an nsRefPtr > > This patch file seems to be empty. :) Sorry about that! I was having some issues with Mercurial. Should be fixed now.

Comment on attachment 8631427 [details] [diff] [review] Change nsStyleBasicShape pointer to an nsRefPtr ># HG changeset patch ># User Wilmer Paulino <wpaulino.moz@gmail.com> ># Date 1436420364 14400 ># Thu Jul 09 01:39:24 2015 -0400 ># Node ID c8b06c16529e8814ca01d7b81a4f5a42c076330c ># Parent 9f8def04e27f0590df283fb440eba590b5c5a95a >Bug 1081362 - Change nsStyleBasicShape pointer to an nsRefPtr > >diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp >--- a/layout/style/nsRuleNode.cpp >+++ b/layout/style/nsRuleNode.cpp >@@ -8894,17 +8894,17 @@ nsRuleNode::SetStyleClipPathToCSSValue(n > { > MOZ_ASSERT(aValue->GetUnit() != eCSSUnit_ListDep || > aValue->GetUnit() != eCSSUnit_List, > "expected a basic shape or reference box"); > > const nsCSSValueList* cur = aValue->GetListValue(); > > uint8_t sizingBox = NS_STYLE_CLIP_SHAPE_SIZING_NOBOX; >- nsStyleBasicShape* basicShape = nullptr; >+ nsRefPtr<nsStyleBasicShape> basicShape = nullptr; Just one nit (sorry, should have mentioned in comment 5) -- nsRefPtr is initialized to null by default, so please drop the "= nullptr", as it's unnecessary. r=me with that -- please post an updated patch with that change and with "r=dholbert" at the end of your commit message, and this will be good to go. Thanks!

[setting mentor=me, since I co-opted mentorship here while erahm was on PTO. (muahaha)]

Comment on attachment 8631431 [details] [diff] [review] Change nsStyleBasicShape pointer to an nsRefPtr The final state looks good, but a few things: (1) this latest patch looks like it applies on top of your earlier patch -- this patch is expecting the underlying thing to already be a nsRefPtr, and it just drops the "= nullptr". I think you need to fold this together with your earlier patch, to get the final One True Patch. :) (If you're using mercurial queues (qpush/qpop/etc), you probably want to use "hg qfold". If not, I'm not sure what the best thing is -- maybe just directly applying both patches in sequence to a clean tree and using "hg qnew". irc.mozilla.org #introduction or #developers can help if you get stuck, too.) (2) Since you're already tweaking the patch, I'll mention one more thing that I just noticed -- the commit message could be a bit more descriptive/explanatory. Better: Bug 1081362 - Change nsStyleBasicShape pointer to an nsRefPtr, to avoid leak in unexpected case. r=dholbert (3) It looks like you may have accidentally been using the "Edit Attachment As Comment" button for the past few comments. That effectively pastes the full text of the patch into each of your comments. Generally, you only want to use that button if you want to discuss some particular line(s) in the patch -- and then you'd want to edit out the rest of the patch from your comment field. Thanks! This is almost there.

unexpected case. r=dholbert

Comment on attachment 8631595 [details] [diff] [review] Change nsStyleBasicShape pointer to an nsRefPtr, to avoid leak in Looks good! Just one nit (which we can probably just fix when checking in the patch, or which you can fix in a newly-updloaded version if you like): ># HG changeset patch ># User Wilmer Paulino <wpaulino.moz@gmail.com> ># Date 1436453791 14400 ># Thu Jul 09 10:56:31 2015 -0400 ># Node ID a698db1a3ff3d003f8cd3f3ebb2fc384c021a8da ># Parent d9529bde0280a47d10733bcf39fd7ecc091f5886 >Bug 1081362 - Change nsStyleBasicShape pointer to an nsRefPtr, to avoid leak in >unexpected case. r=dholbert There's a newline in the middle of the commit message here - that needs to be removed. (It needs to just be one long line.) hg makes a semantic difference between the first line (up to the newline character) vs. subsequent lines -- the first line is the main description, and subsequent lines can contain extra details & explanation/prose. This matters because various tools like... https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml ...and... http://treeherder.mozilla.org/ ...will display *exactly* the first line of the commit message, as a summary of the commit. So these tools would report your current patch as simply: "Bug 1081362 - Change nsStyleBasicShape pointer to an nsRefPtr, to avoid leak in" ...which isn't what we want. :)

I removed the newline locally (un-wrapping the line) and pushed the patch to Try to give it some testing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3727c6694365 Here's that tweaked patch, which can be checked in once we've gotten good Try results back. (More info on Try here if you're curious: https://wiki.mozilla.org/ReleaseEngineering/TryServer )

The Try run looks good! (just a few known-intermittent test-failures, unrelated to this patch) So, this is ready to land -- I'm adding the "checkin-needed" keyword, which means someone will come along and (after verifying that it's been reviewed) check it in in the next day or so. Congratulations on your first patch-landing! (soon)

(In reply to Daniel Holbert [:dholbert] from comment #22) > The Try run looks good! (just a few known-intermittent test-failures, > unrelated to this patch) > > So, this is ready to land -- I'm adding the "checkin-needed" keyword, which > means someone will come along and (after verifying that it's been reviewed) > check it in in the next day or so. > > Congratulations on your first patch-landing! (soon) Sweet! Thanks for all your help throughout the process!

Wilmer, thank you for your contribution! Daniel, thanks for taking over for me!