Currently when a subtree is inserted into the tree we use a mutation observer to traverse that subtree to look for elements with ids/names that needs to be inserted into our id-hash. Similarly when a subtree is removed, or when attributes are modified. It should be faster to let elements take responsibility to register/unregister themselves instead. The attached patch seems to work, but I'm still seeing some issues on tryserver. Figuring out if that's due to this patch or something else. So not ready for review, but should give an idea about how things work.

This passes tryserver. It seems like we have tests covering pretty much any edge case, however I still want to write some tests that cover it all in one place. Also so that we don't accidentally loose some tests down the road as some of the existing tests only tested id handling as a side effect. Will attach this separately.

The atom arg for the name table methods should be called aName, right? I love IsNamedItem going away. I'd prefer that we use IsInAnonymousSubtree() checks instead of directly calling GetBindingParent(). Or use nsContentUtils::IsInSameAnonymousTree (which we should consider just inlining). Or something... I agree that just directly calling GetBindingParent() is faster. :( It's too bad we have to GetBindingParent() at all in nsStyledElement::BindToTree, but the only easy way around is to make BindToTree modify aBindingParent (not just its local value, but make it inout). Probbaly not worth it. You need anonymous content checks in AddTo/RemoveFromNameTable, right? Is ParseAtom guaranteed to succeed? You have code that assumes that GetAtomValue() unconditionally there is ok. FindNamedItems will no longer find nodes by id with your changes... is that ok? I guess the id stuff is completely live. I wonder when we forgot to remove that gunk.... Love the flush removal in ResolveName! I wish the ParseAttribute methods shared more between styled and XML element. Maybe an inline helper function that takes the id attr name? I don't see how the DoGetID thing for XMLElement makes any sense... should we not at least update the NODE_HAS_ID flag there if we parse as atom suddenly? Please have enn look over the xul content builder change? I seem to recall asking him about this at some point, but I forget now what he said. Is this just ok because we'll now add ourselves from BindToTree?

