Members of the KDE Community are recommended to subscribe to the kde-community mailing list at https://mail.kde.org/mailman/listinfo/kde-community to allow them to participate in important discussions and receive other important announcements

Commit 3c4f16ea authored by Tobias Deiminger's avatar Tobias Deiminger Committed by Albert Astals Cid

Fix crash due to dangling pointer in MouseAnnotation

Summary:
BUG: 388228

Diff applies to Applications/17.12, and should be easy to merge to master. It's kept quite minimal as suggested by Albert.

Albert also suggested to add a dedicated unit test and I'd agree, but am not yet sure how to do it. The original bug involves several classes, including UI: Document, Page, AddAnnotationCommand, PageView, PageViewAnnotator, MouseAnnotation - to name a few. So a test for the exact bug scenario would become a bigger integration test rather than an isolated unit test. The other approach would be to do a real unit test on MouseAnnotation. But again, MouseAnnotation has nasty dependencies (e.g., needs a parent PageView) which I'd have to mock. Any ideas? I'd be interested in a discussion on this topic.

Test Plan:
 # Load a document (e.g. [[ http://www.philipebert.info/resources/WhatMathematicalKnowledgeCouldNotBe.pdf | linked PDF from bug report ]]) and enable highlight toolbar (F6).
 # Create highlight annotation.
 # Move the view port so that the annotation position is right beside the highlight tool icon.
 # Move the mouse over the annotation, and then horizontally left until you reach the tool icon; it's important to stay over the highlight annotation as long as in viewport.
 # Press ctrl-z for undo.
 # Click on highlight tool, move right into the document, create new highlight annotation.
 # Okular doesn't crash.

Reviewers: #okular

Subscribers: aacid, ngraham

Tags: #okular

Differential Revision: https://phabricator.kde.org/D9852
parent 77ee07ce
......@@ -1411,11 +1411,7 @@ void PageView::notifyPageChanged( int pageNumber, int changedFlags )
}
}
QLinkedList< Okular::Annotation * >::ConstIterator annIt = qFind( annots, d->mouseAnnotation->annotation() );
if ( annIt == annItEnd )
{
d->mouseAnnotation->cancel();
}
d->mouseAnnotation->notifyAnnotationChanged( pageNumber );
}
if ( changedFlags & DocumentObserver::BoundingBox )
......
......@@ -42,6 +42,23 @@ bool AnnotationDescription::isValid() const
return ( annotation != nullptr );
}
bool AnnotationDescription::isContainedInPage( const Okular::Document * document, int pageNumber ) const
{
if ( AnnotationDescription::pageNumber == pageNumber )
{
/* Don't access page via pageViewItem here. pageViewItem might have been deleted. */
const Okular::Page * page = document->page( pageNumber );
if ( page != nullptr )
{
if ( page->annotations().contains( annotation ) )
{
return true;
}
}
}
return false;
}
void AnnotationDescription::invalidate()
{
annotation = nullptr;
......@@ -403,6 +420,24 @@ Qt::CursorShape MouseAnnotation::cursor() const
return Qt::ArrowCursor;
}
void MouseAnnotation::notifyAnnotationChanged( int pageNumber )
{
const AnnotationDescription emptyAd;
if ( m_focusedAnnotation.isValid() &&
! m_focusedAnnotation.isContainedInPage( m_document, pageNumber ) )
{
setState( StateInactive, emptyAd );
}
if ( m_mouseOverAnnotation.isValid() &&
! m_mouseOverAnnotation.isContainedInPage( m_document, pageNumber ) )
{
m_mouseOverAnnotation = emptyAd;
m_pageView->updateCursor();
}
}
void MouseAnnotation::updateAnnotationPointers()
{
if (m_focusedAnnotation.annotation)
......@@ -552,9 +587,12 @@ void MouseAnnotation::finishCommand()
void MouseAnnotation::updateViewport( const AnnotationDescription & ad ) const
{
const QRect & changedPageViewItemRect = getFullBoundingRect( ad );
m_pageView->viewport()->update( changedPageViewItemRect
.translated( ad.pageViewItem->uncroppedGeometry().topLeft() )
.translated( -m_pageView->contentAreaPosition() ) );
if ( changedPageViewItemRect.isValid() )
{
m_pageView->viewport()->update( changedPageViewItemRect
.translated( ad.pageViewItem->uncroppedGeometry().topLeft() )
.translated( -m_pageView->contentAreaPosition() ) );
}
}
/* eventPos: Mouse position in uncropped page coordinates.
......
......@@ -46,9 +46,10 @@ class AnnotationDescription
{
public:
AnnotationDescription()
: annotation( nullptr ), pageViewItem( nullptr ), pageNumber( 0 ) {}
: annotation( nullptr ), pageViewItem( nullptr ), pageNumber( -1 ) {}
AnnotationDescription( PageViewItem * newPageViewItem, const QPoint& eventPos );
bool isValid() const;
bool isContainedInPage( const Okular::Document * document, int pageNumber ) const;
void invalidate();
bool operator==( const AnnotationDescription & rhs ) const
{
......@@ -116,7 +117,10 @@ public:
Qt::CursorShape cursor() const;
// needs to be called after document save
/* Forward DocumentObserver::notifyPageChanged to this method. */
void notifyAnnotationChanged( int pageNumber );
/* Forward DocumentObserver::notifySetup to this method. */
void updateAnnotationPointers();
enum MouseAnnotationState {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment