Last Comment Bug 766305 - "ASSERTION: could not get text to delete" with large text node
: "ASSERTION: could not get text to delete" with large text node
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on:
Blocks: 325861 336383
  Show dependency treegraph
 
Reported: 2012-06-19 14:15 PDT by Jesse Ruderman
Modified: 2012-06-24 20:08 PDT (History)
3 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (370 bytes, text/html)
2012-06-19 14:15 PDT, Jesse Ruderman
no flags Details
stack trace (3.08 KB, text/plain)
2012-06-19 14:17 PDT, Jesse Ruderman
no flags Details
Patch v1 (3.47 KB, patch)
2012-06-21 06:49 PDT, :Aryeh Gregor (away until August 15)
no flags Details | Diff | Review
Patch v2, changes a couple more bogus PRInt16's (3.47 KB, patch)
2012-06-21 08:02 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Review

Description Jesse Ruderman 2012-06-19 14:15:52 PDT
Created attachment 634586 [details]
testcase

###!!! ASSERTION: could not get text to delete.: 'NS_SUCCEEDED(res)', file editor/libeditor/base/DeleteTextTxn.cpp, line 74
Comment 1 Jesse Ruderman 2012-06-19 14:17:13 PDT
Created attachment 634587 [details]
stack trace
Comment 2 :Aryeh Gregor (away until August 15) 2012-06-21 06:49:43 PDT
Created attachment 635290 [details] [diff] [review]
Patch v1

So apparently, whoever designed WSFragment thought that 16k chars would be enough for anyone, since they made mStartOffset and mEndOffset PRInt16 . . .

Something I don't understand, though -- where's the downcast to PRInt16 happening here?  Shouldn't the compiler refuse to try storing a PRInt32 in a PRInt16 without a cast?  I don't see any casts here, and didn't have to remove any for the crashtest to pass, but this seems like a clear underflow bug.

I changed the NS_ASSERTION to MOZ_ASSERT, because the only way this can fail is if the start offset is longer than the node's length, and that's something that should never realistically happen.

Try: https://tbpl.mozilla.org/?tree=Try&rev=5029f6106538

Thanks for all these bugs, Jesse!  FYI, this test-case is *much* more comprehensible than many previous ones, because it explicitly sets the selection with collapse().  I didn't have to poke around with debug statements to see where the selection actually was (and thus what was really happening).
Comment 3 :Aryeh Gregor (away until August 15) 2012-06-21 08:02:13 PDT
Created attachment 635314 [details] [diff] [review]
Patch v2, changes a couple more bogus PRInt16's

I converted a couple more PRInt16's when writing my last patch, but forgot to write out the file, so it didn't get included in the patch.  Can we enable -Wconversion to catch things like this, by any chance?

New try: https://tbpl.mozilla.org/?tree=Try&rev=41030dc2465d
Comment 4 Jesse Ruderman 2012-06-21 16:52:31 PDT
> Can we enable -Wconversion to catch things like this, by any chance?

It was disabled in bug 450196.  Is modern gcc or clang better?  If you file a new bug, please mark it as blocking bug 450196, and CC me.
Comment 5 :Aryeh Gregor (away until August 15) 2012-06-22 00:13:53 PDT
It sounds like it's a useful warning as such, it just produces lots and lots and lots of noise.  That's unsurprising.  For it to be useful, we'd have to rewrite a lot of code to be more careful what types we use, and to use a lot of casting.
Comment 6 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-22 07:07:25 PDT
Comment on attachment 635314 [details] [diff] [review]
Patch v2, changes a couple more bogus PRInt16's

Review of attachment 635314 [details] [diff] [review]:
-----------------------------------------------------------------

(In C++, conversion from a wider integral type to a shorter one is implicit.  Please send your thank-you notes to the designers of the language!  ;-)
Comment 7 :Aryeh Gregor (away until August 15) 2012-06-23 23:55:44 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/4968c2a22bec
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-06-24 20:08:37 PDT
https://hg.mozilla.org/mozilla-central/rev/4968c2a22bec

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