Boris, thanks for the heads-up, but no action here yet ;-)

Yes, I'm still writing the patches. I'll post them after I run them through try, at least, because they have some Windows-only changes I can't really test locally.

But note that you can start fixing the bugs blocking this one before the patches for this one land. That's why the dependency is in the direction it's in.

Removing them all together because the only non-comm-central consumer is all the same and this way there's no intermediate state to be maintained there where we need both the nsINode and the nsIDOMNode. MozReview-Commit-ID: GDjrroN1jao

MozReview-Commit-ID: DB3pSrA9ffy

MozReview-Commit-ID: Jg0Tuvdi6uX

MozReview-Commit-ID: Aqt4NDjcdKW

MozReview-Commit-ID: LKsBgKcqtBS

MozReview-Commit-ID: 9ZbIEIRtYPL

MozReview-Commit-ID: I5cIxa5WAk

MozReview-Commit-ID: DXhiXZ3qQHg

MozReview-Commit-ID: 3x3wJ9H38Wx

MozReview-Commit-ID: FhJs1MXAyeO

MozReview-Commit-ID: 5jCdAmSuPx8

MozReview-Commit-ID: 7UJFaxEnT9Q

MozReview-Commit-ID: DTaivhMORXr

MozReview-Commit-ID: JyQjEYngKAT

MozReview-Commit-ID: GFc2sv4E7b2

MozReview-Commit-ID: JqfAFxPBz41

MozReview-Commit-ID: 4xzDwwEqnvE

MozReview-Commit-ID: FbP5fVQ3c6m

MozReview-Commit-ID: KvKjeKIOB9K

Comment on attachment 8944890 [details] [diff] [review] part 2. Clean up the string handling in HTMLURIRefObject::GetNextURI Review of attachment 8944890 [details] [diff] [review]: ----------------------------------------------------------------- ::: editor/libeditor/HTMLURIRefObject.cpp @@ +111,3 @@ > continue; > } > + nit: trailing whitespace. @@ +138,4 @@ > continue; > } > + > + // XXXbz And if it is? Are these comments all intentionally left in? @@ +176,4 @@ > } > + // codebase >> OBJECT > + else if (attrInfo.mName->Equals(nsGkAtoms::codebase)) { > + if (!element->IsHTMLElement(nsGkAtoms::object)) { This looks like it was "meta" before but now it is "object".

> Are these comments all intentionally left in? Yes, because in theory all that stuff would be implemented. > This looks like it was "meta" before but now it is "object". Right. The "meta" was a clear copy/paste bug in the original code; there is no useful "codebase" attribute on <meta>. Not that this matters much, since the code does nothing in that case anyway...

Comment on attachment 8944897 [details] [diff] [review] part 7. Remove nsIDOMNode's textContent attribute Review of attachment 8944897 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webbrowserpersist/WebBrowserPersistLocalDocument.cpp @@ +862,5 @@ > + nsCOMPtr<nsINode> nodeIn = do_QueryInterface(aNodeIn); > + nsCOMPtr<nsINode> nodeOut; > + nsresult rv = FixupNode(nodeIn, aSerializeCloneKids, > + getter_AddRefs(nodeOut)); > + if (nodeOut) { Should this be checking NS_SUCCEEDED(rv) instead?

> Should this be checking NS_SUCCEEDED(rv) instead? Good point. Fixed.

> Good point. Fixed. Actually, no. The point is, FixupNode can return NS_OK and a null outparam. See the early returns at its top. I'll add a comment.

Comment on attachment 8944900 [details] [diff] [review] part 10. Remove nsIDOMNode's lastChild attribute Review of attachment 8944900 [details] [diff] [review]: ----------------------------------------------------------------- ::: editor/libeditor/HTMLTableEditor.cpp @@ +990,5 @@ > // Prevent rules testing until we're done > AutoRules beginRulesSniffing(this, EditAction::deleteNode, nsIEditor::eNext); > > + while (cell->GetLastChild()) { > + nsresult rv = DeleteNode(cell->GetLastChild()); Should you have a strong stack reference to cell->GetLastChild() here? This sets my UAF spidey sense tingling. ;)

