Bug 145175 - ConfigManager is not empty when quitting LO Writer (and LO)
Summary: ConfigManager is not empty when quitting LO Writer (and LO)
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
7.3.0.0 alpha0+
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: console-noise
  Show dependency treegraph
 
Reported: 2021-10-16 16:38 UTC by Eyal Rozenberg
Modified: 2023-12-31 21:09 UTC (History)
2 users (show)

See Also:
Crash report or crash signature:


Attachments
bt with debug symbols for "Office.Commands/Execute" (10.83 KB, text/plain)
2021-10-17 08:13 UTC, Julien Nabet
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eyal Rozenberg 2021-10-16 16:38:53 UTC
If you run a recent daily build of LO (see build info below), create a new LO writer document, then quite, you get a console message saying:

warn:unotools.config:4895:4895:unotools/source/config/configmgr.cxx:153: ConfigManager not empty

Now, either it's a problem that the ConfigManager is not empty, in which case it should be properly emptied; or it's not a problem in which case the warning needs to be removed as console noise.
Comment 1 Eyal Rozenberg 2021-10-16 16:39:34 UTC
Build info:

Version: 7.3.0.0.alpha0+ / LibreOffice Community
Build ID: c998691e22ceda15c89d55cf7005201f0392dadb
CPU threads: 4; OS: Linux 5.10; UI render: default; VCL: gtk3
Locale: en-US (en_IL); UI: en-US
TinderBox: Linux-rpm_deb-x86_64@86-TDF, Branch:master, Time: 2021-10-14_11:54:20
Calc: threaded
Comment 2 Julien Nabet 2021-10-17 08:11:18 UTC
On pc Debian x86-64 with master sources updated today, I could reproduce this.
Comment 3 Julien Nabet 2021-10-17 08:13:04 UTC
Created attachment 175790 [details]
bt with debug symbols for "Office.Commands/Execute"

After adding some trace at registerConfigItem and removeConfigItem and when comparing, the only remnant was "Office.Commands/Execute".

So I retrieved a bt from where "Office.Commands/Execute" is registered.
Comment 4 Julien Nabet 2021-10-17 08:22:23 UTC
Stephan/Noel: In framework/source/services/frame.cxx I tried to change m_aCommandOptions from SvtCommandOptions to std::unique_ptr<SvtCommandOptions> but it failed with:
#0  std::__shared_ptr<SvtCommandOptions_Impl, (__gnu_cxx::_Lock_policy)2>::get() const (this=0x20) at /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/shared_ptr_base.h:1330
#1  0x00007fbbb1490848 in std::__shared_ptr_access<SvtCommandOptions_Impl, (__gnu_cxx::_Lock_policy)2, false, false>::_M_get() const (this=0x20)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/shared_ptr_base.h:1027
#2  0x00007fbbb1485c35 in std::__shared_ptr_access<SvtCommandOptions_Impl, (__gnu_cxx::_Lock_policy)2, false, false>::operator->() const (this=0x20)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/shared_ptr_base.h:1021
#3  0x00007fbbb1484616 in SvtCommandOptions::EstablishFrameCallback(com::sun::star::uno::Reference<com::sun::star::frame::XFrame> const&) (this=0x0, xFrame=
    uno::Reference to ((anonymous namespace)::XFrameImpl *) 0x2cbd490) at unotools/source/config/cmdoptions.cxx:340
#4  0x00007fbbb61dc9b2 in (anonymous namespace)::XFrameImpl::initListeners() (this=0x2cbd420) at framework/source/services/frame.cxx:509

I noticed the use of weak reference and std::make_shared in unotools/source/config/cmdoptions.cxx. Any idea how to fix this part?
Comment 5 Noel Grandin 2021-10-17 17:02:13 UTC
LibreOffice generates tons of false positive warnings, it is just one of those things that happens, no need to worry.
Comment 6 Julien Nabet 2021-10-17 17:11:50 UTC
Noel: so does it mean we may remove the SAL_WARN_IF in:
    152 utl::ConfigManager::~ConfigManager() {
    153     SAL_WARN_IF(!items_.empty(), "unotools.config", "ConfigManager not empty");
    154 }

