"ASSERTION: bad args" with out-of-tree selection, insertorderedlist

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
x86_64
Mac OS X
assertion, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
Created attachment 636973 [details]
testcase

###!!! ASSERTION: null node passed to nsEditor::Tag(): 'aNode', file editor/libeditor/base/nsEditor.cpp, line 3851

###!!! ASSERTION: bad args: '(inChild && outParent && outOffset)', file editor/libeditor/base/nsEditor.cpp, line 3153
(Reporter)

Comment 1

5 years ago
Created attachment 636974 [details]
stack trace
Created attachment 638177 [details] [diff] [review]
Patch part 1, v1 -- Clean up nsHTMLEditor::IsNext/PrevCharWhitespace

There turned out to be huge amounts of dead code here:

 5 files changed, 68 insertions(+), 204 deletions(-)

The end of each function only did anything when node was both a block and a text node, i.e., never.  These were the only callers of NextNodeInBlock, which was the only caller of IsTextOrElementNode.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #638177 - Flags: review?(ehsan)
Created attachment 638179 [details] [diff] [review]
Patch part 2, v1 -- Rewrite nsHTMLEditRules::GetPromotedPoint

This is what was causing the assert.  If the point was in a text node, it would get promoted to the text node's parent, but nothing checked that the parent actually existed.  Fortunately this didn't crash, just asserted.  I suspect this kind of bug is because people would assume that GetNodeLocation returned an error code if the parent was null, which it doesn't.
Attachment #638179 - Flags: review?(ehsan)
Try: https://tbpl.mozilla.org/?tree=Try&rev=7f57af2ee95a
Comment on attachment 638177 [details] [diff] [review]
Patch part 1, v1 -- Clean up nsHTMLEditor::IsNext/PrevCharWhitespace

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

::: editor/libeditor/html/nsHTMLEditor.h
@@ +251,5 @@
>    NS_IMETHOD PreDestroy(bool aDestroyingFrames);
>  
>    /** Internal, static version */
>    static nsresult NodeIsBlockStatic(nsIDOMNode *aNode, bool *aIsBlock);
> +  static bool NodeIsBlockStatic(nsINode* aNode);

I don't think you implemented this, did you?
Oops, you're right.  That was from an older patch version.  Deleted.
Comment on attachment 638177 [details] [diff] [review]
Patch part 1, v1 -- Clean up nsHTMLEditor::IsNext/PrevCharWhitespace

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

::: editor/libeditor/html/nsHTMLEditor.cpp
@@ +957,4 @@
>      }
>    }
> +
> +  // XXX: we don't handle the case where the next char is in another node

Let's rename these methods to make this more apparent: IsNextCharInNodeWhitespace, etc.
Attachment #638177 - Flags: review?(ehsan) → review+
Comment on attachment 638179 [details] [diff] [review]
Patch part 2, v1 -- Rewrite nsHTMLEditRules::GetPromotedPoint

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

Looks very good!

::: editor/libeditor/html/nsHTMLEditRules.cpp
@@ +5385,5 @@
>      *outOffset = offset;
> +    return;
> +  }
> +
> +  // kEnd

Nit: aWhere == kEnd

@@ +5437,5 @@
> +    nearNode = mHTMLEditor->GetNextHTMLNode(node, offset, true);
> +  }
> +  *outNode = node->AsDOMNode();
> +  *outOffset = offset;
> +  return;

Nit: drop the return.
Attachment #638179 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bfd425ea2e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/e467906239f6
Flags: in-testsuite+
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/2bfd425ea2e5
https://hg.mozilla.org/mozilla-central/rev/e467906239f6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.