1870379 - Crash in [@ mozilla::detail::InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | nsTArray_Impl<T>::operator[] | mozilla::dom::Gamepad::SetButton]

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P3)

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/a03ec71f-d449-43d3-81a1-e79ac0231215

Reason: EXCEPTION_BREAKPOINT

Top 10 frames of crashing thread:

0  mozglue.dll  MOZ_Crash  mfbt/Assertions.h:281
0  mozglue.dll  mozilla::detail::InvalidArrayIndex_CRASH  mfbt/Assertions.cpp:50
1  xul.dll  nsTArray_Impl<RefPtr<mozilla::dom::GamepadButton>, nsTArrayInfallibleAllocator>::ElementAt  xpcom/ds/nsTArray.h:1208
1  xul.dll  nsTArray_Impl<RefPtr<mozilla::dom::GamepadButton>, nsTArrayInfallibleAllocator>::operator[]  xpcom/ds/nsTArray.h:1245
1  xul.dll  mozilla::dom::Gamepad::SetButton  dom/gamepad/Gamepad.cpp:88
2  xul.dll  mozilla::dom::GamepadManager::SetGamepadByEvent  dom/gamepad/GamepadManager.cpp:540
3  xul.dll  mozilla::dom::GamepadManager::Update  dom/gamepad/GamepadManager.cpp:466
4  xul.dll  mozilla::dom::  dom/gamepad/ipc/GamepadEventChannelChild.cpp:21
5  xul.dll  mozilla::RunnableTask::Run  xpcom/threads/TaskController.cpp:549
5  xul.dll  mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal  xpcom/threads/TaskController.cpp:876

The volume isn't super high here but I figured I'd file it in case there was something useful to be done. It looks like the button value passed in to SetButton does not fall within the mButtons array. I don't see the array bounds crash annotation which is odd.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:cmartin, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[Chris Martin [:cmartin]]

Might be worth investigating. There is a known synchronization issue on Mac OS X where the gamepad monitor thread will continue to deliver events after shutdown was requested because the requesting thread doesn't actually wait() on the shutdown to complete before it deletes all its state objects. Normally, this is not a huge problem since the GamepadManager will just silently ignore events if it's already deleted everything; however, in some cases it can do weird things... This may be one of them.

It has been on my todo list forever to fix this, but there's just never any time to work on gamepad stuff these days.

Comment 3

[Dianna Smith [:diannaS]]

:cmartin could you take a look at this? looks like this might have started again after bug 1898774 landed in 140.

Comment 4

[Chris Martin [:cmartin]]

I am currently investigating this. I may not be able to fix it easily, but I'll at least see if I can add a check in the meantime.

Comment 5

[Chris Martin [:cmartin]]

Attachment: Bug 1870379 - Fix assertions from OOB gamepad array accesses

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b23aa50c2d7a
https://hg.mozilla.org/integration/autoland/rev/beb0fab27e10
Fix assertions from OOB gamepad array accesses r=gsvelto

Comment 7

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/beb0fab27e10

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:cmartin, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox142 to wontfix.

For more information, please visit BugBot documentation.

Comment 9

[Chris Martin [:cmartin]]

Comment on attachment 9505559 [details]
Bug 1870379 - Fix assertions from OOB gamepad array accesses

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Users will continue to experience these random out-of-bounds crashes. It's very much not urgent, but it's also so low-risk that we might as well uplift it.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It just adds a bounds check where there wasn't one. There's practically zero potential for it to do anything negative. Worst case is that it only fixes some of the problem.
• String changes made/needed:
• Is Android affected?: Yes

Comment 10

[Dianna Smith [:diannaS]]

Comment on attachment 9505559 [details]
Bug 1870379 - Fix assertions from OOB gamepad array accesses

We are out of 142 betas and currently in RC week.
Moving request to release for the upcoming dot release or in case of an rc rebuild.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9505559 [details]
Bug 1870379 - Fix assertions from OOB gamepad array accesses

Approved for 140.3esr.

Comment 12

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/b5307c1d1b39

Comment 13

[Dianna Smith [:diannaS]]

Comment on attachment 9505559 [details]
Bug 1870379 - Fix assertions from OOB gamepad array accesses

Approved for 142.0.1

Comment 14

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/859f96e4e17f
https://hg.mozilla.org/releases/mozilla-release/rev/32456559b41a