Last Comment Bug 768748 - "ASSERTION: bad args" with out-of-tree selection, insertorderedlist
: "ASSERTION: bad args" with out-of-tree selection, insertorderedlist
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on:
Blocks: 336383
  Show dependency treegraph
 
Reported: 2012-06-26 19:05 PDT by Jesse Ruderman
Modified: 2012-07-05 08:13 PDT (History)
3 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (300 bytes, text/html)
2012-06-26 19:05 PDT, Jesse Ruderman
no flags Details
stack trace (18.34 KB, text/plain)
2012-06-26 19:05 PDT, Jesse Ruderman
no flags Details
Patch part 1, v1 -- Clean up nsHTMLEditor::IsNext/PrevCharWhitespace (16.09 KB, patch)
2012-07-01 04:58 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Review
Patch part 2, v1 -- Rewrite nsHTMLEditRules::GetPromotedPoint (22.43 KB, patch)
2012-07-01 05:00 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Review

Description Jesse Ruderman 2012-06-26 19:05:29 PDT
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
Comment 1 Jesse Ruderman 2012-06-26 19:05:46 PDT
Created attachment 636974 [details]
stack trace
Comment 2 :Aryeh Gregor (away until August 15) 2012-07-01 04:58:28 PDT
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.
Comment 3 :Aryeh Gregor (away until August 15) 2012-07-01 05:00:38 PDT
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.
Comment 4 :Aryeh Gregor (away until August 15) 2012-07-01 05:00:50 PDT
Try: https://tbpl.mozilla.org/?tree=Try&rev=7f57af2ee95a
Comment 5 :Ms2ger 2012-07-01 05:53:02 PDT
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?
Comment 6 :Aryeh Gregor (away until August 15) 2012-07-01 05:57:05 PDT
Oops, you're right.  That was from an older patch version.  Deleted.
Comment 7 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-03 11:56:00 PDT
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.
Comment 8 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-03 12:01:18 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.