GNOME Bugzilla – Bug 760299
Evince crashes when using annotation + copy of pdf
Last modified: 2017-08-07 23:08:13 UTC
If are present unsaved annotations, Evince crashes when a copy of the pdf is opened. Steps to reproduce: Open a pdf Add a annotation Hambuger Menu -> Open a Copy Evince crashes only if it shows a page with a unsaved annotation, for example: Open a pdf Add annotation to page N Go to page N+2 Hamburger Menu -> Open a Copy Evince doen't crash until the page N is reached.
Actually it doesn't matter if the annotations are unsaved. It crashes when it shows the same annotation from two "copy" of the same file.
I can reproduce it. However, when I create the annotation with Evince. If I open a document with annotations created with another application it works as expected. Although, I have not tried with highlighting.
Here the backtrace from evince (4405f3e) and poppler master (01d4bb2): (gdb) bt
+ Trace 235881
This is a hard to fix issue... Annotations related objects live inside EvDocument.. which is shared by the two copies of EvView widgets... Unfortunately, these annotations objects have backlinks to the corresponding Annotation Widgets (like the popup window)... which is obiously wrong as this object only is valid for one of the two EvView widgets... Hence you get a crash.
José, But the problem also happens if the document only have highlight annotations. I understand that such annotations can have a text message (with a window), but do we create the windows with such annotation in spite they do not have anything yet?
Possibly related to Bug 706048
German, I think you are right and we create the annotation window for them. Look at the stacktrace. We are crashing in in show_annotation_windows()... This will look for the pointers saved in EvDocument (to widgets inside one EvView) and will try to show them on the other EvView.
*** Bug 706048 has been marked as a duplicate of this bug. ***
Created attachment 331799 [details] [review] view: use a hashtable to map annots to their popups. This prevents a crash when using "Open Copy" function because the popup is set to a window widget which is a child of another EvView.
The previous patch solves the issue. It looked to me that using a HashTable was the easiest workaround to the direct map used by g_object_get_data and g_object_set_data
Review of attachment 331799 [details] [review]: It works for me in the sense that it does not crashes anymore. I may have found another bug, though. Likely unrelated, but I comment it here just in case: 1. Open a document (D) 2. Add an annotation. 3. Open a copy of the document (C) 4. In (C) add an annotation. 5. Go back to (D) 6. Press Ctrl+R to reload the document. ==> The annotations are gone in (D).
Additionally: 7. Close (D) => segfault.
Hmm.. I tried to get a stacktrace but I was unable to reproduce the step 7 :-/
The bug about annotations dissapearing when reloading the page is unrelated to this bug... It occurs also on unpatched version of evince. I will provide a patch shortly for this as well.
Review of attachment 331799 [details] [review]: This makes sense, the annot windows are something that belongs to the view and not to the document. ::: libview/ev-view.c @@ +3111,3 @@ G_CALLBACK (ev_view_annotation_save_contents), view); + g_hash_table_replace (view->annot_window_map, annot, window); There's no point in using replace, I think, since the map doesn't own neither the key nor the value, if it already exists (something that shouldn't happen anyway) it's fine to use the existing key. @@ +3162,3 @@ if (window) { ev_annotation_window_set_annotation (EV_ANNOTATION_WINDOW (window), annot); + g_hash_table_replace (view->annot_window_map, annot, window); Ditto. @@ +6828,3 @@ g_object_unref (view->image_dnd_info.image); view->image_dnd_info.image = NULL; + g_hash_table_destroy (view->annot_window_map); You should also remove from the map the deleted annotations, in ev_view_remove_annotation() probably. @@ +7715,3 @@ view->zoom_center_x = -1; view->zoom_center_y = -1; + view->annot_window_map = g_hash_table_new (NULL, NULL); I think we could create this on demand, for documents without annots, or never adding an annot we don't really need this. We could add a helper method that gets or creates the map. I also prefer to pass the hash function explicitly, g_direct_hash in this case, so that you don't need to think what NULL, NULL means. It's fine to pass NULL as the equal, though, because you already know it's direct hash and not using a function is a little bit more efficient.
(In reply to Germán Poo-Caamaño from comment #11) > Review of attachment 331799 [details] [review] [review]: > > It works for me in the sense that it does not crashes anymore. > > I may have found another bug, though. Likely unrelated, but I comment it > here just in case: > > 1. Open a document (D) > 2. Add an annotation. > 3. Open a copy of the document (C) > 4. In (C) add an annotation. > 5. Go back to (D) > 6. Press Ctrl+R to reload the document. > ==> The annotations are gone in (D). Yes, the whole open a copy feature is indeed tricky.
Created attachment 332028 [details] [review] view: use a hashtable to map annots to their popups. This prevents a crash when using "Open Copy" function because the popup is set to a window widget which is a child of another EvView.
updated patch according to review. added to helper function that make things easier to read I think and create the hashtable on demand
Created attachment 332029 [details] [review] view: Don't look for annotation windows twice The only way to create an annotation window is through create_annotation_window. As this function always set the annot_window_map, we can always use this map to get the annotation window.
actually I also think that we can remove all these lines after applying the first patch...
(In reply to José Aliste from comment #14) > The bug about annotations dissapearing when reloading the page is unrelated > to this bug... It occurs also on unpatched version of evince. I will provide > a patch shortly for this as well. I filed a new bug. See Bug 769123
Review of attachment 332028 [details] [review]: LGTM, thanks!
Comment on attachment 332029 [details] [review] view: Don't look for annotation windows twice Do we still need ev_view_find_window_child_for_annot then?
(In reply to Carlos Garcia Campos from comment #23) > Comment on attachment 332029 [details] [review] [review] > view: Don't look for annotation windows twice > > Do we still need ev_view_find_window_child_for_annot then? We still use it once... We look for the window child only to remove it... We could change the func to ev_view_remove_window_child_for_annot and remove the link directly from there. This would be a bit more efficient since we wouldn't traverse the list twice.
Created attachment 332124 [details] [review] view: Don't look for annotation windows twice The only way to create an annotation window is through create_annotation_window. As this function always set the annot_window_map, we can always use this map to get the annotation window.
Here is the new patch that does what I say in the previous comment. It tested out fine...
Review of attachment 332028 [details] [review]: Thanks
*** Bug 769421 has been marked as a duplicate of this bug. ***
Comment on attachment 332124 [details] [review] view: Don't look for annotation windows twice I had forgotten this, sorry. Feel free to push it then.
The following fix has been pushed: 3663591 view: Don't look for annotation windows twice
(In reply to Germán Poo-Caamaño from comment #30) > The following fix has been pushed: > 3663591 view: Don't look for annotation windows twice (In reply to Carlos Garcia Campos from comment #29) > Comment on attachment 332124 [details] [review] [review] > view: Don't look for annotation windows twice > > I had forgotten this, sorry. Feel free to push it then. This fix causes another bug leading to a busy loop in Evince. See https://bugzilla.gnome.org/show_bug.cgi?id=785975 for details.