Closed Bug 1274837 Opened 8 years ago Closed 8 years ago

Crash in nsPlainTextSerializer::AddToLine

Categories

(Core :: DOM: Serializers, defect)

x86
Windows 7
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed
thunderbird_esr45 47+ fixed

People

(Reporter: wsmwk, Assigned: jorgk-bmo)

References

Details

(Keywords: crash, topcrash-thunderbird, Whiteboard: [regression:TB45])

Crash Data

Attachments

(1 file, 1 obsolete file)

#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.
Component: General → Serializers
Product: Thunderbird → Core
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #8755204 - Flags: review?(masayuki)
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?
Attachment #8755204 - Flags: review?(masayuki) → review-
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/
Flags: needinfo?(masayuki)
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?)
Flags: needinfo?(masayuki)
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 ;-)
Attachment #8755204 - Attachment is obsolete: true
Attachment #8755435 - Flags: review?(masayuki)
Comment on attachment 8755435 [details] [diff] [review]
Patch to stop addressing beyond length of string with a little more clean-up

Thank *you*!
Attachment #8755435 - Flags: review?(masayuki) → review+
Keywords: checkin-needed
Keywords: checkin-needed
Kent should consider inclusion at the earliest convenience. Now we don't insert erroneous spaces for our Asian users, instead we crash ;-(
Flags: needinfo?(rkent)
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.
Flags: needinfo?(rkent)
(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
Blocks: 1225864
(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.
https://hg.mozilla.org/mozilla-central/rev/acd9850f1f21
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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.
Attachment #8755435 - Flags: approval-mozilla-aurora?
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
Attachment #8755435 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.