Per bug 201236 comment 39, we should consolidate nsGenericElement::RemoveChild and nsDocument::RemoveChild so they call a common nsGenericElement::doRemoveChild. I'm filing this as a separate bug, because I do not feel comfortable writing a patch for that. This is probably very simple.

So... could you maybe see the way I did replaceChild/insertBefore and do something similar for removeChild? I'm not going to be able to get to this in the near future, and the more people we have that know this code the better, imo. ;)

I ran SeaMonkey against the W3C Test Suite for DOM Core Level 1, before and after the patch. Before the patch, we failed 29 tests. Of those, two concerned RemoveChild, but for attributes. After the patch, I had the same results. This patch is my best guess, based on peterv's r- comment, on my analysis of nsGenericElement::InsertBefore, nsDocument::InsertBefore, nsContentOrDocument::InsertChildAt, and the corresponding RemoveChild methods. One thing that is confusing to me is the aNotify arguments. The documentation in the code does not clearly explain what they signify. Also, nsDocument::RemoveChild would return NS_ERROR_DOM_HIERARCHY_REQUEST_ERR in a case where nsGenericElement::RemoveChild would return NS_ERROR_DOM_NOT_FOUND_ERR. Based on the DOM Level 3 specification, I used NS_ERROR_DOM_NOT_FOUND_ERR in doRemoveChild.

Comment on attachment 199710 [details] [diff] [review] patch, v1 (best guess) caillon asked me to do some tests both with and without the patch in regards to playing with the document type node. I see no regressions. However, caillon also spotted this being removed from old code and not inserted in the new doRemoveChild code: >Index: mozilla/content/base/src/nsDocument.cpp >- if (content == mRootContent) { >- DestroyLinkMap(); >- mRootContent = nsnull; >- } >- >- content->UnbindFromTree(); I'm not sure what that code does, but it looks to me either crash defense or leak plugging. Can someone advise me how relevant that is to the patch, and if I need to update the patch for that?

> I'm not sure what that code does It prevents the UnbindFromTree call from taking forever and a day. Do NOT remove that code.

(In reply to comment #4) > It prevents the UnbindFromTree call from taking forever and a day. Do NOT > remove that code. Careful examination of the source code, and stepping through via the debugger, shows that the call itself isn't actually removed. The call still takes place in the static doRemoveChildAt call: http://lxr.mozilla.org/seamonkey/source/content/base/src/nsGenericElement.cpp#2946 The DestroyLinkMap call happens when nsDocument::SetRootContent(nsnull) occurs there: http://lxr.mozilla.org/seamonkey/source/content/base/src/nsDocument.cpp#1649 So while I thought for a moment I needed a new patch to answer your concern, apparently I do not.

Comment on attachment 199710 [details] [diff] [review] patch, v1 (best guess) Please, somebody, review my patch!!!

Comment on attachment 199710 [details] [diff] [review] patch, v1 (best guess) >+nsGenericElement::doRemoveChild(nsIDOMNode* aOldChild, ... >+ nsCOMPtr<nsIContent> content(do_QueryInterface(aOldChild)); Please use |nsCOMPtr<nsIContent> content = do_QI| syntax instead. IMHO it's much easier on the eye. >+ if (!content) { > /* > * If we're asked to remove something that doesn't support nsIContent > * it can not be one of our children, i.e. we return NOT_FOUND_ERR. > */ > return NS_ERROR_DOM_NOT_FOUND_ERR; > } This test isn't actually needed, the IndexOfChild call further down will always return -1 for a null pointer. No need to optimize for an uncommon case. One thing that is a bit scary is that I noticed that SetRootContent does things in a very different order the elements. Elements remove the child pointer, calls the nsIDocumentObserver notification and then unbinds. But that's for a different patch.

I'm not likely to get to this for at least a week or two. Might want to try jst or peterv if you want faster review (or at least check with them on ETAs).

Comment on attachment 199710 [details] [diff] [review] patch, v1 (best guess) >Index: mozilla/content/base/src/nsGenericElement.h >+ * Actual implementation of the DOM ReplaceChild method. Shared by "RemoveChild method". With that and sicking's comments fixed, sr=bzbarsky. Post an updated patch and I'll check it in? Sorry this took so long; I thought this was a lot longer than it actually is...

Here you go. Thanks for the reviews!

Checked in

It looks like this could potentially pass null to nsAttrAndChildArray::IndexOfChild, since it removes the null checks after QI to nsIContent. It's not documented whether that function is null-safe, and whether it is depends, I suppose, on whether there are any edge cases in which the array could be sparse.

David, see comment 7... Perhaps we should document that IndexOfChild will always return -1 for null?