1373912 - Crash in mozilla::dom::TimeoutManager::~TimeoutManager

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect, P3)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-7084d971-6b17-431f-9fc9-93bc40170616.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::dom::TimeoutManager::~TimeoutManager() 	dom/base/TimeoutManager.cpp:283
1 	xul.dll 	nsPIDOMWindow<nsISupports>::~nsPIDOMWindow<nsISupports>() 	dom/base/nsGlobalWindow.cpp:1026
2 	xul.dll 	nsGlobalWindow::~nsGlobalWindow() 	dom/base/nsGlobalWindow.cpp:1826
3 	xul.dll 	nsGlobalWindow::`scalar deleting destructor'(unsigned int) 	
4 	xul.dll 	nsGlobalWindow::DeleteCycleCollectable() 	dom/base/nsGlobalWindow.cpp:2230
5 	xul.dll 	mozilla::DOMEventTargetHelper::cycleCollection::DeleteCycleCollectable(void*) 	obj-firefox/dist/include/mozilla/DOMEventTargetHelper.h:69
6 	xul.dll 	SnowWhiteKiller::~SnowWhiteKiller() 	xpcom/base/nsCycleCollector.cpp:2651
7 	xul.dll 	nsCycleCollector::FreeSnowWhite(bool) 	xpcom/base/nsCycleCollector.cpp:2826
8 	xul.dll 	nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) 	xpcom/base/nsCycleCollector.cpp:3829
9 	xul.dll 	nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) 	xpcom/base/nsCycleCollector.cpp:3650
10 	xul.dll 	nsCycleCollector_collect(nsICycleCollectorListener*) 	xpcom/base/nsCycleCollector.cpp:4187
11 	xul.dll 	nsJSContext::CycleCollectNow(nsICycleCollectorListener*, int) 	dom/base/nsJSEnvironment.cpp:1633
12 	xul.dll 	nsJSEnvironmentObserver::Observe(nsISupports*, char const*, char16_t const*) 	dom/base/nsJSEnvironment.cpp:526
13 	xul.dll 	nsObserverList::NotifyObservers(nsISupports*, char const*, char16_t const*) 	xpcom/ds/nsObserverList.cpp:112
14 	xul.dll 	nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*) 	xpcom/ds/nsObserverService.cpp:288
15 	xul.dll 	nsThread::DoMainThreadSpecificProcessing(bool) 	xpcom/threads/nsThread.cpp:1641
16 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1326
17 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:96
18 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:231
19 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:211
20 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp:156
21 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp:271
22 	xul.dll 	nsAppStartup::Run() 	toolkit/components/startup/nsAppStartup.cpp:283
23 	xul.dll 	XREMain::XRE_mainRun() 	toolkit/xre/nsAppRunner.cpp:4569
24 	xul.dll 	XREMain::XRE_main(int, char** const, mozilla::BootstrapConfig const&) 	toolkit/xre/nsAppRunner.cpp:4749
25 	xul.dll 	XRE_main(int, char** const, mozilla::BootstrapConfig const&) 	toolkit/xre/nsAppRunner.cpp:4844
26 	xul.dll 	mozilla::BootstrapImpl::XRE_main(int, char** const, mozilla::BootstrapConfig const&) 	toolkit/xre/Bootstrap.cpp:45
27 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:115
28 	firefox.exe 	__scrt_common_main_seh 	f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:253
29 	kernel32.dll 	BaseThreadInitThunk 	
30 	ntdll.dll 	__RtlUserThreadStart 	
31 	ntdll.dll 	_RtlUserThreadStart

crashes with this signature are regressing since 55.0a1 - they are happening with MOZ_RELEASE_ASSERT(mWindow.AsInner()->InnerObjectsFreed()) that got added in bug 1364858.

Comment 1

[Andreas Farre [:farre]]

So ~TimeoutManager is only destroyed when the Window is, right? At that point I'd expect that InnerObjectsFreed should've been called. Or am I missing something. I'm thinking that this might be a too strict assert. It should be true, but because some transient state it happens? Do you have an idea Ben? Should we just remove the assert?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

It appears we don't call nsGlobalWindow::FreeInnerObjects() when the window is cleaned up via cycle collection.  So the assertion is indeed too strict.

The question, though, is how to handle the throttle timer correctly if we enter this cleanup path.  We are checking InnerObjectsFreed() path to avoid creating the timer, right?  That may not be adequate now.

Comment 3

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

(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #2)
> It appears we don't call nsGlobalWindow::FreeInnerObjects() when the window
> is cleaned up via cycle collection.  
We don't? How can that happen? Is it only during shutdown?

But anyhow, if it happens that is quite a bad bug. We don't end up calling
NotifyDOMWindowDestroyed or anything like that.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Olli Pettay [:smaug] from comment #3)
> (In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #2)
> > It appears we don't call nsGlobalWindow::FreeInnerObjects() when the window
> > is cleaned up via cycle collection.  
> We don't? How can that happen? Is it only during shutdown?

Maybe we do, but I don't see it.  Or maybe we release TimeoutManager before calling FreeInnerObjects() in CC case.

Comment 5

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

Hmm, this is memory-pressure.

Comment 6

[Andreas Farre [:farre]]

I'll have one more look at the timer handling, but I'm leaning towards handling this by just allowing the timer to for multiple times and add the timer itself as a condition for creating the timer. Or something like that.

Comment 7

[Mike Taylor [:miketaylr]]

Gonna mark this as fix-optional for 55 given the low volume.

Comment 8

[Wayne Mery (:wsmwk)]

bp-eb1c561a-6f31-4dd7-95b2-04cbf0171215 (previously unsubmitted from a few days ago, so I don't know what was going on)

Comment 9

[Wayne Mery (:wsmwk)]

bp-86033717-f53e-41e9-8e6c-aae040180330 - not reproducible :(

Comment 10

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

I'm not seeing reports for this in 66. The ones listed on top right of this bug are bogus reports, the top of the stack is wrong.
But 65 seems to have still 2 valid crashes.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.