566416 - GetFirstChild broken for attribute nodes

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

See bug 564979 comment 4.

The issue is that we never set mFirstChild for attribute nodes.

At the same time, I would _really_ rather not introduce a virtual function call back in GetFirstChild...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: How about this?

I am now full of well-reasoned hate for this code.

Comment 2

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

Comment on attachment 445909 [details] [diff] [review]
How about this?

I pity the fool that touches attribute nodes. That will make all attribute modifications a lot slower from then on out :(

Oh well, even more incentive for us to nuke the whole attribute-node thing.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> That will make all attribute modifications a lot slower from then on out :(

Until gc, no?  I would assume we don't hold strong refs to attribute nodes?

Note that our UI touches attr nodes in some cases; e.g. this code in sessionstore:

  1219       Array.forEach(aTab.attributes, function(aAttr) {
  1220         if (this.xulAttributes.indexOf(aAttr.name) > -1)
  1221           tabData.attributes[aAttr.name] = aAttr.value;
  1222       }, this);

Comment 4

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

(In reply to comment #3)
> > That will make all attribute modifications a lot slower from then on out :(
> 
> Until gc, no?  I would assume we don't hold strong refs to attribute nodes?

That seems to be an incorrect assumption :(
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDOMAttributeMap.h#163

> Note that our UI touches attr nodes in some cases; e.g. this code in
> sessionstore:
> 
>   1219       Array.forEach(aTab.attributes, function(aAttr) {
>   1220         if (this.xulAttributes.indexOf(aAttr.name) > -1)
>   1221           tabData.attributes[aAttr.name] = aAttr.value;
>   1222       }, this);

sounds like we need to get a move on changing the ownership model or otherwise improves things then...

Comment 5

[Boris Zbarsky [:bzbarsky]]

> That seems to be an incorrect assumption :(

Ugh.

Do we want to hold off on this (and go back to having GetFirstChild not use mFirstChild) for now then?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Note that the sessionstore thing is touching chrome, not content, and only touching leaves, so shouldn't slow things down too much.

Comment 7

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

I guess this is fine to land as-is.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/19b38b92bda3.

This is tested by tests in the test suite once bug 564979 is checked in.