Created attachment 634586 [details] testcase ###!!! ASSERTION: could not get text to delete.: 'NS_SUCCEEDED(res)', file editor/libeditor/base/DeleteTextTxn.cpp, line 74
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).
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
> 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! ;-)