Last Comment Bug 766025 - "Assertion failure: aEditor && aCharData && aNumCharsToDelete" with forwardDelete, <select>
: "Assertion failure: aEditor && aCharData && aNumCharsToDelete" with forwardDe...
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla16
Assigned To: :Aryeh Gregor (working until September 2)
:
Mentors:
Depends on: 771431 771105
Blocks: 336383 343943
  Show dependency treegraph
 
Reported: 2012-06-18 22:10 PDT by Jesse Ruderman
Modified: 2012-07-06 07:47 PDT (History)
3 users (show)
ayg: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (asserts fatally when loaded) (214 bytes, application/xhtml+xml)
2012-06-18 22:10 PDT, Jesse Ruderman
no flags Details
stack trace+ (13.92 KB, text/plain)
2012-06-18 22:11 PDT, Jesse Ruderman
no flags Details
Patch v1 -- Remove the assert (1019 bytes, patch)
2012-06-19 01:20 PDT, :Aryeh Gregor (working until September 2)
ehsan: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-06-18 22:10:12 PDT
Created attachment 634296 [details]
testcase (asserts fatally when loaded)
Comment 1 Jesse Ruderman 2012-06-18 22:11:35 PDT
Created attachment 634297 [details]
stack trace+

aNumCharsToDelete is 0.
Comment 2 Jesse Ruderman 2012-06-18 22:15:35 PDT
Assertion failure: aEditor && aCharData && aNumCharsToDelete, at /Users/jruderman/trees/mozilla-central/editor/libeditor/base/DeleteTextTxn.cpp:43
Comment 3 :Aryeh Gregor (working until September 2) 2012-06-19 01:20:23 PDT
Created attachment 634315 [details] [diff] [review]
Patch v1 -- Remove the assert

Caused by bug 762183 part 1.  On reflection, I think there's no reason to assert here at all -- it should just be a no-op.  It's legitimate for a caller to try deleting an arbitrary range of text without checking that it has nonzero length.

Ehsan, do you think this needs a test?

Try: https://tbpl.mozilla.org/?tree=Try&rev=0e374b45834c
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2012-06-19 05:34:40 PDT
We should at least land Jesse's test case as a crashtest, IMO.
Comment 5 :Aryeh Gregor (working until September 2) 2012-06-19 06:15:07 PDT
When I try to land it as a crashtest, it passes on an unpatched build, even though the exact same contents crash when loaded as a normal web page.  It seems like execCommand() is throwing NS_ERROR_FAILURE before it hits the assert.  Not sure why.
Comment 6 :Ehsan Akhgari 2012-06-19 08:04:35 PDT
(In reply to Aryeh Gregor from comment #5)
> When I try to land it as a crashtest, it passes on an unpatched build, even
> though the exact same contents crash when loaded as a normal web page.  It
> seems like execCommand() is throwing NS_ERROR_FAILURE before it hits the
> assert.  Not sure why.

Yes, this does need a test.  Can you please look into why the test doesn't repro the bug in the crashtest suite?
Comment 7 Jesse Ruderman 2012-06-19 09:44:30 PDT
> When I try to land it as a crashtest, it passes on an unpatched build

Did it have an .xhtml extension? Was the window focused?
Comment 8 :Aryeh Gregor (working until September 2) 2012-06-19 11:47:04 PDT
Yeah, it was failing because I was making it .html instead of .xhtml -- I didn't notice the test-case was XHTML.  This probably made a difference in what the DOM looked like, and then a difference in where the cursor happened to be when forwardDelete was called.  These fuzzer-generated issues have consistently not set the selection position explicitly, so they're very sensitive to the exact markup unless you modify them to explicitly position the selection where it would naturally end up.

But now the test fails with leaks.  Here's a log of running the newly-added crashtest:

http://paste.ubuntu.com/1049390/

Here's a try run, which shows leaks on all debug builds that have completed so far: https://tbpl.mozilla.org/?tree=Try&rev=783b109f42c0

Obviously I can't push this test with the leaks, but I don't know how to debug or fix them either.
Comment 9 :Ehsan Akhgari 2012-06-19 12:47:31 PDT
Are you on a good base revision?
Comment 10 :Aryeh Gregor (working until September 2) 2012-06-20 00:35:55 PDT
Yes, in the sense that if I qpop the top patch and recompile, editor crashtests pass.  Also, if I run all editor crashtests except the one I added in this commit (by commenting it out of crashtest.list), there's no leak.
Comment 11 :Ehsan Akhgari 2012-06-20 09:44:33 PDT
Hmm, you can use the instructions here: https://wiki.mozilla.org/Performance:Leak_Tools if you wanna debug this...
Comment 12 :Aryeh Gregor (working until September 2) 2012-06-24 07:38:45 PDT
The instructions are not very clear to me.  It's just a list of tons of tools with only vague hints as to how to use them or how to choose which one to use.  If there's a step-by-step guide somewhere on how to figure out what code is causing the problem, I'll give it a shot.
Comment 13 :Ehsan Akhgari 2012-06-25 10:42:54 PDT
Oh, sorry I linked you to the wrong page, here's what I meant to link to: <http://www-archive.mozilla.org/performance/leak-tutorial.html>

note that this is an exploratory process.  I've never debugged two leaks with the same exact steps to figure out each of them.  :-)
Comment 14 :Aryeh Gregor (working until September 2) 2012-07-02 02:55:46 PDT
I have enough other things to work on right now, I think.  Can I check in the patch without the test and file a followup on fixing the leak and adding the test?  Or perhaps I could try to find a test that doesn't leak and check that in instead?
Comment 15 :Ehsan Akhgari 2012-07-03 14:22:40 PDT
(In reply to comment #14)
> I have enough other things to work on right now, I think.  Can I check in the
> patch without the test and file a followup on fixing the leak and adding the
> test?  Or perhaps I could try to find a test that doesn't leak and check that
> in instead?

Checking in the patch as it is would make the test suite go perma-orange right?  I'm fine with checking a version of the test in which doesn't cause any leaks (and that may in fact lead us to have some idea on what causes the leak.)
Comment 16 :Aryeh Gregor (working until September 2) 2012-07-05 05:42:32 PDT
I fiddled with the test a little and couldn't get it not to leak while still actually testing the patch.  I filed bug 771105 on allowing expected leaks in crashtests.  For the time being, could I push the patch without a test and file a followup on the test?  That way at least the bug will be fixed, even if we don't have a test.
Comment 17 :Ehsan Akhgari 2012-07-05 14:32:19 PDT
(In reply to comment #16)
> I fiddled with the test a little and couldn't get it not to leak while still
> actually testing the patch.  I filed bug 771105 on allowing expected leaks in
> crashtests.  For the time being, could I push the patch without a test and file
> a followup on the test?  That way at least the bug will be fixed, even if we
> don't have a test.

Yes, please go ahead and do that for now...
Comment 18 :Ehsan Akhgari 2012-07-05 14:33:27 PDT
(And please file a follow-up to figure out the leak.)
Comment 19 :Aryeh Gregor (working until September 2) 2012-07-06 00:58:42 PDT
Follow-up in bug 771431.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc819724b3a4

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