tdf#155017: make sure that the correct shell is popped from SfxDispatcher
Honestly, I don't quite understand the idea of using the stack of shells
in SfxDispatcher, when the order of addition of shells there does not
match the order of removal.
After opening the bugdoc, SfxDispatcher has these shells in xImp->aStack
(from top to bottom):
[8] SwWebView
[7] SwWebTableShell
[6] SwWebTextShell
[5] SwWebListShell
[4] FmFormShell
[3] SwWebDocShell
[2] SfxViewFrame
[1] SwModule
[0] SfxApplication
SfxViewFrame dtor calls ReleaseObjectShell_Impl.
* First of all, it calls PopShellAndSubShells_Impl for SfxViewShell;
* Then it calls SfxDispatcher::Pop for SfxObjectShell;
* Then it calls SfxDispatcher::RemoveShell_Impl for SfxModule;
* Then SfxObjectShell is destroyed;
* And finally, SfxDispatcher::SetDisableFlags is called.
PopShellAndSubShells_Impl (for SfxViewShell) finds the passed SwWebView
at the top of the stack (pos. 8), and removes it.
SfxDispatcher::Pop for SfxObjectShell queues removal of the passed
SwWebDocShell *without* SfxDispatcherPopFlags::POP_UNTIL mode.
SfxDispatcher::RemoveShell_Impl first calls Flush, which actually
executes the queued actions. At this point, an SwWebTableShell (pos. 7)
is the top of the stack; SfxDispatcher::FlushImpl will pop it, and stop
(because there was no SfxDispatcherPopFlags::POP_UNTIL at the previous
step), so the intended removal of SwWebDocShell (which is at pos. 3)
will not happen. Then RemoveShell_Impl will proceed searching for the
specific shell (SwModule), and removing specifically that (at pos. 1).
At the moment of destruction of object shell, the dispatcher's stack
looks like this:
[5] SwWebTextShell
[4] SwWebListShell
[3] FmFormShell
[2] SwWebDocShell <-- The problem is here
[1] SfxViewFrame
[0] SfxApplication
and pos.2 points to a destructed object.
Finally, SetDisableFlags iterates all the shells still in the stack,
setting their flags - and obviously accessing already destroyed object.
In debug builds, this crashes reliably; in release builds, where dtors
do not fill memory, the access of the just-destroyed objects likely
often goes unnoticed.
In different documents, the order and the set of shells there is
different, e.g. an empty Writer document would have view shell just
above object shell, resulting in correct removal of the object shell.
I don't know what strategy is intended here. I decided to implement a
small change, that makes sure that without POP_UNTIL, popping a shell
removes exactly that shell that was requested for removal.
Change-Id: I670d024056a5b32d5485f00a4799a8b0bacb6485
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/151003
Tested-by: Mike Kaganski <mike.kaganski@collabora.com>
Reviewed-by: Mike Kaganski <mike.kaganski@collabora.com>
diff --git a/sfx2/source/control/dispatch.cxx b/sfx2/source/control/dispatch.cxx
index b1bb3c7a..e78789e 100644
--- a/sfx2/source/control/dispatch.cxx
+++ b/sfx2/source/control/dispatch.cxx
@@ -1321,12 +1321,25 @@ void SfxDispatcher::FlushImpl()
else
{
// Actually pop
SfxShell* pPopped = nullptr;
bool bFound = false;
do
if (!i->bUntil)
{
// pop exactly the requested shell
if (auto it = std::find(xImp->aStack.begin(), xImp->aStack.end(), i->pCluster);
it != xImp->aStack.end())
{
xImp->aStack.erase(it);
i->pCluster->SetDisableFlags(SfxDisableFlags::NONE);
bFound = true;
// Mark the moved Shell
aToDoCopy.push_front(SfxToDo_Impl(false, i->bDelete, false, *i->pCluster));
}
}
while (!bFound)
{
DBG_ASSERT( !xImp->aStack.empty(), "popping from empty stack" );
pPopped = xImp->aStack.back();
SfxShell* pPopped = xImp->aStack.back();
xImp->aStack.pop_back();
pPopped->SetDisableFlags( SfxDisableFlags::NONE );
bFound = (pPopped == i->pCluster);
@@ -1334,7 +1347,6 @@ void SfxDispatcher::FlushImpl()
// Mark the moved Shell
aToDoCopy.push_front(SfxToDo_Impl(false, i->bDelete, false, *pPopped));
}
while(i->bUntil && !bFound);
DBG_ASSERT( bFound, "wrong SfxShell popped" );
}
}
