769967 - "ASSERTION: MoveNode failed" with (de-)subscript, insertText

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: MoveNode failed: 'NS_SUCCEEDED(res)', file editor/libeditor/base/nsEditor.cpp, line 1730

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

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

Attachment: Patch part 1 -- Clean up nsEditor::GetChildOffset

Comment 3

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

Attachment: Patch part 2 -- Clean up nsEditor::GetNodeLocation

Comment 4

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

Attachment: Patch part 3 -- Clean up DeleteElementTxn and nsEditor::DeleteNode

Comment 5

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

Attachment: Patch part 4 -- Rename DeleteElementTxn to DeleteNodeTxn

Comment 6

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

Okay, so after all that cleanup, here's the story: nsHTMLEditor::DeleteNode looks for a selectAllNode like so:

  nsCOMPtr<nsIDOMNode> selectAllNode = FindUserSelectAllNode(aNode);

  if (selectAllNode) 
  { 
    return nsEditor::DeleteNode(selectAllNode);
  } 

So if the node to be deleted has an ancestor with -moz-user-select: all, that node will be deleted instead.  Which fails, because it's the root element, so is of course not editable.  MoveNode() runs DeleteNode() followed by InsertNode(), so the error propagates back.

Question: why the heck does DeleteNode() look for -moz-user-select: all?  CVS blame points to bug 180034.  I don't understand the rationale there.  The user cannot create selections that contain only part of the element, so why should DeleteNode() have to be special?

I'll try writing a patch to just kill this special case and see if that causes any breakage.

Comment 7

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

Attachment: Patch part 5 -- Don't special-case -moz-user-select: all elements for deletion

I'm not *totally* sure this is correct, but I sure don't see how it's not.  Try: https://tbpl.mozilla.org/?tree=Try&rev=1856f941d9f8

Comment 8

[(no longer active)]

Comment on attachment 638323 [details] [diff] [review]
Patch part 1 -- Clean up nsEditor::GetChildOffset

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

r=me, and thanks a lot for doing this kind of cleanup.  Reviewing these patches always fill me with warmness and fuzziness.  :-)

::: editor/libeditor/base/nsEditor.h
@@ +435,5 @@
>                           bool        aNodeToKeepIsFirst);
>  
>    /**
> +   * Return the offset of aChild in aParent.  Asserts if parent or child is
> +   * null, or parent is not child's parent.

Nit: note that the assertions are fatal.

::: editor/libeditor/base/nsIEditorSupport.h
@@ +52,5 @@
>                             nsIDOMNode  *aNodeToJoin,
>                             nsIDOMNode  *aParent,
>                             bool         aNodeToKeepIsFirst)=0;
>  
> +  static PRInt32 GetChildOffset(nsIDOMNode* aChild, nsIDOMNode* aParent);

Please update the IID.

Also, can you please file a bug to merge nsIEditorSupport into nsEditor, and get rid of this interface?  It is not used anywhere in m-c or c-c, and continuing to support it is just silly.  Ms2ger might be interested in submitting a patch, if not, mark it as [mentor=ehsan] please.  :-)

::: editor/libeditor/html/nsHTMLEditor.cpp
@@ -1895,5 @@
> -    if (NS_SUCCEEDED(res))
> -    {
> -      // Collapse selection to just after desired element,
> -      res = selection->Collapse(parent, offsetInParent+1);
> -#if 0 //def DEBUG_cmanske

Sigh...

Comment 9

[(no longer active)]

Comment on attachment 638324 [details] [diff] [review]
Patch part 2 -- Clean up nsEditor::GetNodeLocation

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

::: editor/libeditor/base/nsEditor.cpp
@@ +3157,1 @@
>    }

Hmm, I think you should also set *outOffset to -1 in an else branch, since some of the callers pass an uninitialized local to this function (cause still GetParentNode _might_ fail in optimized builds...)

::: editor/libeditor/base/nsEditor.h
@@ +442,5 @@
>                                  nsIDOMNode *aParent);
>  
>    /**
>     *  Set aParent to the parent of aChild.
> +   *  Set aOffset to the offset of aChild in aParent.

Nit: outParent, outChild and outOffset.

@@ +447,4 @@
>     */
> +  static void GetNodeLocation(nsIDOMNode* outChild,
> +                              nsCOMPtr<nsIDOMNode>* outParent,
> +                              PRInt32* outOffset);

