235640 - nsXULElement don't like having their XPConnect wrapper reparented.

Status: NEW

(Core :: DOM: Core & HTML, defect, P5)

Description

[Johnny Stenback (:jst)]

As evident in bug 233137, nsXULElement's don't having their XPConnect wrappers
reparented when they're moved from one document to another. Part of the reason
is probably the fact that a XUL elements nodeinfo is not always "correct",
sometime it's from the elements prototype, sometime it's from the original
document, which may not be the current document, etc. A big mess, to put it in
plain words.

Comment 1

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

Based on the numbers in bug 198533 comment 8 I think we should just go ahead and
make mNodeInfo a member of nsXULElement (and remove it from the
nsXULElement::Slots struct of course). I really doubt that an extra 6k added to
each window will be a noticable difference.

Other then bloat adding an mNodeInfo member could affect performance since we'll
have to get a nodeinfo object every time we create an dom-element around a
prototype. One way to lessen this could be to add a

virtual nsresult GetNodeInfo(nsINodeInfo* aNodeInfo, nsINodeInfo** aResult) = 0;

function to nsINodeInfoManager. This way we would be able to use the mInner
object inside aNodeInfo to do the hashlookup. Though in all honesty I doubt it'd
make a big difference. Maybe more important could be to deCOMtaminate
nsINodeInfoManager, i filed bug 236408 on that.

Comment 2

[Johnny Stenback (:jst)]

If we do this (which it seems like we should), it might be worth making all
prototype documents share nodeinfo managers too, which would mean we'd have less
nodeinfo's in memory (since a window prototype element's nodeinfo would be
shared with another ones, etc), assuming we have more than one XUL prototype
document loaded, which seems very likely. :-)

Comment 3

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

Once petervs patch for bug 27382 is checked in it won't actually cause us any
bloat to move mNodeInfo into the xulelement. The reason is that having a real
mNodeInfo means that we'll be able to get rid of mDocument.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Whoa!  Sweet....

Comment 5

[Boris Zbarsky [:bzbarsky]]

So... with bug 198533 fixed, are we now actually in a position to remove those
!newContent->IsContentOfType(eXUL) checks?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Smaug, see comment 5?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #5)
> So... with bug 198533 fixed, are we now actually in a position to remove those
> !newContent->IsContentOfType(eXUL) checks?

I tested this and Bug 233137 was back :(

Comment 8

[Boris Zbarsky [:bzbarsky]]

Any idea why?  XUL elements have the right mNodeInfo now, no?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: just a test

With this one I can't see Bug 233137, but I do get a lot of warnings:
WARNING: Moving XPConnect wrappedNative to new scope, but can't fixup
__proto__, file xpcwrappednative.cpp, line 1057
Most of those warnings come when opening the "Customize Toolbar" dialog.

jst, any idea? xpcwrappednative.cpp:1057 is possibly your code ;)

And yes, XUL elements should have right nodeinfo now.

/me continues debugging...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Does ReParentContentWrapper need to deal with the weirdness from
http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsDOMClassInfo.cpp#5022 ?

Comment 11

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.