Apache OpenOffice (AOO) Bugzilla – Issue 92372
problems with Window::SetParent() and task pane list
Last modified: 2013-09-12 15:20:43 UTC
I am investigating a crash in a use case where OOo Writer is embedded in another application on Windows. The immediate cause for the crash is that TaskPaneList::AddWindow() is called when the TaskPaneList's list of windows (mTaskPanes) contains a pointer to a Window object that has already been deleted. The destructor of the Window class does try to remove the window from its system window's task pane list, but fails. As far as I can see the problem is in Window::SetParent(). In a nutshell, for some reason if the Window has no "border window" (mpWindowImpl->mpBorderWindow == NULL), the SetParent() method doesn't actually change the parent (mpWindowImpl->mpRealParent is not set to pNewParent). Still the TaskPaneList-related code in SetParent removes the Window from its old "system window's" task pane list and adds the Window to the task pane list of the system window of the window that it was asked to reparent to (but didn't). As the code for the Window class is rather complex... I have no idea how to fix this. If I just move the assignment mpWindowImpl->mpRealParent = pNewParent; out of the if ( mpWindowImpl->mpBorderWindow ) test, will something else then break horribly...?
Let me add that the crash happens by opening one of the "shape" menus at the bottom of the embedded OOo Writer window, for instance the "star" one, and select one of the stars, for instance the eight-cornered one.
Thanks for the debugging, but a step by step method for reproduction would be helpful. So what application and how does one embed OOo into it ?
I am not sure if I am at liberty to tell what the application is. And unfortunately I don't know of any minimal embedding host "testbed".
Ok, I'll have a look then on a theoretical basis. Whether something breaks horribly when you change that code sadly all to often can only be determined after it already has. I'll look at the code and try to see the implications.
Thanks. One possible way to fix the problem, I think (haven't actually tried writing the code yet), which wouldn't affect what else is going on, would be to introduce a new field in the WindowImpl class that would keep track of the TaskPaneList, if any, to which the window has been added. Then when removing the window from its current TaskPaneList (which happens in SetParent() and in the destructor, if I recall correctly) one would simply use that pointer instead of relying on ImplGetLastSystemWindow() or the equivalent code in the destructor.
SetParent sets the parent in any case; hower in case of a bordered window it dispatches the work to its borderwindow: actually the borderwindow itself gets reparented (setting the mprealparent before is just to prevent a recursion. In case of an unbordered window (like for instance the just mentioned window the SetParent was dispatched to), SetParent does the real work, removing the reparented window from its old parent's children list (see ImplRemoveWindow) and then adding it to the new parent's children list (see ImplInsertWindow).
So are you saying there is no problem? I do think there is. I still can't pinpoint exactly what it is, but here are some annotated debugging printouts where I try to understand what is happening... Unfortunately producing such an annotated debugging printout is hard, as when you notice that you really would need more debugging printout, you add it, re-run the test, and then of course all the pointer values are different, and it is a pain to copy the pieces of the printout back into the annotation... After adding lots of debugging printfs in interesting places in window.cxx, syswin.cxx and taskpanelist.cxx, going through the output, and trying to understand the code, I think I understand what happens. Here is the scenario that leads to the crash. Below are excerpts from the debugging output with comments. I will attach the versions of window.cxx, syswin.cxx and taskpanelist.cxx with the debugging printfs for reference. A window is created: Window::Window(WindowType)): #113, this=07079760 This window 0795CF38 is what we are tracing here. It's mpWindowImpl->mpRealParent is set: Window::ImplInsertWindow(Window*): this=07079760 setting mpWindowImpl->mpRealParent=053710F8 It is added to a TaskPaneList: TaskPaneList::AddWindow: this=0538B218 pWindow=07079760 Then SetParent() is called on it: Window::SetParent(): this=07079760 newParent=05321E58 pSysWin=053710F8 current mpWindowImpl->mpRealParent=053710F8 The test if ( pSysWin && pSysWin->ImplIsInTaskPaneList( this ) ) succeeds because pSyswin->ImplIsInTaskPaneList() returns true: SystemWindow::ImplIsInTaskPaneList(WINDOW*): this=053710F8 pWin=07079760 mpImplData=05370FA0, mpImplData->mpTaskPaneList=0538B218 SystemWindow::ImplIsInTaskPaneList(WINDOW*): mpImplData->mpTaskPaneList->IsInList returned TRUE, returning that Also we get a pNewSysWin: Window::SetParent(): this=07079760 pNewSysWin=05373C48 so the code to remove the window from its old TaskPaneList is invoked, and bChangeTaskPaneList is set to TRUE: Window::SetParent(): bChangeTaskPaneList=TRUE pSysWin=053710F8 removing this=07079760 from its TaskPaneList=0538B218 TaskPaneList::RemoveWindow: this=0538B218 pWindow=07079760 TaskPaneList::RemoveWindow: this=0538B218 pWindow=07079760 found it, removing! The window has no border window: Window::SetParent(): this=07079760 no mpWindowImpl->mpBorderWindow!? Then ImplInsertWindow() is called to really set the parent: Window::ImplInsertWindow(Window*): this=07079760 setting mpWindowImpl->mpRealParent=05321E58 and then as bChangeTaskPaneList is TRUE, the window is added to the new system window's TaskPaneList: Window::SetParent(): pNewSysWin=05373C48 adding this=07079760 to its TaskPaneList=053B2008 TaskPaneList::AddWindow: this=053B2008 pWindow=07079760 TaskPaneList::AddWindow: *p=05321E58 Then a bit later SetParent() is called again: Window::SetParent(): this=07079760 newParent=0534D998 pSysWin=05373C48 current mpWindowImpl->mpRealParent=05321E58 Nothing odd this time either: SystemWindow::ImplIsInTaskPaneList(WINDOW*): this=05373C48 pWin=07079760 mpImplData=05373EB0, mpImplData->mpTaskPaneList=053B2008 SystemWindow::ImplIsInTaskPaneList(WINDOW*): mpImplData->mpTaskPaneList->IsInList returned TRUE, returning that Window::SetParent(): this=07079760 pNewSysWin=053710F8 Window::SetParent(): bChangeTaskPaneList=TRUE pSysWin=05373C48 removing this=07079760 from its TaskPaneList=053B2008 TaskPaneList::RemoveWindow: this=053B2008 pWindow=07079760 TaskPaneList::RemoveWindow: this=053B2008 pWindow=07079760 found it, removing! Window::SetParent(): this=07079760 no mpWindowImpl->mpBorderWindow!? Window::ImplInsertWindow(Window*): this=07079760 setting mpWindowImpl->mpRealParent=0534D998 Window::SetParent(): pNewSysWin=053710F8 adding this=07079760 to its TaskPaneList=0538B218 TaskPaneList::AddWindow: this=0538B218 pWindow=07079760 Then again a bit later SetParent() is called a third time. Now the newParent is the same as the current mpWindowImpl->mpRealParent. Note that for some reason the pSysWin calculated by ImplGetLastSystemWindow() is not the pNewSysWin from last time SetParent() was called. This is probably the cause of the problem. For some reason the pNewSysWin calculated in SetParent() isn't actually true. Window::SetParent(): this=07079760 newParent=05321E58 pSysWin=05373C48 current mpWindowImpl->mpRealParent=05321E58 This time the ImplIsInTaskPaneList() returns FALSE: SystemWindow::ImplIsInTaskPaneList(WINDOW*): this=05373C48 pWin=07079760 mpImplData=05373EB0, mpImplData->mpTaskPaneList=053B2008 SystemWindow::ImplIsInTaskPaneList(WINDOW*): mpImplData->mpTaskPaneList->IsInList returned FALSE, returning that so pSysWin->GetTaskPaneList()->RemoveWindow(this) doesn't get called, and bChangeTaskPaneList isn't set to TRUE. Window::SetParent(): this=07079760 no mpWindowImpl->mpBorderWindow!? Window::ImplInsertWindow(Window*): this=07079760 setting mpWindowImpl->mpRealParent=05321E58 Window::SetParent(): bChangeTaskPaneList is FALSE, not adding this=07079760 to pNewSysWin=00000000 's (if any) TaskPaneList=00000000 Then the window gets destroyed. It first tries to remove itself from the TaskPaneList, but fails: Window::~Window(): this=07079760 mpWindowImpl->mbIsInTaskPaneList=1 Window::~Window(): this=07079760 pMySysWin=05373C48 , its TaskPaneList=053B2008 SystemWindow::ImplIsInTaskPaneList(WINDOW*): this=05373C48 pWin=07079760 mpImplData=05373EB0, mpImplData->mpTaskPaneList=053B2008 SystemWindow::ImplIsInTaskPaneList(WINDOW*): mpImplData->mpTaskPaneList->IsInList returned FALSE, returning that Window::~Window(): 07079760 not found in TaskPaneList Window::~Window(): #31, this=07079760 The Window has now been destructed. However, the Window pointer is still left in the TaskPaneList 0538B218, so what another window then later tries to add itself to that TaskPaneList, the code comes across this bogus pointer which causes a crash because the Window object contains bogus data, and the pWindow->IsWindowOrChild(*p) call tries to call methods on it. Window::Window(WindowType)): #120, this=06F89AE0 Window::ImplInsertWindow(Window*): this=06F89AE0 setting mpWindowImpl->mpRealParent=053710F8 TaskPaneList::AddWindow: this=0538B218 pWindow=06F89AE0 TaskPaneList::AddWindow: *p=07079760
Created attachment 55581 [details] window.cxx with debugging printfs
Created attachment 55582 [details] syswin.cxx with debugging printfs
Created attachment 55583 [details] taskpanelist.cxx with debugging printfs
Adding even more debugging printouts gives some hints why the SetParent() doesnt "stick": after Window::ImplInsertWindow() has set the mpWindowImpl->mpRealParent(), and SetParent() has returned, shortly afterwards ImplDockingWindowWrapper::StartPopupMode() gets called, and that then sets the mpRealPrent of the window in question's mpWindowImpl back to the value it had before the SetParent() was called. But the window is still on the old "new system window's" TaskPaneList. Sigh, this code is so complex, I wonder if anybody has ever actually understood it completely.
Created attachment 55596 [details] Proposed patch
The above patch fixes the crash for me. The patch removes the ImplWinData::mbIsInTaskPaneList boolean field and instead adds an explicit TaskPaneList pointer mpTaskPaneList. Then in the Window class the ImplIsInTaskPaneList(BOOL) method is replaced with a ImplIsInTaskPaneList(TaskPaneList*) method that sets this field. And then in Window::~Window(), instead of checking mbIsInTaskPaneList and traversing the parent chaing looking for a system window in the task pane list of the window might be, simply check if mpWindowImpl->mpTaskPaneList is non-NULL, and if it is, call mpWindowImpl->mpTaskPaneList->RemoveWindow(this). Ditto in Window::SetParent(), call mpWindowImpl->mpTaskPaneList->RemoveWindow(this) instead of searching for the system window which has the window in its task pane list. The TaskPaneList::AddWindow() calls are kept as is, as that method will call back to Window::ImplIsInTaskPaneList() to set itself as the Window's TaskPaneList.