Hmm, I don't see any reason why GetNodeLocation should return void and then return the offset in outOffset.  Please either make it return the offset, or file a follow-up for that.

Also, please mark it as static, as it doesn't need to reference this.

Comment 10

[(no longer active)]

Comment on attachment 638326 [details] [diff] [review]
Patch part 3 -- Clean up DeleteElementTxn and nsEditor::DeleteNode

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

::: editor/libeditor/base/DeleteElementTxn.cpp
@@ +43,2 @@
>  
>    // do nothing if the parent is read-only

Nit: "if the node doesn't have a parent or if its parent is read-only"

::: editor/libeditor/base/nsEditor.cpp
@@ +1521,5 @@
> +    mActionListeners[i]->DidDeleteNode(aNode->AsDOMNode(), res);
> +  }
> +
> +  NS_ENSURE_SUCCESS(res, res);
> +  return NS_OK;

Nit: return res, and get rid of NS_ENSURE_SUCCESS.

@@ +4626,5 @@
> +  nsresult res = txn->Init(this, aNode, &mRangeUpdater);
> +  NS_ENSURE_SUCCESS(res, res);
> +
> +  txn.forget(aTxn);
> +  return res;

Nit: return NS_OK;

Comment 11

[(no longer active)]

Comment on attachment 638327 [details] [diff] [review]
Patch part 4 -- Rename DeleteElementTxn to DeleteNodeTxn

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

