1255627 - crash in mozilla::widget::TSFTextStore::NotifyTSFOfLayoutChange

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[[:philipp]]

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.

Comment 1

[yoan]

[Tracking Requested - why for this release]:

Comment 2

[Jim Mathies [:jimm]]

Is this something we should be tracking?

Also, what's the status of tsf on release?

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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.

Comment 4

[Jim Mathies [:jimm]]

(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

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d080abf345b4

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: MozReview Request: Bug 1255627 Don't call methods of TSFTextStore::sEnabledTextStore without independent strong reference to it r?m_kato

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/

Comment 8

[Makoto Kato [: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.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7eedb39844a

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c7eedb39844a

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

[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 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/035a7708f988

Comment 16

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f0fa0700be78

Comment 18

[Sylvestre Ledru [:Sylvestre]]

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

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr45/rev/28d6bd309620