New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Git extensions crash when using the mouse wheel #5621

Open
Noctis- opened this Issue Oct 25, 2018 · 29 comments

Comments

Projects
None yet
7 participants
@Noctis-

Noctis- commented Oct 25, 2018

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
Git extension will crash when using the mouse wheel

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem.
Whenever i use the mouse wheel in 2.51 or 3.0 beta, it happens for me.

What is the expected behavior?
Git extensions scrolling without crashing :)

Environment you encounter the issue:

  • GitExtensions version: GitExtensions.exe [2.51.05] (also happens on the 3 Beta)
  • GIT version: 2.19.1.windows.1
  • OS version: windows 10 Version 1709 (OS Build 16299.726)
  • .NET version: 4.7.2

** Extra Info / hardware details **
MSI GS65 laptop
currently using a Logitch Pro mouse (logitech gaming software Version 9.02.65 installed)
Other thoughts / variables: I've probably installed git and extensions via chocolatey.

Did this work in previous version of GitExtensions (which)?
Must have, because i've never had this problem before (reinstalled windows last week, seems to be happening since)

** Exception and stack trace: **

System.OverflowException
Arithmetic operation resulted in an overflow.

   at GitUI.MouseWheelRedirector.PreFilterMessage(Message& m)
   at System.Windows.Forms.Application.ThreadContext.ProcessFilters(MSG& msg, Boolean& modified)
   at System.Windows.Forms.Application.ThreadContext.PreTranslateMessage(MSG& msg)
   at System.Windows.Forms.Application.ComponentManager.System.Windows.Forms.UnsafeNativeMethods.IMsoComponentManager.FPushMessageLoop(IntPtr dwComponentID, Int32 reason, Int32 pvLoopData)
   at System.Windows.Forms.Application.ThreadContext.RunMessageLoopInner(Int32 reason, ApplicationContext context)
   at System.Windows.Forms.Application.ThreadContext.RunMessageLoop(Int32 reason, ApplicationContext context)
   at System.Windows.Forms.Form.ShowDialog(IWin32Window owner)
   at GitUI.GitUICommands.<>c__DisplayClass332_0.<StartCommitDialog>b__0()
   at GitUI.GitUICommands.DoActionOnRepo(IWin32Window owner, Boolean requiresValidWorkingDir, Boolean changesRepo, GitUIEventHandler preEvent, GitUIPostActionEventHandler postEvent, Func`1 action)
   at GitUI.GitUICommands.StartCommitDialog(IWin32Window owner, Boolean showOnlyWhenChanges)
   at GitUI.GitUICommands.Commit(Dictionary`2 arguments)
   at GitExtensions.Program.Main()
@RussKie

This comment has been minimized.

Member

RussKie commented Oct 25, 2018

@Noctis-

This comment has been minimized.

Noctis- commented Oct 25, 2018

Happened all the time today, whenever i've scrolled.
Open repository -> hover over the path, tried scrolling to get the next one -> crash
Open browse repo -> scrolled over commits -> crash
Open browse -> click to a diff , drag the scrollbar, works, use mouse scroll wheel -> crash

@Noctis-

This comment has been minimized.

Noctis- commented Oct 25, 2018

Talked to a guy on my team, took his mouse (steel series 110 i think), plugged it in ... no crashes with his mouse.
Tried mine, no crashes.
unplugged his mouse, mine still is happy (in v 2.51). gonna try the 3 as well ... and maybe give the machine a restart to see how it behaves after one ...

@Noctis-

This comment has been minimized.

Noctis- commented Oct 25, 2018

worked in 3 beta as well ... restarted the machine ... seems to be working without a hitch now !?

I do have a 24 MB .dmp file in my crashdump if it's any help ... ?

@RussKie

This comment has been minimized.

Member

RussKie commented Oct 25, 2018

@drewnoakes

This comment has been minimized.

Member

drewnoakes commented Oct 25, 2018

@Noctis- what are the UI scaling settings in play here? Are you or your colleague running at other than 100% scaling?

@Noctis-

This comment has been minimized.

Noctis- commented Oct 25, 2018

Nope. No scaling applied.

Laptop screen 1920x1080 , 100% scale.
Dell runs 2560x1600, 100% scale

SSMS usually throws a fit as well ..had to disable hardware acceleration on it for it to look ok...

@vbjay

This comment has been minimized.

Contributor

vbjay commented Oct 25, 2018

@Noctis-

This comment has been minimized.

Noctis- commented Oct 25, 2018

@vbjay the SSMS , might be ... but i doubt this one is ...

