1432186 - Remove some more nsIDOMNode bits

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Comment 1

[Jorg K (CEST = GMT+2)]

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Remove nsIDOMNode's prefix, namespaceURI, localName attributes

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Clean up the string handling in HTMLURIRefObject::GetNextURI

MozReview-Commit-ID: DB3pSrA9ffy

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Remove nsIDOMNode's nodeName attribute

MozReview-Commit-ID: Jg0Tuvdi6uX

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Remove nsIDOMNode's nodeValue attribute

MozReview-Commit-ID: Aqt4NDjcdKW

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Remove nsIDOMNode's nodeType attribute

MozReview-Commit-ID: LKsBgKcqtBS

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Remove nsIDOMNode::RemoveChild

MozReview-Commit-ID: 9ZbIEIRtYPL

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Remove nsIDOMNode's textContent attribute

MozReview-Commit-ID: I5cIxa5WAk

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Remove nsIDOMNode::DOCUMENT_POSITION_* constants

MozReview-Commit-ID: DXhiXZ3qQHg

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Remove nsIDOMNode::CloneNode

MozReview-Commit-ID: 3x3wJ9H38Wx

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 10. Remove nsIDOMNode's lastChild attribute

MozReview-Commit-ID: FhJs1MXAyeO

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 11. Remove nsIDOMNode's firstChild attribute

MozReview-Commit-ID: 5jCdAmSuPx8

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 12. Remove nsIDOMNode's previousSibling attribute

MozReview-Commit-ID: 7UJFaxEnT9Q

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 13. Remove nsIDOMNode's nextSibling attribute

MozReview-Commit-ID: DTaivhMORXr

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: part 14. Remove nsIDOMNode's childNodes attribute

MozReview-Commit-ID: JyQjEYngKAT

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: part 15. Remove nsIDOMNode::HasChildNodes

MozReview-Commit-ID: GFc2sv4E7b2

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: part 16. Remove nsIDOMNode's ownerDocument attribute

MozReview-Commit-ID: JqfAFxPBz41

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: part 17. Remove nsIDOMNode's parentNode attribute

MozReview-Commit-ID: 4xzDwwEqnvE

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: part 18. Remove no-longer-needed nsIDOMNode-forwarding defines

MozReview-Commit-ID: FbP5fVQ3c6m

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: part 19. Remove the nsIDOMNode::*_NODE constants

MozReview-Commit-ID: KvKjeKIOB9K

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5 diff -w

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 11 diff -w

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 12 diff -w

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 15 diff -w

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 16 diff -w

Comment 28

[Andrew McCreight [:mccr8]]

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".

Comment 29

[Boris Zbarsky [:bzbarsky]]

> 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 30

[Andrew McCreight [:mccr8]]

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?

Comment 31

[Boris Zbarsky [:bzbarsky]]

> Should this be checking NS_SUCCEEDED(rv) instead?

Good point.  Fixed.

Comment 32

[Boris Zbarsky [:bzbarsky]]

> 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 33

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Stop returning NS_EDITOR_ELEMENT_NOT_FOUND from nsIHTMLEditor::GetSelectedElement

The only C++ caller doesn't care, and JS callers can't tell that a non-NS_OK success code was returned anyway.

MozReview-Commit-ID: FNhQYR0a9ur

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Make nsIHTMLEditor.getSelectedElement return nsISupports

The only C++ caller immediately QIs the result anyway, and the exact type
returned doesn't matter for JS callers because the return values are DOM
objects.  This makes it simpler to rejigger the internals to work with nsINode
and Element and whatnot.

MozReview-Commit-ID: 863IDGECqYY

Comment 35

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Take an early return from HTMLEditor::GetSelectedElement when we can

MozReview-Commit-ID: EAEzpom7oXo

Comment 36

[Andrew McCreight [:mccr8]]

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 37

[Andrew McCreight [:mccr8]]

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 38

[Andrew McCreight [:mccr8]]

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 39

[Andrew McCreight [:mccr8]]

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.

Comment 40

[Boris Zbarsky [:bzbarsky]]

> 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 41

[Andrew McCreight [:mccr8]]

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?

Comment 42

[Boris Zbarsky [:bzbarsky]]

> Maybe mention who is the caller? 

Done.

Comment 43

[Boris Zbarsky [:bzbarsky]]

> 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 44

[Andrew McCreight [:mccr8]]

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 45

[Andrew McCreight [:mccr8]]

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.

Comment 46

[Boris Zbarsky [:bzbarsky]]

> nit: trailing space

Fixed.

> nit: trailing whitespace

> This is the only use of tableParent, so you could inline it.

Done.

Comment 47

[Andrew McCreight [:mccr8]]

(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 48

[Andrew McCreight [:mccr8]]

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!

Comment 49

[Andrew McCreight [:mccr8]]

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.

Comment 50

[Boris Zbarsky [:bzbarsky]]

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.

Comment 51

[Pulsebot]

Pushed by [e-mail]:
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

Comment 52

[Raul Gurzau (:RaulG)]

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