27382 - ownerDocument of orphan text and attr nodes is null

Status: RESOLVED FIXED

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

Description

[Erik Arvidsson]

The property ownerDocument of orphan nodes return null. Shouldn't the
ownerDocument be set when the node is created with document.createNode?

function test(doc) {
   var n = doc.createElement("SPAN");
   alert("Works? " + (doc == n.ownerDocument));
}

The following is taken from the DOM2 Candidate Recomendation

> ownerDocument of type Document, readonly, modified in DOM Level 2
> The Document object associated with this node. This is also the Document
> object used to create new nodes. When this node is a Document or a
> DocumentType which is not used with any Document yet, this is null.

Comment 1

[Erik Arvidsson]

Attachment: doc == node.ownerDocument?

Comment 2

[Erik Arvidsson]

Maybe that became a bit unclear. The error is according to DOM level 1 as well.

> ownerDocument
> The Document object associated with this node. This is also the Document
> object used to create new nodes. When this node is a Document this is null.

One of the reason I really need this to be fixed is that it blocks a lot of
features on ranges and other document dependant interfaces.

Comment 3

[vidur (gone)]

Known problem, but thanks for opening up the bug for it. I haven't thought of a 
good implementation for ownerDocument. Currently, content elements have a 
reference to their document if and only if they are attached to the document 
tree (i.e. they are not orphaned). There is code around that is based on this 
assumption. I definitely don't want to burn up another content slot to store the 
owner document. Also, the current document reference from content is weak and is 
cleared up on document destruction - I'm not sure how this would work with 
orphaned content, since the forward reference doesn't exist to do the cleanup. I 
suppose we could use the nsWeakPtr stuff to deal with cleanup.

Marking M17 for now. May consider not fixing for efficiency reasons, even though 
it compromises DOM Level 1 compatibility.

Comment 4

[Bob Clary [:bc] (inactive)]

Something to consider about this bug:  If the ownerDocument of 'orphaned' nodes
is not set, then it is not possible to detect the situation when a node created
by one Document is inserted into a different Document.

The standard is to throw a WRONG_DOCUMENT_ERR in those circumstances.  In
preliminary testing, I have found that you can indeed insert a node created in
one Document into a different Document without throwing a WRONG_DOCUMENT_ERR.

The parentNode should be used to determine if a node has been inserted into a
Document rather than the ownerDocument.  ownerDocument is essentially to
determine which factory methods should be used to create Attr, Element,... as
well as being a shortcut to the root of the tree.  Using ownerDocument == null
to determine 'orphan' nodes is just not correct.

Anyway, that is my $0.02 worth.

Comment 5

[Johnny Stenback (:jst)]

This bug has been marked "future" because the original netscape engineer
working on this is over-burdened. If you feel this is an error, that you or
another known resource will be working on this bug,or if it blocks your work
in some way -- please attach your concern to the bug for reconsideration.

Comment 6

[Jan Carpenter]

Mass update of qa contact

Comment 7

[Prashant Desale]

QA contact Update

Comment 8

[Jeff Yates]

This bug has shown no activity for a long time.  This is an important issue and 
needs to be resolved due to it's influence on the Range object which should 
soon be functional.  Please advise on the status on this.

Jeff.

Comment 9

[Johnny Stenback (:jst)]

Taking.

Comment 10

[Johnny Stenback (:jst)]

Really taking.

Comment 11

[Prashant Desale]

Updating QA contact to Shivakiran Tummala.

Comment 12

[Erik Arvidsson]

This bug seems to have been fixed. The old test case works and I'm also 
attaching a new test case that shows that it works in a frame environment as 
well.

Comment 13

[Erik Arvidsson]

Attachment: Creating a DIV in a frame as well as in same document

Comment 14

[Erik Arvidsson]

Sorry... I'm being too quick to judge... The bug is still there but it seems to 
be working for Elements. I still get the problem with text nodes and attribute 
nodes... Someone should go through all create* methods of DOM1 and 2 and verify 
which work and which don't. Maybe I can do this but not now...

Comment 15

[Erik Arvidsson]

Attachment: Creates a text node and an attribute node and both these should have ownerDocument defined

Comment 16

[Brian Clark]

Just tested with Mozilla 0.9.3 on MacOS 9.1 and it is still broken.  

I tested using Netscape's DOM Level 2 Conformance Tests of Node.ownerDocument. 
This is the Netscape test case "tc_hcom500a Test Comment Properties" available
by clicking on the link for DOM Level 2 Conformance on this page: 
http://developer.netscape.com/evangelism/tools/testsuites/

Comment 17

[Julien Wajsberg [:julienw]]

