829946 - crash in nsIMEStateManager::OnDestroyPresContext @ nsPresContext::GetNearestWidget

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[Scoobidiver (away)]

With that stack trace, it first showed up in 21.0a1/20130109. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dccab70d8554&tochange=7bce868864bf
It might be caused by bug 823041.

Signature 	nsPresContext::GetNearestWidget(nsPoint*) More Reports Search
UUID	72c9df1e-6485-479b-9b1b-964002130112
Date Processed	2013-01-12 12:58:33
Uptime	85
Last Crash	11.7 weeks before submission
Install Age	12.7 hours since version was first installed.
Install Time	2013-01-12 00:13:29
Product	Firefox
Version	21.0a1
Build ID	20130111030906
Release Channel	nightly
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
Build Architecture	x86
Build Architecture Info	AuthenticAMD family 20 model 1 stepping 0
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x4
App Notes 
AdapterVendorID: 0x1002, AdapterDeviceID: 0x9802, AdapterSubsysID: 00000000, AdapterDriverVersion: 8.812.2.1000
D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ 
EMCheckCompatibility	True
Adapter Vendor ID	0x1002
Adapter Device ID	0x9802
Total Virtual Memory	4294836224
Available Virtual Memory	3770261504
System Memory Use Percentage	38
Available Page File	9371803648
Available Physical Memory	3700944896

Frame 	Module 	Signature 	Source
0 	xul.dll 	nsPresContext::GetNearestWidget 	layout/base/nsPresContext.cpp:1186
1 	xul.dll 	nsIMEStateManager::OnDestroyPresContext 	content/events/src/nsIMEStateManager.cpp:130
2 	xul.dll 	PresShell::Destroy 	layout/base/nsPresShell.cpp:971
3 	xul.dll 	nsDocumentViewer::DestroyPresShell 	layout/base/nsDocumentViewer.cpp:4356
4 	xul.dll 	nsDocumentViewer::Destroy 	layout/base/nsDocumentViewer.cpp:1628
5 	xul.dll 	nsDocumentViewer::Destroy 	layout/base/nsDocumentViewer.cpp:1669
6 	xul.dll 	nsDocShell::Destroy 	docshell/base/nsDocShell.cpp:4912
7 	xul.dll 	nsXULWindow::Destroy 	xpfe/appshell/src/nsXULWindow.cpp:479
8 	xul.dll 	nsWebShellWindow::Destroy 	xpfe/appshell/src/nsWebShellWindow.cpp:754
9 	xul.dll 	nsWebShellWindow::RequestWindowClose 	xpfe/appshell/src/nsWebShellWindow.cpp:311
10 	xul.dll 	nsWindow::ProcessMessage 	widget/windows/nsWindow.cpp:4784
11 	xul.dll 	nsWindow::WindowProcInternal 	widget/windows/nsWindow.cpp:4407
12 	xul.dll 	CallWindowProcCrashProtected 	xpcom/base/nsCrashOnException.cpp:32
13 	xul.dll 	nsWindow::WindowProc 	widget/windows/nsWindow.cpp:4359
14 	user32.dll 	InternalCallWinProc 	
15 	user32.dll 	UserCallWinProcCheckWow 	
16 	user32.dll 	DispatchClientMessage 	
17 	user32.dll 	__fnDWORD 	
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsPresContext%3A%3AGetNearestWidget%28nsPoint*%29

Comment 1

[Mats Palmgren (inactive)]

Yeah, bug 823041 seems the likely culprit.

It appears GetView() returns null for the root frame in the top stack frame.
Maybe because the surrounding shell is being destroyed, and the view
hierarchy has already been detached from any sub-shell root frames?
I suspect that prior to bug 823041 we got a nsIMEStateManager::OnRemoveContent
via nsDocument::Destroy() which would set sPresContext to NULL and then
OnDestroyPresContext would not call GetNearestWidget.  The difference is that
OnRemoveContent checks for ContentIsDescendantOf, but OnDestroyPresContext
doesn't check if aPresContext is an ancestor.

