crash in nsBaseWidget::DestroyCompositor()

RESOLVED FIXED in Firefox 40

Status

()

defect
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: alex_mayorga, Assigned: jimm)

Tracking

({crash, regression, topcrash})

Trunk
mozilla40
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

(crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

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
[Tracking Requested - why for this release]: topcrash
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?
Flags: needinfo?(jmathies)
Flags: needinfo?(aklotz)
- 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.
Flags: needinfo?(jmathies)
Flags: needinfo?(aklotz)
Blocks: 1152289
David, do you know what the deal is with that 0x80 crash address? If mWidget were null, shouldn't that be 0x0?
Flags: needinfo?(dmajor)
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>
Flags: needinfo?(dmajor)
Smaug your d&d work landed in here. Mind checking for a potential cause of this crash in that work?
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
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.
Assignee: nobody → jmathies
Posted patch patch (obsolete) — Splinter Review
Posted patch widget patch (obsolete) — Splinter Review
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().
Attachment #8592830 - Attachment is obsolete: true
Attachment #8593314 - Flags: review?(roc)
Thanks for the quick review!

sanity try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=87f9f93f2232
Posted patch patch (r=roc)Splinter Review
Attachment #8593314 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c24fb4429029
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Looking good so far, no crashes in the 20150418 build.
Depends on: 1156283
Looks like the crash is fixed, when we (@lizzard & myself) looked at it in triage, so we untracked it for 40.
You need to log in before you can comment on or make changes to this bug.