67384 - getAnonymousNodes returns null, not list, for a node with no binding

Status: RESOLVED WONTFIX

(Core :: XBL, defect)

Description

[Vino Crescini]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux 2.2.12-20 i586; en-US; 0.7) Gecko/20010105
BuildID:    2001010517

in JS, a call to the function document.getAnonymousNodes(somenode) where
somenode is a xul element with no anonymous content will cause a crash.Shouldn't
it simply return an empty list?


Reproducible: Always
Steps to Reproduce:
run the xul file below

Actual Results:
release build:
abnormal exit
debug build:
###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().:
'mRawPtr != 0', file ../../../../dist/include/nsCOMPtr.h, line 648
###!!! Break: at file ../../../../dist/include/nsCOMPtr.h, line 648


Expected Results:
getAnonymousNodes() should have returned an array of length 0

<?xml version="1.0"?>
<window
  xmlns:html="http://www.w3.org/TR/REC-html40"
  xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
  width="100"
  height="100"
  onload="start();">
  <script language="javascript">
    function start() {
      var boxElement     = document.getElementById("box1");
      var anonChildArray = document.getAnonymousNodes(boxElement);
      for (var i = 0; i &lt; anonChildArray.length; i++)
        dump("element = " + anonChildArray[i].nodeName + "\n");
      return;
    }
  </script>
  <box id="box1"/>
</window>

Comment 1

[Vino Crescini]

Attachment: test file

Comment 2

[John Morrison]

Well, no sooner does one find a bug than hyatt decides to rewrite great reams
of code. This was crashing in the now-deceased nsXBLService::GetContentList().

I'll see if this condition/crash is fixed in the new code tomorrow. Thanks
for the nice test case.

Comment 3

[Peter Trudelle]

Okay, I will really check it now (jrgm using trudelle's login).

Comment 4

[Vino Crescini]

I just tried it on 0.8. doesn't cause a crash anymore but it returns a null when
there are no anonymous children. shouldn't it return an empty array instead?

Comment 5

[John Morrison]

Right you are. No crash, but that method should return the empty list, rather
than |null| to be consistent with other DOM methods. 

Comment 6

[Peter Trudelle]

easy workaround, can fix later, ->future

Comment 7

[Blake Ross]

Attachment: "patch"

Comment 8

[Blake Ross]

This "patch" fixes the problem by doing what we would have done in 
nsXULElement::GetChildNodes if box had a binding.  I can't say I really 
understand why this is necessary.  Hoping David "xpconnect" Bradley can explain.

Comment 9

[David Hyatt]

This is not good.  I'm marking this as wontfix.

Comment 10

[John Morrison]

I'm not saying that there is any pressing need to fix this, but I think 
this is still a valid bug.

DOM methods such as getElementsByTagName() and childNodes() always return 
a NodeList, which is a, possibly zero-length, list. They don't return null
when they have no members. I don't see why it's preferable to have 
getAnonymousNodes() not return an empty list.

Comment 11

[David Hyatt]

Well, then we'd have to make a cheesy internal method that did return null just
to deal with this.  The problem is this method is called all the time as you
walk down a frame tree, and you'll end up allocating all of these node lists for
the frames that don't have anonymous content.  I would guess that Blake's patch
would spike page load times by 5-10%.

Comment 12

[David Hyatt]

Anyway, the binding manager method should return null.  I'll accept a patch to
DocumentXBL's method to make a nodelist, but the binding manager method can't be
changed.

Comment 13

[John Morrison]

> > I don't see why it's preferable to ...
> 
> ... would spike page load times by 5-10% ...

Ok, now I see :-]. 

Comment 14

[Blake Ross]

Yes, I didn't expect to check this in, hence "patch"; I notice we hit the
!binding case many, many times.

Comment 15

[Blake Ross]

I'm still confused though about how this method is just returning an object,
though, and not a nodelist (as shown by alert(typeof retval)), given
http://lxr.mozilla.org/seamonkey/source/dom/public/idl/xbl/nsIDOMDocumentXBL.idl#46
 Can someone explain that to me, just for my own knowledge?

Comment 16

[John Bandhauer]

blake: try alert(retval)

alert(typeof window) will yield 'object' too. It is not "just an object", but it
*is* an object (not a string, number, function, or undefined).

Comment 17

[Michael Kohler [:mkohler]]

This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.

Comment 18

[Boris Zbarsky [:bzbarsky]]

XBL is now disabled in Firefox (Bug 1583314) and is in the process of being removed from Gecko (Bug 1566221), so closing bugs requesting changes to its implementation as wontfix.