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

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

Attachment: testcase

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

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: 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 (:ayg) (no longer with Mozilla)]

Attachment: 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]

> 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 (:ayg) (no longer with Mozilla)]

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

[(no longer active)]

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 (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4968c2a22bec

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4968c2a22bec