1153939 - crash in nsBaseWidget::DestroyCompositor()

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[alex_mayorga]

This bug was filed from the Socorro interface and is 
report bp-ba616ebb-7a3c-4f9f-8b2f-b82742150413.
=============================================================

This signature is #2 over at https://crash-stats.mozilla.com/topcrasher/products/Firefox/versions/40.0a1

Operating System 	Percentage 	Number Of Crashes
Windows 7 	53.65 %	169
Windows 8.1 	40.95 %	129
Windows Unknown 	2.54 %	8
Windows 8 	2.54 %	8
Windows Vista 	0.32 %	1

Crashing Thread
Frame 	Module 	Signature 	Source
0 	xul.dll 	nsBaseWidget::DestroyCompositor() 	widget/nsBaseWidget.cpp
1 	xul.dll 	WidgetShutdownObserver::Observe(nsISupports*, char const*, wchar_t const*) 	widget/nsBaseWidget.cpp
2 	xul.dll 	nsObserverList::NotifyObservers(nsISupports*, char const*, wchar_t const*) 	xpcom/ds/nsObserverList.cpp
3 	xul.dll 	nsObserverService::NotifyObservers(nsISupports*, char const*, wchar_t const*) 	xpcom/ds/nsObserverService.cpp
4 	xul.dll 	ScopedXPCOMStartup::~ScopedXPCOMStartup() 	toolkit/xre/nsAppRunner.cpp
5 	xul.dll 	ScopedXPCOMStartup::`scalar deleting destructor'(unsigned int) 	
6 	xul.dll 	XREMain::XRE_main(int, char** const, nsXREAppData const*) 	toolkit/xre/nsAppRunner.cpp
7 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp
8 	firefox.exe 	do_main 	browser/app/nsBrowserApp.cpp
9 	firefox.exe 	NS_internal_main(int, char**) 	browser/app/nsBrowserApp.cpp
10 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp
11 	firefox.exe 	__tmainCRTStartup 	f:/dd/vctools/crt/crtw32/startup/crt0.c:255
12 	kernel32.dll 	BaseThreadInitThunk 	
13 	ntdll.dll 	__RtlUserThreadStart 	
14 	ntdll.dll 	_RtlUserThreadStart

Comment 1

[Mats Palmgren (inactive)]

[Tracking Requested - why for this release]: topcrash

Comment 2

[(Away)]

Well this is lovely, mWidget is null even though it was null-checked relatively recently: http://hg.mozilla.org/mozilla-central/annotate/8f57f60ee58a/widget/nsBaseWidget.cpp#l171

This began in 20150409030206, so the range is https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=078128c2600a&tochange=8f57f60ee58a

jimm/aklotz any ideas?

Comment 3

[Jim Mathies [:jimm]]

- only happens with e10s enabled
- doesn't seem to be tied to AsyncPluginInit

There's a lot of stuff in that range. Will take a more detailed look tomorrow.

Comment 4

[Jim Mathies [:jimm]]

David, do you know what the deal is with that 0x80 crash address? If mWidget were null, shouldn't that be 0x0?

Comment 5

[(Away)]

this==nullptr in nsBaseWidget::DestroyCompositor, then it tries to offset 0x80 from |this| in order to read |mCompositorChild|:

xul!nsBaseWidget::DestroyCompositor+0x4:
000007fe`e559ea04 488b8180000000  mov     rax,qword ptr [rcx+80h] ds:00000000`00000080=????????????????

0:000> dt nsBaseWidget
xul!nsBaseWidget
[...]
   +0x080 mCompositorChild : nsRefPtr<mozilla::layers::CompositorChild>

Comment 6

[Jim Mathies [:jimm]]

Smaug your d&d work landed in here. Mind checking for a potential cause of this crash in that work?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

The first reports happen in a build which didn't have the dnd work.
Also, I don't know how that could have caused this.

Comment 8

[Jim Mathies [:jimm]]

WidgetShutdownObserver:

https://dxr.mozilla.org/mozilla-central/source/widget/nsBaseWidget.h#59

WidgetShutdownObserver creation and registration:

https://dxr.mozilla.org/mozilla-central/source/widget/nsBaseWidget.cpp#156

Note no unregistration here, WidgetShutdownObserver relies on nsBaseWidget for this:

https://dxr.mozilla.org/mozilla-central/source/widget/nsBaseWidget.cpp#220

where we set mWidget to nullptr, and then unregister - 

https://dxr.mozilla.org/mozilla-central/source/widget/nsBaseWidget.cpp#225

I think we need a ref up the widget when we enter Observe so the widget can't pull the rug out from underneath WidgetShutdownObserver.

Comment 9

[Jim Mathies [:jimm]]

Attachment: patch

Comment 10

[Jim Mathies [:jimm]]

Attachment: widget patch

Looks like I've addressed this based on about 25 retriggers:

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

This patch just streamlines the shutdown of these objects which we keep in nsBaseWidget. It looks like we were setting mWidget in WidgetShutdownObserver to null while we were in WidgetShutdownObserver::Observe().

Comment 11

[Jim Mathies [:jimm]]

Thanks for the quick review!

sanity try push:

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

Comment 12

[Jim Mathies [:jimm]]

Attachment: patch (r=roc)

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Jim Mathies [:jimm]]

Looking good so far, no crashes in the 20150418 build.

Comment 16

[jen strickland :jenstrickland]

Looks like the crash is fixed, when we (@lizzard & myself) looked at it in triage, so we untracked it for 40.