Crash on exit: ScCsvGrid leaks past vcl lifetime

[M. Edward (Ed) Borasky]

Reproduction scenario:
1/ open a csv file with libreoffice calc
2/ Import wizard appears -> press Cancel
3/ Exit LibreOffice

Expected behaviour:
LibreOffice shuts down cleanly.

Actual behaviour:
LibreOffice crashes on exit. (not entirely reproducable, but repeating steps 1/ and 2/ a few times usually soon triggers this)

ProblemType: Crash
DistroRelease: Ubuntu 16.04
Package: libreoffice-core 1:5.1.1-0ubuntu3
ProcVersionSignature: Ubuntu 4.4.0-16.32-generic 4.4.6
Uname: Linux 4.4.0-16-generic x86_64
ApportVersion: 2.20.1-0ubuntu1
Architecture: amd64
CurrentDesktop: GNOME
Date: Mon Apr 4 15:57:52 2016
ExecutablePath: /usr/lib/libreoffice/program/soffice.bin
InstallationDate: Installed on 2016-04-03 (1 days ago)
InstallationMedia: Ubuntu-GNOME 16.04 LTS "Xenial Xerus" - Beta amd64 (20160323.1)
ProcCmdline: /usr/lib/libreoffice/program/soffice.bin --calc file:///tmp/mozilla_znmeb0/DKSalaries-1.csv --splash-pipe=5
ProcEnviron:
 PATH=(custom, user)
 XDG_RUNTIME_DIR=<set>
 LANG=en_US.UTF-8
 SHELL=/bin/bash
 LD_LIBRARY_PATH=<set>
SegvAnalysis:
 Segfault happened at: 0x299c100: push %rax
 PC (0x0299c100) in non-executable VMA region: 0x016e7000-0x02c4d000 rw-p [heap]
 source "%rax" ok
 destination "(%rsp)" (0x7fffed7e99f0) ok
SegvReason: executing writable VMA [heap]
Signal: 11
SourcePackage: libreoffice
StacktraceTop:
 ?? ()
 ?? () from /usr/lib/libreoffice/program/libmergedlo.so
 ?? () from /usr/lib/libreoffice/program/libmergedlo.so
 Menu::~Menu() () from /usr/lib/libreoffice/program/libmergedlo.so
 ScCsvGrid::~ScCsvGrid() () from /usr/lib/libreoffice/program/../program/libsclo.so
Title: soffice.bin crashed with SIGSEGV in Menu::~Menu()
UpgradeStatus: No upgrade log present (probably fresh install)
UserGroups: adm cdrom dip docker libvirtd lpadmin plugdev sambashare sudo

[Apport retracing service (apport)]

StacktraceTop:
 ?? ()
 MenuItemData::~MenuItemData (this=0x2972470, __in_chrg=<optimized out>) at /build/libreoffice-ViFeg2/libreoffice-5.1.1/vcl/source/window/menuitemlist.cxx:44
 MenuItemList::~MenuItemList (this=0x29301a0, __in_chrg=<optimized out>) at /build/libreoffice-ViFeg2/libreoffice-5.1.1/vcl/source/window/menuitemlist.cxx:50
 Menu::~Menu (this=0x2933638, __in_chrg=<optimized out>) at /build/libreoffice-ViFeg2/libreoffice-5.1.1/vcl/source/window/menu.cxx:174
 ScCsvGrid::~ScCsvGrid (this=0x2933390, __in_chrg=<optimized out>) at /build/libreoffice-ViFeg2/libreoffice-5.1.1/sc/source/ui/dbgui/csvgrid.cxx:88

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Björn Michaelsen (bjoern-michaelsen)]

This appears to be a regression from https://github.com/LibreOffice/core/commit/2660d24a07866e083c5135ea263030f3e3a2e729 (havent verified that yet):

1/ Since that change mxAccessible in ScCsvGrid holds a rtl::Reference on a ScAccessibleCsvGrid
2/ Which in turn holds a VclPtr<> (aka a rtl::Reference with lipstick) on the ScCsvControl

These are a circular references, making both of them live forever and leak past the point where on LibreOffice close all of Vcl is long gone, when these are dtored. Trying to kill Vcl stuff at that point then blows up (because the stuff to kill is long dead).

[Björn Michaelsen (bjoern-michaelsen)]

Fix submitted, reviewed and commited to upstream master and 5-1 branches as:
https://gerrit.libreoffice.org/#/c/24020
https://gerrit.libreoffice.org/#/c/24021/

[Björn Michaelsen (bjoern-michaelsen)]

Created attachment 124413
patch to look for leaked VclPtrs at the end of DeInitVCL

