Closed Bug 601437 Opened 11 years ago Closed 11 years ago

Leak nsCSSValue::Array with appendMedium

Categories

(Core :: CSS Parsing and Computation, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- final+

People

(Reporter: jruderman, Assigned: dbaron)

References

Details

(Keywords: memory-leak, testcase)

Attachments

(2 files)

Attached file testcase
1. Run Firefox with trace-refcnt enabled.
2. Load the testcase.
3. Quit.

Result: trace-refcnt reports a leak.
Leaked classes:

nsBaseURLParser
nsCSSValue::Array
nsCSSValue::URL
nsPrincipal
nsSimpleURI
nsStandardURL
nsStringBuffer
nsTArray_base
Two things going wrong here:

 (1) we go through the Clone path in EnsureUniqueInner (not sure why)

 (2) nsCSSFontFaceRule doesn't have a copy-constructor, which means the reference count of the copy is mis-initialized by the default copy-constructor
Blocks: 596140
Assignee: nobody → dbaron
blocking2.0: --- → final+
I think the leak is a regression from bug 596140, so blocking+.
Attached patch patchSplinter Review
Maybe Boris knows why we're cloning in this case.  If not, one of us should probably investigate.
Attachment #480487 - Flags: review?(bzbarsky)
>  (2) nsCSSFontFaceRule doesn't have a copy-constructor, which means the
> reference count of the copy is mis-initialized by the default copy-constructor

Eep! Filed bug 601441 asking for a static analysis to catch bugs like this.
We're cloning because of this:

#4  0x000000010047bb5a in mozilla::css::Loader::LoadSheet (this=0x1253ab700, aURL=0x1244d0d10, aOriginPrincipal=0x1253ab840, aCharset=@0x7fff5fbfc210, aObserver=0x1244d0d70) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/style/Loader.cpp:2135
#5  0x0000000100635777 in nsDocument::PreloadStyle (this=0x106083200, uri=0x1244d0d10, charset=@0x11eb09150) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/base/src/nsDocument.cpp:7893
#6  0x0000000100b133c4 in nsHtml5TreeOpExecutor::PreloadStyle (this=0x1244dbf20, aURL=@0x11eb09140, aCharset=@0x11eb09150) at /Users/bzbarsky/mozilla/vanilla/mozilla/parser/html/nsHtml5TreeOpExecutor.cpp:887

Henri, why are we preloading here??
Comment on attachment 480487 [details] [diff] [review]
patch

r=me
Attachment #480487 - Flags: review?(bzbarsky) → review+
(In reply to comment #6)
> Henri, why are we preloading here??

<link rel=stylesheet> gets preloaded unconditionally. data: URLs aren't special cased in any way in the preloader. Also, everything the preloader can preload gets preloaded even if there haven't been any scripts. (I wasn't aware of this causing any problems. I thought it was cheap to promote a preload into a real load later.)
It's reasonably cheap; does involve constructing a few more objects, and slows down the first access to the stylesheet data from script a bit.  I had thought we only preloaded when blocked; if we preload everything always that explains it.
Are we doing an extra *parse* of the markup in order to preload, even when parsing isn't blocked?
No, we preload off the same token stream that we create the DOM from.  It's just that when blocked we continue speculatively creating the token stream and throw it away in the (rare) case that the script we blocked on invalidates it.
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/c6cab3b69ee1
Status: NEW → RESOLVED
Closed: 11 years ago
OS: Linux → All
Priority: -- → P3
Hardware: x86_64 → All
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b8
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.