Crash in nsPlainTextSerializer::AddToLine

VERIFIED FIXED in Firefox 48

Status

()

Core
Serializers
--
critical
VERIFIED FIXED
11 months ago
10 months ago

People

(Reporter: wsmwk, Assigned: Jorg K (GMT+2))

Tracking

({crash, topcrash-thunderbird})

unspecified
mozilla49
x86
Windows 7
crash, topcrash-thunderbird
Points:
---

Firefox Tracking Flags

(firefox48 fixed, firefox49 fixed, thunderbird_esr4547+ fixed)

Details

(Whiteboard: [regression:TB45], crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 months ago
#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
(Assignee)

Comment 1

11 months ago
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.
(Assignee)

Updated

11 months ago
Component: General → Serializers
Product: Thunderbird → Core
(Assignee)

Comment 2

11 months ago
Created attachment 8755204 [details] [diff] [review]
Simple patch to stop addressing beyond length of string
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #8755204 - Flags: review?(masayuki)
(Assignee)

Updated

11 months ago
Keywords: regression, regressionwindow-wanted
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-
(Assignee)

Comment 4

11 months ago
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?
(Assignee)

Comment 5

11 months ago
s/to far/too far/
(Assignee)

Updated

11 months ago
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 {
  ...

?
(Assignee)

Comment 8

11 months ago
Thanks for looking into it. I'll prepare a new patch within the next 24 hours.
(Assignee)

Comment 9

11 months ago
Created attachment 8755435 [details] [diff] [review]
Patch to stop addressing beyond length of string with a little more clean-up

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+
(Assignee)

Updated

11 months ago
Keywords: checkin-needed

Comment 11

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

Updated

11 months ago
Keywords: checkin-needed
(Assignee)

Comment 12

11 months ago
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)

Comment 13

11 months ago
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)
(Assignee)

Comment 14

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

Comment 15

11 months ago
Sorry: I meant to say: CAN'T get worse than crashing ;-)
(Reporter)

Comment 16

11 months ago
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

Comment 17

11 months ago
(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 18

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/acd9850f1f21
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Comment 19

11 months ago
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?

Comment 20

11 months ago
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+

Comment 22

11 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/c2507c070a55
status-firefox48: --- → fixed

Comment 23

10 months ago
Pushed  to THUNDERBIRD_45_VERBRANCH http://hg.mozilla.org/releases/mozilla-esr45/rev/ec50b82997a8
status-thunderbird_esr45: affected → fixed
tracking-thunderbird_esr45: ? → 47+
(Reporter)

Comment 24

10 months ago
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.