Issue 99010 - toolkit: setting a MenuBar on a Top Window crashes OOo
Summary: toolkit: setting a MenuBar on a Top Window crashes OOo
Status: ACCEPTED
Alias: None
Product: App Dev
Classification: Unclassified
Component: api (show other issues)
Version: 3.3.0 or older (OOo)
Hardware: All All
: P3 Trivial
Target Milestone: 4.0.0
Assignee: AOO issues mailing list
QA Contact:
URL:
Keywords: crash
Depends on:
Blocks:
 
Reported: 2009-02-08 14:33 UTC by Ariel Constenla-Haile
Modified: 2017-05-20 11:27 UTC (History)
3 users (show)

See Also:
Issue Type: PATCH
Latest Confirmation in: ---
Developer Difficulty: ---


Attachments
Crash report, from a product build (Linux) (21.76 KB, text/xml)
2009-02-08 14:37 UTC, Ariel Constenla-Haile
no flags Details
Proposed patch (2.56 KB, text/plain)
2009-02-08 14:39 UTC, Ariel Constenla-Haile
no flags Details
Test document (run the Main macro) (9.44 KB, application/vnd.oasis.opendocument.text)
2009-02-08 14:46 UTC, Ariel Constenla-Haile
no flags Details
patch taking comments into account (3.00 KB, patch)
2009-02-18 10:41 UTC, Ariel Constenla-Haile
no flags Details | Diff
Revised patch. (3.01 KB, text/plain)
2009-02-26 13:03 UTC, carsten.driesner
no flags Details
patch working on DEV300_m83 (3.16 KB, patch)
2010-06-25 16:38 UTC, Ariel Constenla-Haile
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description Ariel Constenla-Haile 2009-02-08 14:33:56 UTC
How to reproduce this, and where OOo crashes, depends on the build
(product / non-product), I will attach a Writer document, but you can also try
with the GUI example on the SDK 
[the "issue" I reported in the SDK GUI example
http://api.openoffice.org/servlets/ReadMsg?list=dev&msgNo=19085
is in fact a MenuBar issue].

With the test document, and in a product build, do the following:
* open the test document and close every other OOo document
* from the Tools menu execute the Main macro 
* a top window with a MenuBar will show up
* close this top window
* now close the test document but leaving OOo open; that is: only close the
document and let the StartModule appear, use for this the menu "File" - "Close" 
* OOo will crash
Comment 1 Ariel Constenla-Haile 2009-02-08 14:37:49 UTC
Created attachment 60023 [details]
Crash report, from a product build (Linux)
Comment 2 Ariel Constenla-Haile 2009-02-08 14:39:53 UTC
Created attachment 60024 [details]
Proposed patch
Comment 3 Ariel Constenla-Haile 2009-02-08 14:43:15 UTC
changed issue type to patch
added crash keyword
Comment 4 Ariel Constenla-Haile 2009-02-08 14:46:01 UTC
Created attachment 60025 [details]
Test document (run the Main macro)
Comment 5 Ariel Constenla-Haile 2009-02-08 15:13:28 UTC
A little explanation about the crash and the proposed patch.

On a non-product build is very easy to follow the stack: 

