The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla16



5 years ago
5 years ago


(Reporter: Jesse Ruderman, Assigned: ayg)


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

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

Firefox Tracking Flags

(Not tracked)



(3 attachments, 1 obsolete attachment)



5 years ago
Created attachment 634586 [details]

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

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.


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
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:
Attachment #635290 - Attachment is obsolete: true
Attachment #635290 - Flags: review?(ehsan)
Attachment #635314 - Flags: review?(ehsan)

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+
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla16
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.