Removed code causing race condition #1082
Conversation
|
At first glance, it does look like it is weird duplication. But this is written this way to handle another, possibly more common edge case (which the comments do an incredibly bad job of highlighting) - focus stolen / lost mid-drag. I'm curious - did you experience a bug in-app from this? I'd love to know what the system config / browser / etc was! |
|
That said, does need to be re-assessed, as you're right; the block below can never run. |
|
Hey @webfiltered . I feel like if the focus is lost, then we could tank a misclick without worrying. Most users wouldn't click-and-drag, then tab out, let go, then tab back in, then expect something rational to happen, right? You could also reset on window blur to handle that, I think. I'm turiyag on Discord if you wanted to see it happen or chat. I was invited to QA things by DeliciousElephant8. Try flicking and letting go, have your cursor moving quickly while you let go. Here's what happens if you put a console log on each mouse event, see my branch here: 
I am on OSX, and on every browser (Chrome, Firefox, and Safari) there is a seeming 30% chance that the move-with-no-mouseup state happens. It may be based on how I personally click and drag, with a moving swiping action. Users who bring their cursor to rest and then lift the mouse would experience it less often, I expect. It could be exacerbated by the Retina display, since the pixel density is very high, smaller mouse movements will hit more pixels. I don't think it's the OS, I think it's my mouse velocity and pixel density on release. But the core issue is that mousemove and mouseup events are fired independently, and you may not receive a mouseup before a mousemove. So if the user is moving their cursor while letting go, the behavior is undefined. There is a second race condition that I just found by experimenting a bit more. mouseDOWN is also independent of mousemove, so if you put a little delay on the firing of mousemove events, then you hit another condition where a mousemove event fires as the user moves the mouse with no buttons held over the thing they're about to click. Then when they click down, it fires the mousedown event, which sets everything properly. Then when the mousemove event fires, since it didn't have a button held down at the time, e.buttons==0, and it resets things. This is much less likely because when initially clicking on a thing, the user isn't moving their cursor really fast. I don't have the personal 360-no-scope ability to hit an output dot without slowing down, so it's less of an issue. |
|
Actually, @webfiltered , try opening up this super basic HTML file. Click and drag all over, and if you see a bright thick red line, that's the edge case: |
|
I moved the console.log for the move event below the e.down return, and was, during an insane click-frenzy, able to reproduce a 0-buttons event with the down event still present. I'm using a MacBook Pro M4 - are you testing on older / overloaded hardware, by any chance? I understand from your perspective it feels far more important, but I struggled to reproduce this at all, with fast click flicks or anything else. Not saying it's not an issue, just saying this appears to be exacerbated by an underlying issue with your specific device (e.g. any special input apps in use? Karabiner-for-mice or something?). The bigger problem is that focus being stolen is not likely to be from a user alt-tabbing mid-drag. This can be caused by any number of OS-initiated focus changes. It is not acceptable for our app to fail to handle this - the result would be tabbing back to the page, and being in a dragging state, something "attached" to the cursor, but with no buttons held down. It is not correct to assume the user would be fine with the drag event resulting in the drop happening at any random point along the way, so we cancel the entire operation and reset. We do need to find a fix for this, but we need something that accommodates both. |
|
I'm on an M2 CPU, with 24GB of RAM. It doesn't appear to be related to performance, how many tabs I have open or processes running in the background seems not to affect the error rate. But at any rate, the M2 is still excellent hardware. Well, maybe we just reset on blur? I changed the example HTML file, so it would be white on blur. Very strangely, it seems like we get to watch what the mouse is doing until the user lets go of the button, even if they're doing it in another window. But it's detectable as a blurred window, so I've made the line white. We could presumably do the reset on blur, rather than on the first mousemove where buttons==0? Try this example, the mousemoves are weird, it seems we also get a single event when blurred, (like if you have a Firefox window half covering the canvas), when the mouse enters the canvas: I can implement reset on blur, if you want. Or poke me on Discord if you like. |
|
Ok I assumed your thumbs-up meant to implement blur handling. How is this code now? I can't reproduce the issue on my machine, and it seems good on blur too. |
|
Want any more changes? What's the process for merging PRS here? |
|
Apologies, my reply did not get sent. Your submission came in a very busy period. We're working on two major features, and many PRs will be piling up until those are finalised. Once there's some time for a reviewer (realistically right now, me) to come back and give this some attention, we can check on the changes/merge. |
|
@webfiltered what timeline should I expect for it? |
2 participants
There is an issue with litegraph where clicking and dragging sometimes doesn't do the thing the user set in "Action on link release" in the settings. The cause is that the onDragEnd event handler is deleted in CanvasPointer.reset function. The reset function was called when a mousemove event is fired with no buttons held down, before the mouseUp event is fired. Since there is no guaranteed ordering of mousemove vs mouseup events, this creates a race condition. If mouseup fired before mousemove, then the correct action would happen. But if mousemove fired before mouseup, then it would reset instead.
The code right below what this PR deleted correctly handles all the cases. Unfortunately, the code would never run, because the condition on top would never allow it to run.