If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Crash in mozilla::dom::TimeoutManager::~TimeoutManager

NEW
Assigned to

Status

()

Core
DOM
P3
critical
3 months ago
11 days ago

People

(Reporter: philipp, Assigned: farre)

Tracking

({crash, regression})

55 Branch
crash, regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 wontfix, firefox56 wontfix, firefox57 fix-optional)

Details

(crash signature)

(Reporter)

Description

3 months ago
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.

Updated

3 months ago
Flags: needinfo?(afarre)
(Assignee)

Comment 1

3 months ago
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?
Flags: needinfo?(afarre) → needinfo?(bkelly)

Comment 2

3 months ago
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.
Flags: needinfo?(bkelly) → needinfo?(afarre)

Comment 3

3 months ago
(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

3 months ago
(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

3 months ago
Hmm, this is memory-pressure.
(Assignee)

Comment 6

3 months ago
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.
Flags: needinfo?(afarre)
Gonna mark this as fix-optional for 55 given the low volume.
status-firefox55: affected → fix-optional
Assignee: nobody → afarre
Priority: -- → P3
status-firefox55: fix-optional → wontfix
status-firefox56: affected → wontfix
status-firefox57: --- → fix-optional
You need to log in before you can comment on or make changes to this bug.