crash in mozilla::widget::TSFTextStore::NotifyTSFOfLayoutChange

RESOLVED FIXED in Firefox 46

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: philipp, Assigned: masayuki)

Tracking

({crash, inputmethod, regression})

44 Branch
mozilla48
x86
Windows NT
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2?, tracking-b2g:backlog, firefox45 wontfix, firefox46 fixed, firefox47 fixed, firefox48 fixed, firefox-esr4546+ fixed)

Details

(crash signature)

Attachments

(1 attachment)

Reporter

Description

3 years ago
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.
Keywords: inputmethod

Comment 1

3 years ago
[Tracking Requested - why for this release]:
blocking-b2g: --- → 2.2?
Is this something we should be tracking?

Also, what's the status of tsf on release?
Flags: needinfo?(masayuki)
(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)
(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)
(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: nobody → masayuki
Status: NEW → ASSIGNED
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 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+
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c7eedb39844a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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?
[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.
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 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+

Updated

3 years ago
Depends on: 1263653
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+
You need to log in before you can comment on or make changes to this bug.