Right now the id-table holds strong references since we don't guarantee that elements are always removed from the id-table in all edge cases. Once bug 564863 is fixed, we can make the remaining effort to make it so that the id table is always correct and up to date. There are two big complications: * IDs for XML elements which can dynamically change which attribute is the ID attribute. * Mutation events. Patch coming up to fix

FWIW, I'm not 100% sure we want to do this given how much complexity there is. Though if we got rid of mutation events that would take care of almost all of the complexity.

Does the addref/release show up badly in profiles? I guess it does.

Comment on attachment 447495 [details] [diff] [review] Patch to fix > nsIdentifierMapEntry::RemoveIdElement(Element* aElement) > { >+ NS_PRECONDITION(aElement, "Missing element"); >+ > // This should only be called while the document is in an update. > // Assertions near the call to this method guarantee this. > >+ // This could fire in OOM situations >+ // Only assert this in HTML documents for now as XUL does all sorts of weird >+ // crap. >+ NS_ASSERTION(!aElement->GetOwnerDoc() || >+ !aElement->GetOwnerDoc()->IsHTML() || >+ mIdContentList.IndexOf(aElement) >= 0, >+ "Removing id entry that doesn't exist"); Please file followups for the crap. And what all badness does XUL do? But this isn't too bad, especially if you could have some inline method like MaybeUpdatedIDAttribute() { nsMutationGuard::DidMutate(); } That would be, IMO, easier to understand than just non-commented nsMutationGuard::DidMutate() call.

> But this isn't too bad, especially if you could have some > inline method like MaybeUpdatedIDAttribute() { nsMutationGuard::DidMutate(); } > That would be, IMO, easier to understand than just non-commented > nsMutationGuard::DidMutate() > call. Not sure where you want me to make this change? Note that we've previously not made a distinction of different types of mutations. So we might later end up with code that cares about other attribute changes, not just id-related ones. It could make sense to add a comment to SetPrefix describing why this mutation is significant?

Well, add comment to all the places where you add nsMutationGuard::DidMutate(). That would be as helpful as inline method.

Filed bug 568282 on the XUL crappyness.

Comment on attachment 447495 [details] [diff] [review] Patch to fix Jonas, do you still want feedback here? Patch looks generally ok to me at first glance...

Should this be pushed?

This was pushed a while ago. Forgot marking this fixed http://hg.mozilla.org/mozilla-central/rev/e1b3964e946c