Closed
Bug 1255627
Opened 8 years ago
Closed 8 years ago
crash in mozilla::widget::TSFTextStore::NotifyTSFOfLayoutChange
Categories
(Core :: Widget: Win32, defect)
Tracking
()
People
(Reporter: philipp, Assigned: masayuki)
References
Details
(Keywords: crash, inputmethod, regression)
Crash Data
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
m_kato
:
review+
ritu
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-esr45+
|
Details |
This bug was filed from the Socorro interface and is report bp-b6781bb1-b835-47f9-a319-2c1a22160310. ============================================================= Crashing Thread (0) Frame Module Signature Source 0 xul.dll mozilla::widget::TSFTextStore::NotifyTSFOfLayoutChange() widget/windows/TSFTextStore.cpp 1 xul.dll mozilla::widget::TSFTextStore::OnLayoutChangeInternal() widget/windows/TSFTextStore.cpp 2 xul.dll mozilla::widget::IMEHandler::NotifyIME(nsWindow*, mozilla::widget::IMENotification const&) widget/windows/WinIMEHandler.cpp 3 xul.dll nsWindow::NotifyIMEInternal(mozilla::widget::IMENotification const&) widget/windows/nsWindow.cpp 4 xul.dll nsBaseWidget::NotifyIME(mozilla::widget::IMENotification const&) widget/nsBaseWidget.cpp 5 xul.dll mozilla::IMEStateManager::NotifyIME(mozilla::widget::IMENotification const&, nsIWidget*, bool) dom/events/IMEStateManager.cpp 6 @0x13e0e00f 7 xul.dll nsCOMPtr_base::~nsCOMPtr_base() xpcom/glue/nsCOMPtr.h 8 xul.dll PresShell::DispatchEventToDOM(mozilla::WidgetEvent*, nsEventStatus*, nsPresShellEventCB*) layout/base/nsPresShell.cpp 9 xul.dll mozilla::IMEContentObserver::TryToFlushPendingNotifications() dom/events/IMEContentObserver.cpp 10 xul.dll nsFrameManager::GetPlaceholderFrameFor(nsIFrame const*) layout/base/nsFrameManager.cpp 11 xul.dll nsLayoutUtils::GetParentOrPlaceholderFor(nsIFrame*) layout/base/nsLayoutUtils.cpp 12 xul.dll PresShell::HandlePositionedEvent(nsIFrame*, mozilla::WidgetGUIEvent*, nsEventStatus*) layout/base/nsPresShell.cpp 13 dwmapi.dll DwmDefWindowProc this crash signature is regressing since firefox 44 builds in windows vista and upwards & it's likely due to bug 1234422. many of the user comments seem to link the crashes to viewing a pdf and particularly navigating to a particular page by manually putting in a page number.
Updated•8 years ago
|
Keywords: inputmethod
[Tracking Requested - why for this release]:
blocking-b2g: --- → 2.2?
tracking-b2g:
--- → backlog
![]() |
||
Comment 2•8 years ago
|
||
Is this something we should be tracking? Also, what's the status of tsf on release?
Flags: needinfo?(masayuki)
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #2) > Is this something we should be tracking? I'm not sure if this is e10s only bug, so, I don't know. > Also, what's the status of tsf on release? TSF will be enabled even with e10s. I'll restart to work on TSF stability especially for e10s and Facebook in both modes.
Flags: needinfo?(masayuki)
![]() |
||
Comment 4•8 years ago
|
||
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #3) > (In reply to Jim Mathies [:jimm] from comment #2) > > Is this something we should be tracking? > > I'm not sure if this is e10s only bug, so, I don't know. > > > Also, what's the status of tsf on release? > > TSF will be enabled even with e10s. I'll restart to work on TSF stability > especially for e10s and Facebook in both modes. Hey Masayuki, my request was unrelated to e10s this time around. We're doing general platform triage now on Mondays. The triage team is curious about a couple things: 1) should this crash track 47? 2) when are we planning on rolling tsf out to release
Flags: needinfo?(masayuki)
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #4) > Hey Masayuki, my request was unrelated to e10s this time around. We're doing > general platform triage now on Mondays. The triage team is curious about a > couple things: > > 1) should this crash track 47? I think so. > 2) when are we planning on rolling tsf out to release TSF is already enabled in release builds since Firefox 41.
Flags: needinfo?(masayuki)
Assignee | ||
Comment 6•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d080abf345b4
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•8 years ago
|
||
TSFTextStore::sEnabledTextStore is a static variable to grab a reference to focused TextStore instance. So, this may be changed by accidentally during a call of instance methods of TSFTextStore. Then, focused TextStore may be destroyed during running a method and crash when it accesses a member variable. For avoiding this crash, static methods which call a method of sEnabledTextStore should create an independent RefPtr to it before calling the method. Review commit: https://reviewboard.mozilla.org/r/44733/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44733/
Attachment #8738876 -
Flags: review?(m_kato)
Comment 8•8 years ago
|
||
Comment on attachment 8738876 [details] MozReview Request: Bug 1255627 Don't call methods of TSFTextStore::sEnabledTextStore without independent strong reference to it r?m_kato https://reviewboard.mozilla.org/r/44733/#review41511 Humm, I think that there may be another issue except to releasing object by this crash signature. But it is OK to clear whether this crash reason is reference issue. We should watch same crash signature after landing this.
Attachment #8738876 -
Flags: review?(m_kato) → review+
Assignee | ||
Comment 9•8 years ago
|
||
Thank you for your review! (In reply to Makoto Kato [:m_kato] from comment #8) > Humm, I think that there may be another issue except to releasing object by > this crash signature. Yeah, possibly. > But it is OK to clear whether this crash reason is > reference issue. Unfortunately, I don't know the STR. However, there are some patterns of this signature. So, I think that at least some of them will be fixed by this patch. > We should watch same crash signature after landing this. Yeah.
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c7eedb39844a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8738876 [details] MozReview Request: Bug 1255627 Don't call methods of TSFTextStore::sEnabledTextStore without independent strong reference to it r?m_kato Approval Request Comment [Feature/regressing bug #]: Bug bug 1234422 (perhaps) [User impact if declined]: User may meet this crash. [Describe test coverage new/current, TreeHerder]: Landed on m-c. [Risks and why]: No, because just grabbing TSFTextStore with using RefPtr before calling methods of it from static methods. [String/UUID change made/needed]: Nothing.
Attachment #8738876 -
Flags: approval-mozilla-beta?
Attachment #8738876 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 13•8 years ago
|
||
[Tracking Requested - why for this release]: There are over 500 crash reports per week on 45. And the patch must be enough safe to landed onto ESR.
status-firefox-esr45:
--- → affected
tracking-firefox-esr45:
--- → ?
Comment on attachment 8738876 [details] MozReview Request: Bug 1255627 Don't call methods of TSFTextStore::sEnabledTextStore without independent strong reference to it r?m_kato Even though the crash volume in 47.0a2 is very low, I am still taking it because it's limited to TSF only. Aurora47+
Attachment #8738876 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 15•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/035a7708f988
Comment on attachment 8738876 [details] MozReview Request: Bug 1255627 Don't call methods of TSFTextStore::sEnabledTextStore without independent strong reference to it r?m_kato Crash fix, not high volume in beta but let's try it for beta 10.
Attachment #8738876 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 18•8 years ago
|
||
Comment on attachment 8738876 [details] MozReview Request: Bug 1255627 Don't call methods of TSFTextStore::sEnabledTextStore without independent strong reference to it r?m_kato Taking it in esr45 too. Should be in 45.1.0
Attachment #8738876 -
Flags: approval-mozilla-esr45+
Updated•8 years ago
|
Comment 19•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr45/rev/28d6bd309620
You need to log in
before you can comment on or make changes to this bug.
Description
•