Closed Bug 766305 Opened 12 years ago Closed 12 years ago

"ASSERTION: could not get text to delete" with large text node

Categories

(Core :: DOM: Editor, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: jruderman, Assigned: ayg)

References

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 1 obsolete file)

Attached file testcase
###!!! ASSERTION: could not get text to delete.: 'NS_SUCCEEDED(res)', file editor/libeditor/base/DeleteTextTxn.cpp, line 74
Attached file stack trace
Attached patch Patch v1 (obsolete) — Splinter Review
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).
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #635290 - Flags: review?(ehsan)
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
Attachment #635290 - Attachment is obsolete: true
Attachment #635290 - Flags: review?(ehsan)
Attachment #635314 - Flags: review?(ehsan)
> 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.
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 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!  ;-)
Attachment #635314 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/4968c2a22bec
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/4968c2a22bec
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.