I guess we could just null-check GetView(), or if it really is necessary to
call SetIMEState on a widget that is about to be destroyed, we could check
if sPresContext is a descendant in OnDestroyPresContext.  Or both.

Comment 2

[Mats Palmgren (inactive)]

Actually, the widget is probably *not* being destroyed in this case.

Comment 3

[Timothy Nikkel (:tnikkel)]

Hmm, as far as I can tell there isn't a way to make frame->GetView() return null after it has returned non-null, even if that non-null value pointers to de-allocated memory. (The NS_FRAME_HAS_VIEW bit is never unset and the ViewProperty frame property is only ever set to something non-null.)

Comment 4

[Mats Palmgren (inactive)]

Attachment: fix

https://tbpl.mozilla.org/?tree=Try&rev=9da0f5c516ca

Comment 5

[Mats Palmgren (inactive)]

> The NS_FRAME_HAS_VIEW bit is never unset

OK, the only possibility I can think of then is that sPresContext was deleted and
now points to random memory.  GetNearestWidget isn't virtual so I think we might
read a bogus mShell, then mRootFrame which state bits happens to have a zero
NS_FRAME_HAS_VIEW and thus we get null from GetView().
I don't see how that could happen though, without OnDestroyPresContext being
called.

Perhaps nsIMEStateManager should just NS_ADDREF/RELEASE its sPresContext
as it does for sContent.

Comment 6

[Mats Palmgren (inactive)]

Attachment: fix v2

Added ref counting for sPresContext.

https://tbpl.mozilla.org/?tree=Try&rev=5a2ef594652c

Comment 7

[Mats Palmgren (inactive)]

Comment on attachment 701548 [details] [diff] [review]
fix v2

I think we should take this and see if it helps.  What do you think?

Comment 8

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren [:mats] from comment #1)
> I suspect that prior to bug 823041 we got a
> nsIMEStateManager::OnRemoveContent
> via nsDocument::Destroy() which would set sPresContext to NULL and then
> OnDestroyPresContext would not call GetNearestWidget.  The difference is that
> OnRemoveContent checks for ContentIsDescendantOf, but OnDestroyPresContext
> doesn't check if aPresContext is an ancestor.

Hmm, except ContentIsDescendantOf doesn't cross document boundaries, so if that is the problem we shouldn't need to cross document boundaries when checking the prescontext, should we?

And if we don't need to look at other prescontexts then we can't be dealing with a deleted prescontext because the pointer is presumably live in PresShell:Destroy right before this call. Unless that pointer is pointer to deleted memory already somehow?

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 701548 [details] [diff] [review]
fix v2

I don't think this is bad, but I wouldn't mind at least having a theory behind making a change before landing it and seeing if it works.

Comment 10

[Mats Palmgren (inactive)]

> the pointer is presumably live in PresShell:Destroy right before this call

Ah, good point.

> Unless that pointer is pointer to deleted memory already somehow?

I don't see how.

Comment 11

[Mats Palmgren (inactive)]

I'm puzzled by the stack frames:
4 nsDocumentViewer::Destroy 	layout/base/nsDocumentViewer.cpp:1628
5 nsDocumentViewer::Destroy 	layout/base/nsDocumentViewer.cpp:1669

I don't see a path from 5 to 4.  At first I though it might be an error
in the stack walker and that #5 should be nsDocumentViewer::~nsDocumentViewer
http://hg.mozilla.org/mozilla-central/annotate/8592c41069c2/layout/base/nsDocumentViewer.cpp#l583
because nsDocShell::mContentViewer was the last ref.  But nsDocShell::Destroy
explicitly did mContentViewer->Close(nullptr); mContentViewer->Destroy();
before that line, so I don't see how "mPresShell || mPresContext" could
have been true (in ~nsDocumentViewer).

Comment 12

[Scoobidiver (away)]

Crashes stopped after 21.0a1/20130206. The working range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bc108d2ce8d1&tochange=04e13fc9dbff

Comment 13

[Timothy Nikkel (:tnikkel)]

Looks like bug 838001 probably fixed this.