The bug is still there with Build 2001122106 (Mozilla 0.9.7).
it poses problems with an implementation of IE's insertAdjacentHTML suing Ranges
and the proprietary createContextualFragment function.
(see http://webfx.nu/dhtml/ieemu/htmlmodel.html)

Will propose a testcase soon.

Comment 18

[Fabian Guisset]

jst, can we use the nsINodeInfo pointer for text and attr nodes? Do we want to
make this change? I'll be glad to do it if the answers are affirmative...

Comment 19

[Johnny Stenback (:jst)]

#text nodes don't have nsINodeInfo :-( Attribute nodes could simply ask the
"owner" element for its owner document and it should just work.

Comment 20

[Fabian Guisset]

Downgrading to P3 because:
- There are no real world websites affected by this as far as I know
- IE5 fails all the testcases attached in this bug
- It works with Element's

This means it's unlikely to be fixed before 1.0 unless a patch is contributed. 
If you need this fixed or found a real world website (other than webfx.nu, which
we already know about) affected by this bug, please speak up, and we will
reconsider.

Reassigning to jst to get rid of the reopened status, since this bug was never
fixed.

Comment 21

[Jerald Dawson]

well, seeing as how this bug is something that would affect intranets instead of
websites in general, have you ever considered that it is affecting people who
can't put their sites here for review? That is my case. I am developing a Web
App for a large client and am having to limit browser support to IE on windows
becuase of this bug. Specificly, I need to be able to change text that the user
selects in a TextArea control programaticly and IEWin is the only browser that
does that. As a Mac user it burns me up and right now I'm rather disgusted at
the browsers on MacOS becuase of this. So I add my call to this, PLEASE FIX ASAP..

Comment 22

[Johnny Stenback (:jst)]

I fail to see how this bug could possibly cause you problems in trying to do
something like that on a webpage.

Comment 23

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 24

[Julien Wajsberg [:julienw]]

Any news for this bug ?

Like Jerald Dawson, I'm also using this feature on an intranet...

Comment 25

[Peter Van der Beken [:peterv]]

Taking.

Comment 26

[Anindya Paramanik]

 5 div tags are on one page. XML files are being loaded on these five div from a
js file. A separate js file has been used for all javascript functionality. Some
buttons are there on xml files. When I try to access any div by its id or name
from any loaded xml file button I am getting null or undefined . I tried to use
getElementById,getElementByName and properties of DOM like parentNode, childNode
etc. but unable to access any div. It appears quite similar to bug# 27382. Do we
have any solution for it ?

Comment 27

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

That is something compleatly different then this bug. This bug soly affects the
.ownerDocument property of certain nodes.

Comment 28

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 29

[Boris Zbarsky [:bzbarsky]]

that patch looks like it would fix bug 26430 too.

Comment 30

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 31

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

what's different with this new patch? I've already started reviewing the
previous one.

Comment 32

[Peter Van der Beken [:peterv]]

Just updated my tree, no changes in the patch itself.

Comment 33

[Peter Van der Beken [:peterv]]

Hmm, so Jst had a proposal upon looking at the patch: for nsGenericDOMDataNode
get the document from the parent, and if there's no parent store the document
pointer in the parent pointer. He has a patent pending "2-in-1"(tm) trick to be
applied to the bits for the RangeList and EventListenerManager bits: we'd just
store whether one (or both) of them are set and then check the hash whether we
have one or the other. We can then use the freed bit to signal whether the
pointer in mParentPtrBits is an nsIContent or an nsIDocument.
I started going down that road, and I'm still a bit unsure about where I'm
heading. I ended up with not fixing this bug, but just removing mDocument :-).
So let's try to figure out if we can fix this bug with that approach. We need to
store orphan nodes in a hash in the document, so that when the document goes
away the document pointer gets nulled out. When someone tries to get the
ownerDocument, we check if there's a parent and return its ownerDocument. If
there's no parent, then we use the document pointer stored in the parent pointer
if there is one. If there is none, the document has gone away and we're screwed,
but that should be a rare case (it wouldn't happen in JS, right?). When someone
tries to get the parent node, we check if we have a parent and return it or if
there is no parent we get the document pointer and check whether this node is in
the document's orphans hash. If it is in the orphans hash, we return null, if it
isn't we return the document.
When do we store a node in the orphans hash? When someone creates a node through
the DOM API? I think we want to avoid it for our internal APIs, since internaly
we mostly create a node and immediately give it a parent. Should we make sure it
gets a parent by adding a parent argument to the NS_New* functions? I think
probably not, we just need to ensure that every caller knows what it's doing.
And that will include Transformiix with the patch in bug 221335. We need to
remove nodes from the orphans hash once they get a parent.
The document pointers get nulled out when the document goes away.
For elements, this is handled through nsNodeInfoManager::DropDocumentReference.
For the other nodes, either they're a child of an element in which case there's
nothing to null out, they're a child of the document so we can null out the
document pointer in the document's destructor or they're in the orphans hash and
we can null out the document pointer in the document's destructor too.
Do we want to store elements in the orphans hash? I assume we don't, unless we
need it for the fix for bug 52732?
Sound good? Oh, and happy birthday Jst!

Comment 34

[Johnny Stenback (:jst)]

(In reply to comment #33)
> Hmm, so Jst had a proposal upon looking at the patch: for nsGenericDOMDataNode
> get the document from the parent, and if there's no parent store the document
> pointer in the parent pointer. He has a patent pending "2-in-1"(tm) trick to be
> applied to the bits for the RangeList and EventListenerManager bits: we'd just

Yeah, and I might just grant you permission to use that patented idea, even :-)

> I started going down that road, and I'm still a bit unsure about where I'm
> heading. I ended up with not fixing this bug, but just removing mDocument :-).

Ok, I'm fine with taking this change as a separate patch (in its own bug?) if
you prefer that.

> So let's try to figure out if we can fix this bug with that approach. We need to
> store orphan nodes in a hash in the document, so that when the document goes
> away the document pointer gets nulled out. When someone tries to get the
> ownerDocument, we check if there's a parent and return its ownerDocument. If
> there's no parent, then we use the document pointer stored in the parent pointer
> if there is one. If there is none, the document has gone away and we're screwed,
> but that should be a rare case (it wouldn't happen in JS, right?). When 

Correct, if a JS reference is holding a current (or former) child of a document
alive, then that reference would also keep the document alive through the JS
object's reference to its parent (the document, though possibly indirectly).

> there is no parent we get the document pointer and check whether this node is in
> the document's orphans hash. If it is in the orphans hash, we return null, if it
> isn't we return the document.

Correct. This'll make accessing the document (through nsIContent::GetDocument())
on an immediate non-element child of the document a bit slower, but that's a
rare call, so I doubt it matters.

> When do we store a node in the orphans hash? When someone creates a node through
> the DOM API? I think we want to avoid it for our internal APIs, since internaly
> we mostly create a node and immediately give it a parent. Should we make sure 

Yeah, this primarily matters for the DOM, so let's try to not slow down internal
code with this for no reason. Though there might be cases during node removal
where we need to worry, or at least make sure the parent/document pointer gets
set approporiately in all cases to ensure no dangling document pointers.

> gets a parent by adding a parent argument to the NS_New* functions? I think
> probably not, we just need to ensure that every caller knows what it's doing.
> And that will include Transformiix with the patch in bug 221335. We need to
> remove nodes from the orphans hash once they get a parent.

Sounds good. Ideally there will rarely be orphan nodes, which means the orphan
node hash will be empty most of the time, which means we can even avoid hitting
the hash in most cases (if table.entryCount is 0).

> Do we want to store elements in the orphans hash? I assume we don't, unless we
> need it for the fix for bug 52732?

Right, I hope we wouldn't need to put elements in the orphan hash, at least not
yet :-)

> Sound good? Oh, and happy birthday Jst!

Yeah, sounds all good. And thank you :-)

Comment 35

[Peter Van der Beken [:peterv]]

