Closed
Bug 1291082
Opened 8 years ago
Closed 8 years ago
[e10s] Crash in mozilla::ContentCache::TextRectArray::GetUnionRectAsFarAsPossible(unsigned int, unsigned int, bool)
Categories
(Core :: Widget, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
e10s | - | --- |
firefox48 | --- | unaffected |
firefox49 | --- | unaffected |
firefox50 | + | fixed |
firefox51 | + | fixed |
People
(Reporter: marcia, Assigned: masayuki)
References
(Depends on 1 open bug)
Details
(Keywords: crash, inputmethod, regression, Whiteboard: tpi:+)
Crash Data
Attachments
(3 files, 1 obsolete file)
58 bytes,
text/x-review-board-request
|
m_kato
:
review+
ritu
:
approval-mozilla-aurora-
ritu
:
approval-mozilla-release+
|
Details |
2.05 KB,
patch
|
m_kato
:
review+
ritu
:
approval-mozilla-aurora-
ritu
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
1.79 KB,
patch
|
m_kato
:
review+
ritu
:
approval-mozilla-aurora-
ritu
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Comment 2•8 years ago
|
||
[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
status-firefox50:
--- → affected
tracking-firefox50:
--- → ?
tracking-firefox51:
--- → ?
Component: Graphics → Widget
Keywords: inputmethod,
regression
Hardware: Unspecified → All
Summary: Crash in mozilla::gfx::BaseRect<T>::Union → [e10s] Crash in mozilla::ContentCache::TextRectArray::GetUnionRectAsFarAsPossible(unsigned int, unsigned int, bool)
Assignee | ||
Comment 3•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=40494a0a76e2
Assignee | ||
Comment 4•8 years ago
|
||
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)
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
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)
Updated•8 years ago
|
status-firefox48:
--- → unaffected
status-firefox49:
--- → unaffected
Reporter | ||
Comment 7•8 years ago
|
||
Makes sense to track this crash for 50/51 due to the regression noted in Comment 2.
Comment 8•8 years ago
|
||
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
Updated•8 years ago
|
tracking-e10s:
--- → -
Updated•8 years ago
|
Priority: -- → P3
Whiteboard: tpi:+
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/30f222a7ff2b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Assignee | ||
Comment 11•8 years ago
|
||
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
Assignee | ||
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7acd2d6f4f84
Assignee | ||
Comment 13•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=caff17cdcd3a
Assignee | ||
Comment 14•8 years ago
|
||
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)
Assignee | ||
Comment 15•8 years ago
|
||
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 16•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8779382 -
Flags: review?(m_kato)
Assignee | ||
Comment 17•8 years ago
|
||
Oh, good point. I'll check it.
Assignee | ||
Comment 18•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=15a23ebc9684
Assignee | ||
Comment 19•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ced37bc38802
Assignee | ||
Comment 20•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d06aa5c0241
Assignee | ||
Comment 21•8 years ago
|
||
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)
Assignee | ||
Comment 22•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8781909 -
Flags: review?(m_kato) → review+
Updated•8 years ago
|
Attachment #8781910 -
Flags: review?(m_kato) → review+
Assignee | ||
Comment 23•8 years ago
|
||
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
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/10ca767648a5 https://hg.mozilla.org/mozilla-central/rev/c127bdf8186f
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Hello Masayuki, could you please nominate for uplift to Aurora50?
Flags: needinfo?(masayuki)
Assignee | ||
Comment 26•8 years ago
|
||
Okay, we don't have any new crash reports after landing the additional patches. Let's uplift them.
Flags: needinfo?(masayuki)
Assignee | ||
Comment 27•8 years ago
|
||
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?
Assignee | ||
Comment 28•8 years ago
|
||
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?
Assignee | ||
Comment 29•8 years ago
|
||
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-
Assignee | ||
Comment 31•8 years ago
|
||
> 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.
Comment 32•8 years ago
|
||
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]
Assignee | ||
Comment 33•8 years ago
|
||
: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)
Assignee | ||
Comment 35•7 years ago
|
||
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?
Assignee | ||
Comment 36•7 years ago
|
||
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?
Assignee | ||
Comment 37•7 years ago
|
||
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+
Comment 39•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-release/rev/3efa36accd3f https://hg.mozilla.org/releases/mozilla-release/rev/590db93819a5 https://hg.mozilla.org/releases/mozilla-release/rev/5e550887033a
You need to log in
before you can comment on or make changes to this bug.
Description
•