Issue 92372 - problems with Window::SetParent() and task pane list
Summary: problems with Window::SetParent() and task pane list
Alias: None
Product: gsl
Classification: Code
Component: code (show other issues)
Version: OOo 2.4.1
Hardware: PC Windows, all
: P3 Trivial (vote)
Target Milestone: ---
Assignee: AOO issues mailing list
QA Contact:
Depends on:
Reported: 2008-08-01 12:35 UTC by tml
Modified: 2013-09-12 15:20 UTC (History)
1 user (show)

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

window.cxx with debugging printfs (344.66 KB, text/plain)
2008-08-05 22:32 UTC, tml
no flags Details
syswin.cxx with debugging printfs (34.94 KB, text/plain)
2008-08-05 22:32 UTC, tml
no flags Details
taskpanelist.cxx with debugging printfs (12.89 KB, text/plain)
2008-08-05 22:33 UTC, tml
no flags Details
Proposed patch (5.22 KB, patch)
2008-08-06 13:00 UTC, tml
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description tml 2008-08-01 12:35:47 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
Comment 1 tml 2008-08-01 13:25:28 UTC
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.
Comment 2 philipp.lohmann 2008-08-01 13:50:47 UTC
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 ?
Comment 3 tml 2008-08-01 13:54:15 UTC
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".
Comment 4 philipp.lohmann 2008-08-01 15:31:11 UTC
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.
Comment 5 tml 2008-08-01 16:38:45 UTC
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.
Comment 6 philipp.lohmann 2008-08-04 16:42:01 UTC
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).
Comment 7 tml 2008-08-05 22:31:26 UTC
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

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

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
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

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::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

Nothing odd this time either:

SystemWindow::ImplIsInTaskPaneList(WINDOW*): this=05373C48 pWin=07079760
mpImplData=05373EB0, mpImplData->mpTaskPaneList=053B2008
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
Window::SetParent(): pNewSysWin=053710F8 adding this=07079760 to its
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

Window::SetParent(): this=07079760 newParent=05321E58 pSysWin=05373C48 current

This time the ImplIsInTaskPaneList() returns FALSE:

SystemWindow::ImplIsInTaskPaneList(WINDOW*): this=05373C48 pWin=07079760
mpImplData=05373EB0, mpImplData->mpTaskPaneList=053B2008
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
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
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
TaskPaneList::AddWindow: this=0538B218 pWindow=06F89AE0
TaskPaneList::AddWindow: *p=07079760
Comment 8 tml 2008-08-05 22:32:31 UTC
Created attachment 55581 [details]
window.cxx with debugging printfs
Comment 9 tml 2008-08-05 22:32:58 UTC
Created attachment 55582 [details]
syswin.cxx with debugging printfs
Comment 10 tml 2008-08-05 22:33:34 UTC
Created attachment 55583 [details]
taskpanelist.cxx with debugging printfs
Comment 11 tml 2008-08-05 23:24:22 UTC
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.
Comment 12 tml 2008-08-06 13:00:58 UTC
Created attachment 55596 [details]
Proposed patch
Comment 13 tml 2008-08-06 13:08:03 UTC
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.