Comment on attachment 8944902 [details] [diff] [review] part 11. Remove nsIDOMNode's firstChild attribute Review of attachment 8944902 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsFocusManager.cpp @@ +2454,3 @@ > } > + > + nsIContent* firstChild = aContent->GetFirstChild(); Maybe inline firstChild now that there's only a single use of it? That would make me less nervous about UAFs when looking at this code. (The scope of firstChild is live across mutation which is always concerning.) @@ +3652,5 @@ > } > > do { > + // GetParent is OK here, instead of GetParentNode, because the only case > + // where the latte returns something different from the former is when nit: latter. Unless coffee is somehow involved. ;) ::: editor/libeditor/HTMLTableEditor.cpp @@ +246,3 @@ > NS_ENSURE_TRUE(tableElement, NS_ERROR_NULL_POINTER); > > + nsIContent* tableChild = tableElement->GetFirstChild(); I think this would be better left as an nsCOMPtr, as the loop below is not trivial and I'm guessing this is not performance sensitive code, just to avoid silly UAFs. @@ +256,5 @@ > + // Look for row in one of the row container elements > + if (tableChild->IsAnyOfHTMLElements(nsGkAtoms::tbody, > + nsGkAtoms::thead, > + nsGkAtoms::tfoot)) { > + nsIContent* rowNode = tableChild->GetFirstChild(); Likewise, though the scope of this one is smaller.

Comment on attachment 8944903 [details] [diff] [review] part 12. Remove nsIDOMNode's previousSibling attribute Review of attachment 8944903 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the various -w diffs. It made reviewing easier. ::: editor/txtsvc/nsFilteredContentIterator.cpp @@ +286,2 @@ > { > + // XXXbz we have a caller below who passes null for aNextContent! Maybe mention who is the caller? Though I guess you do remark on that at the call site. ::: layout/inspector/inDOMView.cpp @@ -1195,5 @@ > -inDOMView::GetRealPreviousSibling(nsIDOMNode* aNode, nsIDOMNode* aRealParent, nsIDOMNode** aSibling) > -{ > - // XXXjrh: This won't work for some cases during some situations where XBL insertion points > - // are involved. Fix me! > - aNode->GetPreviousSibling(aSibling); Well, there's an amazing method.

Comment on attachment 8944904 [details] [diff] [review] part 13. Remove nsIDOMNode's nextSibling attribute Review of attachment 8944904 [details] [diff] [review]: ----------------------------------------------------------------- Well, that was an easy one to remove.

> Should you have a strong stack reference to cell->GetLastChild() here? Hmm. It's safe, because the delete transaction takes a strong ref to the node before anything else happens. But you're right that it would be clearer if we held a ref here; I'll do that: while (nsCOMPtr<nsINode> child = cell->GetLastChild()) { nsresult rv = DeleteNode(child); > Maybe inline firstChild now that there's only a single use of it? Good catch, done. > nit: latter. Yes, fixed. ;) > I think this would be better left as an nsCOMPtr OK, done. > Likewise, though the scope of this one is smaller. Also done.

Comment on attachment 8944905 [details] [diff] [review] part 14. Remove nsIDOMNode's childNodes attribute Review of attachment 8944905 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/forms/nsTextControlFrame.cpp @@ +937,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > > nsCOMPtr<nsIContent> rootContent = do_QueryInterface(rootElement); > + nsCOMPtr<nsINode> rootNode; > + rootNode = rootContent; Could you put this assignment be on the same line as the declaration?

> Maybe mention who is the caller? Done.

> Could you put this assignment be on the same line as the declaration? Not trivially. If I write it like so: nsCOMPtr<nsINode> rootNode = rootContent; Then C++ helpfully uses a constructor, not an assignment operator. And nsCOMPtr<nsINode> has no constructor that takes nsCOMPtr<nsIContent>& as an arg. So you get: error: conversion from ‘nsCOMPtr<nsIContent>’ to non-scalar type ‘nsCOMPtr<nsINode>’ requested nsCOMPtr<nsINode> rootNode = rootContent; I could do: nsCOMPtr<nsINode> rootNode = rootContent.get(); but I'm not sure that's better than the two separate lines....

Comment on attachment 8944908 [details] [diff] [review] part 16. Remove nsIDOMNode's ownerDocument attribute Review of attachment 8944908 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/inspector/inDOMView.cpp @@ +142,5 @@ > // destroyed before we are > + nsCOMPtr<nsINode> node = do_QueryInterface(aNode); > + nsIDocument* doc = node->OwnerDoc(); > + mRootDocument = do_QueryInterface(doc); > + nit: trailing space

Comment on attachment 8944909 [details] [diff] [review] part 17. Remove nsIDOMNode's parentNode attribute Review of attachment 8944909 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDocumentEncoder.cpp @@ +1805,3 @@ > } > + > + *outParent = do_QueryInterface(parent); nit: trailing whitespace ::: editor/libeditor/HTMLTableEditor.cpp @@ +3131,5 @@ > // We didn't find a cell > // Set selection to just before the table > + nsCOMPtr<nsINode> table = do_QueryInterface(aTable); > + nsINode* tableParent = table->GetParentNode(); > + if (tableParent) { This is the only use of tableParent, so you could inline it.

