1055032 - Clean up nsEditor::MoveNode, CreateNode; DeleteTextTxn, InsertElementTxn

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 1 -- Clean up nsEditor::MoveNode

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 2 -- Clean up nsEditor::CreateNode

Note the added XXX in CreateElementTxn::RedoTransaction.  :/

nsEditor::MarkNodeDirty(nsINode*) isn't ever defined, so using it is a linker error.  So I removed it from the class declaration.

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 3 -- Use cleaned-up nsEditor::CreateNode

I didn't replace all callers -- the nsIDOMNode* version has to stay around anyway because it's part of the interface, so I figured there's no need to change the callers that still want an nsIDOMNode* back.

Is there any way to ensure that the XPCOM version of a method like this is not called in our codebase, only the nsINode* version?  I suppose it's not important.

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 4 -- Clean up DeleteTextTxn and friends

This class actually wants to delete chardata, not just text.  It's misnamed.  nsGenericDOMDataNode doesn't quite match up to nsIDOMCharacterData (sigh), but it should be close enough.

Pity we need an Init() method that does nothing but run a single check.  There's probably a nicer way to do this.

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 5 -- Rename InsertElementTxn to InsertNodeTxn and clean up

Comment 6

[(no longer active)]

Comment on attachment 8474598 [details] [diff] [review]
Part 2 -- Clean up nsEditor::CreateNode

Review of attachment 8474598 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/CreateElementTxn.cpp
@@ +76,5 @@
> +    return rv.ErrorCode();
> +  }
> +
> +  mOffsetInParent = std::min(mOffsetInParent,
> +                             SafeCast<int32_t>(mParent->GetChildCount()));

SafeCast, contrary to its name, doesn't really provide any real safety, as it does a debug only check.  Please just use static_cast, as that is more honest.

@@ +118,4 @@
>  
> +  // First, reset mNewNode so it has no attributes or content
> +  // XXX We never actually did this, we only cleared mNewNode's contents if it
> +  // was a CharacterData node (which it's not, it's an Element)

Sigh...

::: editor/libeditor/CreateElementTxn.h
@@ +5,5 @@
>  
>  #ifndef CreateElementTxn_h__
>  #define CreateElementTxn_h__
>  
> +#include "mozilla/dom/Element.h"

Would it be possible to get rid of this include by forward declaring Element?

::: editor/libeditor/nsEditor.cpp
@@ +1342,5 @@
> +{
> +  nsCOMPtr<nsIAtom> tag = do_GetAtom(aTag);
> +  nsCOMPtr<nsINode> parent = do_QueryInterface(aParent);
> +  NS_ENSURE_STATE(parent);
> +  *aNewNode = GetAsDOMNode(CreateNode(tag, parent, aPosition).take());

Nit: .forget() please.

Comment 7

[(no longer active)]

(In reply to :Aryeh Gregor from comment #3)
> Is there any way to ensure that the XPCOM version of a method like this is
> not called in our codebase, only the nsINode* version?  I suppose it's not
> important.

Not that I know of.

Comment 8

[(no longer active)]

Comment on attachment 8474601 [details] [diff] [review]
Part 3 -- Use cleaned-up nsEditor::CreateNode

Review of attachment 8474601 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/html/nsHTMLEditRules.cpp
@@ +9368,4 @@
>            ToLowerCase(listTag);
>            // create a new nested list of correct type
> +          nsCOMPtr<nsIDOMNode> curParentDOM = curParent->AsDOMNode();
> +          res = SplitAsNeeded(&listTag, address_of(curParentDOM), &offset);

FWIW SplitAsNeeded only uses its string argument to construct an nsIAtom*.  It would be nice to make it accept an nsIAtom* and fix up all of these callers in a follow-up.

::: editor/libeditor/nsEditor.cpp
@@ +4128,5 @@
>    nsCOMPtr<nsINode> node = selection->GetAnchorNode();
>    uint32_t offset = selection->AnchorOffset();
>  
>    nsCOMPtr<nsIDOMNode> newNode;
> +  *aNewNode = GetAsDOMNode(CreateNode(tag, node, offset).take());

.forget()

Comment 9

[(no longer active)]

Comment on attachment 8474605 [details] [diff] [review]
Part 4 -- Clean up DeleteTextTxn and friends

Review of attachment 8474605 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/DeleteRangeTxn.cpp
@@ +196,5 @@
>      }
>  
>      if (numToDelete) {
> +      nsRefPtr<DeleteTextTxn> txn = new DeleteTextTxn(*mEditor,
> +          *static_cast<nsGenericDOMDataNode*>(aNode), start, numToDelete,

Adding a charDataNode variable similar to the case above will make this a bit easier to read...

::: editor/libeditor/DeleteTextTxn.cpp
@@ +35,5 @@
> +  mCharData->GetLength(&length);
> +  NS_ASSERTION(length >= aOffset + aNumCharsToDelete,
> +               "Trying to delete more characters than in node");
> +#endif
> +  mDeletedText.Truncate();

This is not needed.

::: editor/libeditor/html/nsHTMLEditRules.cpp
@@ +2533,5 @@
>            if (len > (uint32_t)startOffset)
>            {
>              NS_ENSURE_STATE(mHTMLEditor);
> +            res = mHTMLEditor->DeleteText(
> +                *static_cast<nsGenericDOMDataNode*>(node.get()), startOffset,

Ditto.

@@ +2547,5 @@
>            if (endOffset)
>            {
>              NS_ENSURE_STATE(mHTMLEditor);
> +            res = mHTMLEditor->DeleteText(
> +                *static_cast<nsGenericDOMDataNode*>(node.get()), 0, endOffset);

And here.

::: editor/libeditor/html/nsWSRunObject.cpp
@@ +1342,5 @@
>  
>    if (aStartNode == aEndNode && aStartNode->GetAsText()) {
> +    return mHTMLEditor->DeleteText(*aStartNode->GetAsText(),
> +        SafeCast<uint32_t>(aStartOffset),
> +        SafeCast<uint32_t>(aEndOffset - aStartOffset));

Same comment about SafeCast.

Comment 10

[(no longer active)]

Actually I think it would be a bit more straightforward to store a reference to the nsEditor class in these transaction objects.

Comment 11

[(no longer active)]

Comment on attachment 8474610 [details] [diff] [review]
Part 5 -- Rename InsertElementTxn to InsertNodeTxn and clean up

Review of attachment 8474610 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/InsertNodeTxn.cpp
@@ +49,3 @@
>  
> +  uint32_t count = mParent->GetChildCount();
> +  if (mOffset > SafeCast<int32_t>(count) || mOffset == -1) {

Same for SafeCast.

::: editor/libeditor/InsertNodeTxn.h
@@ +48,5 @@
>    /** the index in mParent for the new node */
>    int32_t mOffset;
> +
> +  /** the editor for this transaction */
> +  nsEditor* mEditor;

nsEditor& please.

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 8474598 [details] [diff] [review]
Part 2 -- Clean up nsEditor::CreateNode

Review of attachment 8474598 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/CreateElementTxn.cpp
@@ +76,5 @@
> +    return rv.ErrorCode();
> +  }
> +
> +  mOffsetInParent = std::min(mOffsetInParent,
> +                             SafeCast<int32_t>(mParent->GetChildCount()));

I don't think this use of SafeCast has the intent of providing "real safety" beyond a claim that the underlying cast itself is safe.  Aryeh, did it?

If the assertion is that it's never possible for a node to have >= 2**31 children, and that's actually possible, you actually do have to handle that case.  MOZ_CRASH if nothing else.  Simply changing this to static_cast<> will do nothing except eliminate the assertion that would be hit if such a node were ever created and edited in a debug build (say, because a reported exploit did so -- in which case the assertion would be very useful, and worth having).

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 8474605 [details] [diff] [review]
Part 4 -- Clean up DeleteTextTxn and friends

Review of attachment 8474605 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/html/nsWSRunObject.cpp
@@ +1342,5 @@
>  
>    if (aStartNode == aEndNode && aStartNode->GetAsText()) {
> +    return mHTMLEditor->DeleteText(*aStartNode->GetAsText(),
> +        SafeCast<uint32_t>(aStartOffset),
> +        SafeCast<uint32_t>(aEndOffset - aStartOffset));

These SafeCast are completely safe, actually, because they're widening a non-negative int32_t into a uint32_t (presumably offset implies non-negative, and end >= start).  Unless you're worried about the possibility of one of these being negative, in which case an assertion on entry to the function seems warranted, and not worry about these particular places.

Comment 14

[Jeff Walden [:Waldo]]

Actually, on second thought, and after a quick conversation with jst.  These cases are almost *exactly* what SafeCast was designed for.  I'd forgotten this, but it's impossible for a node to have >= 2**31 children because of ATTRCHILD_ARRAY_MAX_CHILD_COUNT as used in nsAttrAndChildArray.  So any time you have a uint32_t that's a child index or count, it's going to be safe to cast it to int32_t.

Perhaps this dependency is best expressed with a sibling comment or so, to be sure.  But this is pretty near the canonical use case for SafeCast.

Comment 15

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #6)
> ::: editor/libeditor/CreateElementTxn.h
> @@ +5,5 @@
> >  
> >  #ifndef CreateElementTxn_h__
> >  #define CreateElementTxn_h__
> >  
> > +#include "mozilla/dom/Element.h"
> 
> Would it be possible to get rid of this include by forward declaring Element?

It seems so.  I thought this doesn't work in general when there are nsCOMPtr member variables, but seemingly here it does.  (It's only included in two files that both include this header anyway, though.)

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #10)
> Actually I think it would be a bit more straightforward to store a reference
> to the nsEditor class in these transaction objects.

Pity we can't do this for the nsCOMPtr members.  (We can't, can we?)

Comment 16

[(no longer active)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14)
> Actually, on second thought, and after a quick conversation with jst.  These
> cases are almost *exactly* what SafeCast was designed for.  I'd forgotten
> this, but it's impossible for a node to have >= 2**31 children because of
> ATTRCHILD_ARRAY_MAX_CHILD_COUNT as used in nsAttrAndChildArray.  So any time
> you have a uint32_t that's a child index or count, it's going to be safe to
> cast it to int32_t.

Yes, I know that.  :-)

> Perhaps this dependency is best expressed with a sibling comment or so, to
> be sure.  But this is pretty near the canonical use case for SafeCast.

I don't understand this.  What benefit does SafeCast provide here again?  What I was trying to say before is that *if* you manage to get a node with that many children somehow, many other things will break because we make this assumption all over the place.  I would be *shocked* if the browser manages to run without bugs for long enough to hit the assertions inside SafeCast called from this code.

Comment 17

[(no longer active)]

(In reply to :Aryeh Gregor from comment #15)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #6)
> > ::: editor/libeditor/CreateElementTxn.h
> > @@ +5,5 @@
> > >  
> > >  #ifndef CreateElementTxn_h__
> > >  #define CreateElementTxn_h__
> > >  
> > > +#include "mozilla/dom/Element.h"
> > 
> > Would it be possible to get rid of this include by forward declaring Element?
> 
> It seems so.  I thought this doesn't work in general when there are nsCOMPtr
> member variables, but seemingly here it does.  (It's only included in two
> files that both include this header anyway, though.)

Ah, what you seem to be referring to is the default destructor.  Basically if you don't declare an out of line destructor, the destructor would need access to ~nsCOMPtr, which needs the full declaration of the class because it calls Release on it.  In those cases, you either need to add an include to the header, or add an out of line destructor.

> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #10)
> > Actually I think it would be a bit more straightforward to store a reference
> > to the nsEditor class in these transaction objects.
> 
> Pity we can't do this for the nsCOMPtr members.  (We can't, can we?)

In many of these classes we just hold a raw ptr to nsEditor because the editor outlives the transaction object.  But yes, C++ references and nsCOMPtr don't work together.

Comment 18

[Jeff Walden [:Waldo]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #16)
> I don't understand this.  What benefit does SafeCast provide here again? 

The benefit is that it is an explicit annotation that says, "I know what I'm doing, I'm performing a cast here, and I am explicitly claiming that the cast will never produce a value different from the input".

Contrast with a simple cast, where the reader doesn't know if the cast changes the value (intentionally or unintentionally, potentially safely [casting to unsigned type] or unsafely [to signed type]) and doesn't know if the author carefully concluded the cast was safe or simply just did it to get a value of the type he wanted.  And because the compiler generates the code for static_cast and similar, no one has any way of knowing if the cast actually does produce a different value, or invokes undefined behavior by dint of the destination being a signed type.

> What I was trying to say before is that *if* you manage to get a node with
> that many children somehow, many other things will break because we make
> this assumption all over the place.  I would be *shocked* if the browser
> manages to run without bugs for long enough to hit the assertions inside
> SafeCast called from this code.

That's fine.  You have something against more backstopping?  I want every possible place where requirements are violated to have assertions detecting violations, as early as possible, so we have a fighting chance of finding and fixing them.  (And also because earlier assertions are more easily tracked back to the point where the initial mistake was made.)  Plain C++ casts give us absolutely nothing in this regard.

I'd be somewhat more willing to live without SafeCast if I knew that something like ASAN performed such assertions on its own, and I knew that everyone compiling would stumble across such things.  But it's not the case that everyone runs that way.  And SafeCast also supports use in cases that C++ completely defines -- static_cast<uint32_t>(-1) being perfectly valid, but sometimes you really would want to know and reort an error.  SafeCast<uint32_t>(val) where val is a signed integer addresses that need.

Comment 19

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #18)
> The benefit is that it is an explicit annotation that says, "I know what I'm
> doing, I'm performing a cast here, and I am explicitly claiming that the
> cast will never produce a value different from the input".

"Safe"Cast doesn't seem like a good name, then.  It suggests that somehow the cast is actually safe, when it's not -- it will just assert if used incorrectly.  An explicit static_cast is clearly not safe.

Comment 20

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://tbpl.mozilla.org/?tree=Try&rev=3ff035fae451

https://hg.mozilla.org/integration/mozilla-inbound/rev/4439b62ad770
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bcb489af542
https://hg.mozilla.org/integration/mozilla-inbound/rev/c562f6baed3d
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cbeaefbca7c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e59ab9d1ae98

Comment 21

[(no longer active)]

(In reply to :Aryeh Gregor from comment #19)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #18)
> > The benefit is that it is an explicit annotation that says, "I know what I'm
> > doing, I'm performing a cast here, and I am explicitly claiming that the
> > cast will never produce a value different from the input".
> 
> "Safe"Cast doesn't seem like a good name, then.  It suggests that somehow
> the cast is actually safe, when it's not -- it will just assert if used
> incorrectly.  An explicit static_cast is clearly not safe.

Indeed.  See bug 1045801.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #18)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #16)
> > I don't understand this.  What benefit does SafeCast provide here again? 
> 
> The benefit is that it is an explicit annotation that says, "I know what I'm
> doing, I'm performing a cast here, and I am explicitly claiming that the
> cast will never produce a value different from the input".
> 
> Contrast with a simple cast, where the reader doesn't know if the cast
> changes the value (intentionally or unintentionally, potentially safely
> [casting to unsigned type] or unsafely [to signed type]) and doesn't know if
> the author carefully concluded the cast was safe or simply just did it to
> get a value of the type he wanted.  And because the compiler generates the
> code for static_cast and similar, no one has any way of knowing if the cast
> actually does produce a different value, or invokes undefined behavior by
> dint of the destination being a signed type.

I think once we rename SafeCast to actually express what it does, this semantics makes sense.

> > What I was trying to say before is that *if* you manage to get a node with
> > that many children somehow, many other things will break because we make
> > this assumption all over the place.  I would be *shocked* if the browser
> > manages to run without bugs for long enough to hit the assertions inside
> > SafeCast called from this code.
> 
> That's fine.  You have something against more backstopping?  I want every
> possible place where requirements are violated to have assertions detecting
> violations, as early as possible, so we have a fighting chance of finding
> and fixing them.  (And also because earlier assertions are more easily
> tracked back to the point where the initial mistake was made.)  Plain C++
> casts give us absolutely nothing in this regard.

That's fair.

> I'd be somewhat more willing to live without SafeCast if I knew that
> something like ASAN performed such assertions on its own, and I knew that
> everyone compiling would stumble across such things.  But it's not the case
> that everyone runs that way.  And SafeCast also supports use in cases that
> C++ completely defines -- static_cast<uint32_t>(-1) being perfectly valid,
> but sometimes you really would want to know and reort an error. 
> SafeCast<uint32_t>(val) where val is a signed integer addresses that need.

Would -fsanitize=signed-integer-overflow and -fsanitize=unsigned-integer-overflow give you what you mean here?  We _could_ try to run then on our ASAN builds...

Comment 22

[Jeff Walden [:Waldo]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #21)
> Would -fsanitize=signed-integer-overflow and
> -fsanitize=unsigned-integer-overflow give you what you mean here?  We
> _could_ try to run then on our ASAN builds...

I believe we run with -fsanitize=undefined which includes the first.  The second is not, and never will be, tenable, because unsigned integer overflow is defined and sufficiently often relied upon that we couldn't treat it as an error case.  At least, not without auditing every place for unsigned overflow.  And given how unwilling we (wrongly, I think) seem to be to even fix -Wconversion mistakes, which actually *are* often undefined behavior where unsigned overflow behavior is not, I won't hold my breath.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4439b62ad770
https://hg.mozilla.org/mozilla-central/rev/7bcb489af542
https://hg.mozilla.org/mozilla-central/rev/c562f6baed3d
https://hg.mozilla.org/mozilla-central/rev/2cbeaefbca7c
https://hg.mozilla.org/mozilla-central/rev/e59ab9d1ae98

Comment 24

[(no longer active)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #22)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #21)
> > Would -fsanitize=signed-integer-overflow and
> > -fsanitize=unsigned-integer-overflow give you what you mean here?  We
> > _could_ try to run then on our ASAN builds...
> 
> I believe we run with -fsanitize=undefined which includes the first.

We don't.  See bug 996026.  And bug 919486.

> The
> second is not, and never will be, tenable, because unsigned integer overflow
> is defined and sufficiently often relied upon that we couldn't treat it as
> an error case.  At least, not without auditing every place for unsigned
> overflow.  And given how unwilling we (wrongly, I think) seem to be to even
> fix -Wconversion mistakes, which actually *are* often undefined behavior
> where unsigned overflow behavior is not, I won't hold my breath.

Hmm, yeah you're probably right.