The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla16

Status

()

Core
Editor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: ayg)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla16
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

(Reporter)

Description

5 years ago
Created attachment 638158 [details]
testcase

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

Comment 1

5 years ago
Created attachment 638159 [details]
stack trace
Created attachment 638323 [details] [diff] [review]
Patch part 1 -- Clean up nsEditor::GetChildOffset
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #638323 - Flags: review?(ehsan)
Created attachment 638324 [details] [diff] [review]
Patch part 2 -- Clean up nsEditor::GetNodeLocation
Attachment #638324 - Flags: review?(ehsan)
Created attachment 638326 [details] [diff] [review]
Patch part 3 -- Clean up DeleteElementTxn and nsEditor::DeleteNode
Attachment #638326 - Flags: review?(ehsan)
Created attachment 638327 [details] [diff] [review]
Patch part 4 -- Rename DeleteElementTxn to DeleteNodeTxn
Attachment #638327 - Flags: review?(ehsan)
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.
Depends on: 180034
Created attachment 638337 [details] [diff] [review]
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
Attachment #638337 - Flags: review?(ehsan)
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...
Attachment #638323 - Flags: review?(ehsan) → review+
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.
Attachment #638324 - Flags: review?(ehsan) → review+
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;
Attachment #638326 - Flags: review?(ehsan) → review+
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.)
Attachment #638327 - Flags: review?(ehsan) → review+
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?
Attachment #638337 - Flags: review?(ehsan) → review-
(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.)
Depends on: 770812
Depends on: 770814
(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.
(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!
(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 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.
Attachment #638337 - Flags: review- → review?(ehsan)
(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.
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
Whiteboard: [Leave open after merge]
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.
Attachment #638337 - Flags: review?(ehsan) → review+
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/778e030fa299
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86_64 → All
Whiteboard: [Leave open after merge]
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/778e030fa299
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 771983

Updated

5 years ago
Blocks: 211106
You need to log in before you can comment on or make changes to this bug.