basically VCLXWindow::dispose checks if the Window is still
alive
(http://svn.services.openoffice.org/opengrok/xref/DEV300_m40/toolkit/source/awt/vclxwindow.cxx#1135);
if so, DestroyOutputDevice() starts the process of destroying it.
When an AWT menu bar has been set on the window, all ends in a life time issue:
in a non-product build, the vcl Window
(http://svn.services.openoffice.org/opengrok/xref/DEV300_m40/vcl/source/window/window.cxx#4492)
informs us about a
living child window; and OOo will "abort in non-pro version".

The crash can be avoid from the client code by setting a null css.awt.XMenuBar
on the top window *before* invoking css.lang.XComponent::dispose on the
css.awt.XWindow: 

VCLXTopWindow_Base::setMenuBar
(http://svn.services.openoffice.org/opengrok/xref/DEV300_m40/toolkit/source/awt/vclxtopwindow.cxx#151)
will always invoke (before even testing if the reference is empty)
SystemWindow::SetMenuBar with a NULL arg., and this makes SystemWindow ensure
that MenuBar::ImplDestroy is called to destroy the menu bar
(http://svn.services.openoffice.org/opengrok/xref/DEV300_m40/vcl/source/window/syswin.cxx#933)


To avoid life-time issues with the MenuBar, one has to ensure
MenuBar::ImplDestroy is called, and you can do so by setting a NULL MenuBar: the
LayoutManager does so in LayoutManager::impl_clearUpMenuBar
(http://svn.services.openoffice.org/opengrok/xref/DEV300_m40/framework/source/layoutmanager/layoutmanager.cxx#514).


So the proposed patch adds a flag to track when an AWT MenuBar has been set on
the AWT XTopWindow. If so, a NULL MenuBar is set on VCLXTopWindow::dispose() in
order to solve the life-time issue.
Comment 6 carsten.driesner 2009-02-09 11:44:29 UTC
cd: I had a quick look over your patch. It looks good to me but I want to talk
to get opinions from other people, too.
Comment 7 carsten.driesner 2009-02-09 12:40:02 UTC
cd: Should be fixed for next release OOo 3.2.
Comment 8 carsten.driesner 2009-02-09 12:41:15 UTC
cd->fs,mt: I would like to hear your opinion for the patch provided by Ariel.
Comment 9 Frank Schönheit 2009-02-09 13:20:05 UTC
setting m_bHasAWTMenuBar in VCLXTopWindow_Base, but evaluating it in
VCLXTopWindow only, is definitely Not Good (TM):
If there are (or will be) other derivations of VCLXTopWindow_Base (and I suppose
there are, since the class is declared DLL_PUBLIC), then they will still suffer
from the same problem, sooner or later.

The proper solution here is to introduce a "dispose" method at the
VCLXTopWindow_Base class, and make sure that all existing derived classes call
this method in their own dispose call (and leave a comment for future derived
classes).

As for VCLXTopWindow::dispose: Do not call into the base classes "dispose" with
the own mutex locked. Their might be places in the base class method which
acquire/release their own mutex, and rely on this being the only lock.


As a general note, I think the approach is valid to solve the particular
problem, but to me it looks as if there's a can of worms in the
VCLXMenu/XMenu/Menu(*) implementation/API. Originally, I wanted to mention that
the VLCXTopWindow_Base should of course be a listener at the XMenu, to know when
its being disposed, and thus the underlying Menu dies. Assuming that the
VCLXMenu is of course the owner of the Menu, since it created it ...

Now from looking at the code it seems that the Menu represented by a VLCXMenu
*never* dies. Huh? We do not seem to have a clear ownership concept here:
Creating a css.awt.Menu and releasing it leaves a memory leak, so it seems to
me. On the other hand, now with the patch, the MenuBar is implicitly deleted in
SystemWindow::SetMenuBar( NULL ) (but *not* when SetMenuBar would be called with
a non-NULL new menu bar - what weird logic is this, really?), so it seems the
SystemWindow assumed ownership of the menu bar.

Strange indeed ... Admittedly, this is not related to the patch here ...
Comment 10 Frank Schönheit 2009-02-09 13:26:34 UTC
> Now from looking at the code it seems that the Menu represented by a VLCXMenu
> *never* dies

Hmm, overlooked the most obvious place at
http://svn.services.openoffice.org/opengrok/xref/DEV300_m40/toolkit/source/awt/vclxmenu.cxx#106

Now I really wonder - if the VCLXMenu dies before the top window is closed -
wouldn't this crash? /me is off to the debugger ....
Comment 11 Frank Schönheit 2009-02-09 13:54:32 UTC
additional note: "dispose" in VCLXTopWindow needs a SAL_CALL, else it won't
compile on Windows.
Comment 12 carsten.driesner 2009-02-09 14:10:32 UTC
cd->fs: Indeed this class is DLL_PUBLIC which is very awful. Currently there is
no external project which use this class and therefore I would propose to make
this class private to the toolkit project. No need to make this IMPLEMENTATION
base class public.

cd->fs: You're right that the evaluation of the member should be done in the
base class. That's a problem with a quick look over the patch. As I have written
above this VCLXTopWindow_Base must be made private to the project. There is no
real advantage to make base implementation classes public to other projects if
they are not intended for public use. We only suffer from too many exported
symbols and limited changes in the toolkit project. In the mid-term I would like
to see no implementation classes exported to other projects. The toolkit project
should only be accessed by UNO interfaces/services. Base classes for other
projects should be moved to a different library, e.g. vclunohelper.

cd->fs: There are several problems with the ownership of objects in the toolkit.
I would propose to make the VCLTopWindow window owner of a menu whenever it's
set via setMenuBar(). The VCLTopWindow should held a reference to the menu so
the menu cannot be destroyed as long as the VCLXTopWindow lives. Setting a new
menu or an empty reference destroys the old toolkit based menu.
Comment 13 Frank Schönheit 2009-02-09 14:27:48 UTC
okay, I can read debugger output better than patches :)

VCLXMenu is the owner of it's Menu. Also, VCLXTopWindow holds the given menu bar
as mxMenuBar. So, there's no life time problem here, nor anywhere else around
VCLXMenus :)

Sorry for the noise.
Comment 14 carsten.driesner 2009-02-09 14:29:19 UTC
cd: I am quite sure that the framework code which provides the user interface
configuration API needs to be updated if we want to define the menu ownership as
proposed. Especially
framework\source\uielement\menubarmanager.cxx|menubarwrapper.cxx need to be
checked. I am quite sure that they use the plain VCL pointer to call delete
which would be completely forbidden!
Comment 15 Frank Schönheit 2009-02-18 08:26:08 UTC
didn't see any action here for quite some time ....

To make my point of view clear: I originally thought we might have a problem
with ownerhip of Menu instances, but meanwhile, I am not convinced anymore. The
current architecture is ... complex at least :), but seems to work.

In any way, I don't think that changing this architecture is something which
should be covered by this issue here. I just wanted to make this very clear,
since I suppose the non-action on the issue might be caused by some
misunderstanding on that question.

So, assuming that the original items I mentioned will be addressed, I think we
should continue with the patch.
Comment 16 carsten.driesner 2009-02-18 08:47:04 UTC
cd->fs: As far as I know Ariel is working on a new version of the patch. 
cd->arielch: Do you know when you have a new version of the patch ready?

If Ariel is not able to work on this issue anymore I would take over. Currently
I am also not sure that we have an ownership problem. There is a scenario where
ownership transfer could create severe problems. In the OLE in-place activation
scenario a merged menu bar is set at the TopWindow. The old menu is stored in
the layout manager. Whenever the use leaves the inplace mode the old menu is
restored (set again at the TopWindow). This easy and working process must be
rewritten if we want to change ownership handling.
Comment 17 Ariel Constenla-Haile 2009-02-18 10:39:02 UTC
> didn't see any action here for quite some time ....

sorry for the delay; before answering I wanted to test (==debug) some more
things, but as my holidays came to an end, I only had the weekends for this task.

> cd->fs: As far as I know Ariel is working on a new version of the patch

yes I am. I tried to follow Frank's comments, see below on that

> cd->arielch: Do you know when you have a new version of the patch ready?

I can assure next Monday. If it's too late, let me know.

I'm trying the following in the new patch:

* make VCLXTopWindow_Base class private to the toolkit project (as suggested by
cd). Only class inheriting from it is layoutimpl::VCLXDialog
(http://svn.services.openoffice.org/opengrok/xref/DEV300_m41/toolkit/source/awt/vclxdialog.hxx#48)

* make the member m_bHasAWTMenuBar private

* evaluate it in a protected and non virtual function in
VCLXTopWindow_Base::cleanUpMenuBar. 

If I understood clearly, this is what Frank suggests:

> The proper solution here is to introduce a "dispose" method at the
> VCLXTopWindow_Base class, and make sure that all existing derived classes 
> call this method in their own dispose call (and leave a comment for future 
> derived classes)

If I understand, you are not suggesting to inherit VCLXTopWindow_Base from
css.lang.XComponent, but to add a function to do the job.

* call this method in all existing derived class's dispose(). 
ToDo: add it to layoutimpl::VCLXDialog::dispose() [
http://svn.services.openoffice.org/opengrok/xref/DEV300_m41/toolkit/source/awt/vclxdialog.cxx#106
], and test it.


I attached a new patch, not fully tested/debugged yet. I'll do so on next weekend.
Comment 18 Ariel Constenla-Haile 2009-02-18 10:41:42 UTC
Created attachment 60279 [details]
patch taking comments into account
Comment 19 carsten.driesner 2009-02-18 12:09:20 UTC
cd->arielch: This was just a ping to be sure that are still working on the
patch. No hurry as we want to have a fix for OOo 3.2 and code freeze is many
weeks away. Currently there is no planned date for OOo 3.2 code freeze in the
OpenOffice.org wiki.
Comment 20 Frank Schönheit 2009-02-18 15:39:36 UTC
+1 to what Carsten set - relax :)

I just wanted to ensure that we didn't alienate you with talking about doing
some heavy architectural changes. Do your patch in whatever time frame suits you
most ...
Comment 21 Frank Schönheit 2009-02-18 15:43:30 UTC
new patch looks good to me. Didn't try it, though, will leave this up to Carsten :)
Comment 22 carsten.driesner 2009-02-25 08:59:04 UTC
cd->arielch: Patch looks good to me, too. I will check your patch with a current
dev300 installation.
Comment 23 Ariel Constenla-Haile 2009-02-25 12:57:31 UTC
->cd: notice that a fix for layoutimpl::VCLXDialog::dispose() [
http://svn.services.openoffice.org/opengrok/xref/DEV300_m41/toolkit/source/awt/vclxdialog.cxx#106
], is still missing.

In theory, the fix is rather trivial and simply:


===================================================================
--- ../toolkit/source/awt/vclxdialog.cxx	(revision 267005)
+++ ../toolkit/source/awt/vclxdialog.cxx	(working copy)
@@ -113,6 +113,7 @@
 //            maTabListeners.disposeAndClear( aDisposeEvent );
     }

+    VCLXTopWindow_Base::cleanUpMenuBar();
     VCLXWindow::dispose();
 }


but although I've tried different things this weekend, I re-builded enabling
layout, but couldn't make the test on toolkit/workben/layout run (I always get a
css.uno.RuntimeException no matter what I try).

In practice, setting a MenuBar on a Dialog has no effect at all:

in SystemWindow::SetMenuBar() the condition
[http://svn.services.openoffice.org/opengrok/xref/DEV300_m41/vcl/source/window/syswin.cxx#913]
evaluates always to false, because mpWindowImpl->mpBorderWindow is a null
pointer [a Dialog has no Border Window]; also
http://svn.services.openoffice.org/opengrok/xref/DEV300_m41/vcl/source/window/syswin.cxx#956
evaluates to false, so here there is no life-time issue due to the MenuBarWindow
holding a pointer to a dead parent

In theory layoutimpl::VCLXDialog::dispose() should call
VCLXTopWindow_Base::cleanUpMenuBar(); I just could find the way to test it works.
Comment 24 carsten.driesner 2009-02-26 13:02:46 UTC
cd->arielch: No problem at all. I just wanted to see if your patch works and I
had some trouble building it. I attached a new version of the patch which can be
built on Windows.
Comment 25 carsten.driesner 2009-02-26 13:03:54 UTC
Created attachment 60505 [details]
Revised patch.
Comment 26 carsten.driesner 2009-04-16 12:08:37 UTC
cd->arielch: Could you please give me an update regarding the state of the
patch? Did you try my changes?
Comment 27 Ariel Constenla-Haile 2009-04-16 14:51:02 UTC
arielch->cd: yours works fine in Linux.
As I couldn't see the difference between (my) 
http://www.openoffice.org/nonav/issues/showattachment.cgi/60279/toolkit.vclxtopwindow.18.March.2009.patch

and (yours) 
http://www.openoffice.org/nonav/issues/showattachment.cgi/60505/reworked.patch
I build last weekend for the first time OOo on WinXP to see what error generated
my patch (and so next time I try first things both on Linux and Win ;-)  ).


The fix for layoutimpl::VCLXDialog::dispose() [
http://svn.services.openoffice.org/opengrok/xref/DEV300_m46/toolkit/source/awt/vclxdialog.cxx#106
], is still missing: neither in DEV300_m46 could make the test on
toolkit/workben/layout run (in Linux).
Comment 28 carsten.driesner 2009-09-14 12:23:47 UTC
cd: Patch is not complete therefore no chance to integrate it into OOo 3.2.

cd: I have too much issues to fix all of them for OOo 3.2.  Due to missing time
this issue must be moved to the next release OOo 3.3.
Comment 29 carsten.driesner 2010-06-25 07:45:44 UTC
cd->arielch: Have to move the patch again. I want to work on a revised layout
manager with docking window support and have time to look to other related work.
I will check your patch while working on the layout manager. Do you have any
update regarding the patch?

Comment 30 Ariel Constenla-Haile 2010-06-25 16:36:30 UTC
arielch->cd: no, I have no update, other than some changes to adapt to changes
made (by fs?) in that class. I build using the patch I'll attach here, and
everything works as expected (right now on a DEV300_m83).
That said, the status is yet "incomplete", as I didn't test the layout impl., see
desc24
Any way, --enable-layout does not work (on Linux, I guess on Windows either) since
quite time ago, so we can assume this project is abandoned.
Comment 31 Ariel Constenla-Haile 2010-06-25 16:38:23 UTC
Created attachment 70225 [details]
patch working on DEV300_m83
Comment 32 carsten.driesner 2010-11-04 17:13:44 UTC
cd: Want to apply the patch while working on the revised layout manager. The
first rework of the layout manager, see CWS dockingwindows is now ready for QA.
I want to add this patch to the successor of this CWS which will contain the new
docking windows part.
Comment 33 Rob Weir 2013-03-11 15:04:29 UTC
I'm adding this comment to all open issues with Issue Type == PATCH.  We have 220 such issues, many of them quite old.  I apologize for that.  

We need your help in prioritizing which patches should be integrated into our next release, Apache OpenOffice 4.0.

If you have submitted a patch and think it is applicable for AOO 4.0, please respond with a comment to let us know.

On the other hand, if the patch is no longer relevant, please let us know that as well.

If you have any general questions or want to discuss this further, please send a note to our dev mailing list:  dev@openoffice.apache.org

Thanks!

-Rob
Comment 34 Marcus 2017-05-20 11:27:27 UTC
Reset assigne to the default "issues@openoffice.apache.org".