324465 - <span style="-moz-binding: url(#t);"></span> causes document to leak

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Flummox]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060120 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060120 Firefox/1.6a1

See steps to reproduce

Reproducible: Always

Steps to Reproduce:
1.Go to URL
2.Run test #34845199
3.Run leak-gauge

Actual Results:  
Leaked document at address 95dc600.
 ... with URI
"http://toadstool.se/software/iexploder/demo/iexploder.cgi?lookup=1&test=34845199&subtest=".
Summary:
Leaked 0 out of 8 DOM Windows
Leaked 1 out of 29 documents
Leaked 0 out of 3 docshells

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

This is minimised from the exploder page, basically this seems to be enough to cause the memory leak:
<span style="-moz-binding: url(#t);"></span>

I also get this assertion in a debug build:
###!!! ASSERTION: *** XBL doc with no root element! Something went horribly wron
g! ***: 'Error', file c:/mozilla/mozilla/content/xbl/src/nsXBLService.cpp, line
396
Failed to load XBL document file:///C:/Documents%20and%20Settings/mw22/Bureaubla
d/iexploder.cgi324465_leak_exploder.htm
overflow!

Comment 2

[Boris Zbarsky [:bzbarsky]]

The problem is that nsXBLStreamListener::Load doesn't break the reference cycle between the listener and its mBindingDocument in a lot of cases.

That said, I'd rather we didn't have a cycle at all...  mrbkap, does your work on breaking the document/sink/parser cycle mean to have the parser own the doc, or the other way around?  That is, do you think it would be reasonable to not hold a ref to the doc here in the first place and rely on the parser to keep it alive?  I doubt we make that work now, but perhaps we should?

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

My goal was to make the document hold the parser and the content sink, with a weak reference between the content sink and the document, thus breaking the circular reference there.

I don't think it makes sense for the parser to hold the document, especially since the content viewer always has a strong ref to the document, and the strong refs to the parser (considering that the strong ref that necko keeps on it goes away after the stream finishes loading).

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK.  I was suspecting something like that.  The issues arise when documents are _not_ handled via a content viewer, as here.  I guess we just need to have that circular reference for the duration of the load, then.  Or we need to have the listener be a separate object that holds a weak ref to our nsXBLStreamListener...

Anyway, I'll post a patch shortly.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Not all is so simple; I'm not quite seeing a good way to deal here yet... :(  In particular, it's not clear to me why this code can assume that the Load() event will fire in all error conditions.  Is it?

Comment 6

[Joonas]

Attachment: this one hangs the browser

<select style="-moz-binding: url(#t)"> hangs the browser (iExploder test 10030070). Is this helpful or should I file another bug for this hang?

Comment 7

[Martijn Wargers (dead)]

Joonas, please file a new bug on it, file it under Core->XBL.

Comment 8

[Jesse Ruderman]

The hang in comment 6 was filed as bug 329410.

Comment 9

[Darin Fisher]

Not going to block 1.8.1 for this since the leak is particular to XBL used on the web.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I wonder whether we could just use the cycle collector here.  This is the sort of thing it should be great for (no JS in the cycle and all).

Comment 11

[Jesse Ruderman]

WFM: no leaks shown by trace-refcnt after loading either testcase.  I get a warning

WARNING: *** XBL doc with no root element! Something went horribly wrong! ***: file /Users/jruderman/trunk/mozilla/content/xbl/src/nsXBLService.cpp, line 388

but there are no other signs of bad things happening.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007111918 Minefield/3.0b2pre

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sweet!

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This was fixed by the patch in bug 394052