194834 - Remove unneeded XBL XPCOM interfaces

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Brian Ryner (not reading)]

Now that content and layout have been merged together, we should be able to get
rid of some of the XPCOM interfaces, such as:

nsIXBLBindingAttachedHandler
nsIXBLDocumentInfo
nsIXBLInsertionPoint
nsIXBLPrototypeBinding
nsIXBLPrototypeHandler

Comment 1

[Brian Ryner (not reading)]

Attachment: patch to remove nsIXBLPrototypeHandler

I decided to do this one interface at a time instead of a single massive patch.
 This removes nsIXBLPrototypeHandler.

Comment 2

[Brian Ryner (not reading)]

Comment on attachment 115621 [details] [diff] [review]
patch to remove nsIXBLPrototypeHandler

In case you're wondering, I talked with hyatt about this change (conceptually)
and he said it would be fine, but he didn't really have time to review it.

So, can you guys review?

Comment 3

[Brian Ryner (not reading)]

dbaron is away for a few days; hoping jst or bz can sr this.

Comment 4

[Jan Varga [:janv]]

Dupe of 106152 or this is not full decomtamination ?

Comment 5

[Brian Ryner (not reading)]

This introduced a leak; nsXBLWindowKeyHandler can create a new
nsXBLPrototypeHandler and set it as mHandler, and it's never deleted.

Comment 6

[Brian Ryner (not reading)]

Attachment: remove nsIXBLPrototypeBinding, and fix leak from previous patch

Comment 7

[Brian Ryner (not reading)]

re: Jan's comment, I'm not sure.  The description on 106152 is too vague.

Comment 8

[Jan Varga [:janv]]

I think that hyatt wanted to do exactly the same what you're doing, that is,
reduce memory footprint, but he was blocked by content/layout merge
So Cathleen filed a new bug against him.

Comment 9

[John Keiser (jkeiser)]

Comment on attachment 115966 [details] [diff] [review]
remove nsIXBLPrototypeBinding, and fix leak from previous patch

