Hi Jonathan, thanks for your detailed bug report. I tried to reproduce this bug on Ubuntu 17.10, without success so far.

You're backtrace reveals the crash happens here:

ui/pageviewmouseannotation.cpp:379 (tag 17.04.3)
if ( m_mouseOverAnnotation.annotation->subType() == Okular::Annotation::AMovie )

This involves dereferencing the annotation pointer and a vtable lookup for subType(). So, possible reasons for the crash are a dangling annotation pointer, the annotation object being in inconsistent state, or a corrupted vtable.

It's difficult to go on without further evidence from here. Ubuntu creates crash reports via the apport tool. It should be at /var/crash/_usr_bin_okular.1000.crash or similar and includes a core dump and other valuable debug information. Could you share this file?

@okular devs: The file size will be somewhere in 10..100 MB. Is it ok to attach it here? Where else should such binary debug information go to?

Attach the file if possible, yes.

Ah wait no, we don't care about the coredump much really, what we want is the pdf file you're using to make this crash

and what would be really interesting is if you can update to something newer than 1.1.3

Hi, it happens with virtually every PDF. Random example:
http://www.philipebert.info/resources/WhatMathematicalKnowledgeCouldNotBe.pdf
I highlight the word “comply” in the first paragraph, undo it, repeat this a couple of time, then it does not take too long till okular will crash while releasing the cursor.

Can't reproduce the crash at all.

Can you try running okular under valgrind and attaching the log of when you try to reproduce the crash?

Looks like a corrupted vtable. “pure virtual method called”

==30208== Memcheck, a memory error detector
==30208== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==30208== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==30208== Command: okular WhatMathematicalKnowledgeCouldNotBe.pdf
==30208== 

Illegal icon group:  7
Illegal icon group:  7
Illegal icon group:  7
Illegal icon group:  7
Illegal icon group:  7
Illegal icon group:  7
Illegal icon group:  7
==30208== Invalid read of size 8
==30208==    at 0x1E94427A: MouseAnnotation::cursor() const (pageviewmouseannotation.cpp:379)
==30208==    by 0x1E94988B: PageView::updateCursor(QPoint const&) (pageview.cpp:4032)
==30208==    by 0x1E949939: PageView::updateCursor() (pageview.cpp:3997)
==30208==    by 0x1E93BD1A: PageViewAnnotator::slotToolSelected(int) (pageviewannotator.cpp:981)
==30208==    by 0x823C9FE: QMetaObject::activate(QObject*, int, int, void**) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.9.1)
==30208==    by 0x1E9656B9: toolSelected (moc_pageviewutils.cpp:340)
==30208==    by 0x1E9656B9: ToolBarPrivate::selectButton(ToolBarButton*) [clone .part.38] (pageviewutils.cpp:927)
==30208==    by 0x1E9659D5: selectButton (pageviewutils.cpp:610)
==30208==    by 0x1E9659D5: PageViewToolBar::selectButton(int) (pageviewutils.cpp:610)
==30208==    by 0x1E93D3AE: detachAnnotation (pageviewannotator.cpp:1077)
==30208==    by 0x1E93D3AE: PageViewAnnotator::performRouteMouseOrTabletEvent(AnnotatorEngine::EventType const&, AnnotatorEngine::Button const&, QPointF const&, PageViewItem*) (pageviewannotator.cpp:867)
==30208==    by 0x1E93D42F: PageViewAnnotator::routeMouseEvent(QMouseEvent*, PageViewItem*) (pageviewannotator.cpp:881)
==30208==    by 0x1E95C448: PageView::mouseReleaseEvent(QMouseEvent*) (pageview.cpp:2443)
==30208==    by 0x6EE3DC7: QWidget::event(QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.9.1)
==30208==    by 0x6FC32DD: QFrame::event(QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.9.1)
==30208==  Address 0x27a2b9f0 is 0 bytes inside a block of size 16 free'd
==30208==    at 0x4C3123B: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==30208==    by 0x1EC658E2: Okular::AddAnnotationCommand::~AddAnnotationCommand() (documentcommands.cpp:72)
==30208==    by 0x1EC65928: Okular::AddAnnotationCommand::~AddAnnotationCommand() (documentcommands.cpp:74)
==30208==    by 0x71FC861: QUndoStack::push(QUndoCommand*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.9.1)
==30208==    by 0x1EC49A24: Okular::Document::addPageAnnotation(int, Okular::Annotation*) (document.cpp:3230)
==30208==    by 0x1E93D27F: PageViewAnnotator::performRouteMouseOrTabletEvent(AnnotatorEngine::EventType const&, AnnotatorEngine::Button const&, QPointF const&, PageViewItem*) (pageviewannotator.cpp:858)
==30208==    by 0x1E93D42F: PageViewAnnotator::routeMouseEvent(QMouseEvent*, PageViewItem*) (pageviewannotator.cpp:881)
==30208==    by 0x1E95C448: PageView::mouseReleaseEvent(QMouseEvent*) (pageview.cpp:2443)
==30208==    by 0x6EE3DC7: QWidget::event(QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.9.1)
==30208==    by 0x6FC32DD: QFrame::event(QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.9.1)
==30208==    by 0x1E95EF23: PageView::viewportEvent(QEvent*) (pageview.cpp:3323)
==30208==    by 0x820DACB: QCoreApplicationPrivate::sendThroughObjectEventFilters(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.9.1)
==30208==  Block was alloc'd at
==30208==    at 0x4C3017F: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==30208==    by 0x1E941D2A: TextSelectorEngine::end() (pageviewannotator.cpp:606)
==30208==    by 0x1E93D202: PageViewAnnotator::performRouteMouseOrTabletEvent(AnnotatorEngine::EventType const&, AnnotatorEngine::Button const&, QPointF const&, PageViewItem*) (pageviewannotator.cpp:849)
==30208==    by 0x1E93D42F: PageViewAnnotator::routeMouseEvent(QMouseEvent*, PageViewItem*) (pageviewannotator.cpp:881)
==30208==    by 0x1E95C448: PageView::mouseReleaseEvent(QMouseEvent*) (pageview.cpp:2443)
==30208==    by 0x6EE3DC7: QWidget::event(QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.9.1)
==30208==    by 0x6FC32DD: QFrame::event(QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.9.1)
==30208==    by 0x1E95EF23: PageView::viewportEvent(QEvent*) (pageview.cpp:3323)
==30208==    by 0x820DACB: QCoreApplicationPrivate::sendThroughObjectEventFilters(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.9.1)
==30208==    by 0x6EA3444: QApplicationPrivate::notify_helper(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.9.1)
==30208==    by 0x6EAB28E: QApplication::notify(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.9.1)
==30208==    by 0x820DDE7: QCoreApplication::notifyInternal2(QObject*, QEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.9.1)
==30208== 
pure virtual method called
terminate called without an active exception
==30208== 
==30208== Process terminating with default action of signal 6 (SIGABRT)
==30208==    at 0x8A8A0BB: raise (raise.c:51)
==30208==    by 0x8A8BF5C: abort (abort.c:90)
==30208==    by 0x875F094: __gnu_cxx::__verbose_terminate_handler() (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.24)
==30208==    by 0x875CC85: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.24)
==30208==    by 0x875CCD0: std::terminate() (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.24)
==30208==    by 0x875DABE: __cxa_pure_virtual (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.24)
==30208==    by 0x1E944285: MouseAnnotation::cursor() const (pageviewmouseannotation.cpp:379)
==30208==    by 0x1E94988B: PageView::updateCursor(QPoint const&) (pageview.cpp:4032)
==30208==    by 0x1E949939: PageView::updateCursor() (pageview.cpp:3997)
==30208==    by 0x1E93BD1A: PageViewAnnotator::slotToolSelected(int) (pageviewannotator.cpp:981)
==30208==    by 0x823C9FE: QMetaObject::activate(QObject*, int, int, void**) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.9.1)
==30208==    by 0x1E9656B9: toolSelected (moc_pageviewutils.cpp:340)
==30208==    by 0x1E9656B9: ToolBarPrivate::selectButton(ToolBarButton*) [clone .part.38] (pageviewutils.cpp:927)
==30208== 
==30208== HEAP SUMMARY:
==30208==     in use at exit: 52,979,090 bytes in 337,249 blocks
==30208==   total heap usage: 3,908,136 allocs, 3,570,887 frees, 1,370,549,893 bytes allocated
==30208== 
==30208== LEAK SUMMARY:
==30208==    definitely lost: 7,168 bytes in 27 blocks
==30208==    indirectly lost: 3,469 bytes in 140 blocks
==30208==      possibly lost: 2,498,797 bytes in 8,145 blocks
==30208==    still reachable: 50,469,656 bytes in 328,937 blocks
==30208==                       of which reachable via heuristic:
==30208==                         newarray           : 6,112 bytes in 54 blocks
==30208==                         multipleinheritance: 78,152 bytes in 91 blocks
==30208==         suppressed: 0 bytes in 0 blocks
==30208== Rerun with --leak-check=full to see details of leaked memory
==30208== 
==30208== For counts of detected and suppressed errors, rerun with: -v
==30208== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

Thanks again. I'll try to fix the problem as it occurs in code from a patch I did.

> Looks like a corrupted vtable.

