#10 crash for 45.1.0 Oldest build found is bp-1bbdd46e-daf9-440c-ac89-6190d2160521 0 xul.dll nsPlainTextSerializer::AddToLine(wchar_t const*, int) dom/base/nsPlainTextSerializer.cpp:1334 1 xul.dll nsPlainTextSerializer::Write(nsAString_internal const&) dom/base/nsPlainTextSerializer.cpp:1718 2 xul.dll nsPlainTextSerializer::DoAddText(bool, nsAString_internal const&) dom/base/nsPlainTextSerializer.cpp:1086 3 xul.dll nsPlainTextSerializer::AppendText(nsIContent*, int, int, nsAString_internal&) dom/base/nsPlainTextSerializer.cpp:346 4 xul.dll nsDocumentEncoder::SerializeNodeStart(nsINode*, int, int, nsAString_internal&, nsINode*) dom/base/nsDocumentEncoder.cpp:457 5 xul.dll nsDocumentEncoder::SerializeToStringRecursive(nsINode*, nsAString_internal&, bool, unsigned int) dom/base/nsDocumentEncoder.cpp:560 6 xul.dll nsDocumentEncoder::SerializeToStringRecursive(nsINode*, nsAString_internal&, bool, unsigned int) dom/base/nsDocumentEncoder.cpp:560 7 xul.dll nsDocumentEncoder::SerializeToStringRecursive(nsINode*, nsAString_internal&, bool, unsigned int) dom/base/nsDocumentEncoder.cpp:560 8 xul.dll nsDocumentEncoder::SerializeToStringRecursive(nsINode*, nsAString_internal&, bool, unsigned int) dom/base/nsDocumentEncoder.cpp:560 9 xul.dll nsDocumentEncoder::SerializeToStringRecursive(nsINode*, nsAString_internal&, bool, unsigned int) dom/base/nsDocumentEncoder.cpp:560 10 xul.dll nsDocumentEncoder::EncodeToStringWithMaxLength(unsigned int, nsAString_internal&) dom/base/nsDocumentEncoder.cpp:1222

Also a top crash in TB 45.0. Looks like what we did to fix the "Asian crisis" in bug 1225864 revealed a possible crash. Look at the code: https://hg.mozilla.org/releases/mozilla-esr45/annotate/1f7c05ab920b/dom/base/nsPlainTextSerializer.cpp#l1334 // fallback if the line breaker is unavailable or failed if (!mLineBreaker) { goodSpace = mWrapColumn-prefixwidth; while (goodSpace >= 0 && !nsCRT::IsAsciiSpace(mCurrentLine.CharAt(goodSpace))) { goodSpace--; } } Now, the line breaker was switched off to solve the "Asian crisis", so the code that was previously not run is now run. I'd say that goodSpace should be tested not to exceed mCurrentLine.Length() or else it will crash ;-) That should be reproducible by setting pref mailnews.wraplength to a large value, say: 999999, before sending a message. I could imagine that people in their desperation did this to avoid the erroneous spaces being inserted into Asian text.

Comment on attachment 8755204 [details] [diff] [review] Simple patch to stop addressing beyond length of string > // fallback if the line breaker is unavailable or failed > if (!mLineBreaker) { > goodSpace = mWrapColumn-prefixwidth; >+ if (goodSpace >= mCurrentLine.Length()) { >+ goodSpace = mCurrentLine.Length() - 1; >+ } > while (goodSpace >= 0 && > !nsCRT::IsAsciiSpace(mCurrentLine.CharAt(goodSpace))) { > goodSpace--; > } > } Of course, looks not bad. However, how about the case if mWrapColumn < prefixWidth? And also if mCurrentLine.Length() is 0?