if it walks like a mouse, and it talks like a mouse ... :)

@vbjay

This comment has been minimized.

Contributor

vbjay commented Oct 25, 2018

@Noctis-

This comment has been minimized.

Noctis- commented Oct 25, 2018

@vbjay fair dinkum. I'll attach a screenshot of what SSMS used to do. it also had some flashy gradient color issues, but it seems it's a know issue with hi-res monitors (the dev who pointed me to the fix also has a big monitor).

I doubt the vid drivers are related TBH, but maybe. This is a new install of the machine, with all the latest drivers BTW.

When SSMS goes wild

@RussKie

This comment has been minimized.

Member

RussKie commented Oct 26, 2018

@Noctis-

This comment has been minimized.

Noctis- commented Oct 30, 2018

Ah ha !

so, open GitExt , it fires on my laptop screen (1920x1080), i scroll happily ever after, no problem.
I then move it to the big screen (2560x1600), try to scroll -> exception + force closed.

It happens if i move it fullscreen (Windwos+right/left), or if i move it when it's in a window (so, same res/size).

Dunno if this helps much, but at least i have more info at least :)

@Noctis-

This comment has been minimized.

Noctis- commented Oct 30, 2018

More digging:

Only happens with the Display Port cable.

Connected display with HDMI , max resolution supported was 1920x1200. worked on both screens, no issues when scrolling.

Plugged back with DP cable, used same resolution 1920x1200 -> crash and burn.

So, it's not the resolution per se, but the fact that the nvidia graphic card is using the DP (mini DP tbh).

I've tried to see if i can use the external screen with the Intel gfx card, but it defaults to the nvidia one.

Here are the screen / graphic card details:

Monitor 2
Name DELL 3008WFP on NVIDIA GeForce GTX 1070 with Max-Q Design
Current Resolution 2560x1600 pixels
Work Resolution 2560x1560 pixels
State Enabled
Multiple displays Extended, Secondary, Enabled
Monitor Width 2560
Monitor Height 1600
Monitor BPP 32 bits per pixel
Monitor Frequency 60 Hz
Device \.\DISPLAY4\Monitor0

NVIDIA GeForce GTX 1070 with Max-Q Design
Manufacturer NVIDIA
Model GeForce GTX 1070 with Max-Q Design
Device ID 10DE-1BA1
Revision A2
Subvendor MSI (1462)
Current Performance Level Level 0
Bus Interface PCI Express x16
Temperature 48 °C
Driver version 25.21.14.1634
BIOS Version 86.04.8a.00.88
Memory 4095 MB
Count of performance levels : 1
Level 1 - "Perf Level 0"

Nvidia driver version: 416.34

Any point in contacting Nvidia ?

@Noctis-

This comment has been minimized.

Noctis- commented Oct 30, 2018

And here's a screenshot from the dmp file. Apparently it tries to read from a virtual address without the appropriate access

dmp screenshot

@RussKie

This comment has been minimized.

Member

RussKie commented Oct 30, 2018

Thank you for following it up.

@RussKie

This comment has been minimized.

Member

RussKie commented Oct 30, 2018

Any point in contacting Nvidia ?

I suppose it won't hurt.

@Noctis-

This comment has been minimized.

Noctis- commented Oct 30, 2018

Got in touch with Nvidia, basically said: Make sure you're using your laptop provider latest drivers ... which i thought might be 398, but apparently for my windows version are 391 ... so ... i'll need to go driver uninstalling / installing to see if it still happens ... might happen when i get some time ...

@Noctis-

This comment has been minimized.

Noctis- commented Nov 26, 2018

@RussKie my oh my ... It might not be nvidia after all ...

I didn't give it much thought since i've been busy and it's a bit of an end case, but it seems that it will only crash under the above conditions when StrokesPlus is running. It's a mouse gesture application so, half makes sense to me in hindsight (the original issue suggested mouse).

still no clue why it's only with git extensions, and only on second monitor, but I'll give the developer a ping see if he has any ideas :)

@RussKie

This comment has been minimized.

Member

RussKie commented Nov 26, 2018

@roblarky

This comment has been minimized.

roblarky commented Nov 27, 2018

Hi,

Developer of StrokesPlus here. I instructed Noctis to disable a preference (Enable Mouse Wheel Relay) that was intended for older versions of Windows before it would allow the mouse wheel to scroll over inactive windows. This resolved his issue.

For whatever it's worth, the (simplified) offending code in my app is below; it's just posting the mouse wheel message to the control below the mouse cursor.

