397022 - nsCSSValue::Array leak involving overriding declarations of CSS 'content' property

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

This testcase causes two nsCSSValue::Array objects to leak.  

(Tested with Mac trunk debug, XPCOM_MEM_LEAK_LOG=2.)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The problem is that nsRuleNode::ComputeContentData took the copy-construct codepath to allocate the nsStyleContent but then didn't release the old content->ContentAt().mContent.mCounters .

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Except it should have, from reading the code, through ~nsStyleContentData.  I must have missed something...

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Ah, nsStyleContent::AllocateContents shouldn't have the (aCount != mContentCount) check because we need to run the destructors inside the DELETE_ARRAY_IF.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

AllocateQuotes, AllocateCounterIncrements, and AllocateCounterResets should be OK since they allocate objects where assignment does the necessary destruction.  (Then again, maybe that ought to be true for nsStyleContentData too.)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

This is the simple fix.

The alternative is making nsStyleContentData's members private and making it manage their lifetimes (except it would adopt the string on setting), but that seems like extra cycles and extra code for no present gain.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 281905 [details] [diff] [review]
patch

Makes sense.  r+sr=bzbarsky

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk.

Comment 8

[Jesse Ruderman]

Crashtest checked in.