Closed Bug 1432186 Opened 2 years ago Closed 2 years ago

Remove some more nsIDOMNode bits

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(24 files, 3 obsolete files)

4.39 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
8.69 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
13.49 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
8.87 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
19.01 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
11.23 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
9.87 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
7.68 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
3.29 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
14.40 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
16.17 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
19.76 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
6.26 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
21.07 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
5.83 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
25.03 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
29.14 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
12.34 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
155.35 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
18.83 KB, patch
Details | Diff | Splinter Review
15.50 KB, patch
Details | Diff | Splinter Review
19.64 KB, patch
Details | Diff | Splinter Review
5.36 KB, patch
Details | Diff | Splinter Review
22.53 KB, patch
Details | Diff | Splinter Review
No description provided.
Depends on: 1432194
Depends on: 1432281
Depends on: 1432286
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.
Depends on: 1432351
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.
Depends on: 1432352
Depends on: 1432353
Depends on: 1432377
Depends on: 1432379
Depends on: 1432383
Depends on: 1432387
Depends on: 1432603
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
Attachment #8944888 - Flags: review?(continuation)
MozReview-Commit-ID: DB3pSrA9ffy
Attachment #8944890 - Flags: review?(continuation)
MozReview-Commit-ID: Jg0Tuvdi6uX
Attachment #8944892 - Flags: review?(continuation)
MozReview-Commit-ID: Aqt4NDjcdKW
Attachment #8944893 - Flags: review?(continuation)
MozReview-Commit-ID: LKsBgKcqtBS
Attachment #8944894 - Flags: review?(continuation)
MozReview-Commit-ID: 9ZbIEIRtYPL
Attachment #8944895 - Flags: review?(continuation)
MozReview-Commit-ID: I5cIxa5WAk
Attachment #8944897 - Flags: review?(continuation)
MozReview-Commit-ID: DXhiXZ3qQHg
Attachment #8944898 - Flags: review?(continuation)
MozReview-Commit-ID: 3x3wJ9H38Wx
Attachment #8944899 - Flags: review?(continuation)
MozReview-Commit-ID: FhJs1MXAyeO
Attachment #8944900 - Flags: review?(continuation)
MozReview-Commit-ID: 5jCdAmSuPx8
Attachment #8944902 - Flags: review?(continuation)
MozReview-Commit-ID: 7UJFaxEnT9Q
Attachment #8944903 - Flags: review?(continuation)
MozReview-Commit-ID: DTaivhMORXr
Attachment #8944904 - Flags: review?(continuation)
MozReview-Commit-ID: JyQjEYngKAT
Attachment #8944905 - Flags: review?(continuation)
MozReview-Commit-ID: GFc2sv4E7b2
Attachment #8944907 - Flags: review?(continuation)
MozReview-Commit-ID: JqfAFxPBz41
Attachment #8944908 - Flags: review?(continuation)
MozReview-Commit-ID: 4xzDwwEqnvE
Attachment #8944909 - Flags: review?(continuation)
MozReview-Commit-ID: FbP5fVQ3c6m
Attachment #8944910 - Flags: review?(continuation)
MozReview-Commit-ID: KvKjeKIOB9K
Attachment #8944911 - Flags: review?(continuation)
Priority: -- → P2
Attachment #8944888 - Flags: review?(continuation) → review+
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".
Attachment #8944890 - Flags: review?(continuation) → review+
> 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...
Blocks: 1432944
Attachment #8944892 - Flags: review?(continuation) → review+
Attachment #8944893 - Flags: review?(continuation) → review+
Attachment #8944894 - Flags: review?(continuation) → review+
Attachment #8944895 - Flags: review?(continuation) → review+
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?
Attachment #8944897 - Flags: review?(continuation) → review+
Attachment #8944898 - Flags: review?(continuation) → review+
Attachment #8944899 - Flags: review?(continuation) → review+
> 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.
Attachment #8945286 - Attachment is obsolete: true
Attachment #8945286 - Flags: review?(m_kato)
Attachment #8945287 - Attachment is obsolete: true
Attachment #8945287 - Flags: review?(m_kato)
Attachment #8945288 - Attachment is obsolete: true
Attachment #8945288 - Flags: review?(m_kato)
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. ;)
Attachment #8944900 - Flags: review?(continuation) → review+
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.
Attachment #8944902 - Flags: review?(continuation) → review+
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.
Attachment #8944903 - Flags: review?(continuation) → review+
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.
Attachment #8944904 - Flags: review?(continuation) → review+
> 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?
Attachment #8944905 - Flags: review?(continuation) → review+
> Maybe mention who is the caller? 

Done.
Attachment #8944907 - Flags: review?(continuation) → review+
> 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
Attachment #8944908 - Flags: review?(continuation) → review+
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.
Attachment #8944909 - Flags: review?(continuation) → review+
> 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.
Attachment #8944910 - Flags: review?(continuation) → review+
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!
Attachment #8944911 - Flags: review?(continuation) → review+
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.