Nope, dangling pointer... The __cxa_pure_virtual call is a consecutive fault of that. Your trace shows the annotation object got deleted during undo of AddAnnotationCommand.

==30208==  Address 0x27a2b9f0 is 0 bytes inside a block of size 16 free'd
==30208==    at 0x4C3123B: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==30208==    by 0x1EC658E2: Okular::AddAnnotationCommand::~AddAnnotationCommand() (documentcommands.cpp:72)
==30208==    by 0x1EC65928: Okular::AddAnnotationCommand::~AddAnnotationCommand() (documentcommands.cpp:74)
==30208==    by 0x71FC861: QUndoStack::push(QUndoCommand*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.9.1)
==30208==    by 0x1EC49A24: Okular::Document::addPageAnnotation(int, Okular::Annotation*) (document.cpp:3230)
[...]

MouseAnnotation did not notice the deletion and continued to track the now deleted annotation. On next access (in MouseAnnotation::cursor) we can crash with various kinds of errors, depending on what has happened in the meantime to the freed memory.

You are of course right. Thank you very much!

(In reply to Tobias Deiminger from comment #7)
> Thanks again. I'll try to fix the problem as it occurs in code from a patch
> I did.
> 
> > Looks like a corrupted vtable.
> 
> Nope, dangling pointer... The __cxa_pure_virtual call is a consecutive fault
> of that. Your trace shows the annotation object got deleted during undo of
> AddAnnotationCommand.
> 
> ==30208==  Address 0x27a2b9f0 is 0 bytes inside a block of size 16 free'd
> ==30208==    at 0x4C3123B: operator delete(void*) (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==30208==    by 0x1EC658E2:
> Okular::AddAnnotationCommand::~AddAnnotationCommand()
> (documentcommands.cpp:72)
> ==30208==    by 0x1EC65928:
> Okular::AddAnnotationCommand::~AddAnnotationCommand()
> (documentcommands.cpp:74)
> ==30208==    by 0x71FC861: QUndoStack::push(QUndoCommand*) (in
> /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.9.1)
> ==30208==    by 0x1EC49A24: Okular::Document::addPageAnnotation(int,
> Okular::Annotation*) (document.cpp:3230)
> [...]
> 
> MouseAnnotation did not notice the deletion and continued to track the now
> deleted annotation. On next access (in MouseAnnotation::cursor) we can crash
> with various kinds of errors, depending on what has happened in the meantime
> to the freed memory.

Is this problem still existing? Can you reproduce it? I wonder why it works for me and I can't get valgrind to complain at all :S

> Is this problem still existing? Can you reproduce it?

Yes, it still exists in okular master. And I can reproduce it now, thanks to the valgrind trace telling what's going on.

> I wonder why it works for me and I can't get valgrind to complain at all :S

You need a very special input sequence:
-load document 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
-crash happens on mouse release

I'm preparing a fix for this atm. Basically it will forward DocumentObserver::notifyPageChanged and reset the pointer reference immediately if an annotation was deleted for some reason. I'm tempted to do a bit more cleanup/refactoring, should I keep that separated in another patch? What's the time frame for submitting them?

(In reply to Tobias Deiminger from comment #10)
> I'm preparing a fix for this atm. Basically it will forward
> DocumentObserver::notifyPageChanged and reset the pointer reference
> immediately if an annotation was deleted for some reason. I'm tempted to do
> a bit more cleanup/refactoring, should I keep that separated in another
> patch? What's the time frame for submitting them?

In https://community.kde.org/Schedules/Applications/17.12_Release_Schedule you can see the 17.12 schedule.

It'd be great if you could do a minimal patch to fix this for the stable branch and if you feel like refactoring some of the stuff have a separate branch for master.

And even nicer if you can add a unit test that would fail without the patch :)

Git commit 3c4f16ea4b7e57b57e34830cd4ecf3f0ff80b399 by Albert Astals Cid, on behalf of Tobias Deiminger.
Committed on 25/02/2018 at 18:11.
Pushed by aacid into branch 'Applications/17.12'.

Fix crash due to dangling pointer in MouseAnnotation

Summary:

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

M  +1    -5    ui/pageview.cpp
M  +41   -3    ui/pageviewmouseannotation.cpp
M  +6    -2    ui/pageviewmouseannotation.h

https://commits.kde.org/okular/3c4f16ea4b7e57b57e34830cd4ecf3f0ff80b399

okular crushes when inserting a note.

okular crushes when inserting a note of text.

it seems that inserting text in pdf file with okular will result in this kind of crash if I try to type Chinese character.

If I change the input method to English mode and typing letters and words, no crash happens at all.

By the way, my computer is running arch linux, and my input method is sogoupinyin.

Please do not hijack other people's bugs that have nothing to do with yours.