23905 - [MLK] leaking nsXULElement objects

Status: VERIFIED FIXED

(Core :: XUL, defect, P1)

Description

[Chris Waterson]

I'll attach a test case...

Comment 1

[Chris Waterson]

Attachment: test case

Comment 2

[Chris Waterson]

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.

Comment 3

[Chris Waterson]

need to apply patches for 20677 to fix this leak

Comment 4

[Chris Waterson]

Attachment: proposed fix

Comment 5

[Chris Waterson]

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?

Comment 6

[Chris Waterson]

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.

Comment 7

[Chris Waterson]

Attachment: better diffs. cut from win32, may need to dos2unix before applying

Comment 8

[Chris Waterson]

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?

Comment 9

[Chris Waterson]

Attachment: fixes to generic and HTML DOM

Comment 10

[Chris Waterson]

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?

Comment 11

[Chris Waterson]

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

[David Hyatt]

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.

Comment 13

[Chris Waterson]

<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

[David Hyatt]

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

Comment 15

[David Hyatt]

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

[David Hyatt]

Never mind.  You won't.  I make sure to call SetDocument explicitly.

Comment 17

[Chris Waterson]

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

Comment 18

[leger]

BULK MOVE: Changing component from XUL to XP Toolkit/Widgets: XUL.  XUL 
component will be deleted.

Comment 19

[Chris Waterson]

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.

Comment 20

[Chris Waterson]

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.

Comment 21

[Chris Waterson]

Fix checked in. Ended up making XUL elements work like HTML elements. I suck.

Comment 22

[Chris Waterson]

fix checked in.

Comment 23

[Paul MacQuiddy]

marking verified

Comment 24

[Pulsebot]

Pushed by ffxbld-merge:
https://hg.mozilla.org/releases/mozilla-beta/rev/116701ccce4d
[fenix] For https://github.com/mozilla-mobile/fenix/issues/23905 - Update onboarding icons to use Icon Active color