MSLLHOOKSTRUCT *hookStruct = (MSLLHOOKSTRUCT*)lParam; //lParam as passed into the LowLevelMouseProc hook by Windows
CURSORINFO CursorInfo;
GetCursorInfo(&CursorInfo);
HWND hwndControlBelowMouse = WindowFromPoint(CursorInfo.ptScreenPos);
DWORD fwKeys = 0;
//The checks below are to ensure any keystates are sent along with the mouse message
if((GetKeyState(VK_CONTROL) & 0x8000) != 0){
	fwKeys = fwKeys | MK_CONTROL;
}
if((GetKeyState(VK_SHIFT) & 0x8000) != 0){
	fwKeys = fwKeys | MK_SHIFT;
}
if((GetKeyState(VK_LBUTTON) & 0x8000) != 0){
	fwKeys = fwKeys | MK_LBUTTON;
}
if((GetKeyState(VK_MBUTTON) & 0x8000) != 0){
	fwKeys = fwKeys | MK_MBUTTON;
}
if((GetKeyState(VK_RBUTTON) & 0x8000) != 0){
	fwKeys = fwKeys | MK_RBUTTON;
}		
if((GetKeyState(VK_XBUTTON1) & 0x8000) != 0){
	fwKeys = fwKeys | MK_XBUTTON1;
}
if((GetKeyState(VK_XBUTTON2) & 0x8000) != 0){
	fwKeys = fwKeys | MK_XBUTTON2;
}	
PostMessage(hwndControlBelowMouse, WM_MOUSEWHEEL, MAKEWPARAM(fwKeys,GET_WHEEL_DELTA_WPARAM(hookStruct->mouseData)), MAKELPARAM(CursorInfo.ptScreenPos.x,CursorInfo.ptScreenPos.y));

Based on the exception (Arithmetic operation resulted in an overflow) and glancing at the MouseWheelRedirector code, the only culprit I can think of is:

var pos = new Point(m.LParam.ToInt32());

I'm assuming he's running the 64-bit version of StrokesPlus and the LParam may be sent as such (I haven't tried to repro/debug, FYI). A quick search on the exception and ToInt32 leads to a StackOverflow post about buffers and 32/64-bit process issues. It may be that while Windows will ensure a 32-bit app only receives 32-bit mouse messages, my app is directly posting the message. Or perhaps your app also runs in 64-bit mode and it's really just a matter of improper casting to Int32.

Just some thoughts, I don't think this is really worth anyone spending their time on, nor am I even certain my assumptions are correct. But I figured I could at least bring closure to this and provide whatever extra info I have.

Rob

@RussKie

This comment has been minimized.

Member

RussKie commented Nov 27, 2018

Thank you Rob, much appreciated!

@NikolayXHD you're our WinAPI guru, thoughts?

@NikolayXHD

This comment has been minimized.

Contributor

NikolayXHD commented Nov 27, 2018

@RussKie I conducted an NUnit test which proves @roblarky is right by pointing us at .ToInt32()

[Test]
public void ArythmeticTest()
{
	var val = new IntPtr(0x00000000FFFFFFFF);
	Assert.Throws<OverflowException>(() => val.ToInt32());
}

The test passes which means we should improve the code where we convert LParam to Point more or less like suggested here:

//before
var pos = new Point(m.LParam.ToInt32());

//after
int x = unchecked((short)(long)m.LParam);
int y = unchecked((short)((long)m.LParam >> 16));
var pos = new Point(x, y);
@Noctis-

This comment has been minimized.

Noctis- commented Nov 28, 2018

@vbjay

This comment has been minimized.

Contributor

vbjay commented Nov 28, 2018

image

We use anucpu and do not prefer 32 bit. So if your machine is a 64 bit box, GE is 64 bit.

@Noctis-

This comment has been minimized.

Noctis- commented Nov 28, 2018

You're correct @vbjay . both GitExt & StorkesPlus are running as 64.

@vbjay

This comment has been minimized.

Contributor

vbjay commented Nov 28, 2018

Yes. So the issue is that intptr size varies on 64 vs 32 bit. Someone assumed with ToInt32.

@RussKie

This comment has been minimized.

Member

RussKie commented Nov 28, 2018

@NikolayXHD

This comment has been minimized.

Contributor

NikolayXHD commented Nov 28, 2018

yes, more or less by the weekend

@RussKie RussKie added this to the 3.0.1 milestone Nov 29, 2018

@gerhardol gerhardol modified the milestones: 3.00.01+3.01.00, 3.01.00 Dec 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment