From 3c4f16ea4b7e57b57e34830cd4ecf3f0ff80b399 Mon Sep 17 00:00:00 2001
From: Tobias Deiminger <haxtibal@t-online.de>
Date: Sun, 25 Feb 2018 18:13:25 +0100
Subject: [PATCH] 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
---
 ui/pageview.cpp                |  6 +----
 ui/pageviewmouseannotation.cpp | 44 +++++++++++++++++++++++++++++++---
 ui/pageviewmouseannotation.h   |  8 +++++--
 3 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/ui/pageview.cpp b/ui/pageview.cpp
index 16f57c23d..87aed00cd 100644
--- a/ui/pageview.cpp
+++ b/ui/pageview.cpp
@@ -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 )
diff --git a/ui/pageviewmouseannotation.cpp b/ui/pageviewmouseannotation.cpp
index de9d46f5f..e636be53f 100644
--- a/ui/pageviewmouseannotation.cpp
+++ b/ui/pageviewmouseannotation.cpp
@@ -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.
diff --git a/ui/pageviewmouseannotation.h b/ui/pageviewmouseannotation.h
index 87dbb8db2..31d3f7c81 100644
--- a/ui/pageviewmouseannotation.h
+++ b/ui/pageviewmouseannotation.h
@@ -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 {