diff options
author | Caolán McNamara <caolanm@redhat.com> | 2020-09-24 14:00:35 +0100 |
---|---|---|
committer | Michael Stahl <michael.stahl@cib.de> | 2020-09-29 11:53:54 +0200 |
commit | 01eefecf1eca39969e1b7f04ca80204105452979 (patch) | |
tree | a27202051c9e039850c5195258f73edf78f46099 | |
parent | c70d803de936449926c779b3a30af31526e5a4a7 (diff) |
tdf#64711 pointer deleted out from under std::shared_ptr
the "end" call will close the shell, which is deleted directly so the local shared_ptr remains
pointed to something which is already deleted.
So:
a) Have activate return true/false to indicate the failure and require the caller to call
'end' in response to failure.
b) A bunch of stuff in the call-stack expects the ViewShell not to get deleted while they are
calling it, so additionally launch that 'end' to happen in the next event loop.
==2838867== Invalid read of size 8
==2838867== at 0x32F4B83C: sd::ViewShellBase::GetDocShell() const (ViewShellBase.hxx:97)
==2838867== by 0x335BBCFC: sd::ViewShell::GetDocSh() const (viewshel.cxx:1389)
==2838867== by 0x3357CAC1: sd::PresentationViewShell::~PresentationViewShell() (presvish.cxx:73)
==2838867== by 0x330AA34B: void __gnu_cxx::new_allocator<sd::PresentationViewShell>::destroy<sd::PresentationViewShell>(sd::PresentationViewShell*) (new_allocator.h:156)
==2838867== by 0x330AA2DF: void std::allocator_traits<std::allocator<sd::PresentationViewShell> >::destroy<sd::PresentationViewShell>(std::allocator<sd::PresentationViewShell>&, sd::PresentationViewShell*) (alloc_traits.h:531)
==2838867== by 0x330AA0BE: std::_Sp_counted_ptr_inplace<sd::PresentationViewShell, std::allocator<sd::PresentationViewShell>, (__gnu_cxx::_Lock_policy)2>::_M_dispose() (shared_ptr_base.h:560)
==2838867== by 0x32D10D33: std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() (shared_ptr_base.h:158)
==2838867== by 0x32D10C79: std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() (shared_ptr_base.h:733)
==2838867== by 0x330A03BD: std::__shared_ptr<sd::PresentationViewShell, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() (shared_ptr_base.h:1183)
==2838867== by 0x3309F847: std::shared_ptr<sd::PresentationViewShell>::~shared_ptr() (shared_ptr.h:121)
==2838867== by 0x3320E1E8: sd::SlideShow::activate(sd::ViewShellBase&) (slideshow.cxx:999)
==2838867== by 0x3357CF33: sd::PresentationViewShell::Activate(bool) (presvish.cxx:118)
==2838867== Address 0x2fb5f320 is 256 bytes inside a block of size 272 free'd
==2838867== at 0x483BEDD: operator delete(void*) (vg_replace_malloc.c:584)
==2838867== by 0x33466537: sd::PresentationViewShellBase::~PresentationViewShellBase() (PresentationViewShellBase.cxx:82)
==2838867== by 0x823076C: SfxViewFrame::ReleaseObjectShell_Impl() (viewfrm.cxx:1113)
==2838867== by 0x8234B1C: SfxViewFrame::~SfxViewFrame() (viewfrm.cxx:1652)
==2838867== by 0x8234FEB: SfxViewFrame::~SfxViewFrame() (viewfrm.cxx:1646)
==2838867== by 0x8230D6C: SfxViewFrame::Close() (viewfrm.cxx:1165)
==2838867== by 0x81E4217: SfxFrame::DoClose_Impl() (frame.cxx:142)
==2838867== by 0x821709A: SfxBaseController::dispose() (sfxbasecontroller.cxx:982)
==2838867== by 0x3337A59F: sd::DrawController::dispose() (DrawController.cxx:164)
==2838867== by 0x6F35CC0: (anonymous namespace)::XFrameImpl::setComponent(com::sun::star::uno::Reference<com::sun::star::awt::XWindow> const&, com::sun::star::uno::Reference<com::sun::star::frame::XController> const&) (frame.cxx:1485)
==2838867== by 0x6F3834E: (anonymous namespace)::XFrameImpl::close(unsigned char) (frame.cxx:1692)
==2838867== by 0x81E3CFA: SfxFrame::DoClose() (frame.cxx:108)
==2838867== by 0x823802C: SfxViewFrame::DoClose() (viewfrm.cxx:2525)
==2838867== by 0x3320B971: sd::SlideShow::end() (slideshow.cxx:696)
==2838867== by 0x3320E1C2: sd::SlideShow::activate(sd::ViewShellBase&) (slideshow.cxx:995)
==2838867== by 0x3357CF33: sd::PresentationViewShell::Activate(bool) (presvish.cxx:118)
Change-Id: Ida91228b7584491c9a5dc9c0b3f76ce63218a92a
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/103326
Tested-by: Jenkins
Reviewed-by: Michael Stahl <michael.stahl@cib.de>
-rw-r--r-- | sd/source/ui/inc/PresentationViewShell.hxx | 3 | ||||
-rw-r--r-- | sd/source/ui/inc/slideshow.hxx | 3 | ||||
-rw-r--r-- | sd/source/ui/slideshow/slideshow.cxx | 20 | ||||
-rw-r--r-- | sd/source/ui/view/presvish.cxx | 29 | ||||
-rw-r--r-- | sd/source/ui/view/viewshel.cxx | 7 |
5 files changed, 45 insertions, 17 deletions
diff --git a/sd/source/ui/inc/PresentationViewShell.hxx b/sd/source/ui/inc/PresentationViewShell.hxx index c0020685c02e..7adbc6631461 100644 --- a/sd/source/ui/inc/PresentationViewShell.hxx +++ b/sd/source/ui/inc/PresentationViewShell.hxx @@ -58,9 +58,12 @@ protected: private: ::tools::Rectangle maOldVisArea; + ImplSVEvent* mnAbortSlideShowEvent; virtual void Activate (bool bIsMDIActivate) override; virtual void Paint (const ::tools::Rectangle& rRect, ::sd::Window* pWin) override; + + DECL_LINK(AbortSlideShowHdl, void*, void); }; } // end of namespace sd diff --git a/sd/source/ui/inc/slideshow.hxx b/sd/source/ui/inc/slideshow.hxx index 90a45ac826ab..9b6087a5d311 100644 --- a/sd/source/ui/inc/slideshow.hxx +++ b/sd/source/ui/inc/slideshow.hxx @@ -157,7 +157,8 @@ public: // events void resize( const Size &rSize ); - void activate(ViewShellBase& rBase); + // return false if the activate failed. callers should call end in response to failre + bool activate(ViewShellBase& rBase); void deactivate(); void paint(); diff --git a/sd/source/ui/slideshow/slideshow.cxx b/sd/source/ui/slideshow/slideshow.cxx index 74bbe395d92d..d253bce66b4a 100644 --- a/sd/source/ui/slideshow/slideshow.cxx +++ b/sd/source/ui/slideshow/slideshow.cxx @@ -972,7 +972,7 @@ void SlideShow::resize( const Size &rSize ) mxController->resize( rSize ); } -void SlideShow::activate( ViewShellBase& rBase ) +bool SlideShow::activate( ViewShellBase& rBase ) { if( (mpFullScreenViewShellBase == &rBase) && !mxController.is() ) { @@ -984,23 +984,19 @@ void SlideShow::activate( ViewShellBase& rBase ) CreateController( pShell.get(), pShell->GetView(), rBase.GetViewWindow() ); - if( mxController->startShow(mxCurrentSettings.get()) ) - { - pShell->Resize(); - // Defer the sd::ShowWindow's GrabFocus to here. so that the accessible event can be fired correctly. - pShell->GetActiveWindow()->GrabFocus(); - } - else - { - end(); - return; - } + if (!mxController->startShow(mxCurrentSettings.get())) + return false; + + pShell->Resize(); + // Defer the sd::ShowWindow's GrabFocus to here. so that the accessible event can be fired correctly. + pShell->GetActiveWindow()->GrabFocus(); } } if( mxController.is() ) mxController->activate(); + return true; } void SlideShow::deactivate() diff --git a/sd/source/ui/view/presvish.cxx b/sd/source/ui/view/presvish.cxx index e324b5803f7b..34a789f4dd18 100644 --- a/sd/source/ui/view/presvish.cxx +++ b/sd/source/ui/view/presvish.cxx @@ -61,7 +61,8 @@ void PresentationViewShell::InitInterface_Impl() PresentationViewShell::PresentationViewShell( ViewShellBase& rViewShellBase, vcl::Window* pParentWindow, FrameView* pFrameView) -: DrawViewShell( rViewShellBase, pParentWindow, PageKind::Standard, pFrameView) + : DrawViewShell(rViewShellBase, pParentWindow, PageKind::Standard, pFrameView) + , mnAbortSlideShowEvent(nullptr) { if( GetDocSh() && GetDocSh()->GetCreateMode() == SfxObjectCreateMode::EMBEDDED ) maOldVisArea = GetDocSh()->GetVisArea( ASPECT_CONTENT ); @@ -70,6 +71,9 @@ PresentationViewShell::PresentationViewShell( ViewShellBase& rViewShellBase, vcl PresentationViewShell::~PresentationViewShell() { + if (mnAbortSlideShowEvent) + Application::RemoveUserEvent(mnAbortSlideShowEvent); + if( GetDocSh() && GetDocSh()->GetCreateMode() == SfxObjectCreateMode::EMBEDDED && !maOldVisArea.IsEmpty() ) GetDocSh()->SetVisArea( maOldVisArea ); } @@ -102,6 +106,14 @@ VclPtr<SvxRuler> PresentationViewShell::CreateVRuler(::sd::Window*) return nullptr; } +IMPL_LINK_NOARG(PresentationViewShell, AbortSlideShowHdl, void*, void) +{ + mnAbortSlideShowEvent = nullptr; + rtl::Reference<SlideShow> xSlideShow(SlideShow::GetSlideShow(GetViewShellBase())); + if (xSlideShow.is()) + xSlideShow->end(); +} + void PresentationViewShell::Activate( bool bIsMDIActivate ) { DrawViewShell::Activate( bIsMDIActivate ); @@ -115,7 +127,20 @@ void PresentationViewShell::Activate( bool bIsMDIActivate ) rtl::Reference< SlideShow > xSlideShow( SlideShow::GetSlideShow( GetViewShellBase() ) ); if( xSlideShow.is() ) - xSlideShow->activate(GetViewShellBase()); + { + bool bSuccess = xSlideShow->activate(GetViewShellBase()); + if (!bSuccess) + { + /* tdf#64711 PresentationViewShell is deleted by 'end' due to end closing + the object shell. So if we call xSlideShow->end during Activate there are + a lot of places in the call stack of Activate which understandable don't + expect this ViewShell to be deleted during use. Defer to the next event + loop the abort of the slideshow + */ + if (!mnAbortSlideShowEvent) + mnAbortSlideShowEvent = Application::PostUserEvent(LINK(this, PresentationViewShell, AbortSlideShowHdl)); + } + } if( HasCurrentFunction() ) GetCurrentFunction()->Activate(); diff --git a/sd/source/ui/view/viewshel.cxx b/sd/source/ui/view/viewshel.cxx index ca99e3f7a214..cd7534a89893 100644 --- a/sd/source/ui/view/viewshel.cxx +++ b/sd/source/ui/view/viewshel.cxx @@ -320,8 +320,11 @@ void ViewShell::Activate(bool bIsMDIActivate) rBindings.Invalidate( SID_3D_STATE, true ); rtl::Reference< SlideShow > xSlideShow( SlideShow::GetSlideShow( GetViewShellBase() ) ); - if(xSlideShow.is() && xSlideShow->isRunning() ) - xSlideShow->activate(GetViewShellBase()); + if (xSlideShow.is() && xSlideShow->isRunning()) + { + bool bSuccess = xSlideShow->activate(GetViewShellBase()); + assert(bSuccess && "can only return false with a PresentationViewShell"); (void)bSuccess; + } if(HasCurrentFunction()) GetCurrentFunction()->Activate(); |