[e10s] Crash in mozilla::ContentCache::TextRectArray::GetUnionRectAsFarAsPossible(unsigned int, unsigned int, bool)

RESOLVED FIXED in Firefox 50

Status

()

defect
P3
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: marcia, Assigned: masayuki)

Tracking

(Depends on 1 bug, {crash, inputmethod, regression})

Trunk
mozilla51
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s-, firefox48 unaffected, firefox49 unaffected, firefox50+ fixed, firefox51+ fixed)

Details

(Whiteboard: tpi:+, crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

This bug was filed from the Socorro interface and is 
report bp-dd03e05e-5cc7-4219-b388-169a62160731.
=============================================================

Seen while looking at nightly crash stats: http://bit.ly/2aIQGOq. Summary shows it affects Windows 7 only.

One Comment: when use ctr+space it will crash 

ni on masayuki since it looks like he may have worked in that area.
Flags: needinfo?(masayuki)
Although, I don't know how to reproduce this, but probably, the array length is 0. I'll post a patch soon.
Flags: needinfo?(masayuki)
[Tracking Requested - why for this release]: Due to regression of bug 1280182 (landed onto 50).
Assignee: nobody → masayuki
Blocks: 1280182
Status: NEW → ASSIGNED
Has STR: --- → no
Component: Graphics → Widget
Hardware: Unspecified → All
Summary: Crash in mozilla::gfx::BaseRect<T>::Union → [e10s] Crash in mozilla::ContentCache::TextRectArray::GetUnionRectAsFarAsPossible(unsigned int, unsigned int, bool)
This must be able to reproduce with some IMEs which creates 0 length composition string.  In such case, mTextRectArray isn't available, but mTextRectArray.GetUnionRectAsFarAsPossible() always assumes that it's valid and has at least one rect.  Therefore, it can meet this crash.

Therefore, this patch makes that ContentCacheInParent::GetUnionTextRects() not use mTextRectArray when it's not valid nor doesn't have rects.

Review commit: https://reviewboard.mozilla.org/r/68832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68832/
Attachment #8777235 - Flags: review?(m_kato)
According with the mozilla crash stats - Firefox 48 seems to be also affected:
https://crash-stats.mozilla.com/report/index/a4fbc7c4-e3ca-4832-9e5a-3321d2160803
https://crash-stats.mozilla.com/report/index/721f4a4c-3f6a-435d-bd5d-6a2852160803

Masayuki, do you think these 2 crashes are related with this bug?
Flags: needinfo?(masayuki)
No, I don't think so. It looks like crashed in image related code. Note that gfx::BaseRect is an abstract (template) class of representing rectangle. So, it's used by a lot of modules. You should check non-top of the frame of the stack.
Flags: needinfo?(masayuki)
Makes sense to track this crash for 50/51 due to the regression noted in Comment 2.
Comment on attachment 8777235 [details]
Bug 1291082 ContentCacheInParent::GetUnionTextRects() shouldn't use mTextRectArray when it's empty

https://reviewboard.mozilla.org/r/68832/#review66302
Attachment #8777235 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/30f222a7ff2b
ContentCacheInParent::GetUnionTextRects() shouldn't use mTextRectArray when it's empty r=m_kato
tracking-e10s: --- → -
Priority: -- → P3
Whiteboard: tpi:+
https://hg.mozilla.org/mozilla-central/rev/30f222a7ff2b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Odd. We still have crash reports after the landing. So, it's not enough, but we still don't have any STR...

https://crash-stats.mozilla.com/report/index/309633cc-4a32-4317-807e-064e22160808
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I think that we can *hide* this crash from users with adding |i < mRects.Length()| to the |for| loop. However, I don't understand why there is (at least) one crash path here.

Fortunately, a few crash reports are submitted everyday. So, let's collect very variable values from crash reporter.
Attachment #8779382 - Flags: review?(m_kato)
Ah, MOZ_CRASH() shouldn't be in the patch due to changing the stack signature. So, please ignore it, I'll remove it before landing.
Comment on attachment 8779382 [details] [diff] [review]
Append values of related variables as notes of crash report

Review of attachment 8779382 [details] [diff] [review]:
-----------------------------------------------------------------

Humm, crash condition is that mRect.IsEmpt() might be true.  So I think that startOffset - mStart is negative and both type is unsigned.

In ContentCache::GetUnionRectAsFarAsPossible(),

  uint32_t startOffset = std::max(aOffset, mStart);
  if (aRoundToExistingOffset && startOffset >= EndOffset()) {
    startOffset = EndOffset() - 1;
    ...

If aOffset < mStart and startOffset == EndOffset(), this crash occurs.

So I think that you should fix IsOverlappingWith (or GetUnionRectAsFarAsPossible) before this patch.  It doesn't check whether aOffset >= mStart yet.
Attachment #8779382 - Flags: review?(m_kato)
ContentCache::TextRectArray's end offset is computed as start offset + its length.  Therefore, end offset is the last offset in the range + 1.  So, IsOverlappingWith() shouldn't return true only when end offset is overlapped with given range.

That means that when TextRectArray doesn't have rects, i.e., mRects is empty, IsOverlappingWith() always returns false. And when TextRectArray has one or more rects, IsOverlappingWith() should compare its range and given range without "=".
Attachment #8779382 - Attachment is obsolete: true
Attachment #8781909 - Flags: review?(m_kato)
ContentCache::TextRectArray::GetUnionRectAsFarAsPossible() should avoid crash by itself even if it's caller's bug. This makes parent process more stable, that is what one of the purpose of e10s is.
Attachment #8781910 - Flags: review?(m_kato)
Attachment #8781909 - Flags: review?(m_kato) → review+
Attachment #8781910 - Flags: review?(m_kato) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/10ca767648a511eaaee798ec22dbda768407973e
Bug 1291082 part.2 ContentCache::TextRectArray::IsOverlappingWith() shouldn't include end offset in its range r=m_kato

https://hg.mozilla.org/integration/mozilla-inbound/rev/c127bdf8186fa2de85aa0bb54fd46c45074d66c6
Bug 1291082 part.3 ContentCache::TextRectArray::GetUnionRectAsFarAsPossible() should avoid crash by itself r=m_kato
https://hg.mozilla.org/mozilla-central/rev/10ca767648a5
https://hg.mozilla.org/mozilla-central/rev/c127bdf8186f
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Hello Masayuki, could you please nominate for uplift to Aurora50?
Flags: needinfo?(masayuki)
Okay, we don't have any new crash reports after landing the additional patches. Let's uplift them.
Flags: needinfo?(masayuki)
Comment on attachment 8777235 [details]
Bug 1291082 ContentCacheInParent::GetUnionTextRects() shouldn't use mTextRectArray when it's empty

Approval Request Comment
[Feature/regressing bug #]: bug 1280182
[User impact if declined]: May meet this crash when IME tries to retrieve a rect around start of composition string (we're still not sure how to reproduce this).
[Describe test coverage new/current, TreeHerder]: Landed on m-c and we don't get new crash reports after that.
[Risks and why]: Low. This patch makes ContentCacheInParent::GetUnionTextRects() doesn't call TextRectArray::GetUnionRectAsFarAsPossible() when it doesn't have any rects.
[String/UUID change made/needed]: Nothing.
Attachment #8777235 - Flags: approval-mozilla-aurora?
Comment on attachment 8781909 [details] [diff] [review]
Bug 1291082 part.2 ContentCache::TextRectArray::IsOverlappingWith() shouldn't end offset is in its range

Approval Request Comment
[Risks and why]: Low, this fixes the mistake of treating "end offset" in the method.
[String/UUID change made/needed]: Nothing.

See comment 27 for the other information.
Attachment #8781909 - Flags: approval-mozilla-aurora?
Comment on attachment 8781910 [details] [diff] [review]
Bug 1291082 part.3 ContentCache::TextRectArray::GetUnionRectAsFarAsPossible() should avoid crash by itself

Approval Request Comment
[Risks and why]: Low, this patch makes TextRectArray::GetUnionRectAsFarAsPossible() avoid to crash by itself. This might hide crash bug, but odd behavior of showing IME related window is better than this crash.
[String/UUID change made/needed]: Nothing.

See comment 27 for the other information.
Attachment #8781910 - Flags: approval-mozilla-aurora?
Actually given that this crash is super low volume on Aurora50 at the moment (5 instances in a week), I would like to wontfix for Fx50 as the fix does not seem to straightforward (3 patches!). I should have checked that before requesting uplift. Sorry about that! :-/

If this crash spikes in the future, then we can consider uplifting the fix.
Attachment #8777235 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #8781909 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #8781910 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
> I would like to wontfix for Fx50 as the fix does not seem to straightforward (3 patches!)

The reason why there are 3 patches is, the first patch failed to fix this bug. Therefore, I looked for another bug around the patch. So, 2nd patch is fixing the remaining issue. The 3rd patch is last resort to fix this bug completely. It prevents to crash with unexpected other bugs.
The signature for this crash has changed to [@ InvalidArrayIndex_CRASH | mozilla::ContentCache::TextRectArray::GetUnionRectAsFarAsPossible]. I see 17 crashes on 50 in the last week, and none on 51.
Crash Signature: [@ mozilla::gfx::BaseRect<T>::Union] → [@ mozilla::gfx::BaseRect<T>::Union] [@ InvalidArrayIndex_CRASH | mozilla::ContentCache::TextRectArray::GetUnionRectAsFarAsPossible]
:ritu

Looks like that the crash reports are not so low volume after release.
https://crash-stats.mozilla.com/signature/?signature=InvalidArrayIndex_CRASH%20%7C%20mozilla%3A%3AContentCache%3A%3ATextRectArray%3A%3AGetUnionRectAsFarAsPossible&date=%3E%3D2016-11-06T11%3A37%3A00.000Z&date=%3C2016-11-21T11%3A37%3A00.000Z#graphs

According to the comments and twitter, this crash occurs at writing email, some blog services and office 365. So, I recommend that we take this fix at dot release.
Flags: needinfo?(rkothari)
I am ok with taking this in 50.1.0, the planned dot release mid-December. The 50.0.1 release is minimal and only for addressing severe issues. I would prefer to wait for this fix to go out with 50.1.0 and not this week.
Flags: needinfo?(rkothari)
Comment on attachment 8777235 [details]
Bug 1291082 ContentCacheInParent::GetUnionTextRects() shouldn't use mTextRectArray when it's empty

Approval Request Comment

See comment 27. The number of this crash report increased and the comments indicate that this crash occurs mainly user types big data such as emails, blog entries and office documents. So, this crash may have a risk users to stop using Firefox.

Note that this patch failed to fix this bug, but this is necessary for following patches.
Attachment #8777235 - Flags: approval-mozilla-release?
Comment on attachment 8781909 [details] [diff] [review]
Bug 1291082 part.2 ContentCache::TextRectArray::IsOverlappingWith() shouldn't end offset is in its range

Approval Request Comment:

See comment 27. This patch fixes the crash actually.
Attachment #8781909 - Flags: approval-mozilla-release?
Comment on attachment 8781910 [details] [diff] [review]
Bug 1291082 part.3 ContentCache::TextRectArray::GetUnionRectAsFarAsPossible() should avoid crash by itself

Approval Request Comment:

See comment 27.

Finally, this makes the last resort. Even if previous patch couldn't fix the crash completely, this would avoid crash and put warning in debug build.

Note that these patches are tested for a couple of months mainly in Nightly channel and Aurora channel. So, they must be enough safe to uplift.
Attachment #8781910 - Flags: approval-mozilla-release?
Comment on attachment 8777235 [details]
Bug 1291082 ContentCacheInParent::GetUnionTextRects() shouldn't use mTextRectArray when it's empty

Top crasher on 50, meets the triage bar for inclusion in 50.1.0
Attachment #8777235 - Flags: approval-mozilla-release? → approval-mozilla-release+
Attachment #8781909 - Flags: approval-mozilla-release? → approval-mozilla-release+
Attachment #8781910 - Flags: approval-mozilla-release? → approval-mozilla-release+
You need to log in before you can comment on or make changes to this bug.