(In reply to comment #2) > The atom arg for the name table methods should be called aName, right? Indeed. They are someplace but not everywhere. Will fix. > I'd prefer that we use IsInAnonymousSubtree() checks instead of directly > calling GetBindingParent(). Or use nsContentUtils::IsInSameAnonymousTree > (which we should consider just inlining). Or something... I agree that just > directly calling GetBindingParent() is faster. :( IsInSameAnonymousTree seems wasteful given that we know the doc never is in anon content. I could call IsInAnonymousSubtree, that's just another flag check so not that much more expensive. Maybe we could even make that faster by only checking a flag (in a separate bug). > It's too bad we have to GetBindingParent() at all in > nsStyledElement::BindToTree, but the only easy way around is to make BindToTree > modify aBindingParent (not just its local value, but make it inout). Probbaly > not worth it. Not sure I follow this. You mean make it impossible to have HTML content inside XBL1? > You need anonymous content checks in AddTo/RemoveFromNameTable, right? Wellll.. I decided to cheat. The result is that XBL1 content ends up in the name table, which really shouldn't affect anyone in reality. So I figured I'd save myself the cycles. Should definitely add a comment though. Really, we could do the same for the id-table. The only place where people use XBL1 (that we care about) is in XUL documents. And XUL documents get XBL1 content added to the id table through the call here: http://mxr.mozilla.org/mozilla-central/source/content/xbl/src/nsXBLBinding.cpp#377 Let me know what you think? > Is ParseAtom guaranteed to succeed? You have code that assumes that > GetAtomValue() unconditionally there is ok. What with infallable malloc, i think we should assume this succeeds. It won't crash exploitably anyway. > FindNamedItems will no longer find nodes by id with your changes... is that ok? > I guess the id stuff is completely live. I wonder when we forgot to remove > that gunk.... It seems like the id-handling code in FindNamedItems is just left over gunk from when we were lazy. So it doesn't do anything any more. > Love the flush removal in ResolveName! Though technically that flush isn't needed (and might be a no-op) with the HTML5 parser turned on. > I wish the ParseAttribute methods shared more between styled and XML element. > Maybe an inline helper function that takes the id attr name? I thought about it but didn't feel like it was worth it. There are just two call sites so breaking it out to a separate function would likely result in not that many lines of saved code. > I don't see how the DoGetID thing for XMLElement makes any sense... should we > not at least update the NODE_HAS_ID flag there if we parse as atom suddenly? NODE_HAS_ID is set already if we get into this function anyway. I guess we could clear it if we get in here and find out that the attribute doesn't exist. Really, this stuff should only happen in very rare edge cases. The ones I can think of are: * Adopting an element between documents with different DTDs * Setting the prefix of an element such that the new name has a different id-name. We're already failing in these cases on trunk. I guess it would be possible to fix, but I don't feel like it's terribly important. Ideally I'd like to remove the whole NODE_HAS_ID optimization for nsXMLElement, but I can't see a way to do that without making the flag-check virtual. > Please have enn look over the xul content builder change? I seem to recall > asking him about this at some point, but I forget now what he said. Is this > just ok because we'll now add ourselves from BindToTree? Yes, I think that makes it ok. But I'll ask Enn to review that part.

Comment on attachment 444674 [details] [diff] [review] Patch to fix (needs tests) Neil: Could you look if the changes to the nsXULContentBuilder changes here are ok. In particular. Do we always add these elements to the tree (using AppendChildTo or InsertChildAt)? If we don't always add it (such as error conditions etc), do you really want to add them to the id-map when they are not added to the tree?

> Not sure I follow this. You mean make it impossible to have HTML content inside > XBL1? No. Your issue is that aBindingParent may be null on entry into nsStyledElement::BindToTree and then will be inferred from aParent in nsGenericElement::BindToTree. If the type of aBindingParent were |nsIContent*&| then on return from nsGenericElement::BindToTree you would know the binding parent without having to call the virtual GetBindingParent: it would be sitting in the aBindingParent variable. > The result is that XBL1 content ends up in the name table, which really > shouldn't affect anyone in reality. I think that's a really bad idea. Let's not do that, please. We've had our share of issues because XBL1 content ends up in the id table, which we're now trying to undo (it's on my todo list). > Though technically that flush isn't needed (and might be a no-op) with the > HTML5 parser turned on. Still needed for XHTML, no (except your change makes it unnecessary). > There are just two call sites so breaking it out to a separate function would > likely result in not that many lines of saved code. Yeah, but the question is whether it would be more maintainable (e.g. harder for them to get out of sync). > NODE_HAS_ID is set already if we get into this function anyway. Ah, true. But then our attr is an atom too, no? How could it still be a string? > but I can't see a way to do that without making the flag-check virtual. So is the point that NODE_HAS_ID would always be true for nsXMLElement? If not, how would you remove that optimization?

(In reply to comment #5) > > Not sure I follow this. You mean make it impossible to have HTML content inside > > XBL1? > > No. Your issue is that aBindingParent may be null on entry into > nsStyledElement::BindToTree and then will be inferred from aParent in > nsGenericElement::BindToTree. If the type of aBindingParent were > |nsIContent*&| then on return from nsGenericElement::BindToTree you would know > the binding parent without having to call the virtual GetBindingParent: it > would be sitting in the aBindingParent variable. Ah. This seems overly complicated and somewhat hacky. And it doesn't handle SetAttr. I'd rather just keep the current check (though migrated to IsInAnonymousSubtree). And none of this should be needed with XBL2 since then we won't bind the shadow tree to the document. > > The result is that XBL1 content ends up in the name table, which really > > shouldn't affect anyone in reality. > > I think that's a really bad idea. Let's not do that, please. We've had our > share of issues because XBL1 content ends up in the id table, which we're now > trying to undo (it's on my todo list). Ok, I'll fix it. > > There are just two call sites so breaking it out to a separate function would > > likely result in not that many lines of saved code. > > Yeah, but the question is whether it would be more maintainable (e.g. harder > for them to get out of sync). I'll look into it. > > NODE_HAS_ID is set already if we get into this function anyway. > > Ah, true. But then our attr is an atom too, no? How could it still be a > string? If the elements mNodeInfo changes to one with a different id-attribute-name. Such as if the element is adopted or its prefix changes. > > but I can't see a way to do that without making the flag-check virtual. > > So is the point that NODE_HAS_ID would always be true for nsXMLElement? If > not, how would you remove that optimization? In the patch NODE_HAS_ID on nsXMLElement is mostly an approximation, and can be wrong in either direction. When it is wrong we sometimes have buggy result. This is already the case on trunk where NODE_MAY_HAVE_ID behaves the exact same way, with the exact same consequences. I'd rather deal with this in a separate bug.

> And it doesn't handle SetAttr. Sure; it just makes bind faster, by a bit. Maybe it doesn't matter in practice... > And none of this should be needed with XBL2 Great. Land it? ;) > Such as if the element is adopted or its prefix changes. Ah, I see. OK. > I'd rather deal with this in a separate bug. Sure.

Comment on attachment 444674 [details] [diff] [review] Patch to fix (needs tests) The content builder changes should be ok. The node should always be inserted, although it looks like currently errors will cause the node not to be inserted.

Why do we still need the id-munging in AddSubtreeToDocument and RemoveSubtreeFromDocument with that patch? Can we make it go away (perhaps in a followup)?

I don't know why it's needed, but things definitely broke if I removed it. Will file followup. Btw, I have a patch that fixes reew comments and adds tests. Will attach shortly

Comment on attachment 444674 [details] [diff] [review] Patch to fix (needs tests) Based on the previous comment there is a new patch coming.

Interdiff to previous patch

This fixes bzs review comments. It also fixes a perf issue in the previous patch by making us clear the id-table and name tables on document teardown, rather than removing each element individually as they are unbound. Finally, it adds tests.

(this one is diffed with the correct diff flags)

Comment on attachment 447494 [details] [diff] [review] Patch v2 A few nits... content/base/public/nsINode.h + // Set if the node has the accesskey attribute set. + NODE_HAS_NAME = 0x00800000U, + s/accesskey/node/ content/base/src/nsDocumentFragment.cpp + virtual nsIAtom* DoGetID() const; + virtual nsIAtom *GetIDAttributeName() const; content/xml/content/src/nsXMLElement.h + virtual nsIAtom *GetIDAttributeName() const; + virtual nsIAtom* DoGetID() const; s/ */* /

