Crash when using undo after scrolling down many pages

[stiglers-eponym]

Operating System

Linux

(Linux only) Distribution

Arch Linux

(Linux only) Desktop Environment

sway

(Linux Only) Display Server

Wayland

Installation Method

official repositories

Xournal++ Version

1.2.0 (10ed72c)

libgtk Version

3.24.38

Bug Description

Xournal++ crashes when using undo after scrolling many pages.

Expected Behaviour

no crash

Steps to Reproduce

1. Open a long PDF document.
2. Draw on one of the pages.
3. Scroll down many pages (a few 100)
4. Hit Ctrl+Z to undo the drawing.

Additional Context

A representative crash log:

Date: Sun Aug 13 13:37:38 2023
Error: signal 6
Xournal++ version 1.2.0
Git commit: 10ed72c
Gtk version 3.24.38

[bt]: (0) xournalpp(+0xbdd16) [0x56192b939d16]
[bt]: (1) /usr/lib/libc.so.6(+0x3e710) [0x7f2d18c3e710]
[bt]: (2) /usr/lib/libc.so.6(+0x8e83c) [0x7f2d18c8e83c]
[bt]: (3) /usr/lib/libc.so.6(raise+0x18) [0x7f2d18c3e668]
[bt]: (4) /usr/lib/libc.so.6(abort+0xd7) [0x7f2d18c264b8]
[bt]: (5) /usr/lib/libc.so.6(+0x263dc) [0x7f2d18c263dc]
[bt]: (6) /usr/lib/libc.so.6(+0x36d26) [0x7f2d18c36d26]
[bt]: (7) xournalpp(_ZN9RenderJob17rerenderRectangleERKN3xoj4util9RectangleIdEE+0x14a) [0x56192ba58d4a]
[bt]: (8) xournalpp(_ZN9RenderJob3runEv+0xac) [0x56192ba58e1c]
[bt]: (9) xournalpp(ZN9Scheduler17jobThreadCallbackEPS+0x108) [0x56192b935dd8]
[bt]: (10) /usr/lib/libglib-2.0.so.0(+0x8cd75) [0x7f2d19bcfd75]
[bt]: (11) /usr/lib/libc.so.6(+0x8c9eb) [0x7f2d18c8c9eb]
[bt]: (12) /usr/lib/libc.so.6(+0x11123c) [0x7f2d18d1123c]

Try to get a better stracktrace...
[bt] #1 xournalpp(+0xbe2af) [0x56192b93a2af]
??:0
[bt] #2 /usr/lib/libc.so.6(+0x3e710) [0x7f2d18c3e710]
??:0
[bt] #3 /usr/lib/libc.so.6(+0x8e83c) [0x7f2d18c8e83c]
??:0
[bt] #4 /usr/lib/libc.so.6(raise+0x18) [0x7f2d18c3e668]
??:0
[bt] #5 /usr/lib/libc.so.6(abort+0xd7) [0x7f2d18c264b8]
??:0
[bt] #6 /usr/lib/libc.so.6(+0x263dc) [0x7f2d18c263dc]
??:0
[bt] #7 /usr/lib/libc.so.6(+0x36d26) [0x7f2d18c36d26]
??:0
[bt] #8 xournalpp(_ZN9RenderJob17rerenderRectangleERKN3xoj4util9RectangleIdEE+0x14a) [0x56192ba58d4a]
??:0
[bt] #9 xournalpp(_ZN9RenderJob3runEv+0xac) [0x56192ba58e1c]
??:0
[bt] #10 xournalpp(ZN9Scheduler17jobThreadCallbackEPS+0x108) [0x56192b935dd8]
??:0
[bt] #11 /usr/lib/libglib-2.0.so.0(+0x8cd75) [0x7f2d19bcfd75]
??:0
[bt] #12 /usr/lib/libc.so.6(+0x8c9eb) [0x7f2d18c8c9eb]
??:0
[bt] #13 /usr/lib/libc.so.6(+0x11123c) [0x7f2d18d1123c]
??:0

Execution log:

