tomasklaen / uosc

Feature-rich minimalist proximity-based UI for MPV player.
GNU Lesser General Public License v2.1
1.63k stars 65 forks source link

Repeatedly scrolling through the menu and then clicking on it will crashes #912

Open dyphire opened 2 months ago

dyphire commented 2 months ago

Trying to open the uosc menu and clicking on it after scrolling repeatedly triggers the script to crash.

https://github.com/tomasklaen/uosc/assets/61936050/406871fd-7c39-47aa-8d59-52a1eaf30e5e

The error log contents:

[  77.009][w][uosc] 
[  77.009][w][uosc] stack traceback:
[  77.009][w][uosc]     .../mpv-test/portable_config/scripts/uosc/elements/Menu.lua:665: in function 'handle_wheel_up'
[  77.009][w][uosc]     .../mpv-test/portable_config/scripts/uosc/elements/Menu.lua:1119: in function 'handler'
[  77.009][w][uosc]     ...yer/mpv-test/portable_config/scripts/uosc/lib/cursor.lua:157: in function 'trigger'
[  77.009][w][uosc]     ...yer/mpv-test/portable_config/scripts/uosc/lib/cursor.lua:373: in function 'cb'
[  77.009][w][uosc]     mp.defaults:107: in function 'fn'
[  77.009][w][uosc]     mp.defaults:66: in function 'handler'
[  77.009][w][uosc]     mp.defaults:383: in function 'handler'
[  77.009][w][uosc]     mp.defaults:513: in function 'call_event_handlers'
[  77.009][w][uosc]     mp.defaults:555: in function 'dispatch_events'
[  77.009][w][uosc]     mp.defaults:506: in function <mp.defaults:505>
[  77.009][w][uosc]     [C]: at 0x7ff7d9bce040
[  77.009][w][uosc]     [C]: at 0x7ff7d9bcd670
[  77.009][f][uosc] Lua error: .../mpv-test/portable_config/scripts/uosc/elements/Menu.lua:425: attempt to index local 'menu' (a nil value)
[  77.013][d][uosc] Exiting...

mpv.log

christoph-heinrich commented 2 months ago

I can't reproduce that, and I can't see how that even can happen just from thinking through the code.

Menu.current needs to be nil for that crash to happen, but the only place where it's set to nil is in close(), which happens when the menu gets replaced with a new one for the path selected. However it also gets immediately replaced with a new one, and it doesn't fit the stack trace that it somehow happens in between that.

According to the stack trace you get a wheel up event when clicking on the .. which makes no sense at all :confused:

Right before the crash the log contains Set property: user-data/uosc/menu/type="open-file" -> 1, which happens right before the new menu gets filled with entries and Menu.current gets set. I guess you could insert some print/log statement after the self:update(data) in Menu:init() to make absolutely sure it doesn't crash somewhere in there, but that wouldn't make sense based on the stack trace...

Does that only happen when you go back out to the drives menu? Because that's windows specific (although I still don't see how that could happen there, but that would explain why I can't reproduce it).

dyphire commented 2 months ago

Does that only happen when you go back out to the drives menu? Because that's windows specific (although I still don't see how that could happen there, but that would explain why I can't reproduce it).

After several tests it can be confirmed that the crash only occurs when trying to return to the drive menu and works fine in other subdirectories.

christoph-heinrich commented 2 months ago

Thanks for finding that out.

@tomasklaen can you reproduce that?

tomasklaen commented 2 months ago

Nope :( Can't get it to crash by scrolling and clicking up to drive menu.

dyphire commented 2 months ago

Unfortunately I can reproduce this crash consistently on my side. By the way after testing again, I reproduced the crash both when returning to the parent directory menu and when going to the subdirectory menu, so it's not actually only when returning to the drive menu.

Here's the new crash log file: mpv1.log , mpv2.log

Edit: Not sure if this is related to luajit.

dyphire commented 2 months ago

I'm guessing it's probably some sort of problem created by mouse events, and a simple patch would fix the crash for me:

@@ -422,7 +422,9 @@ end
 ---@param fling_options? Fling
 function Menu:scroll_by(delta, menu, fling_options)
    menu = menu or self.current
-   self:scroll_to((menu.fling and (menu.fling.y + menu.fling.distance) or menu.scroll_y) + delta, menu, fling_options)
+   if menu ~= nil then
+       self:scroll_to((menu.fling and (menu.fling.y + menu.fling.distance) or menu.scroll_y) + delta, menu, fling_options)
+   end
 end

 ---@param index? integer
-- 
christoph-heinrich commented 2 months ago

That prevents the crash, but that should never even get called when self.current == nil, so there is a deeper problem somewhere.