Apache OpenOffice (AOO) Bugzilla – Issue 99010
toolkit: setting a MenuBar on a Top Window crashes OOo
Last modified: 2017-05-20 11:27:27 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
Created attachment 60023 [details] Crash report, from a product build (Linux)
Created attachment 60024 [details] Proposed patch
changed issue type to patch added crash keyword
Created attachment 60025 [details] Test document (run the Main macro)
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.
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.
cd: Should be fixed for next release OOo 3.2.
cd->fs,mt: I would like to hear your opinion for the patch provided by Ariel.
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 ...
> 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 ....
additional note: "dispose" in VCLXTopWindow needs a SAL_CALL, else it won't compile on Windows.
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.
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.
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!
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.
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.
> 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.
Created attachment 60279 [details] patch taking comments into account
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.
+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 ...
new patch looks good to me. Didn't try it, though, will leave this up to Carsten :)
cd->arielch: Patch looks good to me, too. I will check your patch with a current dev300 installation.
->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.
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.
Created attachment 60505 [details] Revised patch.
cd->arielch: Could you please give me an update regarding the state of the patch? Did you try my changes?
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).
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.
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?
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.
Created attachment 70225 [details] patch working on DEV300_m83
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.
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
Reset assigne to the default "issues@openoffice.apache.org".