Some VclPtrs leak past the end of DeInitVCL, which is unfortunate as they might still be subject to callback and assume VCL to be alive.

Applying the attached patch and running:

 make subsequencheck; find workdir/JunitTest/ -name '*log'|xargs grep LEAK|sed -e 's/^.*LEAK/LEAK/'|sort|uniq -c

result in:

      1 LEAKED VCLPTR: P10SfxPrinter refered to from a PK6VclPtrI10SfxPrinterE
      1 LEAKED VCLPTR: P12OpenGLWindow refered to from a PK6VclPtrI12OpenGLWindowE
     27 LEAKED VCLPTR: P12OutputDevice refered to from a PK6VclPtrI12OutputDeviceE
     14 LEAKED VCLPTR: P13VirtualDevice refered to from a PK6VclPtrI13VirtualDeviceE
      2 LEAKED VCLPTR: PN3vcl6WindowE refered to from a PK6VclPtrIN3vcl6WindowEE
      1 LEAKED VCLPTR: PN5chart11ChartWindowE refered to from a PK6VclPtrIN5chart11ChartWindowEE

It would be good to have all those eliminated and explicitly have them set to nullptrs deterministically in DeInitVCL (or earlier).

Additional notes:
- Some VclPtr have a vptr of NULL while still alive, which is discomforting in itself
- Doing a build with the patch and e.g. playing through MozTrap testcases manually to find well-reproducable leaks likely is a nice EasyHack

[Björn Michaelsen (bjoern-michaelsen)]

Whoops, there is more than JunitTests. This:

  find workdir/JunitTest/ workdir/CppunitTest/ workdir/PythonTest/ -name '*log'|xargs grep LEAK|sed -e 's/^.*LEAK/LEAK/'|sort|uniq -c|sort -n

yields:

      1 Binary file workdir/CppunitTest/xmlsecurity_signing.test.log matches
      1 Binary file workdir/JunitTest/unordf_complex/done.log matches
      1 LEAKED VCLPTR: P12OpenGLWindow refered to from a PK6VclPtrI12OpenGLWindowE
      1 LEAKED VCLPTR: P12ScGridWindow refered to from a PK6VclPtrI12ScGridWindowE
      1 LEAKED VCLPTR: P12ScTabControl refered to from a PK6VclPtrI12ScTabControlE
      1 LEAKED VCLPTR: P13ImplTabButton refered to from a PK6VclPtrI13ImplTabButtonE
      1 LEAKED VCLPTR: P13ScTabSplitter refered to from a PK6VclPtrI13ScTabSplitterE
      1 LEAKED VCLPTR: P14ScCornerButton refered to from a PK6VclPtrI14ScCornerButtonE
      1 LEAKED VCLPTR: P14SfxSplitWindow refered to from a PK6VclPtrI14SfxSplitWindowE
      1 LEAKED VCLPTR: P21SfxEmptySplitWin_Impl refered to from a PK6VclPtrI21SfxEmptySplitWin_ImplE
      1 LEAKED VCLPTR: P8ScColBar refered to from a PK6VclPtrI8ScColBarE
      1 LEAKED VCLPTR: P8ScRowBar refered to from a PK6VclPtrI8ScRowBarE
      1 LEAKED VCLPTR: PN5chart11ChartWindowE refered to from a PK6VclPtrIN5chart11ChartWindowEE
      2 LEAKED VCLPTR: P10PushButton refered to from a PK6VclPtrI10PushButtonE
      2 LEAKED VCLPTR: P10TextWindow refered to from a PK6VclPtrI10TextWindowE
      2 LEAKED VCLPTR: P11DecoToolBox refered to from a PK6VclPtrI11DecoToolBoxE
      2 LEAKED VCLPTR: P13SvTreeListBox refered to from a PK6VclPtrI13SvTreeListBoxE
      2 LEAKED VCLPTR: P16ExtMultiLineEdit refered to from a PK6VclPtrI16ExtMultiLineEditE
      2 LEAKED VCLPTR: P16VclMultiLineEdit refered to from a PK6VclPtrI16VclMultiLineEditE
      2 LEAKED VCLPTR: P17SvtIconChoiceCtrl refered to from a PK6VclPtrI17SvtIconChoiceCtrlE
      2 LEAKED VCLPTR: P7ToolBox refered to from a PK6VclPtrI7ToolBoxE
      2 LEAKED VCLPTR: P8Splitter refered to from a PK6VclPtrI8SplitterE
      2 LEAKED VCLPTR: P9FixedLine refered to from a PK6VclPtrI9FixedLineE
      2 LEAKED VCLPTR: P9FixedText refered to from a PK6VclPtrI9FixedTextE
      2 LEAKED VCLPTR: P9StatusBar refered to from a PK6VclPtrI9StatusBarE
      2 LEAKED VCLPTR: PN5dbaui12OTitleWindowE refered to from a PK6VclPtrIN5dbaui12OTitleWindowEE
      2 LEAKED VCLPTR: PN5dbaui13OCreationListE refered to from a PK6VclPtrIN5dbaui13OCreationListEE
      2 LEAKED VCLPTR: PN5dbaui14OPreviewWindowE refered to from a PK6VclPtrIN5dbaui14OPreviewWindowEE
      2 LEAKED VCLPTR: PN5dbaui16OAppBorderWindowE refered to from a PK6VclPtrIN5dbaui16OAppBorderWindowEE
      2 LEAKED VCLPTR: PN5dbaui16OApplicationViewE refered to from a PK6VclPtrIN5dbaui16OApplicationViewEE
      2 LEAKED VCLPTR: PN5dbaui20OAppDetailPageHelperE refered to from a PK6VclPtrIN5dbaui20OAppDetailPageHelperEE
      2 LEAKED VCLPTR: PN5dbaui22OApplicationDetailViewE refered to from a PK6VclPtrIN5dbaui22OApplicationDetailViewEE
      2 LEAKED VCLPTR: PN5dbaui23OApplicationIconControlE refered to from a PK6VclPtrIN5dbaui23OApplicationIconControlEE
      2 LEAKED VCLPTR: PN5dbaui9ODataViewE refered to from a PK6VclPtrIN5dbaui...

[Björn Michaelsen (bjoern-michaelsen)]

Created attachment 124416
patch to look for leaked VclPtrs at the end of DeInitVCL

(bumped diff)

[Noel Grandin (noelgrandin)]

I can't see us fixing all of those, unless they are actually referenced post-deinit.

I am however working on a patch to clean up the statically allocate VclPtr<> vars.

[Björn Michaelsen (bjoern-michaelsen)]

(In reply to Noel Grandin from comment #3)
> I can't see us fixing all of those, unless they are actually referenced
> post-deinit.

I just wonder: Could we have a clang plugin that rewrites VclPtr::operator=() to something VclPtr::assign(ptr, __FILE__, __LINE__) so we can register where a VclPtr was assigned in that global registry? We could then output something like:

LEAKED VCLPTR: P12OutputDevice refered to from a PK6VclPtrI12OutputDeviceE assigned at vcl/foo/bar.cxx:564

which should make hunting down these much easier ...

[Noel Grandin (noelgrandin)]

(In reply to Björn Michaelsen from comment #4)
>
> I just wonder: Could we have a clang plugin that rewrites
> VclPtr::operator=() to something VclPtr::assign(ptr, __FILE__, __LINE__) so
> we can register where a VclPtr was assigned in that global registry? We
> could then output something like:
>

Or we could use the nice stack backtrace functionality mmeeks built to store a backtrace in operator=, then symbolise the relevant ones.

[Michael-meeks-1 (michael-meeks-1)]

Oh ! the way to track vclptr leaks is using:

https://lists.freedesktop.org/archives/libreoffice/2015-May/068067.html

Which uses gdb to track all traces that mutate the reference count and then pairs them up - to throw out most of the noise and (in theory) - this shows you the one guy that unreffed or reffed too much ;-)

At least -that was my gdb assisted solution - that doesn't require code changes.

Prolly ought to be documented in vcl/README.lifecycle =)

Then again - I guess in DBGUTIL mode it'd be great to have something like your tracker included in master Bjoern - ultimately we should chase all of these down, although I suspect that we have a few known leaks still lingering in VCL.

Thanks for chasing this lot though ! =)

[Libreoffice-commits (libreoffice-commits)]

Noel Grandin committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=67d333c608a662621c1069aacdec75e45e33a183

tdf#99352 - Some VclPtrs leak past DeInitVCL

It will be available in 5.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.

[Björn Michaelsen (bjoern-michaelsen)]

Rerun against 024d2fde2aae13b07cf5c7b4d85fc3c6abce6913 yields:

      1 Binary file workdir/CppunitTest/xmlsecurity_signing.test.log matches
      1 LEAKED VCLPTR: P12OpenGLWindow refered to from a PK6VclPtrI12OpenGLWindowE
      1 LEAKED VCLPTR: P12ScGridWindow refered to from a PK6VclPtrI12ScGridWindowE
      1 LEAKED VCLPTR: P12ScTabControl refered to from a PK6VclPtrI12ScTabControlE
      1 LEAKED VCLPTR: P13ImplTabButton refered to from a PK6VclPtrI13ImplTabButtonE
      1 LEAKED VCLPTR: P13ScTabSplitter refered to from a PK6VclPtrI13ScTabSplitterE
      1 LEAKED VCLPTR: P14ScCornerButton refered to from a PK6VclPtrI14ScCornerButtonE
      1 LEAKED VCLPTR: P14SfxSplitWindow refered to from a PK6VclPtrI14SfxSplitWindowE
      1 LEAKED VCLPTR: P21SfxEmptySplitWin_Impl refered to from a PK6VclPtrI21SfxEmptySplitWin_ImplE
      1 LEAKED VCLPTR: P8ScColBar refered to from a PK6VclPtrI8ScColBarE
      1 LEAKED VCLPTR: P8ScRowBar refered to from a PK6VclPtrI8ScRowBarE
      1 LEAKED VCLPTR: PN5chart11ChartWindowE refered to from a PK6VclPtrIN5chart11ChartWindowEE
      2 LEAKED VCLPTR: P10PushButton refered to from a PK6VclPtrI10PushButtonE
      2 LEAKED VCLPTR: P10TextWindow refered to from a PK6VclPtrI10TextWindowE
      2 LEAKED VCLPTR: P11DecoToolBox refered to from a PK6VclPtrI11DecoToolBoxE
      2 LEAKED VCLPTR: P13SvTreeListBox refered to from a PK6VclPtrI13SvTreeListBoxE
      2 LEAKED VCLPTR: P16ExtMultiLineEdit refered to from a PK6VclPtrI16ExtMultiLineEditE
      2 LEAKED VCLPTR: P16VclMultiLineEdit refered to from a PK6VclPtrI16VclMultiLineEditE
      2 LEAKED VCLPTR: P17SvtIconChoiceCtrl refered to from a PK6VclPtrI17SvtIconChoiceCtrlE
      2 LEAKED VCLPTR: P7ToolBox refered to from a PK6VclPtrI7ToolBoxE
      2 LEAKED VCLPTR: P8Splitter refered to from a PK6VclPtrI8SplitterE
      2 LEAKED VCLPTR: P9FixedLine refered to from a PK6VclPtrI9FixedLineE
      2 LEAKED VCLPTR: P9FixedText refered to from a PK6VclPtrI9FixedTextE
      2 LEAKED VCLPTR: P9StatusBar refered to from a PK6VclPtrI9StatusBarE
      2 LEAKED VCLPTR: PN5dbaui12OTitleWindowE refered to from a PK6VclPtrIN5dbaui12OTitleWindowEE
      2 LEAKED VCLPTR: PN5dbaui13OCreationListE refered to from a PK6VclPtrIN5dbaui13OCreationListEE
      2 LEAKED VCLPTR: PN5dbaui14OPreviewWindowE refered to from a PK6VclPtrIN5dbaui14OPreviewWindowEE
      2 LEAKED VCLPTR: PN5dbaui16OAppBorderWindowE refered to from a PK6VclPtrIN5dbaui16OAppBorderWindowEE
      2 LEAKED VCLPTR: PN5dbaui16OApplicationViewE refered to from a PK6VclPtrIN5dbaui16OApplicationViewEE
      2 LEAKED VCLPTR: PN5dbaui20OAppDetailPageHelperE refered to from a PK6VclPtrIN5dbaui20OAppDetailPageHelperEE
      2 LEAKED VCLPTR: PN5dbaui22OApplicationDetailViewE refered to from a PK6VclPtrIN5dbaui22OApplicationDetailViewEE
      2 LEAKED VCLPTR: PN5dbaui23OApplicationIconControlE refered to from a PK6VclPtrIN5dbaui23OApplicationIconControlEE
      2 LEAKED VCLPTR: PN5dbaui9ODataViewE refered to from a PK6VclPtrIN5dbaui9ODataViewEE
      2 LEAKED VCLPTR: PN7svtools20ODocumentInfoPreviewE refered to from a PK6VclPtrIN7svtools20ODocumentInfoPreviewEE
      3 LEAKED VCLPTR: P10SfxPrinter refered to from a PK6VclPtrI10SfxPr...

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Martin Pitt (pitti)]

Hello M., or anyone else affected,

Accepted libreoffice into xenial-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/libreoffice/1:5.1.3-0ubuntu1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

[Björn Michaelsen (bjoern-michaelsen)]

Hard to reproduce, no known reports of this one on 5.1.3 => setting this to verification-done.

[Launchpad Janitor (janitor)]

This bug was fixed in the package libreoffice - 1:5.1.3-0ubuntu1

---------------
libreoffice (1:5.1.3-0ubuntu1) xenial; urgency=medium

  * new upstream bugfix release
  * fix crash with nullptr SdrObjList (LP: #1569500)
  * fix crash with ScCsvGrid living beyond VCL shutdown (LP: #1566050)
  * fix crash with non-empty BlendFrameCache in late VCL shutdown (LP: #1560328)

 -- Bjoern Michaelsen <email address hidden> Thu, 12 May 2016 11:35:38 +0200

[Brian Murray (brian-murray)]

The verification of the Stable Release Update for libreoffice has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[Mathieu Marquer (slasher-fun)]

I just encountered this crash even though I'm using the lastest version:

Package: libreoffice-core 1:5.1.3-0ubuntu1
PackageArchitecture: amd64
ProcVersionSignature: Ubuntu 4.4.0-23.41-generic 4.4.10

SegvAnalysis:
 Segfault happened at: 0x7f8841460837 <__libc_start_main+247>: xor %edx,%edx
 PC (0x7f8841460837) ok
 source "%edx" ok
 destination "%edx" ok
 SP (0x7fff124e6bd0) ok
 Reason could not be automatically determined.
SourcePackage: libreoffice
Stacktrace:
 #0 0x00000000038a2000 in ?? ()
 No symbol table info available.
 #1 0x00007f8843f88c8d in ?? () from /usr/lib/libreoffice/program/libmergedlo.so
 No symbol table info available.
 #2 0x00007f8843f88d8d in ?? () from /usr/lib/libreoffice/program/libmergedlo.so
 No symbol table info available.
 #3 0x00007f8843f7c9c8 in Menu::~Menu() () from /usr/lib/libreoffice/program/libmergedlo.so
 No symbol table info available.
 #4 0x00007f880e4a0165 in ScCsvGrid::~ScCsvGrid() () from /usr/lib/libreoffice/program/../program/libsclo.so
 No symbol table info available.
 #5 0x00007f880e4a0279 in ScCsvGrid::~ScCsvGrid() () from /usr/lib/libreoffice/program/../program/libsclo.so
 No symbol table info available.
 #6 0x00007f8841479fe8 in __run_exit_handlers (status=0, listp=0x7f88418035f8 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true) at exit.c:82
         atfct = <optimized out>
         onfct = <optimized out>
         cxafct = <optimized out>
 #7 0x00007f884147a035 in __GI_exit (status=<optimized out>) at exit.c:104
 No locals.
 #8 0x00007f8841460837 in __libc_start_main (main=0x4006e0, argc=3, argv=0x7fff124e6ca8, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fff124e6c98) at ../csu/libc-start.c:325
         result = <optimized out>
         unwind_buf = {cancel_jmp_buf = {{jmp_buf = {0, -916413476825100205, 4196096, 140733500517536, 0, 0, 915949057318686803, 889668460461002835}, mask_was_saved = 0}}, priv = {pad = {0x0, 0x0, 0x400870, 0x7f884541a8e0 <_dl_fini>}, data = {prev = 0x0, cleanup = 0x0, canceltype = 4196464}}}
         not_first_call = <optimized out>
 #9 0x0000000000400729 in ?? ()
 No symbol table info available.
StacktraceAddressSignature: /usr/lib/libreoffice/program/soffice.bin:11:[heap]+3069000:/usr/lib/libreoffice/program/libmergedlo.so+2528c8d:/usr/lib/libreoffice/program/libmergedlo.so+2528d8d:/usr/lib/libreoffice/program/libmergedlo.so+251c9c8:/usr/lib/libreoffice/program/libsclo.so+796165:/usr/lib/libreoffice/program/libsclo.so+796279:/lib/x86_64-linux-gnu/libc-2.23.so+39fe8:/lib/x86_64-linux-gnu/libc-2.23.so+3a035:/lib/x86_64-linux-gnu/libc-2.23.so+20837:/usr/lib/libreoffice/program/soffice.bin+729
StacktraceTop:
 ?? ()
 ?? () from /usr/lib/libreoffice/program/libmergedlo.so
 ?? () from /usr/lib/libreoffice/program/libmergedlo.so
 Menu::~Menu() () from /usr/lib/libreoffice/program/libmergedlo.so
 ScCsvGrid::~ScCsvGrid() () from /usr/lib/libreoffice/program/../program/libsclo.so

[Launchpad Janitor (janitor)]

This bug was fixed in the package libreoffice - 1:5.1.3-0ubuntu4

---------------
libreoffice (1:5.1.3-0ubuntu4) yakkety; urgency=medium

  * use internal copy of mdds and orcus on yakkety for 5.1 series now

 -- Bjoern Michaelsen <email address hidden> Tue, 24 May 2016 14:25:58 +0200

[Xiscofauli (xiscofauli)]

Hello,
Is this bug fixed?
If so, could you please close it as RESOLVED FIXED?

[Björn Michaelsen (bjoern-michaelsen)]

(In reply to Xisco Faulí from comment #9)
> Is this bug fixed?
> If so, could you please close it as RESOLVED FIXED?

To find out the status quo, one would need to apply http://bugs.documentfoundation.org/attachment.cgi?id=124413 and run tests again to show what is left.

For real world impact, lp#1562971 was dominating the crash reports on Ubuntu for a while, but as said in https://bugs.launchpad.net/ubuntu/+source/libreoffice/+bug/1562971/comments/9 this particular stacktrace occured only twice on LibreOffice 5.2.x and has never been seen on LibreOffice 5.3.x so far.

I will try to rerun the attached patch in the next days, but so far it looks like WORKSFORME.

[Björn Michaelsen (bjoern-michaelsen)]

According to https://errors.ubuntu.com/problem/351dcefbdfe5de30957d15c3d9c06233ec575453, this had:
25607 instances reported
_all_ instance reported with LibreOffice 5.1.4 or earlier

Thus assuming resolved.

[Björn Michaelsen (bjoern-michaelsen)]

Created attachment 131567
patch to look for leaked VclPtrs at the end of DeInitVCL (against 14b4fcf0)

rebased patch on 14b4fcf0.

[Björn Michaelsen (bjoern-michaelsen)]

Here is the output of "find workdir/JunitTest/ workdir/CppunitTest/ workdir/PythonTest/ -name '*log'|xargs grep LEAK|sed -e 's/^.*LEAK/LEAK/'|sort|uniq -c|sort -n":

      1 LEAKED VCLPTR: P12OpenGLWindow refered to from a PK6VclPtrI12OpenGLWindowE
      1 LEAKED VCLPTR: PN5chart11ChartWindowE refered to from a PK6VclPtrIN5chart11ChartWindowEE
    156 LEAKED VCLPTR: P12OutputDevice refered to from a PK6VclPtrI12OutputDeviceE
    159 LEAKED VCLPTR: P13VirtualDevice refered to from a PK6VclPtrI13VirtualDeviceE
    260 LEAKED VCLPTR:

which is ... disappointing:

1/ apparently there are lots of VclPtrs without typeinfo hanging around (or are these just nullptrs?)
2/ the VirtualDevice<->VirtualDevice pointers increased from 64 to 159 since Comment 1
3/ the OutputDevice<->OutputDevice pointers increased from 99 to 156 since Comment 1
4/ the chart::ChartWindow<->chart::ChartWindow pointer is new

On the good news: Most of the rest is gone (which is good, if the 260 are indeed nullptrs).

Notes:
- This run excludes CppunitTest_sw_ooxmlencryption and CppunitTest_xmlsecurity_pdfsigning which segfault against the nss (3.29.1) on my machine.
- This run also excludes CppunitTest_vcl_wmf_test which crashes on a our_vAllVclPtrs.remove(this) on exit, which shouldnt happen.
- This run also excludes CppunitTest_sw_mailmerge which might be looping (or excessively slow). I killed it after some 10 CPU minutes.

[Björn Michaelsen (bjoern-michaelsen)]

(In reply to Björn Michaelsen from comment #12)
> or are these just nullptrs?

Eh, yes. So this is good.

Looking at individual tests:
> 1 LEAKED VCLPTR: PN5chart11ChartWindowE refered to from a
> PK6VclPtrIN5chart11ChartWindowEE

That is in JunitTest/chart2_unoapi.

Most other tests leak 1 OutputDevice, 1 VirtualDevice and 2 nullptrs (or less).

Exceptions leaking more (ignoring nullptrs):

workdir/JunitTest/sw_complex/done.log
      3 LEAKED VCLPTR: P12OutputDevice refered to from a PK6VclPtrI12OutputDeviceE
      3 LEAKED VCLPTR: P13VirtualDevice refered to from a PK6VclPtrI13VirtualDeviceE
workdir/JunitTest/sfx2_complex/done.log
      4 LEAKED VCLPTR: P12OutputDevice refered to from a PK6VclPtrI12OutputDeviceE
      4 LEAKED VCLPTR: P13VirtualDevice refered to from a PK6VclPtrI13VirtualDeviceE
workdir/JunitTest/framework_complex/done.log
      2 LEAKED VCLPTR: P12OutputDevice refered to from a PK6VclPtrI12OutputDeviceE
      2 LEAKED VCLPTR: P13VirtualDevice refered to from a PK6VclPtrI13VirtualDeviceE

[Björn Michaelsen (bjoern-michaelsen)]

(In reply to Björn Michaelsen from comment #13)
> Looking at individual tests:
> > 1 LEAKED VCLPTR: PN5chart11ChartWindowE refered to from a
> > PK6VclPtrIN5chart11ChartWindowEE
>
> That is in JunitTest/chart2_unoapi.

And the:
> 1 LEAKED VCLPTR: P12OpenGLWindow refered to from a PK6VclPtrI12OpenGLWindowE

is also from chart2_unoapi.

[Björn Michaelsen (bjoern-michaelsen)]

(In reply to Björn Michaelsen from comment #13)
> workdir/JunitTest/sw_complex/done.log
> workdir/JunitTest/sfx2_complex/done.log
> workdir/JunitTest/framework_complex/done.log

Rerunning these standalone show the 1 OutputDevice, 1 VirtualDevice from other tests. Curious.

(In reply to Björn Michaelsen from comment #12)
> - This run also excludes CppunitTest_sw_mailmerge which might be looping (or
> excessively slow). I killed it after some 10 CPU minutes.

Rerunning standalone showed no issues. Thus:

> - This run also excludes CppunitTest_vcl_wmf_test which crashes on a our_vAllVclPtrs.remove(this) on exit, which shouldnt happen.

and chart2_unoapi leaking OpenGL and a window. are the only remaining issue. Once that is fixed, we _could_ think about adding the attached patch to debug builds and make those barf up if more than the expected 1 OutputDevice, 1 VirtualDevice are left over. This should help detect regressions early.

[Björn Michaelsen (bjoern-michaelsen)]

      1 LEAKED VCLPTR: P12OpenGLWindow created at unknown line -1 refered to from a PK6VclPtrI12OpenGLWindowE
      1 LEAKED VCLPTR: P13VirtualDevice created at svx/source/svdraw/sdrpaintwindow.cxx line 112 refered to from a PK6VclPtrI13VirtualDeviceE
      1 LEAKED VCLPTR: PN5chart11ChartWindowE created at chart2/source/controller/main/ChartController.cxx line 453 refered to from a PK6VclPtrIN5chart11ChartWindowEE
     15 LEAKED VCLPTR: P13VirtualDevice created at editeng/source/editeng/impedit2.cxx line 213 refered to from a PK6VclPtrI13VirtualDeviceE
    156 LEAKED VCLPTR: P13VirtualDevice created at editeng/source/editeng/eerdll.cxx line 89 refered to from a PK6VclPtrI13VirtualDeviceE
    159 LEAKED VCLPTR: P12OutputDevice created at unknown line -1 refered to from a PK6VclPtrI12OutputDeviceE
    161 LEAKED VCLPTR: P13VirtualDevice created at unknown line -1 refered to from a PK6VclPtrI13VirtualDeviceE

so it seems the stuff we are leaking are:
- EditEngine
- chart::ChartController

The VirtualDevices/OutputDevices likely will dispose along with a proper EditEngine shutdown as the leak counts match up pretty well.

[Libreoffice-commits (libreoffice-commits)]

Bjoern Michaelsen committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=926c0d73e82bb1d5644c49fb95c56e241b6c8f34

tdf#99352: ensure ChartController is disposing its VclPtrs on DeInitVCL

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.

[Michael-meeks-1 (michael-meeks-1)]

Hmm; not really a VclPtr bug surely - as all of these guys would have been leaks in the past anyway ;-) but fair enough to track it there I guess. Nice work !

[Libreoffice-commits (libreoffice-commits)]

Markus Mohrhard committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=60708af4a7ed48028cf3158d1101568aa5b0da81

Revert "tdf#99352: ensure ChartController is disposing its VclPtrs on DeInitVCL"

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.

[Björn Michaelsen (bjoern-michaelsen)]

So with: changeset https://gerrit.libreoffice.org/34883, https://gerrit.libreoffice.org/34877 and https://gerrit.libreoffice.org/34841, and some more info on where these leaking pointers where created this is down to:

      1 LEAKED VCLPTR: P12OpenGLWindow created at OPERATOR= line -1 referred to from a PK6VclPtrI12OpenGLWindowE
      1 LEAKED VCLPTR: P12OutputDevice created at chart2/source/view/main/DrawModelWrapper.cxx line 114 referred to from a PK6VclPtrI12OutputDeviceE
      1 LEAKED VCLPTR: P13VirtualDevice created at svx/source/svdraw/sdrpaintwindow.cxx line 112 referred to from a PK6VclPtrI13VirtualDeviceE
     33 LEAKED VCLPTR: P13VirtualDevice created at RESET line -1 referred to from a PK6VclPtrI13VirtualDeviceE
     50 LEAKED VCLPTR: P13VirtualDevice created at unknown line -1 referred to from a PK6VclPtrI13VirtualDeviceE
    120 LEAKED VCLPTR: P12OutputDevice created at unknown line -1 referred to from a PK6VclPtrI12OutputDeviceE
    260 LEAKED VCLPTR:

That is down from 317 non-null VclPtrs on deinit in Comment 12 to 206 non-null VclPtrs (down by 35%).

[Björn Michaelsen (bjoern-michaelsen)]

(In reply to Commit Notification from comment #19)
> Markus Mohrhard committed a patch related to this issue.
> http://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=60708af4a7ed48028cf3158d1101568aa5b0da81
>
> Revert "tdf#99352: ensure ChartController is disposing its VclPtrs on
> DeInitVCL"
>
> The original patch crashes charts on disposing, e.g. during the UI testing.

@moggi: Do you have a reproduction scenario for that? Some random manual teasing of chart2 didnt immediately yield anything, so something specific would be helpful.

[Markus Mohrhard (moggi)]

(In reply to Björn Michaelsen from comment #21)
> (In reply to Commit Notification from comment #19)
> > Markus Mohrhard committed a patch related to this issue.
> > http://cgit.freedesktop.org/libreoffice/core/commit/
> > ?id=60708af4a7ed48028cf3158d1101568aa5b0da81
> >
> > Revert "tdf#99352: ensure ChartController is disposing its VclPtrs on
> > DeInitVCL"
> >
> > The original patch crashes charts on disposing, e.g. during the UI testing.
>
> @moggi: Do you have a reproduction scenario for that? Some random manual
> teasing of chart2 didnt immediately yield anything, so something specific
> would be helpful.

Make UITest_calc_demo or follow the steps from calc_test.edit_chart.test_select_secondary_axis
Even if I follow the steps manually it crashes in the last deselection of the chart.

[Libreoffice-commits (libreoffice-commits)]

Bjoern Michaelsen committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=1a044b25c279236d9f67847ec6ad426d8c5aac13

tdf#99352: dispose EditEngines when SfxApp dies

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.

[Libreoffice-commits (libreoffice-commits)]

Bjoern Michaelsen committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=c54bb4a9f76a11561a7f4010382dbe46c0d2ef2a

tdf#99352: create editeng::SharedVclRessources

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.

[Ilmari-lauhakangas (ilmari-lauhakangas)]

I guess related to the previous commits, I now get a segfault on CppunitTest/libtest_editeng_borderline.so when building.

[Ilmari-lauhakangas (ilmari-lauhakangas)]

(In reply to Buovjaga from comment #25)
> I guess related to the previous commits, I now get a segfault on
> CppunitTest/libtest_editeng_borderline.so when building.

Apparently fixed by https://cgit.freedesktop.org/libreoffice/core/commit/?id=d7685eb3f417e19a5e61f2fe550eb03d6848365d

[Björn Michaelsen (bjoern-michaelsen)]

(In reply to Buovjaga from comment #26)
> Apparently fixed by
> https://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=d7685eb3f417e19a5e61f2fe550eb03d6848365d

Yes, that were two incompatible, but non-conflicting (in a git sense) changes between my change on gerrit and master between when I pushed the patch for review and when it was cherry-picked.

Anyway, with https://gerrit.libreoffice.org/#/c/35690/ this is down to 173 non-null leaked VclPtrs:
     56 LEAKED VCLPTR: P13VirtualDevice refered to from a PK6VclPtrI13VirtualDeviceE
    118 LEAKED VCLPTR: P12OutputDevice refered to from a PK6VclPtrI12OutputDeviceE
    257 LEAKED VCLPTR:

[Libreoffice-commits (libreoffice-commits)]

Bjoern Michaelsen committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=3ecb9b4bd7dc70664bbb8d7c957ea8dc5015223f

tdf#99352: assert on stray VclPtrs past DeInit

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.

[Björn Michaelsen (bjoern-michaelsen)]

(In reply to Commit Notification from comment #28)
> http://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=3ecb9b4bd7dc70664bbb8d7c957ea8dc5015223f
>
> tdf#99352: assert on stray VclPtrs past DeInit

And with that: Closing this one as from now on DBG_UTL builds (except on Windows) will barf up on any leaked VclPtr, thus we should notice really quickly if we regress here.