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

RESOLVED FIXED in mozilla34

Status

()

--
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

unspecified
mozilla34
Points:
---
Bug Flags:
in-testsuite -
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

Comment hidden (empty)
Created attachment 8474594 [details] [diff] [review]
Part 1 -- Clean up nsEditor::MoveNode
Attachment #8474594 - Flags: review?(ehsan)
Created attachment 8474598 [details] [diff] [review]
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.
Attachment #8474598 - Flags: review?(ehsan)
Created attachment 8474601 [details] [diff] [review]
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.
Attachment #8474601 - Flags: review?(ehsan)
Created attachment 8474605 [details] [diff] [review]
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.
Attachment #8474605 - Flags: review?(ehsan)
Created attachment 8474610 [details] [diff] [review]
Part 5 -- Rename InsertElementTxn to InsertNodeTxn and clean up
Attachment #8474610 - Flags: review?(ehsan)

Updated

4 years ago
Attachment #8474594 - Flags: review?(ehsan) → review+

Comment 6

4 years ago
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.
Attachment #8474598 - Flags: review?(ehsan) → review+

Comment 7

4 years ago
(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

4 years ago
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()
Attachment #8474601 - Flags: review?(ehsan) → review+

Comment 9

4 years ago
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.
Attachment #8474605 - Flags: review?(ehsan) → review+

Comment 10

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

Comment 11

4 years ago
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.
Attachment #8474610 - Flags: review?(ehsan) → review+
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 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.
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.
(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

4 years ago
(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

4 years ago
(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.
(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.
(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 21

4 years ago
(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...
(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 24

4 years ago
(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.
Flags: qe-verify-

Updated

4 years ago
Depends on: 1113121
You need to log in before you can comment on or make changes to this bug.