(In reply to comment #34)
> Sounds good. Ideally there will rarely be orphan nodes, which means the orphan
> node hash will be empty most of the time, which means we can even avoid hitting
> the hash in most cases (if table.entryCount is 0).

Well, maybe we can have a one-slot cache too. Seems like most of the time
someone will create a node and insert it into a parent, that would just add and
immediately remove the node from the hash.

Comment 36

[Peter Van der Beken [:peterv]]

And I think that I'll need to do it as part of this bug anyway, since right now
I set a document in those nodes in their constructor, so if they never get
inserted and the document goes away that'll lead to a problem :-).
Jonas: I'm not sure how far you are with your review but maybe we should wait
with the reviews while I fix this problem?

Comment 37

[Peter Van der Beken [:peterv]]

Attachment: v2

Jst, could you try out this patch on the pageload tests? Thanks.

Comment 38

[Johnny Stenback (:jst)]

Done. No apparent change in Tp. With your changes I got:

Avg. Median : 384 msec		Minimum     : 171 msec
Average     : 385 msec		Maximum     : 855 msec

and w/io your changes I got:

Avg. Median : 387 msec		Minimum     : 173 msec
Average     : 388 msec		Maximum     : 843 msec

Note that these tests were run on a fairly fast windows system, so I don't know
how reliable these numbers are, but they're consistent, that I know. So who
knows, doesn't seem slower at least :-)

Comment 39

[Peter Van der Beken [:peterv]]

Attachment: v2.1

Comment 40

[Johnny Stenback (:jst)]

Comment on attachment 150597 [details] [diff] [review]
v2.1

- In content/base/public/nsIContent.h

Rev the IID.

- In content/base/public/nsIDocument.h

Rev the IID.

+  virtual PRBool IsOrphan(const nsIContent* aContent) = 0;

You'd save yourselfs some casts if this wasn't const. Any particular reson for
it being const?

- In nsDocument::AddOrphan():

+    if (!mOrphans) {
+      mOrphans = new nsOrphansHash;
+      if (!mOrphans || !mOrphans->Init(5)) {
+	 mOrphans = nsnull;
+
+	 return PR_FALSE;
+      }

An alternative approach here would be to have a global ophan->document hash, in
stead of a hash per document. Might save us some memory, even. Performace wise
it should be comparable, since the hash should be empty most of the time. And
I'd make it a global static PLDHashTable, so no need to malloc it, just
initialize etc (and since we're dealing with pointer keys, we can use the
PLDHashTable stubs for all ops). Thoughts?

- In nsGenericDOMDataNode.cpp:

+  if (ParentIsContent()) {
+    nsIContent *parent = ParentPtrBitsAsContent();
+    if (parent) {
...

There's code like this all over this file, how about adding a method that
checks if the parent bits are an nsIContent and rerurns it, or null if not, so
that we don't need to first check, get the ptr, and then check the ptr. I.e.
I'd like to see code that does something like:

+    nsIContent *parent = ParentPtrBitsAsContent();
+    if (parent) {
...
+    else {
+      nsIDocument *doc = ParentPtrBitsAsDocument();
...

@@ -1012,6 +1115,8 @@ nsGenericDOMDataNode::SplitText(PRUint32
     parent->InsertChildAt(content, index+1, PR_TRUE, PR_FALSE);
   }

+  // XXX Shouldn't we handle the case where this is a child of the document?
+

Hmm, probably. Though per the DOM spec, there are no text nodes as part of the
document, but PI's are allowed... Probably good to file a bug to figure this
out...

Didn't get through all of this, next up, nsGenericElement.cpp

Comment 41

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

i'll try to do this this weekend

Comment 42

[Johnny Stenback (:jst)]

Comment on attachment 150597 [details] [diff] [review]
v2.1

The rest looks good to me. Let's get this landed, r+sr=jst!

Comment 43

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

Mind letting me have a lookse on this before landing? I have some things I
wanted to look for while reviewing (mostly perf related). I should be able to
latest tomorrow.

Comment 44

[Johnny Stenback (:jst)]

Fine with me, assuming peterv can wait (which I imagine he can, if you can get
to this real soon).

Comment 45

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

First off, I really like the direction this patch takes us. Even though we're
left in a somewhat more complex situation it's closer to where we want to be. I
especially like the deprecation of GetDocument().

So to actual comments:

Does GetCurrentDoc really need to be virtual? And making GetParent virtual again
kind'a sucks... It was nice to be able to step up the parent chain really fast.
Have you checked performance impacts on for example XSLT (compareTreePosition or
whatever it's called calls GetParent a lot).

Another thing that just occured to me. If we were to give all
nsGenericDOMDataNode an mNodeInfo we could move mNodeInfo into nsIContent. That
way some operations in there could be greatly sped up, like GetTag and
GetOwnerDoc. Granted we wouldn't be able to do the share-parent-and-document
trickery, but i'm not so convinced that's a good idea anyway due to the virtual
GetParent. All data-nodes would share the same nodeinfo of course, so we'd just
have one more per document.

Of course, at this point that might be better to just land as is and ponder this
afterwards.

+PRBool
+nsGenericDOMDataNode::IsInDoc() const
+{
+  if (ParentIsContent()) {
+    nsIContent *parent = ParentPtrBitsAsContent();
+
+    return parent->IsInDoc();

shouldn't that be |parent && parent->IsInDoc()|?

Btw, all this logic to datanodes to keep track of parent being nsIContent or
nsIDocument brings back my thoughts that nsIDocument really should extend
nsIContent. The same pains are gone through in transformiix and in the
treewalker and i bet in more places too.


I'm not quite sure I understand the purpose of the orphan-hash. Is it because
we're out of bits in datanodes so we can't flag it directly there?
I'm also not sure I understand why the SetDocument calls in the sinks and the
CSSFrameConstructor are going, but it's too much 3am here...

(And we should really kill TestAttributes.cpp, i don't think it has built for ages)

Comment 46

[Johnny Stenback (:jst)]

(In reply to comment #45)
> I'm not quite sure I understand the purpose of the orphan-hash. Is it because
> we're out of bits in datanodes so we can't flag it directly there?

No, the orphan hash is there so that the document can clear the document pointer
in orphan nodes when a document goes away. Nodes hold weak references to the
document, and if a node ends up being pulled out of the document, and the
document is destroyed before the node goes away, the document needs to be able
to clear the document pointer in the remaining node, which is no longer
referenced from the document tree...

Comment 47

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

Ah, makes sense.

Of course, in the lateness of last night, I forgot to look at the thing that i
first indended to look at; Uses of GetParent() that assumed that it was a fast
operation.

In bug 224331 bryner did a lot of s/mParent/GetParent()/ assuming that
GetParent() was a very fast call. So it is for example used inside of inner
loops and such. However now that GetParent() turns into a virtual call again I
think we might take a perfhit unless someone goes through and breaks out a

|nsIContent* parent = GetParent()|

from loops and up to top of functions.

Alternativly nsGenericElement could have an GetParentFast() function that is
inlined and nonvirtual and use that one instead...

Comment 48

[Peter Van der Beken [:peterv]]

(In reply to comment #47)
> In bug 224331 bryner did a lot of s/mParent/GetParent()/ assuming that
> GetParent() was a very fast call. So it is for example used inside of inner
> loops and such. However now that GetParent() turns into a virtual call again I
> think we might take a perfhit unless someone goes through and breaks out a
> 
> |nsIContent* parent = GetParent()|
> 
> from loops and up to top of functions.

There's very little of these. I think I found one or two loops and a couple of
places where we called GetParent three times in a row. I'd be very surprised if
those make a difference performance-wise. Anyway, I've fixed most of them.

Comment 49

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

Oh, i missed that you had already done pageloading tests. That's the part I was
mostly concerned about with these 'new' virtual functions. It might still affect
XSLT, but probably not to an extent that we care.

I still want to investigate my idea of moving mNodeInfo into nsIContent but that
should absolutly be done separatly from this bug. If for no other reason to see
how it affects bloat vs perf.

Comment 50

[Peter Van der Beken [:peterv]]

Attachment: Fix for attribute nodes v1

Comment 51

[Peter Van der Beken [:peterv]]

This did regress pageload times, I filed bug 255022 for those.

Comment 52

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 155683 [details] [diff] [review]
Fix for attribute nodes v1

r+sr=bzbarsky

Comment 53

[Peter Van der Beken [:peterv]]

Attachment 155683 [details] [diff] checked in. I backed out a large part of attachment 150597 [details] [diff] [review] to
fix the Tp regression, which means this bug is only fixed for attributes.

Comment 54

[Peter Van der Beken [:peterv]]

Attachment: Backout

This is what I backed out.

Comment 55

[OstGote!]

Besides bugs Bug 255154 (@nsDocument::RemoveOrphans) and Bug 255165 I got with
Mozilla 2004081008 TB533208Q and TB533489G (@RemoveOrphanFromDocument,
@0x00000021). I guess it is related to this. Seems to be a top crasher. I will
check it with newer builds soon.

http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=stacksig&match=contains&searchfor=RemoveOrphan&vendor=All&product=All&platform=All&buildid=&sdate=&stime=00%3A00%3A00&edate=&etime=23%3A59%3A59

Comment 56

[Peter Van der Beken [:peterv]]

Not a problem anymore since that code was backed out.

Comment 57

[[e-mail]]

Is there a reason why CloneContent isn't declared virtual in the base class?

Comment 58

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

Since we now have an mDocument again I think my idea in comment 45 and bug
276698 comment 5 might be worth investigating...

Ack, i miss mozilla hacking

Comment 59

[Peter Van der Beken [:peterv]]

The plan is to get rid of mDocument once we have BindToTree (or however it'll be
called), and I don't really like the idea of giving all nodes a nsNodeInfo.

Comment 60

[Peter Van der Beken [:peterv]]

Attachment: Fix for data nodes v1

So this fixes it for the remaining nodes (text, comment, ...). What do people
think, do we go with this or is the "give data nodes a nodeinfo" approach
preferred? We might as well do that directly if it's where we're going in the
long run.

Comment 61

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

Won't this have the same performance problems as the last patch?

Comment 62

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

Attachment: Move mNodeInfo to nsIContent

This moves nsINodeInfo to nsIContent. No nodes actually get bigger since I can
remove mDocument on all the nodes that didn't have a nodeinfo before. I made
nsNodeInfoManager cache the nodeinfos used for comments and textnodes so there
should be no performance hit from getting a nodeinfo for each textnode.

NS_New(Text|Comment|etc)Node will take an ownerDocument that is used to get the
nodeinfo. I also added a version of NS_NewTextNode that takes an
nsNodeInfoManager to avoid having to go get the document in a pile of places
(and bail if the document is null).

This patch should actually be a speedup since I could inline a pile of
functions on nsIContent (GetDocument, IsInDoc, GetCurrentDoc, GetOwnerDoc,
GetNameSpaceID, Tag and GetNodeInfo)

Comment 63

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

Attachment: Move mNodeInfo to nsIContent -w

Comment 64

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

Note that my patch actually *does* make textnodes 4 bytes bigger then petervs
approach, since his reduces the size of textnnodes. I still think it's the way
to go though given the performance advantages

Comment 65

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 185305 [details] [diff] [review]
Move mNodeInfo to nsIContent  -w

>Index: content/base/public/nsIContent.h
>   // Subclasses may use the low two bits of mParentPtrBits to store other data
>+  enum { PARENT_BIT_INDOCUMENT = 0x01 };

s/low two bits/the 0x2 bit/ perhaps?

>Index: content/base/public/nsITextContent.h

>+ * This is a shortcut method to avoid having to grab a document when
>+ * you already have a nodeinfomanager.

Given that it's simple to get the nodeinfo manager from a document, do we even
need the version that takes a document?  The reason I ask is that the
nodeinfomanager holds a weak ref to the document.  Nodes hold strong refs to
the nodeinfo manager via nodeinfos.  So creating a text-like node of some sort
from an existing element, or via cloning, or whatever can always get the
nodeinfo manager from the nodeinfo but may not be able to get a document...  If
we don't have the document version people can't as accidentally write bogus
code that crashes once the document is gone.  Similar for other cases that take
an owner doc just because they want a nodeinfo manager; might want a separate
patch for those.

>Index: content/base/src/nsCommentNode.cpp

>-  virtual already_AddRefed<nsITextContent> CloneContent(PRBool aCloneText,
>-                                                        nsIDocument *aOwnerDocument);
>+  virtual already_AddRefed<nsITextContent> CloneContent(PRBool aCloneText);

You might want to coordinate with peterv's fix for bug 251025 here.  As in,
maybe leave the aOwnerDocument arg for now, or change it to aNodeInfo or
something.

>Index: content/base/src/nsContentUtils.cpp
>@@ -790,17 +782,13 @@ nsContentUtils::ReparentContentWrapper(n
>-    nsINodeInfo *ni = aContent->GetNodeInfo();
>-
>-    if (ni) {
>-      old_doc = nsContentUtils::GetDocument(ni);
>-    }
>+    old_doc = aContent->NodeInfo()->GetDocument();

How about just aContent->GetOwnerDoc()?  Same amount of code, but clearer
what's going on....

>Index: content/base/src/nsDOMAttributeMap.cpp
>@@ -228,13 +228,13 @@ nsDOMAttributeMap::SetNamedItemInternal(
>-      nsINodeInfo *contentNi = mContent->GetNodeInfo();
>+      nsINodeInfo *contentNi = mContent->NodeInfo();
>       NS_ENSURE_TRUE(contentNi, NS_ERROR_FAILURE);

That can become an assert.

>@@ -249,13 +249,13 @@ nsDOMAttributeMap::SetNamedItemInternal(
>-        nsINodeInfo *contentNi = mContent->GetNodeInfo();
>+        nsINodeInfo *contentNi = mContent->NodeInfo();
>         NS_ENSURE_TRUE(contentNi, NS_ERROR_FAILURE);

Same.

>@@ -329,18 +329,18 @@ nsDOMAttributeMap::Item(PRUint32 aIndex,
>-    nsINodeInfo *contentNi = mContent->GetNodeInfo();
>+    nsINodeInfo *contentNi = mContent->NodeInfo();
>     NS_ENSURE_TRUE(contentNi, NS_ERROR_FAILURE);

Same.  Except contentNi would be completely unused then, so I think you can
remove these two lines altogether.

>Index: content/base/src/nsDOMDocumentType.cpp

> NS_NewDOMDocumentType(nsIDOMDocumentType** aDocType,
>+                      nsIDocument* aOwnerDoc,

Again, may be more straightforward to just require an nsNodeInfoManager*
here... Though I guess we create document-less nsDOMDocumentTypes via
DOMImplementation, so you have to deal with this anyway...

>Index: content/base/src/nsNodeInfoManager.h
>+   * Returns the nodeinfo for text nodes. Will always return non-null.
>+   */
>+  nsINodeInfo* GetTextNodeInfo();
>+  nsINodeInfo* GetCommentNodeInfo();

Except on out-of-memory... document that, please.  The current comment is very
misleading.  Or make the comment true, create those in Init(), drop the Get
prefix, and remove the null-checks on callsites?  Either way is fine by me
(though I think I prefer the latter -- the memory use is not large, but the
code cleanup would be very nice).

>@@ -116,11 +116,13 @@ private:
>+  nsCOMPtr<nsINodeInfo> mTextNodeInfo;
>+  nsCOMPtr<nsINodeInfo> mCommentNodeInfo;

Don't those have strong refs to the manager?  This looks like it leaks.

>Index: content/base/src/nsTextNode.cpp
> NS_NewTextNode(nsITextContent** aInstancePtrResult,
>-               nsIDocument *aOwnerDocument)
>+               nsIDocument *aOwnerDoc)

If we do keep both of these, just have this call straight through to the
nodeinfo manager version, ok?

>Index: content/xul/templates/src/nsXULContentBuilder.cpp
>+            // XXX should this check |child| rather then |element|? Otherwise
>+            //     it should be moved outside the inner loop.

File a followup bug on this, please.

>Index: layout/base/nsCSSFrameConstructor.cpp
>@@ -4021,24 +4021,19 @@ nsCSSFrameConstructor::TableProcessChild

>+      // Sometimes aChildContent is a #text node.  In those cases it we want

s/it we/we/

r- because of the ownership thing, but this looks fantastic if we can resolve
that!

Comment 66

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

(In reply to comment #65)
> Given that it's simple to get the nodeinfo manager from a document, do we even
> need the version that takes a document?  The reason I ask is that the
> nodeinfomanager holds a weak ref to the document.  Nodes hold strong refs to
> the nodeinfo manager via nodeinfos.  So creating a text-like node of some sort
> from an existing element, or via cloning, or whatever can always get the
> nodeinfo manager from the nodeinfo but may not be able to get a document...  
> If we don't have the document version people can't as accidentally write bogus
> code that crashes once the document is gone.

This is the exact reason I wrote the version that takes a nodeinfomanager. The
reason I kept the doc-version around is that a document is a more logical
datatype to take and people will have an easier time to understand it. I'm not
too worried that people will get a document and forget to nullcheck it. If they
do they have bigger problems ;)
I'll make the doc-version be a simple inline that forwards to the other one,
that'll make it explicit how things work.

> Similar for other cases that take
> an owner doc just because they want a nodeinfo manager; might want a separate
> patch for those.

All other types then textnodes we create in so few places that I didn't think it
was worth it to go through the trouble of having a nodeinfomanager version.

> >+  virtual already_AddRefed<nsITextContent> CloneContent(PRBool aCloneText);
> 
> You might want to coordinate with peterv's fix for bug 251025 here.  As in,
> maybe leave the aOwnerDocument arg for now, or change it to aNodeInfo or
> something.

I think that'll use a separate version since the above is specific to
textcontent (the aCloneText argument). In fact, it's not even exposed on any
interfaces.

> > NS_NewDOMDocumentType(nsIDOMDocumentType** aDocType,
> >+                      nsIDocument* aOwnerDoc,
> 
> Again, may be more straightforward to just require an nsNodeInfoManager*
> here... Though I guess we create document-less nsDOMDocumentTypes via
> DOMImplementation, so you have to deal with this anyway...

See above.

> >@@ -116,11 +116,13 @@ private:
> >+  nsCOMPtr<nsINodeInfo> mTextNodeInfo;
> >+  nsCOMPtr<nsINodeInfo> mCommentNodeInfo;
> 
> Don't those have strong refs to the manager?  This looks like it leaks.

Good catch! Made those members rawpointer and null them out in RemoveNodeInfo
when removed from the hash. This means that I'll have to lazily create them so
i'll have to keep the 'Get' prefix on those getters. There are only a couple of
callers so it's no biggie.

Comment 67

[Peter Van der Beken [:peterv]]

(In reply to comment #66)
> (In reply to comment #65)
> > You might want to coordinate with peterv's fix for bug 251025 here.  As in,
> > maybe leave the aOwnerDocument arg for now, or change it to aNodeInfo or
> > something.
> 
> I think that'll use a separate version since the above is specific to
> textcontent (the aCloneText argument). In fact, it's not even exposed on any
> interfaces.

You could have just looked at my patch. I did change the name of the function,
but it still takes a ownerDocument.

Comment 68

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

Attachment: nsIContent::mNodeInfo v2

Comment 69

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

Attachment: nsIContent::mNodeInfo v2 -w

Addresses bzs comments except for the make-factories-take-nodeinfomanager. I
don't feel too strongly either way about it though i sort of like it this way.

Also added an nsINodeInfo argument to the CloneContent function to prepare for
bug 251025.

Comment 70

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

Comment on attachment 185880 [details] [diff] [review]
nsIContent::mNodeInfo v2 -w

Crap, i just realized this gets the refcount wrong for the cached nodeinfos.
New patch comming up.

Comment 71

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

Attachment: nsIContent::mNodeInfo v2.1 -w

This should be better

Comment 72

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 185892 [details] [diff] [review]
nsIContent::mNodeInfo v2.1 -w

r=bzbarsky.

Comment 73

[Peter Van der Beken [:peterv]]

Comment on attachment 185892 [details] [diff] [review]
nsIContent::mNodeInfo v2.1 -w

Mostly nits.

>Index: content/base/public/nsINodeInfo.h
>===================================================================

>@@ -135,13 +135,13 @@ public:

>-      SetDOMStringToNull(aPrefix);
>+      aPrefix.SetIsVoid(PR_TRUE);

There's no need to change this.

>@@ -261,13 +261,16 @@ public:

>-  virtual nsIDocument* GetDocument() const = 0;
>+  nsIDocument* GetDocument() const
>+  {
>+    return mOwnerManager->GetDocument();
>+  }

I don't like this change (and the removal of nsContentUtils::GetDocument). If
you really want to do this then you should bite the bullet and remove this
function from the interface (replace callers with
->NodeInfoManager()->GetDocument()).

>Index: content/base/public/nsIStyledContent.h
>===================================================================

> // IID for the nsIStyledContent class
>+// c59f05f5-6e39-4e98-a1ea-6c555cb7813c
> #define NS_ISTYLEDCONTENT_IID   \
>-{ 0xa7b53093, 0x3516, 0x4392, { 0xb3, 0x3e, 0x12, 0xc0, 0x0d, 0xe7, 0x85, 0xaa } };
>+{ 0xc59f05f5, 0x6e39, 0x4e98, { 0xa1, 0xea, 0x6c, 0x55, 0x5c, 0xb7, 0x81, 0x3c } };

Long line.

>Index: content/base/src/nsDOMAttribute.cpp
>===================================================================

>@@ -263,14 +263,13 @@ nsDOMAttribute::GetFirstChild(nsIDOMNode
>   if (NS_OK != result) {
>     return result;
>   }
>   if (!value.IsEmpty()) {
>     if (!mChild) {
>       nsCOMPtr<nsITextContent> content;
>-
>-      result = NS_NewTextNode(getter_AddRefs(content));
>+      result = NS_NewTextNode(getter_AddRefs(content), mNodeInfo->NodeInfoManager());

Long line.

>Index: content/base/src/nsDOMDocumentType.cpp
>===================================================================

> nsresult
> NS_NewDOMDocumentType(nsIDOMDocumentType** aDocType,
>+                      nsIDocument* aOwnerDoc,
>                       nsIAtom *aName,
>                       nsIDOMNamedNodeMap *aEntities,
>                       nsIDOMNamedNodeMap *aNotations,
>                       const nsAString& aPublicId,
>                       const nsAString& aSystemId,
>                       const nsAString& aInternalSubset)

>+    nimgr = new nsNodeInfoManager();
>+    NS_ENSURE_TRUE(nimgr, NS_ERROR_OUT_OF_MEMORY);
>+    
>+    rv = nimgr->Init(nsnull);

We could make this take a principal and call SetDocumentPrincipal?
nsDOMImplementation does have a mBaseURI.

>Index: content/base/src/nsDocumentFragment.cpp
>===================================================================

> NS_IMETHODIMP    
> nsDocumentFragment::GetOwnerDocument(nsIDOMDocument** aOwnerDocument)
> {
>-  NS_ENSURE_ARG_POINTER(aOwnerDocument);
>-
>-  if (!mOwnerDocument) {
>+  nsIDocument* doc = GetOwnerDoc();
>+  if (!doc) {
>     *aOwnerDocument = nsnull;
>     return NS_OK;
>   }
> 
>-  return CallQueryInterface(mOwnerDocument, aOwnerDocument);
>+  return CallQueryInterface(doc, aOwnerDocument);
> }

I'd do

  NS_IMETHOD	GetOwnerDocument(nsIDOMDocument** aOwnerDocument)
  { return nsGenericElement::GetOwnerDocument(aOwnerDocument); }

in nsDocumentFragment.h

>Index: content/base/src/nsGenericDOMDataNode.cpp
>===================================================================

>@@ -643,13 +674,13 @@ nsGenericDOMDataNode::BindToTree(nsIDocu
>   return NS_OK;
> }
> 
> void
> nsGenericDOMDataNode::UnbindFromTree(PRBool aDeep, PRBool aNullParent)
> {
>-  mDocument = nsnull;
>+  mParentPtrBits &= ~PARENT_BIT_INDOCUMENT;
>   if (aNullParent) {
>     mParentPtrBits &= nsIContent::kParentBitMask;
>   }
> }

We could move this to nsIContent and call it from nsGenericElement. Up to you.

>Index: content/base/src/nsNodeInfoManager.h
>===================================================================

>@@ -116,11 +116,13 @@ private:

>   PLHashTable *mNodeInfoHash;
>   nsIDocument *mDocument; // WEAK
>   nsCOMPtr<nsIPrincipal> mPrincipal;
>+  nsINodeInfo* mTextNodeInfo; // WEAK to avoid circular ownership
>+  nsINodeInfo* mCommentNodeInfo; // WEAK to avoid circular ownership

  nsINodeInfo *mTextNodeInfo; // WEAK to avoid circular ownership
  nsINodeInfo *mCommentNodeInfo; // WEAK to avoid circular ownership

>Index: content/base/src/nsScriptLoader.cpp
>===================================================================

>@@ -254,29 +254,24 @@ nsScriptLoader::InNonScriptingContainer(

>+    nsIAtom *localName = content->Tag();
> 
>       // XXX noframes and noembed are currently unconditionally not
>       // displayed and processed. This might change if we support either
>       // prefs or per-document container settings for not allowing
>       // frames or plugins.
>       if (content->IsContentOfType(nsIContent::eHTML) &&
>           ((localName == nsHTMLAtoms::iframe) ||
>            (localName == nsHTMLAtoms::noframes) ||
>            (localName == nsHTMLAtoms::noembed))) {

Change this to a bunch of ->Equals()?

>Index: content/events/src/nsEventStateManager.cpp
>===================================================================

>@@ -4950,13 +4950,13 @@ nsEventStateManager::IsFrameSetDoc(nsIDo

>-          nsINodeInfo *ni = childContent->GetNodeInfo();
>+          nsINodeInfo *ni = childContent->NodeInfo();
> 
>           if (childContent->IsContentOfType(nsIContent::eHTML) &&
>               ni->Equals(nsHTMLAtoms::frameset)) {

	  if (childContent->NodeInfo()->Equals(nsHTMLAtoms::frameset)) {

>Index: content/events/src/nsXMLEventsManager.cpp
>===================================================================

>@@ -53,13 +53,13 @@ PRBool nsXMLEventsListener::InitXMLEvent

>-  if (aContent->GetNodeInfo()->Equals(nsHTMLAtoms::listener, kNameSpaceID_XMLEvents))
>+  if (aContent->NodeInfo()->Equals(nsHTMLAtoms::listener, kNameSpaceID_XMLEvents))

Long line.

>@@ -372,13 +372,13 @@ nsXMLEventsManager::AttributeChanged(nsI

>-    if (aContent->GetNodeInfo()->Equals(nsHTMLAtoms::listener, kNameSpaceID_XMLEvents)) {
>+    if (aContent->NodeInfo()->Equals(nsHTMLAtoms::listener, kNameSpaceID_XMLEvents)) {

Long line.

>Index: content/html/content/src/nsGenericHTMLElement.cpp
>===================================================================

>@@ -1336,14 +1334,14 @@ nsGenericHTMLElement::BindToTree(nsIDocu
> already_AddRefed<nsIDOMHTMLFormElement>
> nsGenericHTMLElement::FindForm(nsIForm* aCurrentForm)

>-    if (content->IsContentOfType(nsIContent::eHTML) &&
>-        content->GetNodeInfo()->Equals(nsHTMLAtoms::form)) {
>+    if (content->Tag() == nsHTMLAtoms::form &&
>+        content->IsContentOfType(nsIContent::eHTML)) {

Leave that as ->Equals?

> static PRBool
> IsArea(nsIContent *aContent)
> {
>-  nsINodeInfo *ni = aContent->GetNodeInfo();
>-
>-  return (ni && ni->Equals(nsHTMLAtoms::area) &&
>+  return (aContent->Tag() == nsHTMLAtoms::area &&
>           aContent->IsContentOfType(nsIContent::eHTML));

Leave that as ->Equals?

>Index: content/html/content/src/nsHTMLSelectElement.cpp
>===================================================================

> static PRBool IsOptGroup(nsIContent *aContent)
> {
>-  nsINodeInfo *ni = aContent->GetNodeInfo();
>-
>-  return (ni && ni->Equals(nsHTMLAtoms::optgroup) &&
>+  return (aContent->Tag() == nsHTMLAtoms::optgroup &&

Leave that as ->Equals?

>Index: content/html/document/src/nsHTMLDocument.cpp
>===================================================================

>@@ -1727,17 +1721,17 @@ nsHTMLDocument::MatchAnchors(nsIContent 
>       NS_ASSERTION(htmldoc,
>                    "Huh, how did this happen? This should only be used with "
>                    "HTML documents!");
>     }
> #endif
> 
>+  nsINodeInfo *ni = aContent->NodeInfo();
>     if (ni->Equals(nsHTMLAtoms::a,
>                    aContent->GetCurrentDoc()->GetDefaultNamespaceID())) {

I'd do

    PRInt32 namespaceID = aContent->GetCurrentDoc()->GetDefaultNamespaceID();
    if (aContent->NodeInfo()->Equals(nsHTMLAtoms::a, namespaceID)) {

>Index: content/xbl/src/nsXBLContentSink.cpp
>===================================================================

>@@ -167,14 +167,14 @@ nsXBLContentSink::FlushText(PRBool aCrea

>-  if (content && (content->GetNodeInfo()->NamespaceEquals(kNameSpaceID_XBL) || (
>-      content->GetNodeInfo()->NamespaceEquals(kNameSpaceID_XUL) &&
>+  if (content && (content->NodeInfo()->NamespaceEquals(kNameSpaceID_XBL) || (

Move that parenthesis down.

>Index: content/xbl/src/nsXBLPrototypeBinding.cpp
>===================================================================

>@@ -760,13 +759,13 @@ nsXBLPrototypeBinding::LocateInstance(ns

>-    nsINodeInfo *ni = templParent->GetNodeInfo();
>+    nsINodeInfo *ni = templParent->NodeInfo();
> 
>     if (ni->Equals(nsXBLAtoms::children, kNameSpaceID_XBL)) {

    if (templParent->NodeInfo()->Equals(nsXBLAtoms::children,
					kNameSpaceID_XBL)) {

>@@ -987,14 +988,14 @@ DeleteAttributeTable(nsHashKey* aKey, vo

> void
> nsXBLPrototypeBinding::ConstructAttributeTable(nsIContent* aElement)

>-  nsINodeInfo* nodeInfo = aElement->GetNodeInfo();
>-  if (nodeInfo && !nodeInfo->Equals(nsXBLAtoms::children,
>+  nsINodeInfo* nodeInfo = aElement->NodeInfo();
>+  if (!nodeInfo->Equals(nsXBLAtoms::children,
>                                     kNameSpaceID_XBL)) {

  if (!aElement->NodeInfo()->Equals(nsXBLAtoms::children, kNameSpaceID_XBL) {

>Index: content/xml/document/src/nsXMLFragmentContentSink.cpp
>===================================================================

>@@ -279,13 +279,13 @@ nsXMLFragmentContentSink::HandleProcessi

>-  result = NS_NewXMLProcessingInstruction(getter_AddRefs(node), target, data);
>+  result = NS_NewXMLProcessingInstruction(getter_AddRefs(node), target, data, mTargetDocument);

Long line.

>Index: content/xul/content/src/nsXULElement.cpp
>===================================================================

>@@ -1254,14 +1254,14 @@ nsXULElement::RemoveChildAt(PRUint32 aIn

>-    nsINodeInfo *ni = oldKid->GetNodeInfo();
>-    if (ni && ni->Equals(nsXULAtoms::listitem, kNameSpaceID_XUL)) {
>+    nsINodeInfo *ni = oldKid->NodeInfo();
>+    if (ni->Equals(nsXULAtoms::listitem, kNameSpaceID_XUL)) {

  if (oldKid->NodeInfo()->Equals(nsXULAtoms::listitem, kNameSpaceID_XUL)) {

>Index: content/xul/templates/src/nsXULContentBuilder.cpp
>===================================================================

>@@ -1376,14 +1377,16 @@ nsXULContentBuilder::RemoveGeneratedCont

>+            // XXX should this check |child| rather then |element|? Otherwise

s/rather then/instead of/

>+            //     it should be moved outside the inner loop. Bug 297290.
>+            if (element->NodeInfo()->Equals(nsXULAtoms::templateAtom, kNameSpaceID_XUL) ||

Long line.

>+                !element->IsContentOfType(nsIContent::eELEMENT))

This is useless, since we already know element has children. It should probably
be child :-/.

>Index: layout/forms/nsComboboxControlFrame.cpp
>===================================================================

>@@ -2096,25 +2096,25 @@ nsComboboxControlFrame::CreateAnonymousC

>-  NS_NewTextNode(getter_AddRefs(labelContent));
>+  NS_NewTextNode(getter_AddRefs(labelContent),
>+                 nimgr);

Put this on one line.

>Index: layout/generic/nsAreaFrame.cpp
>===================================================================

>@@ -84,13 +84,13 @@ nsAreaFrame::RegUnregAccessKey(nsPresCon

>-  nsINodeInfo *ni = mContent->GetNodeInfo();
>+  nsINodeInfo *ni = mContent->NodeInfo();
> 
>   // only support accesskeys for the following elements
>   if (!ni || !ni->Equals(nsXULAtoms::label, kNameSpaceID_XUL))

No need to null-check ni.

>Index: layout/xul/base/src/tree/src/nsTreeColumns.cpp
>===================================================================

>@@ -516,14 +516,14 @@ nsTreeColumns::EnsureColumns()

>+      if (colContent->NodeInfo()->Equals(nsXULAtoms::treecol,
>+                                         kNameSpaceID_XUL)) { 

Remove the space after the brace.

>Index: accessible/src/msaa/nsAccessibleWrap.cpp
>===================================================================

>@@ -423,14 +423,13 @@ STDMETHODIMP nsAccessibleWrap::get_accRo

>     nsAutoString roleString;
>     if (role != ROLE_CLIENT) {
>       content->GetAttr(kNameSpaceID_XHTML2_Unofficial, nsAccessibilityAtoms::role, roleString);
>     }
>     if (roleString.IsEmpty()) {
>-      nsINodeInfo *nodeInfo = content->GetNodeInfo();
>-      if (nodeInfo) {
>+      nsINodeInfo *nodeInfo = content->NodeInfo();
>         nodeInfo->GetName(roleString);
>         nsAutoString nameSpaceURI;
>         nodeInfo->GetNamespaceURI(nameSpaceURI);
>         if (!nameSpaceURI.IsEmpty()) {
>           // Only append name space if different from that of current document
>           roleString += NS_LITERAL_STRING(", ") + nameSpaceURI;

This relies on GetAttr and GetNamespaceURI to detect elements, please add a
comment to explain that.

>Index: extensions/transformiix/source/base/txAtoms.h
>===================================================================

>+TX_ATOM(processingInstructionTagName, "__moz_pi") \

Undo this change (see below).

>Index: extensions/transformiix/source/xml/parser/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/transformiix/source/xml/parser/Makefile.in,v
>retrieving revision 1.33
>diff -u -6 -p -w -r1.33 Makefile.in
>--- extensions/transformiix/source/xml/parser/Makefile.in	4 Apr 2005 18:38:11 -0000	1.33
>+++ extensions/transformiix/source/xml/parser/Makefile.in	10 Jun 2005 20:54:38 -0000
>@@ -72,7 +72,8 @@ CPPSRCS = txXMLParser.cpp
> FORCE_STATIC_LIB = 1
> 
> include $(topsrcdir)/config/rules.mk
> 
> DEFINES		+= -DXML_DTD -DXML_UNICODE
> 
>-INCLUDES += -I$(srcdir)/../../base -I$(srcdir)/../dom -I$(srcdir)/../../xpath
>+INCLUDES += -I$(srcdir)/../../base -I$(srcdir)/../dom -I$(srcdir)/../../xpath \
>+            -I$(srcdir)/../../xslt

Why?

>Index: extensions/transformiix/source/xpath/txXPathTreeWalker.h
>===================================================================

>@@ -250,14 +251,14 @@ txXPathNodeUtils::localNameEquals(const 

> #else
>     if (aNode.isContent()) {
>-        nsINodeInfo *ni = aNode.mContent->GetNodeInfo();
>-        if (ni) {
>+        nsINodeInfo *ni = aNode.mContent->NodeInfo();
>+        if (!ni->Equals(txXMLAtoms::processingInstructionTagName)) {

!aNode.mContent->IsContentOfType(nsIContent::ePROCESSING_INSTRUCTION)

>             return ni->Equals(aLocalName);

This is actually wrong: comments and textnodes have no expanded name in XPath.
You probably want to do this only for eELEMENT.

Like bz I think the factories should take a nodeinfomanager, all callers have
direct access to one.

Comment 74

[Peter Van der Beken [:peterv]]

Attachment: nsIContent::mNodeInfo v2.2

Updated to tip. Incorporated my review comments. I made the factory functions
take a nodeinfo manager, also made CloneContent take a nodeinfo manager (it's
on nsIContent, and outside of Gecko you can't easily get a nodeinfo in another
document). Added checks for IsContentOfType(eELEMENT) at the beginning of
AddSubtreeToDocument and RemoveSubtreeFromDocument in nsXULDocument, the stuff
they do only applies to elements and they were triggering the assertion that I
added in FindBroadcaster.
Boris, want to give this a quick look before I check it in?

Comment 75

[Boris Zbarsky [:bzbarsky]]

Looks ok to me.  Let's land this.

Comment 76

[Scott MacGregor]

Hey Guys,

The day after this checkin landed, the phishing detector in thunderbird and the
suite stopped working. In particular, 

var msgDocument = document.getElementById('messagepane').contentDocument;
var anchorNodes = msgDocument.getElementsByTagName("a");

anchorNodes.length is suddenly coming back as undefined even when the message
has several anchor elements in it. Could this be fall out from this change
(particularly the nsHTMLDocument changes)?

See Bug 310115 for details.

Comment 77

[Boris Zbarsky [:bzbarsky]]

Sounds like bug 310068.

Comment 78

[Peter Van der Beken [:peterv]]

I'll write a 1.8.0.x branch patch. Current plan is to add a nodeinfo to nsGenericDOMDataNode.

Comment 79

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

alternativly you could hold a strong reference directly to the node info manager.

Comment 80

[Peter Van der Beken [:peterv]]

(In reply to comment #79)
> alternativly you could hold a strong reference directly to the node info
> manager.

I don't think that'll work. We want to access the node info manager through nsIContent (to get the document principal). I think that the only way to do that without changing interfaces is by adding a node info.

Comment 81

[Peter Van der Beken [:peterv]]

Hmm, actually, it seems we only need a ownerDocument.

Comment 82

[Peter Van der Beken [:peterv]]

Attachment: v2.2 ported to 1.8.0 branch

Still need to test this more extensively. Also need to verify that the ...->GetNodeInfo()->... calls are safe.

Comment 83

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

Minusing this as blocker since it turned out that 321299 can be fixed without fixing this.

Comment 84

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

Attachment: updated to branch tip

Comment 85

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

Turns out that this is needed after all, so plussing again.

Comment 86

[Daniel Veditz [:dveditz]]

Moving to 1.8.0.6 to follow bug 321299

Comment 87

[Mike Schroepfer]

Comment on attachment 226539 [details] [diff] [review]
updated to branch tip

Please re-request once reviews are complete

Comment 88

[Daniel Veditz [:dveditz]]

If bug 321299 is blocking 1.8.1 then this should be also.

Comment 89

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

Comment on attachment 226539 [details] [diff] [review]
updated to branch tip

Note that this patch caused the regression in bug 346233 and needs the patch that's in there.

Comment 90

[Daniel Veditz [:dveditz]]

Has this landed on 1.8.1? Doesn't say, but is supposedly required by bug 321299 which is. And caused regression bug 346233 which has been fixed on 1.8.1

Comment 91

[Daniel Veditz [:dveditz]]

Comment on attachment 226539 [details] [diff] [review]
updated to branch tip

removing approval request, this patch was rolled into bug 321299 (please correct if I'm wrong)

Comment 92

[Mike Schroepfer]

Marking fixed1.8.1 per comment 91 - if this is not correct please clear the flag and recomment

Comment 93

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

Fixed together with 321299 on 1.8.0 branch

Comment 94

[alice nodelman [:alice] [:anode]]

https://bugzilla.mozilla.org/attachment.cgi?id=42290&action=view should have correct (non-null) ownerDocument.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/20060901 BonEcho/2.0b2

verified 1.8.1b2

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7) Gecko/20060831 Firefox/1.5.0.7

verified 1.8.0.7