[MLK] leaking nsXULElement objects

VERIFIED FIXED in M14

Status

()

Core
XUL
P1
major
VERIFIED FIXED
18 years ago
10 years ago

People

(Reporter: Chris Waterson, Assigned: Chris Waterson)

Tracking

({memory-leak})

Trunk
memory-leak
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

18 years ago
I'll attach a test case...
(Assignee)

Comment 1

18 years ago
Created attachment 4235 [details]
test case
(Assignee)

Updated

18 years ago
Blocks: 18127
Status: NEW → ASSIGNED
Priority: P3 → P2
Target Milestone: M13
(Assignee)

Comment 2

18 years ago
Attached the test case for bug 18127. To reproduce:

1. Turn on bloat logging (e.g., set XPCOM_MEM_BLOAT_LOG=/tmp/bloat.log on Unix)
2. Run "viewer http://bugzilla.mozilla.org/showattachment.cgi?attach_id=4235"
to load test case.
3. Close viewer.
4. Note "4 nsXULElement objects leaked" in the bloat log.

Expected 0 nsXULElement objects leaked.
(Assignee)

Updated

18 years ago
Depends on: 20677
(Assignee)

Comment 3

18 years ago
need to apply patches for 20677 to fix this leak
(Assignee)

Comment 4

18 years ago
Created attachment 4291 [details] [diff] [review]
proposed fix
(Assignee)

Comment 5

18 years ago
brendan, shaver: please review above patch.

1. Changed |nsIScriptEventOwner|, removed the |SetCompiledEventHandler|, and
replaced it with |CompileEventHandler|. The idea here is that the script event
owner gets "first crack" at compiling the event handler; e.g., in case it wants
to compile the event handler using its own sekr3t context and scope object. The
semantics of this method are such that the event handler must be *bound* to the
specified context/target after the method returns.

2. Change |nsXULPrototypeDocument| so that it has its own secret script context
object that can be used to compile scripts and event handlers. This is necessary
to avoid compile-time linkage to the "first compiled context", which is what was
happening before.

3. Change nsXULDocument and nsXULElement to make use of these new fancy features
(and eliminate the leak).

4. Change nsEventListenerManager and nsXULKeyBindings to use the new APIs.

brendan: One thing I'm a bit unsure of is in
nsXULElement::CompileEventHandler(). In the case that it is compiling a shared
event handler, it will return the shared object out to the caller. Is that
correct?
(Assignee)

Updated

18 years ago
Priority: P2 → P1
Target Milestone: M13 → M14
(Assignee)

Comment 6

18 years ago
Moving the Gordian Knot of 18127, 23905, and 20677 to M14. Fixing this has just
introduced way too many instabilities in my tree that it'll take a couple of
days to sort out. I'll shoot to get these in when M14 opens.
(Assignee)

Comment 7

18 years ago
Created attachment 4343 [details] [diff] [review]
better diffs. cut from win32, may need to dos2unix before applying
(Assignee)

Comment 8

18 years ago
Ok, I've got some better diffs that actually seem to work pretty well.

1. I've added the nsIScriptGlobalObjectData interface to
nsXULPrototypeDocument. This allows event handlers that are compiled vs. the
prototype document to be assigned the proper principal.

2. I've enabled SetDocument(nsnull) in nsXULElement::RemoveChildAt(), but left
the initial SetDocument() in nsXULElement::Create(PRInt32 aNameSpaceID,
nsIAtom* aTag, nsIContent** aResult), because there seem to be some
initialization issues over in the frame code that I haven't quite been able to
work out yet. This unroots the script object when a XUL element is removed from
the document, breaking the cycle.

3. Also, incorporated comments from brendan, most importantly, adding a
refcount from the sekr3t script scope object to the nsXULPrototypeDocument,
which gets released in the script object's (new-in-this-patch) finalizer.

I think these are ready to go in (I'll wait until M14 opens to land). Somebody
wanna review?
(Assignee)

Comment 9

18 years ago
Created attachment 4386 [details] [diff] [review]
fixes to generic and HTML DOM
(Assignee)

Comment 10

18 years ago
vidur: I believe that the above fixes are required to the generic and HTML
contetainer elements. Specifically, AppendChildTo, InsertChildAt, and
ReplaceChildAt were doing "shallow" SetDocument() calls, while RemoveChildAt was
doing a "deep" SetDocument(nsnull) call. This'd leave dangling or null document
pointers in DOM elements that had been moved around the tree (or created "by
hand" via document.createElement).

Does this make sense?
(Assignee)

Comment 11

18 years ago
Documenting for posterity's sake. nsXULElement does not correctly do this, nor
does most of the code I've written. I'll be respinning diffs.

Vidur Apparao wrote:
>
>
> The decision to do a shallow set was made by Kipp when he first wrote
> some of the content code. The thought was that content tree buildup in
> the sink happens from the bottom up (subtrees are built before being
> added to their parents). To prevent O(n^2) invocation of SetDocument
> (where n is the depth of the tree) he decided that any deep calls to
> SetDocument should be done outside the nsIContent content modification
> routines - this should be documented in the header file, but probably
> isn't. You'll notice that the DOM calls do their own deep calls to
> SetDocument before (or after as the case may be) invoking the
> nsIContent equivalents.
>
> --Vidur

Comment 12

18 years ago
This seems wrong to me.  You have an API called "AppendChildTo", and yet it only
does a shallow SetDocument?  That seems really confusing.

Why not just make sure that SetDocument does a check for mDocument == aDocument
and allow it to be invoked n^2 times?  The alternative is that you have methods
that don't behave like their DOM counterparts when (intuitively, given the names
of the methods) they should.
(Assignee)

Comment 13

18 years ago
<gripe>
Strictly speaking, I agree with Hyatt. This is poor API design. But, it's
entrenched. Oh well.

If I were designing the API, I would've made them do a recursive SetDocument(),
and then added a special sekr3t API that'd be only called to append content
from the sink. Especially since the other methods (content replaced, content
inserted) are -never- called from the sink. And RemoveElementAt() has
completely contradictory semantics (it -does- do a recursive clear doc).
</gripe>

/me goes back to fixing xul now.

Comment 14

18 years ago
Wait, are you making XUL behave this way?  We don't have to, do we?

Comment 15

18 years ago
BTW, if you make this change to XUL, I guarantee you that you will totally bust
anonymous content.  I think we should consider fixing what (IMHO) is a broken
API.

Comment 16

18 years ago
Never mind.  You won't.  I make sure to call SetDocument explicitly.
(Assignee)

Comment 17

18 years ago
Oh yeah, I'll break shit galore. Trust me, I've tried it.

Comment 18

18 years ago
BULK MOVE: Changing component from XUL to XP Toolkit/Widgets: XUL.  XUL 
component will be deleted.
Component: XUL → XP Toolkit/Widgets: XUL
(Assignee)

Comment 19

18 years ago
Checked in necessary (but not sufficient) changes to use the XUL prototype 
document's JSContext for compiling scripts. Now on to untangling the 
SetDocument() mess.

Updated

18 years ago
Keywords: mlk
(Assignee)

Comment 20

18 years ago
Bah. I'm throwing in the towel on this one and letting kipp & entropy win. I've 
been running with changes in my tree to make XUL elements work like HTML and 
XML elements for the last two weeks with no problems.
(Assignee)

Comment 21

18 years ago
Fix checked in. Ended up making XUL elements work like HTML elements. I suck.
(Assignee)

Comment 22

18 years ago
fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 23

18 years ago
marking verified
Status: RESOLVED → VERIFIED

Updated

10 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: paulmac → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.