to reduce console noise?
Comment 7 Eyal Rozenberg 2021-10-17 17:52:56 UTC
(In reply to Noel Grandin from comment #5)
> LibreOffice generates tons of false positive warnings,

It actually generates very few false warnings. At least the daily master branch build, anyway.

> it is just one of those things that happens, no need to worry.

Either this is a false warning, in which case it should not be made; or it is a valid warning, in which case the config manager should be emptied. Which is it?
Comment 8 Julien Nabet 2021-10-18 18:30:44 UTC
I gave a try with https://gerrit.libreoffice.org/c/core/+/123762
Comment 9 Chris Sherlock 2021-12-30 17:15:30 UTC
Julien, I tested out your patch. It looks good. I respectfully disagree with the reviewer - a straight loop is fine for now. 

My only question is: why would "Office.Commands/Execute" be a problem? I have a utility that shows that "Setup/L10N" doesn't get cleared...  

Eval, if you want to improve upon the patch, why not fix it after the patch is committed. I can't see any reason why this is more or less readable than having to setup several lambdas for a custom built for_each_if() function! Which would be replaced with std::ranges in C++20.
Comment 10 Chris Sherlock 2021-12-30 17:24:56 UTC
Evan, seems to me that ConfigManager is never emptied. Doesn't seem like a false positive to me... Noel, why do you say it is a false positive?
Comment 11 Chris Sherlock 2021-12-30 19:10:13 UTC
From IRC:

[06:02:55]  <chris_wot> noelgrandin you wrote in https://bugs.documentfoundation.org/show_bug.cgi?id=145175 that "LibreOffice generates tons of false positive warnings, it is just one of those things that happens, no need to worry."
[06:02:57]  <IZBot> bug 145175: LibreOffice-Writer normal/medium NEW ConfigManager is not empty when quitting LO Writer (and LO)
[06:04:08]  <noelgrandin> exactly
[06:04:29]  <noelgrandin> why would we worry? it's not causing a problem
[06:06:23]  <chris_wot> so in that case, better to remove the warning?
[06:06:44]  <chris_wot> if we don't care about the fact that it isn't removed, why muddy up the output with the warning?
[06:06:44]  <noelgrandin> they can be useful, if you are chasing another related bug
[06:07:03]  <chris_wot> I'm confused, is it useful or not?
[06:07:11]  <chris_wot> have we had problems with config mgr?
[06:07:30]  <chris_wot> it's annoying to have it show output on the workben utilities we are creating
[06:08:18]  <chris_wot> noelgrandin does this mean it should be a WONTFIX then?
[06:08:52]  <noelgrandin> it would be a WONTFIX
Comment 12 Eyal Rozenberg 2021-12-30 20:13:19 UTC
(In reply to Chris Sherlock from comment #11)

Which person has which nick and which function w.r.t. LO development? :-)

Also...

> [06:08:52]  <noelgrandin> it would be a WONTFIX

Already explained why that can't be right - whether one believes the config manager should be empty or not. And the IRC chat explained it too...

> I respectfully disagree with the reviewer - a straight loop is fine for now. 

It's not _terrible_; it's just a question of style. I didn't say I reject the patch or anything.

> Eyal, if you want to improve upon the patch, why not fix it after the patch is committed.

Well, possibly because I'm not really an LO developer. There's only a small chance I would dive into checking out, building and testing.

> I can't see any reason why this is more or less readable than having to setup several lambdas for a custom built for_each_if() function! 

Well, because it separates out the different semantic aspects of the code rather than weaving them together. It is idiomatic according to this paradigm:

https://www.youtube.com/watch?v=W2tWOdzgXHA

but obviously it is at least partially a matter of taste.
Comment 13 QA Administrators 2023-12-31 03:14:04 UTC Comment hidden (obsolete)
Comment 14 Eyal Rozenberg 2023-12-31 21:09:10 UTC
Bug still manifests with:

Version: 24.2.0.0.alpha1+ (X86_64) / LibreOffice Community
Build ID: 516f800f84b533db0082b1f39c19d1af40ab29c8
CPU threads: 4; OS: Linux 6.5; UI render: default; VCL: gtk3
Locale: he-IL (en_IL); UI: en-US

The message may differ a little, e.g.:

warn:unotools.config:29423:29423:unotools/source/config/configmgr.cxx:147: ConfigManager not empty