The line that follows the loop is: if (goodSpace == NS_LINEBREAKER_NEED_MORE_TEXT) { where #define NS_LINEBREAKER_NEED_MORE_TEXT -1 So goodSpace==-1 is expected which is what you get with mCurrentLine.Length()==0. If mWrapColumn < prefixWidth then the crash loop doesn't run. Either mWrapColumn-prefixWidth==-1 or even less, but both are covered below. I think my change is all we need. The crash happens in the loop when addressing to far. Please reconsider your decision or what am I missing?

s/to far/too far/

You said, "Now, the line breaker was switched off to solve the "Asian crisis", so the code that was previously not run is now run." in comment 1. That means that this block may have other bugs such as I pointed. So, I strongly believe that you should set goodSpace to expected values by following code. I read nsILineBreaker::Next(). Then, NS_LINEBREAKER_NEED_MORE_TEXT means "not found the word separator". So, indeed, as you said, if mCurrentLine.Length() is 0, goodSpace should be NS_LINEBREAKER_NEED_MORE_TEXT. However, your code is tricky for this case. Could you separate the case like: if (mCurrentLine.IsEmpty()) { goodSpace = NS_LINEBREAKER_NEED_MORE_TEXT; } else { goodSpace = std::min(mWrapColumn - prefixwidth, mCurrentLine.Length() - 1); while (...) { ... } } This is clearer what the block does. (Although, I'm still worry about the case mWrapColumn - prefwidth < 0. Should we add MOZ_ASSERT() before computing goodSpace?)

https://dxr.mozilla.org/mozilla-central/rev/16663eb3dcfa759f25b5e27b101bc79270c156f2/dom/base/nsPlainTextSerializer.cpp#1333 > if (goodSpace == NS_LINEBREAKER_NEED_MORE_TEXT) { > // If we don't found a good place to break, accept long line and > // try to find another place to break > goodSpace=(prefixwidth>mWrapColumn+1)?1:mWrapColumn-prefixwidth+1; Hmm, if prefixwidth is larger than mWrapColumn, looks like we should fallback the case to the following this block. So, should it be: if (mCurrentLine.IsEmpty() || mWrapColumn < prefixwidth) { goodSpace = NS_LINEBREAKER_NEED_MORE_TEXT; } else { ... ?

Thanks for looking into it. I'll prepare a new patch within the next 24 hours.

I followed your two suggestions so the patch should pass the review. Thanks for encouraging a little clean-up to make things clearer even in this old code ;-)

Comment on attachment 8755435 [details] [diff] [review] Patch to stop addressing beyond length of string with a little more clean-up Thank *you*!

https://hg.mozilla.org/integration/mozilla-inbound/rev/acd9850f1f21dd14be319c45fcdcffbffcf09fa8 Bug 1274837 - don't crash by accessing string beyond its length. r=masayuki

Kent should consider inclusion at the earliest convenience. Now we don't insert erroneous spaces for our Asian users, instead we crash ;-(

The choices for timing are: 1) Push to 45.1.1 which we should build this week, even though it did not make our release candidate. I am reluctant to do this, and this bug itself is a good example of how a fix can sometimes cause unforeseen problems. 2) Try to get this in beta 47.0, which we should build next week, then target for 45.2 which we should build in about three weeks. This is what I would prefer. It requires that we add either a Thunderbird 45 version branch for the beta in m-beta, or do a one-time release branch with this patch added. Opinions yay or nay accepted.

(In reply to Kent James (:rkent) from comment #13) > Opinions yay or nay accepted. Can get worse than crashing ;-)

Sorry: I meant to say: CAN'T get worse than crashing ;-)

The crash rate for non-release builds [1] suggests a week or two, especially on beta, would be decent exposure to those areas of code. And the crash rate on release (0.66%) and lack of angry comments suggests not severe enough that we should to rush it through. So choice #2 for me as well [1] https://crash-stats.mozilla.com/search/?date=%3E2015-12-01&signature=%3DnsPlainTextSerializer%3A%3AAddToLine&release_channel=!release&_facets=signature&_columns=date&_columns=version&_columns=build_id&_columns=platform&_columns=email&_columns=user_comments#crash-reports

(In reply to Jorg K (PTO during summer, NI me) from comment #15) > Sorry: I meant to say: CAN'T get worse than crashing ;-) There is, for example, crashing more often that is worse. We are living with this crash, there are other regressions I could imagine that would be more disastrous that would result in shutting off of updates.

Comment on attachment 8755435 [details] [diff] [review] Patch to stop addressing beyond length of string with a little more clean-up Approval Request Comment [Feature/regressing bug #]: Not a regression, was always wrong, but faulty code path exercised due to fixing bug 333064. [User impact if declined]: Needs to be uplifted if bug 333064 is uplifted. [Describe test coverage new/current, TreeHerder]: No special test. [Risks and why]: Low, just stopped addressing beyond the end of a string. Also see bug 333064 comment #81. [String/UUID change made/needed]: None.

Pushed to a release branch on mozilla-beta for Thunderbird 47.0b1

Comment on attachment 8755435 [details] [diff] [review] Patch to stop addressing beyond length of string with a little more clean-up Fix a crash, taking it

Pushed to THUNDERBIRD_45_VERBRANCH http://hg.mozilla.org/releases/mozilla-esr45/rev/ec50b82997a8

so far so good https://crash-stats.mozilla.com/search/?product=Thunderbird&version=45.2.0&signature=~nsPlainTextSerializer%3A%3AAddToLine&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform