Differential D9852

Fix crash due to dangling pointer in MouseAnnotation
ClosedPublic
Actions

Authored by tobiasdeiminger on Jan 13 2018, 9:37 AM.

Details

Reviewers
aacid
Group Reviewers
Okular
Commits
R223:3c4f16ea4b7e: 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
  1. Load a document (e.g. linked PDF from bug report) and enable highlight toolbar (F6).
  2. Create highlight annotation.
  3. Move the view port so that the annotation position is right beside the highlight tool icon.
  4. 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.
  5. Press ctrl-z for undo.
  6. Click on highlight tool, move right into the document, create new highlight annotation.
  7. Okular doesn't crash.

Diff Detail

Repository
R223 Okular
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
tobiasdeiminger requested review of this revision.Jan 13 2018, 9:37 AM
tobiasdeiminger created this revision.
tobiasdeiminger added inline comments.Jan 13 2018, 9:57 AM
ui/pageviewmouseannotation.cpp
423

MouseAnnotation::notifyAnnotationChanged is called frequently while moving or resizing annotations. During these actions each mouse move event causes a call to DocumentObserver::notifyPageChanged. In consequence we call QLinkedList::contains (has O(n)) frequently, see AnnotationDescription::isContainedInPage. Not too bad, but if there was an event that meant "annotation deleted" instead of "annotation changed", that overhead could be avoided. Is there such an event?

ngraham edited the summary of this revision. (Show Details)Jan 13 2018, 3:56 PM
ngraham added a subscriber: ngraham.

Thanks for the patch! FWIW, "BUG: 388228" has to be on its own line.

aacid added a subscriber: aacid.Jan 13 2018, 11:44 PM

So a test for the exact bug scenario would become a bigger integration test rather than an isolated unit test.

Yes, that's what i meant, integration test like all the tests we have in autotests.

aacid added a comment.Jan 14 2018, 10:12 PM

Next time can you please try to use "arc" to submit the patch? It makes using phabricator much nicer since those "Context not available" lines turn into "let me read some more code", otherwise i have to spend time switching between here and the code editor.

aacid accepted this revision.Feb 25 2018, 5:54 PM

Honestly i don't understand the code but it fixes the crash so let's hope it doesn't create more regressions than it fixes :)

I'll commit it in a minute

This revision is now accepted and ready to land.Feb 25 2018, 5:54 PM
Closed by commit R223:3c4f16ea4b7e: Fix crash due to dangling pointer in MouseAnnotation (authored by tobiasdeiminger, committed by aacid). · Explain WhyFeb 25 2018, 6:09 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 28050

View Options

ui/pageview.cpp

Loading...
View Options

ui/pageviewmouseannotation.h

Loading...
View Options

ui/pageviewmouseannotation.cpp

Loading...