Comment on attachment 447494 [details] [diff] [review] Patch v2 >@@ -158,23 +205,38 @@ nsStyledElement::BindToTree(nsIDocument* > nsIContent* aBindingParent, > PRBool aCompileEventHandlers) > { > nsresult rv = nsStyledElementBase::BindToTree(aDocument, aParent, > aBindingParent, > aCompileEventHandlers); > NS_ENSURE_SUCCESS(rv, rv); > >- // XXXbz if we already have a style attr parsed, this won't do >- // anything... need to fix that. >- ReparseStyleAttribute(PR_FALSE); ... >- return rv; >+ if (!IsXUL()) { >+ // XXXbz if we already have a style attr parsed, this won't do >+ // anything... need to fix that. >+ ReparseStyleAttribute(PR_FALSE); >+ } Why this change in this bug? Could you do this in some other bug. >+++ b/content/xul/document/src/nsXULDocument.cpp >@@ -975,35 +975,28 @@ nsXULDocument::AttributeWillChange(nsIDo > NS_PRECONDITION(aAttribute, "Must have an attribute that's changing!"); > > // XXXbz check aNameSpaceID, dammit! > // See if we need to update our ref map. > if (aAttribute == nsGkAtoms::ref || > (aAttribute == nsGkAtoms::id && !aContent->GetIDAttributeName())) { > RemoveElementFromRefMap(aContent); > } >- >- nsXMLDocument::AttributeWillChange(aDocument, aContent, aNameSpaceID, >- aAttribute, aModType); > } Why this removal? nsDocument is still mutation observer so its methods should be called, otherwise it is easy to break something if nsDocument gets AttributeWillChange back. > void > nsXULDocument::AttributeChanged(nsIDocument* aDocument, > nsIContent* aElementContent, PRInt32 aNameSpaceID, > nsIAtom* aAttribute, PRInt32 aModType) > { > NS_ASSERTION(aDocument == this, "unexpected doc"); > > // XXXbz once we change AttributeChanged to take Element, we can nix this line > Element* aElement = aElementContent->AsElement(); > >- // Do this here so that all the exit paths below don't leave this undone >- nsXMLDocument::AttributeChanged(aDocument, aElement, aNameSpaceID, >- aAttribute, aModType); >- Same here. Though you could update the comment. >@@ -1096,65 +1089,46 @@ nsXULDocument::ContentAppended(nsIDocume > // Update our element map > PRUint32 count = aContainer->GetChildCount(); > > nsresult rv = NS_OK; > for (PRUint32 i = aNewIndexInContainer; i < count && NS_SUCCEEDED(rv); > ++i) { > rv = AddSubtreeToDocument(aContainer->GetChildAt(i)); > } >- >- nsXMLDocument::ContentAppended(aDocument, aContainer, aNewIndexInContainer); And here > nsXULDocument::ContentInserted(nsIDocument* aDocument, > nsIContent* aContainer, > nsIContent* aChild, > PRInt32 aIndexInContainer) > { > NS_ASSERTION(aDocument == this, "unexpected doc"); > > AddSubtreeToDocument(aChild); >- >- nsXMLDocument::ContentInserted(aDocument, aContainer, aChild, aIndexInContainer); And here > nsXULDocument::ContentRemoved(nsIDocument* aDocument, > nsIContent* aContainer, > nsIContent* aChild, > PRInt32 aIndexInContainer) > { > NS_ASSERTION(aDocument == this, "unexpected doc"); > > RemoveSubtreeFromDocument(aChild); >- >- nsXMLDocument::ContentRemoved(aDocument, aContainer, aChild, aIndexInContainer); And here.

> Why this change in this bug? It's just preserving the old behavior. nsXULElement used to not call its superclass BindToTree to avoid this reparsing, but this patch changes it to call that method to get the id handling.

(In reply to comment #16) > (From update of attachment 447494 [details] [diff] [review]) > >@@ -158,23 +205,38 @@ nsStyledElement::BindToTree(nsIDocument* > > nsIContent* aBindingParent, > > PRBool aCompileEventHandlers) > > { > > nsresult rv = nsStyledElementBase::BindToTree(aDocument, aParent, > > aBindingParent, > > aCompileEventHandlers); > > NS_ENSURE_SUCCESS(rv, rv); > > > >- // XXXbz if we already have a style attr parsed, this won't do > >- // anything... need to fix that. > >- ReparseStyleAttribute(PR_FALSE); > ... > >- return rv; > >+ if (!IsXUL()) { > >+ // XXXbz if we already have a style attr parsed, this won't do > >+ // anything... need to fix that. > >+ ReparseStyleAttribute(PR_FALSE); > >+ } > Why this change in this bug? Could you do this in some other bug. What bz says in previous comment. See the change to nsXULElement::BindToTree. > >+++ b/content/xul/document/src/nsXULDocument.cpp > >@@ -975,35 +975,28 @@ nsXULDocument::AttributeWillChange(nsIDo > > NS_PRECONDITION(aAttribute, "Must have an attribute that's changing!"); > > > > // XXXbz check aNameSpaceID, dammit! > > // See if we need to update our ref map. > > if (aAttribute == nsGkAtoms::ref || > > (aAttribute == nsGkAtoms::id && !aContent->GetIDAttributeName())) { > > RemoveElementFromRefMap(aContent); > > } > >- > >- nsXMLDocument::AttributeWillChange(aDocument, aContent, aNameSpaceID, > >- aAttribute, aModType); > > } > Why this removal? nsDocument is still mutation observer so its methods should > be called, otherwise > it is easy to break something if nsDocument gets AttributeWillChange back. Not removing these produce a compile error: ../../../../dist/include/nsStubMutationObserver.h:62: error: ‘virtual void nsStubMutationObserver::ContentInserted(nsIDocument*, nsIContent*, nsIContent*, PRInt32)’ is private /Users/sicking/trees/2nd/src/content/xul/document/src/nsXULDocument.cpp:1109: error: within this context

Comment on attachment 447494 [details] [diff] [review] Patch v2 >+nsresult >+nsStyledElement::UnsetAttr(PRInt32 aNameSpaceID, nsIAtom* aAttribute, >+ PRBool aNotify) >+{ >+ PRBool isId = PR_FALSE; >+ if (aAttribute == nsGkAtoms::id && aNameSpaceID == kNameSpaceID_None) { >+ // Have to do this before clearing flag. See RemoveFromIdTable >+ RemoveFromIdTable(); >+ isId = PR_TRUE; >+ } >+ >+ nsresult rv = nsGenericElement::UnsetAttr(aNameSpaceID, aAttribute, aNotify); >+ >+ if (isId) { >+ UnsetFlags(NODE_HAS_ID); >+ } This is wrong after all. UnsetAttr may dispatch mutation event, which re-sets ID attribute. In that case you should not unset the flag. I think you could use AfterSetAttr, that is called before dispatching the mutation event. (and please add a test for this case)

(In reply to comment #18) > What bz says in previous comment. See the change to nsXULElement::BindToTree. ok > Not removing these produce a compile error: > ../../../../dist/include/nsStubMutationObserver.h:62: error: ‘virtual void > nsStubMutationObserver::ContentInserted(nsIDocument*, nsIContent*, nsIContent*, > PRInt32)’ is private > /Users/sicking/trees/2nd/src/content/xul/document/src/nsXULDocument.cpp:1109: > error: within this context Ah, good. Then just remove those useless calls.

(In reply to comment #19) > (From update of attachment 447494 [details] [diff] [review]) > >+nsresult > >+nsStyledElement::UnsetAttr(PRInt32 aNameSpaceID, nsIAtom* aAttribute, > >+ PRBool aNotify) > >+{ > >+ PRBool isId = PR_FALSE; > >+ if (aAttribute == nsGkAtoms::id && aNameSpaceID == kNameSpaceID_None) { > >+ // Have to do this before clearing flag. See RemoveFromIdTable > >+ RemoveFromIdTable(); > >+ isId = PR_TRUE; > >+ } > >+ > >+ nsresult rv = nsGenericElement::UnsetAttr(aNameSpaceID, aAttribute, aNotify); > >+ > >+ if (isId) { > >+ UnsetFlags(NODE_HAS_ID); > >+ } > This is wrong after all. > UnsetAttr may dispatch mutation event, which re-sets ID attribute. > In that case you should not unset the flag. I'm actually fixing this in bug 568188, would you mind if I keep the fix there and land both patches together? Note that it's not a huge deal if we unset the flag as the table holds strong references. So the only thing that'd happen is that we get weird behavior if someone does very evil things in mutation events, which I don't think is a big deal.

ok. But please add a testcase for that.

Comment on attachment 447494 [details] [diff] [review] Patch v2 >+nsresult >+nsXMLElement::UnsetAttr(PRInt32 aNameSpaceID, nsIAtom* aAttribute, >+ PRBool aNotify) >+{ >+ PRBool isId = PR_FALSE; >+ if (aAttribute == mNodeInfo->GetIDAttributeAtom() && >+ aNameSpaceID == kNameSpaceID_None) { I think this breaks XTF. You should call GetIDAttributeName >+nsXMLElement::DoGetID() const >+{ >+ NS_ASSERTION(HasFlag(NODE_HAS_ID), "Unexpected call"); >+ >+ nsIAtom* IDName = mNodeInfo->GetIDAttributeAtom(); Same thing here. >+PRBool >+nsXMLElement::ParseAttribute(PRInt32 aNamespaceID, >+ nsIAtom* aAttribute, >+ const nsAString& aValue, >+ nsAttrValue& aResult) >+{ >+ if (aAttribute == mNodeInfo->GetIDAttributeAtom() && And here.

I finally tracked down why this patch broke things. It turns out that our current situation on trunk is more broken than I realized. On trunk right now, when anonymous content is bound to the tree we don't notify, and so the old code didn't add ids in anon content to the id-table. Except for XUL documents which has explicit code to add anon content to the id-table upon insertion. However, any modifications done to anon content normally notifies all the way up the tree all the way to the document. This means that things like appendChild or setAttribute mutations in the anon tree results in things getting added to the id-table. This patch makes us somewhat more consistent in that now we always add anon content to the id-table in xul documents. Both during insertion of anon content, and during subsequent mutations. For HTML we're also now more consistent in that we never add anon content to the id-table (that was already the case with the previous patch in this bug). I also found another flush-call that we no longer need, in GetElementByIdInternal. And since that flush call was basically all that that function does, I nuked the function. I'll file a followup bug on getting anon content out of the id-table entirely.

Oh, the new assertion in nsDocument::AddToIdTable isn't supposed to be there. Removed locally.

> I'll file a followup bug on getting anon content out of the id-table entirely. Yay!

Comment on attachment 447494 [details] [diff] [review] Patch v2 So this need to be update to handle XTF, and does this actually handle shallow unbind?

Shallow unbind should never happen when you unbind from the document. If it does the IsInDoc flag isn't updated and stuff will really hit the fan. I have XTF fixed locally, will attach a combined patch.

Rolls up both patches in one and fixes review comments from Olli. Sorry, didn't save the review fixes in an interdiff as you had marked the patch r+ earlier.

Checked in http://hg.mozilla.org/mozilla-central/rev/ce0bc496b725 Thanks for the reviews!