Last Comment Bug 766360 - "ASSERTION: bad args" with out-of-tree selection, inserthtml
: "ASSERTION: bad args" with out-of-tree selection, inserthtml
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: :Aryeh Gregor (working until September 2)
:
Mentors:
Depends on: 760052
Blocks: 336383
  Show dependency treegraph
 
Reported: 2012-06-19 15:38 PDT by Jesse Ruderman
Modified: 2012-06-26 01:59 PDT (History)
3 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (308 bytes, text/html)
2012-06-19 15:38 PDT, Jesse Ruderman
no flags Details
stack trace+ (14.29 KB, text/plain)
2012-06-19 15:40 PDT, Jesse Ruderman
no flags Details
Patch v1 (10.58 KB, patch)
2012-06-21 00:28 PDT, :Aryeh Gregor (working until September 2)
ehsan: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-06-19 15:38:05 PDT
Created attachment 634625 [details]
testcase

###!!! ASSERTION: bad args: '(aChild && aParent)', file editor/libeditor/base/nsEditor.cpp, line 3139
Comment 1 Jesse Ruderman 2012-06-19 15:40:55 PDT
Created attachment 634628 [details]
stack trace+

aParent is null.

(By the way, asserting a conjunction results in vague error logs. I prefer two separate assertions.)
Comment 2 :Ehsan Akhgari 2012-06-19 16:29:48 PDT
Aryeh, can you please take a look?  Seems easy to fix.
Comment 3 :Aryeh Gregor (working until September 2) 2012-06-20 04:40:14 PDT
This is another case where we should just be returning false and refusing to try to do anything at all.  insertHTML is not enabled here -- bug 760052.
Comment 4 :Aryeh Gregor (working until September 2) 2012-06-20 04:40:28 PDT
(But we can do more explicit null-checking too.)
Comment 5 :Aryeh Gregor (working until September 2) 2012-06-21 00:28:05 PDT
Created attachment 635210 [details] [diff] [review]
Patch v1

This patch actually keeps the NS_ASSERTION.  The real fix is bug 760052, which refuses the run the command to start with in such a pathological case.  However, as usual, I rewrote the relevant function while trying to figure out what was happening.  This function was pathological even by editor/ standards, particularly the variable naming.

There were only three callers.  Two ignored the out params and just relied on the fact that the selection would be in roughly the right place, so I got rid of the out params and just put the selection in the right place to start with.

In the new function I use both MOZ_ASSERT and NS_ASSERTION, based on how pathological I felt the error case would be.  In all cases I also checked for the error in non-debug builds and did basic handling, so that if they do fail we won't crash or anything.

Really it would probably make more sense for this to be renamed SplitSelection() or something and the delete part moved out.  One of the callers doesn't use the delete behavior anyway because it wants different eStrip/eNoStrip behavior.

Try: https://tbpl.mozilla.org/?tree=Try&rev=e28d01ae0a94
Comment 6 :Aryeh Gregor (working until September 2) 2012-06-23 23:57:42 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/66b3801292a0

This is actually not in the testsuite yet, I just realized.  I'll add the testcase to bug 760052, so marking in-testsuite+.
Comment 7 :Aryeh Gregor (working until September 2) 2012-06-24 00:33:08 PDT
Backed out for now: https://hg.mozilla.org/integration/mozilla-inbound/rev/b507d9bfcd04

MOZ_ASSERT(node, "Selection has no ranges in it"); was causing crashtest failures because bug 760052 hasn't landed yet.  Will reland after bug 760052.
Comment 8 :Aryeh Gregor (working until September 2) 2012-06-25 08:33:47 PDT
Take two: https://hg.mozilla.org/integration/mozilla-inbound/rev/5cca92dac983
Comment 9 Ed Morley [:emorley] 2012-06-26 01:59:13 PDT
https://hg.mozilla.org/mozilla-central/rev/5cca92dac983

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