Bug 103515 - Assertion failed: SolarMutex not locked when opening database with Base
Summary: Assertion failed: SolarMutex not locked when opening database with Base
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Base (show other bugs)
Version:
(earliest affected)
5.3.0.0.alpha0+
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:5.4.0
Keywords:
: 103667 104138 114401 (view as bug list)
Depends on:
Blocks: VCL-Scheduler Crash-Assert
  Show dependency treegraph
 
Reported: 2016-10-26 09:14 UTC by Aron Budea
Modified: 2017-12-14 07:13 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Aron Budea 2016-10-26 09:14:37 UTC
Split off from bug 103507.
I opened a database with Base in a 5.3 master debug build (2e137e4696e8278c3d56ba8a49a8b255534bd048) / Windows 7.

Backtrace:

>	vcllo.dll!ImplDbgTestSolarMutex() Line 49	C++
 	tllo.dll!DbgTestSolarMutex() Line 75	C++
 	vcllo.dll!Scheduler::Start() Line 287	C++
 	vcllo.dll!Timer::Start() Line 94	C++
 	fwklo.dll!`anonymous namespace'::AutoRecovery::implts_updateTimer() Line 2276	C++
 	fwklo.dll!`anonymous namespace'::AutoRecovery::changesOccurred(const com::sun::star::util::ChangesEvent & aEvent) Line 1679	C++
 	fwklo.dll!framework::WeakChangesListener::changesOccurred(const com::sun::star::util::ChangesEvent & rEvent) Line 222	C++
 	configmgrlo.dll!configmgr::Broadcaster::send() Line 187	C++
 	configmgrlo.dll!configmgr::Access::insertByName(const rtl::OUString & aName, const com::sun::star::uno::Any & aElement) Line 1233	C++
 	fwklo.dll!`anonymous namespace'::AutoRecovery::implts_flushConfigItem(const `anonymous-namespace'::AutoRecovery::TDocumentInfo & rInfo, bool bRemoveIt) Line 2092	C++
 	fwklo.dll!`anonymous namespace'::AutoRecovery::implts_registerDocument(const com::sun::star::uno::Reference<com::sun::star::frame::XModel> & xDocument) Line 2520	C++
 	fwklo.dll!`anonymous namespace'::AutoRecovery::documentEventOccured(const com::sun::star::document::DocumentEvent & aEvent) Line 1571	C++
 	fwklo.dll!framework::WeakDocumentEventListener::documentEventOccured(const com::sun::star::document::DocumentEvent & rEvent) Line 258	C++
 	sfxlo.dll!comphelper::OInterfaceContainerHelper2::NotifySingleListener<com::sun::star::document::XDocumentEventListener,com::sun::star::document::DocumentEvent>::operator()(const com::sun::star::uno::Reference<com::sun::star::document::XDocumentEventListener> & listener) Line 272	C++
 	sfxlo.dll!comphelper::OInterfaceContainerHelper2::forEach<com::sun::star::document::XDocumentEventListener,comphelper::OInterfaceContainerHelper2::NotifySingleListener<com::sun::star::document::XDocumentEventListener,com::sun::star::document::DocumentEvent> >(const comphelper::OInterfaceContainerHelper2::NotifySingleListener<com::sun::star::document::XDocumentEventListener,com::sun::star::document::DocumentEvent> & func) Line 286	C++
 	sfxlo.dll!comphelper::OInterfaceContainerHelper2::notifyEach<com::sun::star::document::XDocumentEventListener,com::sun::star::document::DocumentEvent>(void (const com::sun::star::document::DocumentEvent &) * NotificationMethod, const com::sun::star::document::DocumentEvent & Event) Line 299	C++
 	sfxlo.dll!`anonymous namespace'::SfxGlobalEvents_Impl::implts_notifyListener(const com::sun::star::document::DocumentEvent & aEvent) Line 439	C++
 	sfxlo.dll!`anonymous namespace'::SfxGlobalEvents_Impl::documentEventOccured(const com::sun::star::document::DocumentEvent & Event) Line 244	C++
 	dbalo.dll!comphelper::OInterfaceContainerHelper2::NotifySingleListener<com::sun::star::document::XDocumentEventListener,com::sun::star::document::DocumentEvent>::operator()(const com::sun::star::uno::Reference<com::sun::star::document::XDocumentEventListener> & listener) Line 272	C++
 	dbalo.dll!comphelper::OInterfaceContainerHelper2::forEach<com::sun::star::document::XDocumentEventListener,comphelper::OInterfaceContainerHelper2::NotifySingleListener<com::sun::star::document::XDocumentEventListener,com::sun::star::document::DocumentEvent> >(const comphelper::OInterfaceContainerHelper2::NotifySingleListener<com::sun::star::document::XDocumentEventListener,com::sun::star::document::DocumentEvent> & func) Line 286	C++
 	dbalo.dll!comphelper::OInterfaceContainerHelper2::notifyEach<com::sun::star::document::XDocumentEventListener,com::sun::star::document::DocumentEvent>(void (const com::sun::star::document::DocumentEvent &) * NotificationMethod, const com::sun::star::document::DocumentEvent & Event) Line 299	C++
 	dbalo.dll!dbaccess::DocumentEventNotifier_Impl::impl_notifyEvent_nothrow(const com::sun::star::document::DocumentEvent & _rEvent) Line 197	C++
 	dbalo.dll!dbaccess::DocumentEventNotifier_Impl::processEvent(const comphelper::AnyEvent & _rEvent) Line 230	C++
 	comphelper.dll!comphelper::AsyncEventNotifierBase::execute() Line 165	C++
 	comphelper.dll!comphelper::AsyncEventNotifierAutoJoin::run() Line 263	C++
 	comphelper.dll!threadFunc(void * param) Line 185	C++
 	sal3.dll!oslWorkerWrapperFunction(void * pData) Line 59	C
 	[External Code]	
 	[Frames below may be incorrect and/or missing, no symbols loaded for msvcr120d.dll]
