Closed Bug 566416 Opened 12 years ago Closed 12 years ago

GetFirstChild broken for attribute nodes

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

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...
Attached patch How about this?Splinter Review
I am now full of well-reasoned hate for this code.
Attachment #445909 - Flags: review?(jonas)
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.
Attachment #445909 - Flags: review?(jonas) → review+
> 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);
(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...
> 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?
Note that the sessionstore thing is touching chrome, not content, and only touching leaves, so shouldn't slow things down too much.
I guess this is fine to land as-is.
Pushed http://hg.mozilla.org/mozilla-central/rev/19b38b92bda3.

This is tested by tests in the test suite once bug 564979 is checked in.
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Depends on: 598877
Depends on: 600466
Depends on: 600468
Depends on: 600471
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.