+  PRBool hasSheets = mPrototypeBinding->HasStyleSheets();
   if (hasSheets) {
     *aResolveStyle = PR_TRUE;
     return NS_OK;


This could be done without setting a boolean at all, with if
(mPrototypeBinding->HasStyleSheets()).	Not a big deal though, you can change
or not.

-void
-nsXBLPrototypeBinding::GetImmediateChild(nsIAtom* aTag, nsIContent** aResult) 
+already_AddRefed<nsIContent>
+nsXBLPrototypeBinding::GetImmediateChild(nsIAtom* aTag)
 {
-  *aResult = nsnull;
+  nsIContent* result = nsnull;

This could be moved way into the if, since it's only used there.

Comment 10

[Alec Flett]

Comment on attachment 115966 [details] [diff] [review]
remove nsIXBLPrototypeBinding, and fix leak from previous patch

wow, this is great. about the only odd thing I found was the (C) 1998 in one or
two of the licenses

Is there a good place to document the ownership of the concrete
nsXBLPrototypeBinding? I mean I can see from the diff that the hashtable owns
it, but I'd like to see that documented promenantly.

(probably both in the class declaration and in the mBindingTable declaration)
sr=alecf with the extra comments and license stuff

Comment 11

[Boris Zbarsky [:bzbarsky]]

Hmm... could you add a comment to nsIXBLDocumentInfo that GetPrototypeBinding
does _NOT_ addref?

Alternately, are we planning to eliminate nsIXBLDocumentInfo too?

Comment 12

[Brian Ryner (not reading)]

I'm planning to try to, anyway.

Comment 13

[R.K.Aa.]

my builds up to this checkin were fine.
Some days after this checkin i built from CVS again and crash in 0x40cce9bf in
nsXBLPrototypeBinding::BindingDetached(nsIDOMEventReceiver*) ()
I've clobbered to no avail.

Comment 14

[R.K.Aa.]

Attachment: backtrace, current trunk, Linux

ac_add_options --disable-accessibility
ac_add_options --disable-postscript
ac_add_options --disable-ldap
ac_add_options --disable-debug
ac_add_options --disable-tests
ac_add_options --disable-logging
ac_add_options --enable-crypto
ac_add_options --disable-bidi
ac_add_options --enable-optimize=-O2
ac_add_options --disable-jsd
ac_add_options --disable-mathml

Comment 15

[R.K.Aa.]

Hmm. after backing out the build crashes even after clobber:
*** No rule to make target `nsIXBLPrototypeBinding.h', needed by `export'.  Stop.

I deleted my whole tree and start afresh. Since this works for others, it was
probably some local error. So please ignore the above notes.

Comment 16

[R.K.Aa.]

Attachment: still crash

downloaded fresh sources and updated them. Built and crashed again on startup,
another stack. Then backed out some js stuff (see attachment) and it's back to
a crash similar to the first stack i attached.

Comment 17

[R.K.Aa.]

weird. If i start with "mozilla -p profilename" i don't crash.

Comment 18

[R.K.Aa.]

spam-master B strikes again...
I forgot to tell the obvious? I have multiple profiles.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I see the same crash -- I crash when I hit "enter" in the profile manager,
before the dialog comes down:

(gdb) frame
#0  0x41108919 in nsXBLPrototypeHandler::ExecuteHandler (this=0xdadadada, 
    aReceiver=0x8399008, aEvent=0x41569068)
    at
/home/bzbarsky/mozilla/xlib/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp:204
204       if (mType & NS_HANDLER_TYPE_PREVENTDEFAULT) {
(gdb) p this
$1 = (nsXBLPrototypeHandler *) 0xdadadada

That does not look good right there...

#0  0x41108919 in nsXBLPrototypeHandler::ExecuteHandler (this=0xdadadada, 
    aReceiver=0x8399008, aEvent=0x41569068)
    at
/home/bzbarsky/mozilla/xlib/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp:204
#1  0x4110a41a in nsXBLPrototypeHandler::BindingDetached (this=0xdadadada, 
    aReceiver=0x8399008)
    at
/home/bzbarsky/mozilla/xlib/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp:535
#2  0x410fc4f5 in nsXBLPrototypeBinding::BindingDetached (this=0x82cac08, 
    aReceiver=0x8399008)
    at
/home/bzbarsky/mozilla/xlib/mozilla/content/xbl/src/nsXBLPrototypeBinding.cpp:422
#3  0x410f9437 in nsXBLBinding::ExecuteDetachedHandler (this=0x831ee10)
    at /home/bzbarsky/mozilla/xlib/mozilla/content/xbl/src/nsXBLBinding.cpp:1045
#4  0x410f9457 in nsXBLBinding::ExecuteDetachedHandler (this=0x831ee40)
    at /home/bzbarsky/mozilla/xlib/mozilla/content/xbl/src/nsXBLBinding.cpp:1048
#5  0x411146a2 in ExecuteDetachedHandler (aTable=0x8221efc, aHdr=0x8353538,
aNumber=3, 
    aClosure=0x0)

Interestingly enough:

(gdb) frame 1
#1  0x4110a41a in nsXBLPrototypeHandler::BindingDetached (this=0xdadadada, 
    aReceiver=0x8399008)
    at
/home/bzbarsky/mozilla/xlib/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp:535
535       ExecuteHandler(aReceiver, domEvent);
(gdb) p this
$9 = (nsXBLPrototypeHandler *) 0xdadadada
(gdb) frame 2
#2  0x410fc4f5 in nsXBLPrototypeBinding::BindingDetached (this=0x82cac08, 
    aReceiver=0x8399008)
    at
/home/bzbarsky/mozilla/xlib/mozilla/content/xbl/src/nsXBLPrototypeBinding.cpp:422
422         return mImplementation->mDestructor->BindingDetached(aReceiver);
(gdb) p mImplementation->mDestructor
$10 = (nsXBLPrototypeHandler *) 0x41237584

bryner, should I just file a separate bug on this crash?