> nit: trailing space Fixed. > nit: trailing whitespace > This is the only use of tableParent, so you could inline it. Done.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #43) > Not trivially. If I write it like so: Ah, I see. The code just looked weird to me.

Comment on attachment 8944911 [details] [diff] [review] part 19. Remove the nsIDOMNode::*_NODE constants Review of attachment 8944911 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/interfaces/core/nsIDOMNode.idl @@ +17,5 @@ > */ > > [uuid(cc35b412-009b-46a3-9be0-76448f12548d)] > interface nsIDOMNode : nsISupports > { Nice!

There's also a little bit of cleanup that could be done in nsIDOMNode.idl: removing the forward declaration of nsIVariant, changing the #include to nsISupports, maybe deleting the comment.

I'll remove the forward-decl. The comment can probably stay until we nix the interface altogether (soon, I hope!). The domstubs include is effectively cargo-culted by various idls that include nsIDOMNode.idl (e.g. all the things that inherit from it), so removing it is a bit of a process. Will be easier if we remove those descendants first.

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/13772238aa3c part 1. Remove nsIDOMNode's prefix, namespaceURI, localName attributes. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/a327258d366c part 2. Clean up the string handling in HTMLURIRefObject::GetNextURI. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/9b5ef433ce2f part 3. Remove nsIDOMNode's nodeName attribute. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/7fa8ac3f80d8 part 4. Remove nsIDOMNode's nodeValue attribute. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/1d47247b3290 part 5. Remove nsIDOMNode's nodeType attribute. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/d8b7f368b916 part 6. Remove nsIDOMNode::RemoveChild. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/d5f8e6eeb6c6 part 7. Remove nsIDOMNode's textContent attribute. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/3f3c196b451b part 8. Remove nsIDOMNode::DOCUMENT_POSITION_* constants. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/1313d48f3422 part 9. Remove nsIDOMNode::CloneNode. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/d344f9149b97 part 10. Remove nsIDOMNode's lastChild attribute. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/d2cddcd1af40 part 11. Remove nsIDOMNode's firstChild attribute. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/9badfca43f1c part 12. Remove nsIDOMNode's previousSibling attribute. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/47364ad7bf3f part 13. Remove nsIDOMNode's nextSibling attribute. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/22cb7cdac946 part 14. Remove nsIDOMNode's childNodes attribute. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/89b63bd48544 part 15. Remove nsIDOMNode::HasChildNodes. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/f2734b328291 part 16. Remove nsIDOMNode's ownerDocument attribute. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/6a4d5ceffed3 part 17. Remove nsIDOMNode's parentNode attribute. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/59b0998fd7a8 part 18. Remove no-longer-needed nsIDOMNode-forwarding defines. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/49142eb85e3c part 19. Remove the nsIDOMNode::*_NODE constants. r=mccr8

https://hg.mozilla.org/mozilla-central/rev/13772238aa3c https://hg.mozilla.org/mozilla-central/rev/a327258d366c https://hg.mozilla.org/mozilla-central/rev/9b5ef433ce2f https://hg.mozilla.org/mozilla-central/rev/7fa8ac3f80d8 https://hg.mozilla.org/mozilla-central/rev/1d47247b3290 https://hg.mozilla.org/mozilla-central/rev/d8b7f368b916 https://hg.mozilla.org/mozilla-central/rev/d5f8e6eeb6c6 https://hg.mozilla.org/mozilla-central/rev/3f3c196b451b https://hg.mozilla.org/mozilla-central/rev/1313d48f3422 https://hg.mozilla.org/mozilla-central/rev/d344f9149b97 https://hg.mozilla.org/mozilla-central/rev/d2cddcd1af40 https://hg.mozilla.org/mozilla-central/rev/9badfca43f1c https://hg.mozilla.org/mozilla-central/rev/47364ad7bf3f https://hg.mozilla.org/mozilla-central/rev/22cb7cdac946 https://hg.mozilla.org/mozilla-central/rev/89b63bd48544 https://hg.mozilla.org/mozilla-central/rev/f2734b328291 https://hg.mozilla.org/mozilla-central/rev/6a4d5ceffed3 https://hg.mozilla.org/mozilla-central/rev/59b0998fd7a8 https://hg.mozilla.org/mozilla-central/rev/49142eb85e3c