DEBUG : GLib-GIO :: Using cross-namespace EXTERNAL authentication (this will deadlock if server is GDBus < 2.73.3)
DEBUG : GLib-GIO :: _g_io_module_get_default: Found default implementation dconf (DConfSettingsBackend) for ‘gsettings-backend’
DEBUG : GLib-GIO :: _g_io_module_get_default: Found default implementation gvfs (GDaemonVfs) for ‘gio-vfs’
DEBUG : GLib :: unsetenv() is not thread-safe and should not be used after threads are created
MESSAGE : xopp :: TEXTDOMAINDIR = (null), Platform-specific locale dir = /usr/share/xournalpp/../locale, chosen directory = /usr/share/xournalpp/../locale
INFO : xopp :: Loading plugins from: /usr/share/xournalpp/plugins
INFO : xopp :: Loading plugins from: /home/valentin/.config/xournalpp/plugins
MESSAGE : xopp :: Plugin "MigrateFontSizes" UI initialized
CRITICAL: Gtk :: gtk_tree_model_iter_children: assertion 'GTK_IS_TREE_MODEL (tree_model)' failed
CRITICAL: GLib-GObject :: g_object_unref: assertion 'G_IS_OBJECT (object)' failed
DEBUG : GdkPixbuf :: gdk_pixbuf_from_pixdata() called on:
DEBUG : GdkPixbuf :: Encoding raw
DEBUG : GdkPixbuf :: Dimensions: 20 x 25
DEBUG : GdkPixbuf :: Rowstride: 80, Length: 2024
DEBUG : GdkPixbuf :: Copy pixels == false
DEBUG : GdkPixbuf :: gdk_pixbuf_from_pixdata() called on:
DEBUG : GdkPixbuf :: Encoding raw
DEBUG : GdkPixbuf :: Dimensions: 20 x 25
DEBUG : GdkPixbuf :: Rowstride: 80, Length: 2024
DEBUG : GdkPixbuf :: Copy pixels == false
WARNING : xopp :: [Crash Handler] Crashed with signal 6
WARNING : xopp :: [Crash Handler] Wrote crash log to: /home/valentin/.cache/xournalpp/errorlogs/errorlog.20230813-133738.log

On the command line I get this output:

xournalpp: /usr/src/debug/xournalpp/xournalpp/src/core/control/jobs/RenderJob.cpp:48: void RenderJob::rerenderRectangle(const xoj::util::Rectangle&): Assertion `view->buffer.isInitialized()' failed.

(com.github.xournalpp.xournalpp:12355): xopp-WARNING **: 13:37:38.593: [Crash Handler] Crashed with signal 6

(com.github.xournalpp.xournalpp:12355): xopp-WARNING **: 13:37:38.594: [Crash Handler] Wrote crash log to: /home/valentin/.cache/xournalpp/errorlogs/errorlog.20230813-133738.log

(com.github.xournalpp.xournalpp:12355): xopp-WARNING **: 13:37:38.680: Trying to emergency save the current open document…

(com.github.xournalpp.xournalpp:12355): xopp-WARNING **: 13:37:38.685: Successfully saved document to "/home/valentin/.config/xournalpp/emergencysave.xopp"

[stiglers-eponym]

For completeness: I observe the same behaviour in version 1.2.0+dev (2f7fb81) with libgtk 3.24.38, installed via the AUR package xournalpp-git.

[bhennion]

Thanks a lot for the detailled report. This had been reported before (see #4991) but thanks to you, I can now reproduce the crash reliably. I'll investigate in the coming days.
I'm making this the main thread about this issue.

[bhennion]

So, the issue is the following: when hitting Ctrl+Z, we can modify a page that is not displayed (there might be other ways of achieving this).
If the page is far enough so that it is not cached anymore (see Settings->View->Performance), the page does not have a buffer and the assertion view->buffer.isInitialized() fails.

I am not sure what the best fix is though:

1. Easy fix: test whether the page has a buffer instead of asserting. Upside: it's easy - Downside: we should never have called for a RenderJob to begin with if there is no buffer to fill.
2. Easy fix: do not call for a RenderJob in PageView::rerenderRect() if the PageView has no buffer (so is not cached). Issue: what if the buffer gets deleted between the call and the actual execution of the RenderJob? We can't keep the mutex locked for that long.
3. Do both 1. and 2.: avoid unecessary jobs but test upon RenderJob's execution for safety.
4. More complicated: scroll to the modified page when undoing. Do we really want that? It sounds like there may be some annoying corner cases.

I'm leaning towards solution 3. @xournalpp/core any input on that?

[Febbe]

In my opinion it should be visible, what someone just has redone. So the best solution would be, to scroll to the change.

[stiglers-eponym]

I'm also in favor of option 4. I found this bug when trying to check what I had changed before (because I was surprised to have unsaved changes). I cannot imagine a situation in which scrolling to the change would be really annoying.

[Febbe]

I'm also in favor of option 4. I found this bug when trying to check what I had changed before (because I was surprised to have unsaved changes). I cannot imagine a situation in which scrolling to the change would be really annoying.

Like always, it depends. When you delete a page via the page view sidebar, you might just want the sidebar to scroll to that page, so you can continue your work on the current active page. But that change is out of scope currently.
(This would require the undo manager to memorize the origin, from which the change was made).

[bhennion]

This would actually be a lot of work. This means changing every derived class of UndoAction, and some of them may not be trivial to change. This will take a lot of time.

[rolandlo]

@bhennion @Febbe Is this something we want to fix for the 1.2.1. release?

[bhennion]

We could make a dirty fix, but not the elegant solution (that would actually be a feature and cause to much changes for a release version).

[Febbe]

I think, that this is not important enough to block 1.2.1 but the quick fix should be released in 1.2.x anyway.

[rolandlo]

I would go with dirty fix 1:

Easy fix: test whether the page has a buffer instead of asserting. Upside: it's easy - Downside: we should never have called for a RenderJob to begin with if there is no buffer to fill.

It's simple and at least will avoid some crashes.