(Splinter totally hides the fact that you've renamed the files here...  Filed bug 770775.)

Comment 12

[(no longer active)]

Comment on attachment 638337 [details] [diff] [review]
Patch part 5 -- Don't special-case -moz-user-select: all elements for deletion

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

(In reply to :Aryeh Gregor from comment #6)
> Okay, so after all that cleanup, here's the story: nsHTMLEditor::DeleteNode
> looks for a selectAllNode like so:
> 
>   nsCOMPtr<nsIDOMNode> selectAllNode = FindUserSelectAllNode(aNode);
> 
>   if (selectAllNode) 
>   { 
>     return nsEditor::DeleteNode(selectAllNode);
>   } 
> 
> So if the node to be deleted has an ancestor with -moz-user-select: all,
> that node will be deleted instead.  Which fails, because it's the root
> element, so is of course not editable.  MoveNode() runs DeleteNode()
> followed by InsertNode(), so the error propagates back.
> 
> Question: why the heck does DeleteNode() look for -moz-user-select: all? 
> CVS blame points to bug 180034.  I don't understand the rationale there. 
> The user cannot create selections that contain only part of the element, so
> why should DeleteNode() have to be special?

I was obviously not around back then, but here's my best guess.  user-select:all is supposed to pretend that a DOM subtree is all a single chunk.  For example, if you select something in it, the entire subtree must be selected.  I think that the goal there was to make sure that the entire subtree gets deleted too.  This would make sense to users I think (whether or not user-select:all is completely broken is a different discussion that we need to have.  ;-)

Therefore, I think we should continue to keep this working, at least until the day that user-select:all is spec'ed.

Can't we just make sure that we _can_ delete the document root element gracefully?

Comment 13

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

(In reply to Ehsan Akhgari [:ehsan] from comment #9)
> Hmm, I don't see any reason why GetNodeLocation should return void and then
> return the offset in outOffset.  Please either make it return the offset, or
> file a follow-up for that.

I tend to leave things as out params when there are two or more.  Having the node be an out param, but the offset returned, seems asymmetric.  Why not the other way around?  That seems preferable if anything, actually, because that way you can avoid getter_AddRefs().

(In reply to Ehsan Akhgari [:ehsan] from comment #10)
> > +  NS_ENSURE_SUCCESS(res, res);
> > +  return NS_OK;
> 
> Nit: return res, and get rid of NS_ENSURE_SUCCESS.

I deliberately use this pattern because that way it outputs warnings.  More than once I've tried to track down the source of an error or exception, and changed "return res;" temporarily to "NS_ENSURE_SUCCESS(res, res); return NS_OK;" so that I could figure out where the error was being propagated from.

(In reply to Ehsan Akhgari [:ehsan] from comment #12)
> I was obviously not around back then, but here's my best guess. 
> user-select:all is supposed to pretend that a DOM subtree is all a single
> chunk.  For example, if you select something in it, the entire subtree must
> be selected.  I think that the goal there was to make sure that the entire
> subtree gets deleted too.  This would make sense to users I think (whether
> or not user-select:all is completely broken is a different discussion that
> we need to have.  ;-)
> 
> Therefore, I think we should continue to keep this working, at least until
> the day that user-select:all is spec'ed.

I don't see the point.  The user already can't place the cursor inside the element here.  The only reason this can happen is because a script overrides regular selection behavior and forces the cursor in a place where user actions can't place it.  In that case, should we really stop the script from doing whatever it feels like?  The script could just use .removeChild if it liked -- or just remove -moz-user-select temporarily.  Why shouldn't it be able to use execCommand()?  And why should DeleteNode() be different from any other of the zillion types of changes we can do to the inside of -moz-user-select: all nodes?  I see that -moz-user-select: all means that it should be treated as a unit for *user* actions, but not to scripts.

So I'm not convinced my patch is wrong.

> Can't we just make sure that we _can_ delete the document root element
> gracefully?

In this case, we can't delete it because it's not editable.  If we're going to say that it has to be treated as a unit, then its entire contents should be non-editable.  I don't see why deletion should be special.  (Making its contents non-editable would of course mean that editability depends on computed style, which isn't a path I think we want to go down.)

Comment 14

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

(In reply to Ehsan Akhgari [:ehsan] from comment #8)
> Also, can you please file a bug to merge nsIEditorSupport into nsEditor, and
> get rid of this interface?  It is not used anywhere in m-c or c-c, and
> continuing to support it is just silly.  Ms2ger might be interested in
> submitting a patch, if not, mark it as [mentor=ehsan] please.  :-)

Bug 770812.

(In reply to Ehsan Akhgari [:ehsan] from comment #9)
> Hmm, I think you should also set *outOffset to -1 in an else branch, since
> some of the callers pass an uninitialized local to this function (cause
> still GetParentNode _might_ fail in optimized builds...)

And then also fail to handle the error, which is now no longer reported . . . but okay.

> >     *  Set aParent to the parent of aChild.
> > +   *  Set aOffset to the offset of aChild in aParent.
> 
> Nit: outParent, outChild and outOffset.

Actually, it's aChild -- that's not an out param.  (Fixed.)

> Hmm, I don't see any reason why GetNodeLocation should return void and then
> return the offset in outOffset.  Please either make it return the offset, or
> file a follow-up for that.

Bug 770814.

> Also, please mark it as static, as it doesn't need to reference this.

It's already static, isn't it?  I added a comment in the .cpp in case that's what you meant.

Comment 15

[(no longer active)]

(In reply to comment #14)
> > Also, please mark it as static, as it doesn't need to reference this.
> 
> It's already static, isn't it?  I added a comment in the .cpp in case that's
> what you meant.

My bad!

Comment 16

[(no longer active)]

(In reply to comment #13)
> (In reply to Ehsan Akhgari [:ehsan] from comment #9)
> > Hmm, I don't see any reason why GetNodeLocation should return void and then
> > return the offset in outOffset.  Please either make it return the offset, or
> > file a follow-up for that.
> 
> I tend to leave things as out params when there are two or more.  Having the
> node be an out param, but the offset returned, seems asymmetric.  Why not the
> other way around?  That seems preferable if anything, actually, because that
> way you can avoid getter_AddRefs().

That's fine as well, just make sure to return an already_AddRefed.

> (In reply to Ehsan Akhgari [:ehsan] from comment #10)
> > > +  NS_ENSURE_SUCCESS(res, res);
> > > +  return NS_OK;
> > 
> > Nit: return res, and get rid of NS_ENSURE_SUCCESS.
> 
> I deliberately use this pattern because that way it outputs warnings.  More
> than once I've tried to track down the source of an error or exception, and
> changed "return res;" temporarily to "NS_ENSURE_SUCCESS(res, res); return
> NS_OK;" so that I could figure out where the error was being propagated from.

OK, then that's fine!

> (In reply to Ehsan Akhgari [:ehsan] from comment #12)
> > I was obviously not around back then, but here's my best guess. 
> > user-select:all is supposed to pretend that a DOM subtree is all a single
> > chunk.  For example, if you select something in it, the entire subtree must
> > be selected.  I think that the goal there was to make sure that the entire
> > subtree gets deleted too.  This would make sense to users I think (whether
> > or not user-select:all is completely broken is a different discussion that
> > we need to have.  ;-)
> > 
> > Therefore, I think we should continue to keep this working, at least until
> > the day that user-select:all is spec'ed.
> 
> I don't see the point.  The user already can't place the cursor inside the
> element here.  The only reason this can happen is because a script overrides
> regular selection behavior and forces the cursor in a place where user actions
> can't place it.  In that case, should we really stop the script from doing
> whatever it feels like?  The script could just use .removeChild if it liked --
> or just remove -moz-user-select temporarily.  Why shouldn't it be able to use
> execCommand()?  And why should DeleteNode() be different from any other of the
> zillion types of changes we can do to the inside of -moz-user-select: all
> nodes?  I see that -moz-user-select: all means that it should be treated as a
> unit for *user* actions, but not to scripts.
> 
> So I'm not convinced my patch is wrong.

I see your reasoning here.  Honestly I don't know what the right fix is here.  Let me think about it a bit more...

> > Can't we just make sure that we _can_ delete the document root element
> > gracefully?
> 
> In this case, we can't delete it because it's not editable.  If we're going to
> say that it has to be treated as a unit, then its entire contents should be
> non-editable.  I don't see why deletion should be special.  (Making its
> contents non-editable would of course mean that editability depends on computed
> style, which isn't a path I think we want to go down.)

Oh, hrm, OK I missed that part about non-editability.

Comment 17

[(no longer active)]

Comment on attachment 638337 [details] [diff] [review]
Patch part 5 -- Don't special-case -moz-user-select: all elements for deletion

Resetting the review request so that I don't forget about this.

Comment 18

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

(In reply to Ehsan Akhgari [:ehsan] from comment #10)
> ::: editor/libeditor/base/DeleteElementTxn.cpp
> @@ +43,2 @@
> >  
> >    // do nothing if the parent is read-only
> 
> Nit: "if the node doesn't have a parent or if its parent is read-only"

Actually, the condition is

  NS_ENSURE_TRUE(!mParent || mEditor->IsModifiableNode(mParent),
                 NS_ERROR_FAILURE);

So I changed the comment to "do nothing if the node has a parent and it's read-only".  If it doesn't have a parent, it initializes successfully and is a no-op at DoTransaction() time.

Comment 19

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

Didn't want the intrusive GetNodeLocation change to bitrot, so I pushed the first four patches.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ff15f925e26
https://hg.mozilla.org/integration/mozilla-inbound/rev/4efdb783d5d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/d513793b2826
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebee137a5573

Comment 20

[(no longer active)]

Comment on attachment 638337 [details] [diff] [review]
Patch part 5 -- Don't special-case -moz-user-select: all elements for deletion

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

I'm still not feeling 100% good about this, but let's take this, and be prepared to back it out if it regresses something.

Comment 21

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/6ff15f925e26
https://hg.mozilla.org/mozilla-central/rev/4efdb783d5d9
https://hg.mozilla.org/mozilla-central/rev/d513793b2826
https://hg.mozilla.org/mozilla-central/rev/ebee137a5573

Comment 22

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/778e030fa299

Comment 23

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/778e030fa299