Closed Bug 766025 Opened 10 years ago Closed 10 years ago
"Assertion failure: a
Editor && a Char Data && a Num Chars To Delete" with forward Delete, <select>
No description provided.
aNumCharsToDelete is 0.
Assertion failure: aEditor && aCharData && aNumCharsToDelete, at /Users/jruderman/trees/mozilla-central/editor/libeditor/base/DeleteTextTxn.cpp:43
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
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #634315 - Flags: review?(ehsan)
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: 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.
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.)
OS: Mac OS X → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla16
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.