Created attachment 634296 [details]
testcase (asserts fatally when loaded)
Created attachment 634297 [details]
aNumCharsToDelete is 0.
Assertion failure: aEditor && aCharData && aNumCharsToDelete, at /Users/jruderman/trees/mozilla-central/editor/libeditor/base/DeleteTextTxn.cpp:43
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?
We should at least land Jesse's test case as a crashtest, IMO.
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.
(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?
> 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?
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:
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.
Are you on a good base revision?
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.
Hmm, you can use the instructions here: https://wiki.mozilla.org/Performance:Leak_Tools if you wanna debug this...
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.
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. :-)
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?
(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.)
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.
(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...
(And please file a follow-up to figure out the leak.)
Follow-up in bug 771431.