ownerDocument of orphan text and attr nodes is null

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
DOM: Core & HTML
P3
normal
RESOLVED FIXED
18 years ago
3 years ago

People

(Reporter: Erik Arvidsson, Assigned: sicking)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, 4 keywords)

Trunk
mozilla1.9alpha1
dom1, testcase, verified1.8.0.7, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 +
blocking1.8.0.5 -
blocking1.8.0.7 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: required by 321299, URL)

Attachments

(10 attachments, 8 obsolete attachments)

361 bytes, text/html
Details
967 bytes, text/html
Details
661 bytes, text/html
Details
155.87 KB, patch
jst
: review+
Details | Diff | Splinter Review
1.14 KB, patch
Details | Diff | Splinter Review
30.67 KB, patch
Details | Diff | Splinter Review
28.72 KB, patch
Details | Diff | Splinter Review
208.27 KB, patch
Details | Diff | Splinter Review
73.26 KB, patch
Details | Diff | Splinter Review
82.15 KB, patch
sicking
: review+
sicking
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

18 years ago
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.
(Reporter)

Comment 1

18 years ago
Created attachment 5160 [details]
doc == node.ownerDocument?
(Reporter)

Comment 2

18 years ago
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.
Priority: P3 → P2

Comment 3

18 years ago
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.
Status: NEW → ASSIGNED
Target Milestone: M17

Comment 4

17 years ago
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.
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.
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: M17 → Future

Comment 6

17 years ago
Mass update of qa contact
QA Contact: gerardok → janc
Keywords: dom1
Component: DOM Level 1 → DOM Core

Comment 7

17 years ago
QA contact Update
QA Contact: janc → desale

Comment 8

16 years ago
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.

Updated

16 years ago
Blocks: 30838
Taking.
Really taking.
Assignee: vidur → jst
Status: ASSIGNED → NEW

Comment 11

16 years ago
Updating QA contact to Shivakiran Tummala.
QA Contact: desale → stummala
(Reporter)

Comment 12

16 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Reporter)

Comment 13

16 years ago
Created attachment 42289 [details]
Creating a DIV in a frame as well as in same document
(Reporter)

Comment 14

16 years ago
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...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 15

16 years ago
Created attachment 42290 [details]
Creates a text node and an attribute node and both these should have ownerDocument defined

Comment 16

16 years ago
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

16 years ago
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.
Depends on: 52732

Comment 18

16 years ago
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...
#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

16 years ago
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.
Status: REOPENED → NEW
Keywords: testcase
Priority: P2 → P3
Summary: ownerDocument of orphan nodes → ownerDocument of orphan text and attr nodes is null

Comment 21

15 years ago
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..
I fail to see how this bug could possibly cause you problems in trying to do
something like that on a webpage.
Mass-reassigning bugs to dom_bugs@netscape.com
Assignee: jst → dom_bugs

Comment 24

14 years ago
Any news for this bug ?

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

Updated

14 years ago
Blocks: 200784
Taking.
Assignee: general → peterv

Comment 26

14 years ago
 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 ?
That is something compleatly different then this bug. This bug soly affects the
.ownerDocument property of certain nodes.
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla1.8alpha
Created attachment 143601 [details] [diff] [review]
v1
Blocks: 235640
that patch looks like it would fix bug 26430 too.
Blocks: 26430
Attachment #143601 - Flags: review?(bugmail)
Created attachment 146411 [details] [diff] [review]
v1
Attachment #143601 - Attachment is obsolete: true
Attachment #146411 - Flags: review?(bugmail)
Attachment #143601 - Flags: review?(bugmail)
what's different with this new patch? I've already started reviewing the
previous one.
Just updated my tree, no changes in the patch itself.
Blocks: 47903
Blocks: 242348
Attachment #146411 - Flags: superreview?(jst)
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!
(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 :-)
(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.
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?
Created attachment 149301 [details] [diff] [review]
v2

Jst, could you try out this patch on the pageload tests? Thanks.
Attachment #146411 - Attachment is obsolete: true
Attachment #146411 - Flags: superreview?(jst)
Attachment #146411 - Flags: review?(bugmail)
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 :-)
Created attachment 150597 [details] [diff] [review]
v2.1
Attachment #149301 - Attachment is obsolete: true
Attachment #150597 - Flags: superreview?(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
i'll try to do this this weekend
Comment on attachment 150597 [details] [diff] [review]
v2.1

The rest looks good to me. Let's get this landed, r+sr=jst!
Attachment #150597 - Flags: superreview?(jst)
Attachment #150597 - Flags: superreview+
Attachment #150597 - Flags: review+
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.
Fine with me, assuming peterv can wait (which I imagine he can, if you can get
to this real soon).
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)
(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...
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...
(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.
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.
Blocks: 251025
Created attachment 155683 [details] [diff] [review]
Fix for attribute nodes v1
Attachment #155683 - Flags: superreview?(bzbarsky)
Attachment #155683 - Flags: review?(bzbarsky)
This did regress pageload times, I filed bug 255022 for those.
Blocks: 255022
Comment on attachment 155683 [details] [diff] [review]
Fix for attribute nodes v1

r+sr=bzbarsky
Attachment #155683 - Flags: superreview?(bzbarsky)
Attachment #155683 - Flags: superreview+
Attachment #155683 - Flags: review?(bzbarsky)
Attachment #155683 - Flags: review+
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.
Created attachment 155728 [details] [diff] [review]
Backout

This is what I backed out.

Comment 55

13 years ago
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

Not a problem anymore since that code was backed out.

Comment 57

13 years ago
Is there a reason why CloneContent isn't declared virtual in the base class?
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
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.
Created attachment 181636 [details] [diff] [review]
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.
Won't this have the same performance problems as the last patch?
Created attachment 185304 [details] [diff] [review]
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)
Created attachment 185305 [details] [diff] [review]
Move mNodeInfo to nsIContent  -w
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
Assignee: peterv → bugmail
Status: ASSIGNED → NEW
Attachment #185305 - Flags: superreview?(peterv)
Attachment #185305 - Flags: review?(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!
Attachment #185305 - Flags: review?(bzbarsky) → review-
(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.
(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.
Created attachment 185879 [details] [diff] [review]
nsIContent::mNodeInfo v2
Attachment #185304 - Attachment is obsolete: true
Created attachment 185880 [details] [diff] [review]
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.
Attachment #185305 - Attachment is obsolete: true
Attachment #185880 - Flags: superreview?(bzbarsky)
Attachment #185880 - Flags: review?(peterv)
Attachment #185305 - Flags: superreview?(peterv)
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.
Attachment #185880 - Attachment is obsolete: true
Attachment #185880 - Flags: superreview?(bzbarsky)
Attachment #185880 - Flags: review?(peterv)
Created attachment 185892 [details] [diff] [review]
nsIContent::mNodeInfo v2.1 -w

This should be better
Attachment #185892 - Flags: superreview?(peterv)
Attachment #185892 - Flags: review?(bzbarsky)
Comment on attachment 185892 [details] [diff] [review]
nsIContent::mNodeInfo v2.1 -w

r=bzbarsky.
Attachment #185892 - Flags: review?(bzbarsky) → review+
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.
Attachment #185892 - Flags: superreview?(peterv) → superreview+
Created attachment 196196 [details] [diff] [review]
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?
Attachment #185879 - Attachment is obsolete: true
Attachment #185892 - Attachment is obsolete: true
Looks ok to me.  Let's land this.
Status: NEW → RESOLVED
Last Resolved: 16 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.8alpha1 → mozilla1.9alpha

Comment 76

12 years ago
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.
Sounds like bug 310068.

Updated

12 years ago
Depends on: 310927

Updated

12 years ago
No longer depends on: 310927
Blocks: 321299
Flags: blocking1.8.0.5?
Whiteboard: required by 321299
I'll write a 1.8.0.x branch patch. Current plan is to add a nodeinfo to nsGenericDOMDataNode.
alternativly you could hold a strong reference directly to the node info manager.
(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.
Hmm, actually, it seems we only need a ownerDocument.
Created attachment 220971 [details] [diff] [review]
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.
Flags: blocking1.8.0.5? → blocking1.8.0.5+
Flags: blocking1.8.0.5+ → blocking1.8.0.5-
Minusing this as blocker since it turned out that 321299 can be fixed without fixing this.
Created attachment 226539 [details] [diff] [review]
updated to branch tip
Attachment #226539 - Flags: superreview?(bugmail)
Attachment #226539 - Flags: review?(bugmail)
Attachment #226539 - Flags: approval1.8.1?
Attachment #226539 - Flags: approval1.8.0.5?
Turns out that this is needed after all, so plussing again.
Flags: blocking1.8.0.5- → blocking1.8.0.5+
Attachment #226539 - Flags: approval1.8.0.5? → approval1.8.0.5+
Moving to 1.8.0.6 to follow bug 321299
Flags: blocking1.8.0.6+
Flags: blocking1.8.0.5-
Flags: blocking1.8.0.5+
Attachment #226539 - Flags: approval1.8.0.5+ → approval1.8.0.6?

Comment 87

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

Please re-request once reviews are complete
Attachment #226539 - Flags: approval1.8.1?
If bug 321299 is blocking 1.8.1 then this should be also.
Flags: blocking1.8.1?

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+
Attachment #226539 - Flags: superreview?(bugmail)
Attachment #226539 - Flags: superreview+
Attachment #226539 - Flags: review?(bugmail)
Attachment #226539 - Flags: review+
Depends on: 346233
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.
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 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)
Attachment #226539 - Flags: approval1.8.0.7?

Comment 92

11 years ago
Marking fixed1.8.1 per comment 91 - if this is not correct please clear the flag and recomment
Keywords: fixed1.8.1
Fixed together with 321299 on 1.8.0 branch
Keywords: fixed1.8.0.7
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
Keywords: fixed1.8.0.7, fixed1.8.1 → verified1.8.0.7, verified1.8.1
Flags: in-testsuite?

Updated

9 years ago
Component: DOM: Core → DOM: Core & HTML
QA Contact: stummala → general
You need to log in before you can comment on or make changes to this bug.