User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0 Build ID: 20160726073904 Steps to reproduce: I was developing with a non-standard joystick (Saitek X52 Flight Control System) that had greater than 32 buttons. Since the controller is mapped in a non-standard fashion, the controller data is mapped using raw data which the Gamepad API specifications recommends. Joystick information was being accurately read into the device up 32 buttons, which appears to be a hard limit in both Firefox and Chrome. We discovered after moving the scroll wheel on the throttle control up or down that Mozilla instantly crashes. Actual results: I was able to reproduce this error consistently every time after moving the scroll wheel. This occurs both with and without the joystick drivers installed. The scroll wheel up and down correspond to button index 32 and 33 in Direct X. Expected results: When pressing a button outside of the normal 32 button range, the application should either ignore the button press or support a much larger button range without crashing.

qDot, can you take a quick look here?

Adding cleu to the ni? list, just to have an extra dev in on this. Unfortunately vjoy isn't being recognized by the browser (DInput probably) and I couldn't get it to overrun when I set things like kMaxButtons to 1, so not sure what's up yet. Can look at it again tomorrow if no one beats me to it.

Oops: https://dxr.mozilla.org/mozilla-central/rev/1576e7bc1bec7232e9e4ba78cce62526b1a6380b/dom/gamepad/windows/WindowsGamepad.cpp#44 https://dxr.mozilla.org/mozilla-central/rev/1576e7bc1bec7232e9e4ba78cce62526b1a6380b/dom/gamepad/windows/WindowsGamepad.cpp#804 That either needs to cap the button usage value at kMaxButtons or we need to dynamically allocate that array.

So that's the weird part. Notice that we cap those values right before on line 802. https://dxr.mozilla.org/mozilla-central/rev/1576e7bc1bec7232e9e4ba78cce62526b1a6380b/dom/gamepad/windows/WindowsGamepad.cpp#802 So I don't think the overflow is there. My try at a quick and dirty repro of this earlier today was to set kMaxButtons equal to 1, but even then I couldn't overflow it. Feel like I'm missing something somewhere else.

I am investigating on this issue, and I found a bug introduced in my previous patch in bug1221730. bug1292475 This bug will make gamepad api cannot send event correctly under windows. I think it is why vJoy is not recognized in the browser. I can reproduce this crash with vJoy after fix the aforementioned bug. By the stacktrace, I think it is something wrong in HandleRawInput.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3) > Oops: > https://dxr.mozilla.org/mozilla-central/rev/ > 1576e7bc1bec7232e9e4ba78cce62526b1a6380b/dom/gamepad/windows/WindowsGamepad. > cpp#44 > https://dxr.mozilla.org/mozilla-central/rev/ > 1576e7bc1bec7232e9e4ba78cce62526b1a6380b/dom/gamepad/windows/WindowsGamepad. > cpp#804 > > That either needs to cap the button usage value at kMaxButtons or we need to > dynamically allocate that array. I tried to put some boundary checking here to ensure no out-of-bound access. It does solve the crash, but it also makes some buttons dead for an ordinary gamepad. I think we can take a detail look in this part.

> That either needs to cap the button usage value at kMaxButtons or we need to > dynamically allocate that array. Ok, actually, you're right. There's that usageLength clamp, but then it's used to access the buttons array via buttons[usages[i] - 1]. So hitting button 33 still crashes it. I think we should both increase the button count and add a check.

Comment on attachment 8778444 [details] [diff] [review] Patch 1 (v1) - Allow arbitrary number of axes/buttons in windows gamepad api Review of attachment 8778444 [details] [diff] [review]: ----------------------------------------------------------------- Pretty sure there are better ways to fills nsTArrays with default values than this, but I couldn't remember what they were and just wanted to get a patch up, so please feel free to correct that. Otherwise, this runs fine against vjoy with a silly number of buttons.

Comment on attachment 8778444 [details] [diff] [review] Patch 1 (v1) - Allow arbitrary number of axes/buttons in windows gamepad api Review of attachment 8778444 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: dom/gamepad/windows/WindowsGamepad.cpp @@ +144,5 @@ > + type(aType), > + present(true) > + { > + for (uint32_t i = 0; i < numButtons; ++i) { > + buttons.AppendElement(false); Yeah, you can just call buttons.SetLength(numButtons) :)

Comment on attachment 8778444 [details] [diff] [review] Patch 1 (v1) - Allow arbitrary number of axes/buttons in windows gamepad api Review of attachment 8778444 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, with some nits fixed. ::: dom/gamepad/windows/WindowsGamepad.cpp @@ +117,4 @@ > // WindowsGamepadService::mGamepads. > int id; > > + Nit: trailing whitespace here @@ +127,5 @@ > + nsTArray<bool> buttons; > + struct axisValue { > + HIDP_VALUE_CAPS caps; > + double value; > + }; nit: Indentation

Pushed by kmachulis@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/007d1308c533 Allow arbitrary number of axes/buttons in Windows Gamepad API; r=ted r=cleu

Fixed nits, added zeroing of temp buttons array to fix stuck value issue.

Comment on attachment 8779093 [details] [diff] [review] Patch 1 (v2; final) - Allow arbitrary number of axes/buttons in windows gamepad api; r=ted r=cleu Approval Request Comment [Feature/regressing bug #]: Bug 996078 [User impact if declined]: Joysticks with > 32 buttons or axes will crash firefox on use [Describe test coverage new/current, TreeHerder]: Hand tested, still figuring out long term platform specific testing strategy [Risks and why]: None known [String/UUID change made/needed]: None

Need to see if patch will apply cleanly to beta before trying to uplift that far back.

Hello Kyle, how common is this scenario of using joysticks > 32 buttons? I understand that crashes are bad but the patch doesn't look trivial (one-liner) and since this has been a regression since 48, can this just ride the Nightly train to release in 51?

Well, the regressing patch was landed in 04/2014, so this has been around ~2.5 years and we just found it... I guess it can ride the trains.

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #18) > Well, the regressing patch was landed in 04/2014, so this has been around > ~2.5 years and we just found it... I guess it can ride the trains. I think that is definitely a safe path to take. Thanks Kyle!