The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla16

Status

()

Core
Editor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: ayg)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Trunk
mozilla16
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 634586 [details]
testcase

###!!! ASSERTION: could not get text to delete.: 'NS_SUCCEEDED(res)', file editor/libeditor/base/DeleteTextTxn.cpp, line 74
(Reporter)

Comment 1

5 years ago
Created attachment 634587 [details]
stack trace
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).
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #635290 - Flags: review?(ehsan)
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
Attachment #635290 - Attachment is obsolete: true
Attachment #635290 - Flags: review?(ehsan)
Attachment #635314 - Flags: review?(ehsan)
(Reporter)

Comment 4

5 years ago
> 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
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.