Starting with -P profilename does not crash, but that doesn't mean we wouldn't
crash when other XBL bindings with desructors unload....

Comment 20

[John Morrison]

actually, a separate bug already exists for the crash: bug 196446. (And two 
others that are really the same deal).

Comment 21

[David Bradley]

FWIW: it looks like this patch might be responsible for the following crashes.
At least these started showing up at the same time in talkback. A thought, could
users' xul cache be causing these? I know little about the cache and how it
works, but could it have references back to old/changed things?

nsXBLPrototypeBinding::BindingDetached
nsXBLPrototypeHandler::ExecuteHandler
nsXBLPrototypeBinding::GetBindingElement
nsXBLBinding::GetImmediateChild
nsXBLPrototypeBinding::GetRuleProcessors

Comment 22

[John Morrison]

> A thought, could users' xul cache be causing these? I know little about the 
> cache and how it works, but could it have references back to old/changed 
> things?

The xul cache is in-memory, so it does not persist past the end of a process.
(There is also the fastload file which does persist between sessions, but
will invalidate itself when used with a newer build [technically, when used
with jar files that have a different timestamp or path]).

There are also the component registry files. I don't know if they have any way
to remove entries. [But only people who build themselves would be affected if 
there were a problem with that; nightly users would get a component reg 
specific to that build, I think].

Comment 23

[Brian Ryner (not reading)]

Attachment: remove nsIXBLInsertionPoint and nsIAnonymousContentList

Comment 24

[Brian Ryner (not reading)]

Comment on attachment 117585 [details] [diff] [review]
remove nsIXBLInsertionPoint and nsIAnonymousContentList

just fyi, that patch should also include the removal of nsIXBLInsertionPoint.h,
but I did the diff wrong.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 117585 [details] [diff] [review]
remove nsIXBLInsertionPoint and nsIAnonymousContentList

I may not be able to get to this in time for 1.4a....

Comment 26

[Boris Zbarsky [:bzbarsky]]

-    point = getter_AddRefs((nsIXBLInsertionPoint*)(mElements->ElementAt(i)));
-    point->ChildCount(&pointCount);
+    nsXBLInsertionPoint* point = (nsXBLInsertionPoint*)mElements->ElementAt(i);
+    pointCount = point->ChildCount();

Could we possibly have an nsCOMArray-like thing for typesafe arrays of
single-type non-refcounted objects?  nsOwningArray<T> ?  It could delete all the
elements in the destructor, do typesafe access, etc, etc...

Thoughts?

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 117585 [details] [diff] [review]
remove nsIXBLInsertionPoint and nsIAnonymousContentList

OK.  I read partway through this... up-to-now comments:

1)  Why remove MOZ_DECL_CTOR_COUNTER(nsAnonymousContentList) ?	You still use
the counter in the constructor....

2)  You have some functions that take an nsVoidArray* and assume ownership of
it, deleting it at some time of their own choosing.  Document this clearly.

3) You have some functions which pass out a nsVoidArray* that they continue to
own and can delete at some time of their choosing.  Document this clearly.

4)  Please use NS_STATIC_CAST in preference to C-style casts.

Comment 28

[Brian Ryner (not reading)]

Attachment: remove nsIXBLInsertionPoint and nsIAnonymousContentList

addressed bz's comments

Comment 29

[Brian Ryner (not reading)]

Comment on attachment 118259 [details] [diff] [review]
remove nsIXBLInsertionPoint and nsIAnonymousContentList

carrying over jkeiser's r=

Comment 30

[Brian Ryner (not reading)]

Comment on attachment 118259 [details] [diff] [review]
remove nsIXBLInsertionPoint and nsIAnonymousContentList

Since Boris seems to be otherwise busy, would you be able to review this jst?

Comment 31

[Johnny Stenback (:jst)]

Comment on attachment 118259 [details] [diff] [review]
remove nsIXBLInsertionPoint and nsIAnonymousContentList

- In nsBindingManager.cpp:

@@ -1570,22 +1555,18 @@
...
+    PRBool isAnonymousContentList;
+    GetXBLChildNodesInternal(point, getter_AddRefs(nodeList),
+			      &isAnonymousContentList);
+
+    if (nodeList && isAnonymousContentList) {
+      // Find a non-pseudo-insertion point and remove ourselves.
+      nsAnonymousContentList* contentList =
NS_STATIC_CAST(nsAnonymousContentList*, NS_STATIC_CAST(nsIDOMNodeList*,
nodeList));

I don't think you need the double cast there, this be enough?:

+      nsAnonymousContentList* contentList =
NS_STATIC_CAST(nsAnonymousContentList*, nodeList.get());

- In nsXBLBinding::GetInsertionPointsFor():

   if (!mInsertionPointTable)
-    mInsertionPointTable = new nsSupportsHashtable(4);
+    mInsertionPointTable = new nsObjectHashtable(nsnull, nsnull,
+						  nsnull, nsnull, 4);

   nsISupportsKey key(aParent);
-  *aResult = NS_STATIC_CAST(nsISupportsArray*,
mInsertionPointTable->Get(&key));
+  *aResult = NS_STATIC_CAST(nsVoidArray*, mInsertionPointTable->Get(&key));

Add some error checking there, return NS_ERROR_OUT_OF_MEMORY if new fails.

   if (!*aResult) {
-    NS_NewISupportsArray(aResult);
+    *aResult = new nsVoidArray();
     mInsertionPointTable->Put(&key, *aResult);

Same here...

   }

sr=jst

Comment 32

[Boris Zbarsky [:bzbarsky]]

Sorry about taking so long, bryner... :(

Question. Around this checkin, the leak numbers on brad went up from "244B" to
"1.05KB"; the log says:

--NEW-LEAKS-----------------------------------leaks------leaks%-----------------------
nsVoidArray                                     832          -
TOTAL                                           832

and the full bloat log says:

   534 nsVoidArray                                     8      832    25148     
104 ( 3838.20 +/-  1676.90)        0        0 (    0.00 +/-     0.00)

(that's 104 leaking nsVoidArrays, each one counted as 8 bytes big).

The full checkin list in that window is
http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&date=explicit&mindate=1050425460&maxdate=1050440759
but this one is the most likely culprit for leaking nsVoidArray...

Comment 33

[Brian Ryner (not reading)]

Attachment: more cleanup + leak fix

- Remove nsIXBLBindingAttachedHandler (unused)
- Fix the nsVoidArray leak that bzbarsky mentioned (I saw it too when I was
running this patch under valgrind).
- Stop using nsISupports for attribute and insertion point tables in
nsXBLPrototypeBinding.cpp

Comment 34

[Brian Ryner (not reading)]

Attachment: more cleanup + leak fix

same as above but with a couple out-of-memory checks added

Comment 35

[Johnny Stenback (:jst)]

Comment on attachment 120792 [details] [diff] [review]
more cleanup + leak fix

sr=jst

Comment 36

[Brian Ryner (not reading)]

I checked in the last patch.  Just one interface to go (nsIXBLDocumentInfo).

I do have another idea that's related to this - nsIXBLBinding is needed from a
few places outside of layout, but we could try constructing an XPCOM wrapper for
it on-demand (and making the internal binding class non-virtual).  However, if
the calls from outside layout occur frequently and for every binding, we're not
going to save anything (since we'd have to cache the wrapper to avoid allocation
overhead).  Need to investigate this some more.

Comment 37

[Boris Zbarsky [:bzbarsky]]

*** Bug 106152 has been marked as a duplicate of this bug. ***

Comment 38

[Brian Ryner (not reading)]

Attachment: remove nsIXBLBinding

I left these as refcounted objects (more on that in the comments in
nsXBLBinding.h).  Boris, any thoughts on my XXX comment in
nsXBLBinding::ChangeDocument?

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175213 [details] [diff] [review]
remove nsIXBLBinding

So I'm a little confused about the role of nsIBindingManager here. Do we
support using it outside content/layout?  If so, do we only support some of the
methods?  I don't see us exporting nsXBLBinding.h, one couldn't use
GetBinding() outside the layout module, right?

Should we have a separate interface we expose for "public" methods or
something?

Comment 40

[Brian Ryner (not reading)]

nsIBindingManager is used in a very limited capacity outside of
dom/content/layout.  These are the methods that are used, as far as I can tell:

FlushSkinBindings
GetInsertionParent
GetAnonymousNodesFor  (this can easily be replaced by
nsIDOMDocumentXBL::GetAnonymousNodes)
GetContentListFor

Aside from the FlushSkinBindings case, which might be changed to an observer
notification, there are generally two things people are trying to do using the
binding manager:

1. Walk up the "full" DOM tree, using GetInsertionParent.
2. Walk down the full DOM tree, using GetAnonymousNodes and falling back to
GetContentList.  This is really just GetXBLChildNodesFor(), I think.

If we could provide an alternate (and perhaps easier to understand) way of
accomplishing those tasks, it would not be difficult to get rid of
nsIBindingManager as well.

For now, I'd echo roc's comment in nsIFrame.h: "If you're not in layout, then
you won't be able to link to many of the functions defined here. Too bad."

Comment 41

[Brian Ryner (not reading)]

(In reply to comment #40)
> If we could provide an alternate (and perhaps easier to understand) way of
> accomplishing those tasks, it would not be difficult to get rid of
> nsIBindingManager as well.

Perhaps a subset of http://www.hixie.ch/specs/xbl/XBL2.html#the-nodexbl

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175213 [details] [diff] [review]
remove nsIXBLBinding

> For now, I'd echo roc's comment in nsIFrame.h:

Sounds reasonable.

>Index: content/xbl/src/nsBindingManager.cpp

>+nsXBLBinding*
>+nsBindingManager::GetBinding(nsIContent* aContent)
>+  if (mBindingTable.IsInitialized()) {
>+    nsXBLBinding *binding;
>+    if (mBindingTable.Get(aContent, &binding))
>+      return binding;

Shouldn't this use GetWeak()?  Or should the method use already_AddRefed?

>+nsBindingManager::SetBinding(nsIContent* aContent, nsXBLBinding* aBinding)
>+  if (!mBindingTable.IsInitialized())
>+    mBindingTable.Init();

Init() can fail... you probably want to return NS_ERROR_OUT_OF_MEMORY if it
does.

>+  mAttachedStack.AppendElement(aBinding);

This can fail... You probably want to make sure it doesn't, _then_ addref the
binding.

> nsBindingManager::ProcessAttachedQueue()
>+    nsXBLBinding *binding = NS_STATIC_CAST(nsXBLBinding*,
>+                                           mAttachedStack.ElementAt(lastItem));

You can probably use FastElementAt() here...

> nsBindingManager::ExecuteDetachedHandlers()
>+  if (mBindingTable.IsInitialized())
>+    mBindingTable.EnumerateRead(ExecuteDetachedHandler, nsnull);

Hmm... If detached handlers happen to change the DOM such that bindings get
attached or handlers explicitly add or remove bindings via nsIDOMDocumentXBL,
we'll get attempted writes to the binding table during the enumeration.  I
guess that's ok because those will just fail, even with us just using
EnumerateRead()?  But that can cause the script in some of those destructors to
throw....

Worth filing a separate bug on, perhaps; the existing code had similar issues.

>+MarkForDeath(nsISupports *aKey, nsXBLBinding *aBinding, void* aClosure)
>   if (!strncmp(path.get(), "/skin", 5))

Want to also file a followup bug on fixing this to only mark for death when the
URI is chrome, and when it's really a skin?  Should probably involve chrome reg
somehow.... (to decide what sort of URI this is).

Is nsBindingManager::InheritsStyle actually used?  If not, can it be removed?

>Index: content/xbl/src/nsXBLBinding.cpp
> // Implement our nsISupports methods
>-NS_IMPL_ISUPPORTS1(nsXBLBinding, nsIXBLBinding)
>+//NS_IMPL_ISUPPORTS1(nsXBLBinding, nsIXBLBinding)

Why not just remove these two lines?

>+nsXBLBinding::SetBaseBinding(nsXBLBinding* aBinding)
> {
>   if (mNextBinding) {
>     NS_ERROR("Base XBL binding is already defined!");
>-    return NS_OK;

Either keep the return here, or change this if to:

 NS_ASSERTION(!mNextBinding, "Base XBL binding is already defined!");

> nsXBLBinding::ChangeDocument(nsIDocument* aOldDocument, nsIDocument* aNewDocument)
>+    // XXX does this need to be a strong ref? does any of this release
>+    // the reference in mContent?
>+    nsCOMPtr<nsIContent> anonymous = mContent; 

No, I think that can be a weak ref...

>+nsXBLBinding*
>+nsXBLBinding::GetRootBinding()

Want to rename this to RootBinding()?  It shouln't be returning null, so....

>+nsXBLBinding::ImplementsInterface(REFNSIID aIID) const
>+  PRBool result = mPrototypeBinding->ImplementsInterface(aIID);
>+  if (!result && mNextBinding)
>+    return mNextBinding->ImplementsInterface(aIID);
>+  return result;

how about:

  return mPrototypeBinding->ImplementsInterface(aIID) ||
    (mNextBinding && mNextBinding->ImplementsInterface(aIID));

?

>+already_AddRefed<nsIDOMNodeList>
>+nsXBLBinding::GetAnonymousNodes()

Is it really ok to silently return null on OOM here?  I guess if none of the
callers check the return value....

>Index: content/xbl/src/nsXBLBinding.h

>+  void Release()
>+    if (mRefCnt == 0)
>+      delete this;

Set mRefCnt to 1 before calling delete, please.  That prevents us reentering
the delete call if someone decides to refcount us in code we called from the
destructor... (see what NS_IMPL_RELEASE_WITH_DESTROY does).

Want to do a bit of documenting of some of the methods here (eg
GetAnonymousContent, GetSingleInsertionPoint, etc, etc) so that it's clear what
it is they do, exactly?

>Index: content/xbl/src/nsXBLPrototypeBinding.cpp
> nsXBLPrototypeBinding::GetInsertionPoint(nsIContent* aBoundElement,
>+  nsCOMPtr<nsIContent> realContent;
>+    realContent = LocateInstance(nsnull, templContent, aCopyRoot, content);

Is there a reason LocateInstance() returns already_AddRefed<nsIContent> instead
of nsIContent*?  If so, then returning this value for here without its ref is
bogus; if not, why not change it to return nsIContent*?

From a brief glance, it looks like it could easily return nsIContent*.

> nsXBLPrototypeBinding::GetSingleInsertionPoint(nsIContent* 
>+  if (!mInsertionPointTable)
>+    return nsnull;

Shouldn't you init *aMultipleInsertionPoints and *aIndex to sane values first?

>Index: content/xbl/src/nsXBLPrototypeBinding.h

Again, a bit of documentation would be nice, if you have time.

>Index: content/xbl/src/nsXBLService.cpp

>@@ -609,9 +608,7 @@ nsXBLService::LoadBindings(nsIContent* a
>+  *aBinding = newBinding->GetFirstBindingWithConstructor();
>+  if (*aBinding)
>+    NS_ADDREF(*aBinding);

Why not use NS_IF_ADDREF?

>Index: layout/base/nsCSSFrameConstructor.cpp

> struct nsAutoEnqueueBinding
>+    mDocument(aDocument), mBinding(nsnull)

No need for this change, is there?

>Index: xpcom/ds/nsRefPtrHashtable.h

Why the changes to this file?

Comment 43

[Brian Ryner (not reading)]

(In reply to comment #42)
> >Index: content/xbl/src/nsBindingManager.cpp
> 
> >+nsXBLBinding*
> >+nsBindingManager::GetBinding(nsIContent* aContent)
> >+  if (mBindingTable.IsInitialized()) {
> >+    nsXBLBinding *binding;
> >+    if (mBindingTable.Get(aContent, &binding))
> >+      return binding;
> 
> Shouldn't this use GetWeak()?  Or should the method use already_AddRefed?

Should be GetWeak() since the binding manager holds a reference; this was
leftover from a brief time when I didn't think these needed to be refcounted.

> 
> > nsBindingManager::ExecuteDetachedHandlers()
> >+  if (mBindingTable.IsInitialized())
> >+    mBindingTable.EnumerateRead(ExecuteDetachedHandler, nsnull);
> 
> Hmm... If detached handlers happen to change the DOM such that bindings get
> attached or handlers explicitly add or remove bindings via nsIDOMDocumentXBL,
> we'll get attempted writes to the binding table during the enumeration.  I
> guess that's ok because those will just fail, even with us just using
> EnumerateRead()?  But that can cause the script in some of those destructors to
> throw....
> 
> Worth filing a separate bug on, perhaps; the existing code had similar issues.

Sure; might be best to snapshot the table before actually executing the handlers.

> 
> Want to do a bit of documenting of some of the methods here (eg
> GetAnonymousContent, GetSingleInsertionPoint, etc, etc) so that it's clear what
> it is they do, exactly?

Hmm... I'd have to figure that out myself first.

> 
> >Index: content/xbl/src/nsXBLPrototypeBinding.cpp
> > nsXBLPrototypeBinding::GetInsertionPoint(nsIContent* aBoundElement,
> >+  nsCOMPtr<nsIContent> realContent;
> >+    realContent = LocateInstance(nsnull, templContent, aCopyRoot, content);
> 
> Is there a reason LocateInstance() returns already_AddRefed<nsIContent> instead
> of nsIContent*?  If so, then returning this value for here without its ref is

There's not, as far as I can see.  I noticed this but decided not to try to fix
everything in this patch.

> >@@ -609,9 +608,7 @@ nsXBLService::LoadBindings(nsIContent* a
> >+  *aBinding = newBinding->GetFirstBindingWithConstructor();
> >+  if (*aBinding)
> >+    NS_ADDREF(*aBinding);
> 
> Why not use NS_IF_ADDREF?
> 

Because it doesn't work if your AddRef() returns void... and I don't really see
a point in returning the refcount.

> >Index: xpcom/ds/nsRefPtrHashtable.h
> 
> Why the changes to this file?
> 

Same as above.

Comment 44

[Brian Ryner (not reading)]

Though since the entire AddRef implementation is inlined, the compiler should be
able to see that the return value is unused and optimize it away.  So I'm ok
with changing it to return nsresult.

Comment 45

[Brian Ryner (not reading)]

Attachment: remove nsIXBLBinding

I think this addresses all of your comments except for the documentation
request; I'm just not sure I know what these all do well enough to write
something.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175478 [details] [diff] [review]
remove nsIXBLBinding

>Index: content/xbl/src/nsBindingManager.cpp
>+nsBindingManager::GetBinding(nsIContent* aContent)
>+    nsXBLBinding *binding = mBindingTable.GetWeak(aContent);
>+    if (binding)
>+      return binding;

How about:

  return mBindingTable.GetWeak(aContent);

?  That has the same effect.

>+nsBindingManager::AddToAttachedQueue(nsXBLBinding* aBinding)
>+  if (mAttachedStack.AppendElement(aBinding))
>+    NS_ADDREF(aBinding);
>   return NS_OK;

How about NS_ERROR_OUT_OF_MEMORY when AppendElement fails?

r+sr=bzbarsky with that and the bugs on followup issues filed.

Comment 47

[Brian Ryner (not reading)]

Filed bug 283698 and bug 283701, and checked in the patch.  Leaving bug open for
further work.

Comment 48

[Boris Zbarsky [:bzbarsky]]

The insertion point part caused crash bug 296375

Comment 49

[Boris Zbarsky [:bzbarsky]]

All of these interfaces have been gone for a while now...