Comment 1 Stephan Bergmann 2016-10-26 10:50:53 UTC
looks like a hit of <https://cgit.freedesktop.org/libreoffice/core/commit/?id=c00d8271ba443c4f0acf657c226eea4824597f95> "vcl: assert solar mutex is held for various timer / scheduler ops."
Comment 2 Michael Meeks 2016-10-26 15:09:03 UTC
Well - the code indeed looks problematic. The issue is that I added this warning - since the scheduler code is clearly not thread safe, and requires the SolarMutex - or somesuch to be locked in order to manipulate its structures.

Of course - the timer and idle logic is also really the obvious way to inject events into the main-loop by people who don't want to hold the solar mutex.

So - quite probably we should introduce a custom lock into the VCL scheduler and audit it rather carefully for concurrency issues. Clearly we want to hold the solar mutex over any timer / idle callbacks - but at least pushing events in there could be improved.

For now - I guess, we could hold the SolarMutex in the AutoRecovery code to avoid it intermittently corrupting the scheduler's data structures [ not an ideal outcome ], its the easy fix - but not so satisfying.

Aaron - interested in making the VCL scheduler (+timer+idle+auto-timer) thread-safe ? hopefully an easy "code-lock" style pattern there (?)
Comment 3 Julien Nabet 2016-12-03 22:36:51 UTC
Aron: I think tdf#103667 I created is in fact a duplicate of yours.
For the test, could you disable autorecovery (Tools/Options/Load-Save/General, uncheck "Save Autorecovery information every")?
Comment 4 Aron Budea 2016-12-04 00:38:29 UTC
Michael: I saw back then that Jan-Marek's new VLC scheduler was kind of ready to be merged into master, at least in parts, is that still planned sometimes? What would be the preferred way to move forward here?

Julien: indeed, I confirm if autorecovery is turned off there's no assertion failed error. I wonder why this is only triggered in Base...
Comment 5 Julien Nabet 2016-12-04 11:15:33 UTC
*** Bug 103667 has been marked as a duplicate of this bug. ***
Comment 6 Michael Meeks 2016-12-05 09:09:32 UTC
I'm not hyper-happy about some of the bits I reviewed in Jan's work back then; but he's re-done it since and I've not looked. The 'easy' fix for this - is probably to do an Application::PostUserEvent - and in the event callback (which happens in the main-loop and is thread-safe) add the idle / timeout there. Should be an easy-ish fix I think. We could even implement an Application::AddTimeout() or whatever that would do that if the solar mutex is not lock-able with try_lock (?) =)
Comment 7 Julien Nabet 2016-12-20 19:17:11 UTC
Michael: any update about new VLC scheduler?
Comment 8 Michael Meeks 2016-12-20 22:13:45 UTC
Oh; hmm - not really - so we should prolly remove that assertion on the release branch, and continue to hope - it's a theoretically incorrect thing to do - but the race is prolly small =)
Comment 9 Aron Budea 2017-01-19 19:13:16 UTC
A commit from Michael Stahl today, seems relevant:

"framework: AutoRecovery uses Timer without SolarMutex
This results in a SolarMutex assert when creating a new Base document."
https://cgit.freedesktop.org/libreoffice/core/commit/?id=b2c467e47f438b2011aa304cca9bf403eaa1c8e2
Comment 10 Julien Nabet 2017-01-19 19:58:57 UTC
(In reply to Aron Budea from comment #9)
> A commit from Michael Stahl today, seems relevant:
> 
> "framework: AutoRecovery uses Timer without SolarMutex
> This results in a SolarMutex assert when creating a new Base document."
> https://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=b2c467e47f438b2011aa304cca9bf403eaa1c8e2

Indeed! I don't reproduce the duplicate tdf#103667 with Michael Stahl's patch.
Comment 11 Aron Budea 2017-01-22 05:06:59 UTC
Me neither. Closing as fixed.

Version: 5.4.0.0.alpha0+
Build ID: efd58dee26a534ba89d41efff44821b2d8967928
CPU Threads: 4; OS Version: Windows 6.1; UI Render: default; 
Locale: hu-HU (hu_HU); Calc: group
Comment 12 Xisco Faulí 2017-10-28 18:23:24 UTC
*** Bug 104138 has been marked as a duplicate of this bug. ***
Comment 13 Xisco Faulí 2017-12-14 07:13:05 UTC
*** Bug 114401 has been marked as a duplicate of this bug. ***