1129632 - MOZ_ASSERT(aIndex < Length(), "invalid array index"); coming from widget/gonk/GeckoTouchDispatcher.cpp:250

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect)

Description

[Gregor Wagner [:gwagner]]

Program received signal SIGSEGV, Segmentation fault.
0xb46d9518 in nsTArray_Impl<mozilla::SingleTouchData, nsTArrayInfallibleAllocator>::ElementAt (this=this@entry=0xbecb2728, aIndex=aIndex@entry=1) at ../../dist/include/nsTArray.h:936
936	    MOZ_ASSERT(aIndex < Length(), "invalid array index");
(gdb) bt
#0  0xb46d9518 in nsTArray_Impl<mozilla::SingleTouchData, nsTArrayInfallibleAllocator>::ElementAt (this=this@entry=0xbecb2728, aIndex=aIndex@entry=1) at ../../dist/include/nsTArray.h:936
#1  0xb505aae4 in operator[] (aIndex=1, this=0xbecb2718) at ../../dist/include/nsTArray.h:971
#2  ResampleTouch (aTouchDiff=..., aFrameDiff=..., aCurrent=..., aBase=..., aOutTouch=...) at ../../../widget/gonk/GeckoTouchDispatcher.cpp:250
#3  mozilla::GeckoTouchDispatcher::ResampleTouchMoves (this=this@entry=0xb0970d60, aOutTouch=..., aVsyncTime=...) at ../../../widget/gonk/GeckoTouchDispatcher.cpp:322
#4  0xb505ad58 in mozilla::GeckoTouchDispatcher::DispatchTouchMoveEvents (this=0xb0970d60, aVsyncTime=...) at ../../../widget/gonk/GeckoTouchDispatcher.cpp:207
#5  0xb505ad90 in mozilla::DispatchTouchEventsMainThread::Run (this=0xac1916c0) at ../../../widget/gonk/GeckoTouchDispatcher.cpp:88
#6  0xb4213c24 in nsThread::ProcessNextEvent (this=0xb6afe320, aMayWait=<optimized out>, aResult=0xbecb2837) at ../../../xpcom/threads/nsThread.cpp:855
#7  0xb4228bcc in NS_ProcessNextEvent (aThread=0xb6afe320, aMayWait=aMayWait@entry=false) at /Volumes/2mac/moz/ib2g/xpcom/glue/nsThreadUtils.cpp:265
#8  0xb43de308 in mozilla::ipc::MessagePump::Run (this=0xb25c3160, aDelegate=0xb25bb1a0) at ../../../ipc/glue/MessagePump.cpp:99
#9  0xb43ca1a8 in MessageLoop::RunInternal (this=this@entry=0xb25bb1a0) at ../../../ipc/chromium/src/base/message_loop.cc:233
#10 0xb43ca1c2 in RunHandler (this=0xb25bb1a0) at ../../../ipc/chromium/src/base/message_loop.cc:226
#11 MessageLoop::Run (this=0xb25bb1a0) at ../../../ipc/chromium/src/base/message_loop.cc:200
#12 0xb504dd66 in nsBaseAppShell::Run (this=0xb2774340) at ../../widget/nsBaseAppShell.cpp:164
#13 0xb548a0ba in nsAppStartup::Run (this=0xb277fa60) at ../../../../toolkit/components/startup/nsAppStartup.cpp:281
#14 0xb54a7b36 in XREMain::XRE_mainRun (this=this@entry=0xbecb29c0) at ../../../toolkit/xre/nsAppRunner.cpp:4160
#15 0xb54a7d34 in XREMain::XRE_main (this=this@entry=0xbecb29c0, argc=argc@entry=1, argv=argv@entry=0xb6a2b190, aAppData=aAppData@entry=0xb6f24858 <_ZL8sAppData>) at ../../../toolkit/xre/nsAppRunner.cpp:4236
#16 0xb54a7eae in XRE_main (argc=1, argv=0xb6a2b190, aAppData=0xb6f24858 <_ZL8sAppData>, aFlags=<optimized out>) at ../../../toolkit/xre/nsAppRunner.cpp:4456
#17 0xb6f09126 in do_main (argc=argc@entry=1, argv=argv@entry=0xb6a2b190) at ../../../b2g/app/nsBrowserApp.cpp:167
#18 0xb6f09262 in b2g_main (argc=argc@entry=1, argv=argv@entry=0xbecb3c94) at ../../../b2g/app/nsBrowserApp.cpp:299
#19 0xb6f08f90 in RunProcesses (aReservedFds=..., argv=0xbecb3c94, argc=1) at ../../../b2g/app/B2GLoader.cpp:225
#20 main (argc=1, argv=0xbecb3c94) at ../../../b2g/app/B2GLoader.cpp:290
(gdb) 

Seems like the winner is mason here :)

Comment 1

[Gregor Wagner [:gwagner]]

STR: Current trunk debug build on flame.
Open maps.google.com and try to zoom.

Comment 2

[Mason Chang [Inactive] [:mchang]]

Attachment: Ensure we only resample the minimum number of touch events

We had an array out of bounds error here. (https://dxr.mozilla.org/mozilla-central/source/widget/gonk/GeckoTouchDispatcher.cpp?from=GeckoTouchDispatcher.cpp&case=true#249).

We look at two different touch events, but if the aCurrent MultiTouchEvent had multiple more touch events than the aBase MultiTouchEvent, we would read past the aBase.mTouches array. This would occur during finger transitions, e.g. had 1 finger down then add another or have 2 fingers down and remove one. 

This patch ensures we only resample the minimum number of touches across all MultiTouchEvents.

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: Ensure we only resample the minimum number of touch events

Comment 4

[Michael Wu [:mwu]]

Comment on attachment 8559421 [details] [diff] [review]
Ensure we only resample the minimum number of touch events

Review of attachment 8559421 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +224,5 @@
>  static const SingleTouchData&
>  GetTouchByID(const SingleTouchData& aCurrentTouch, MultiTouchInput& aOtherTouch)
>  {
>    int32_t index = aOtherTouch.IndexOfTouch(aCurrentTouch.mIdentifier);
> +  MOZ_ASSERT(index >= 0);

Don't see how this assertion is valid. If the base touch had two points, and the first point was lifted, the identifier from the lifted point isn't going to match anything in the current touch.

Comment 5

[Mason Chang [Inactive] [:mchang]]

Attachment: Resample touch events based on the current touch

Actually something easier. Resample based on the current touch event and look for the corresponding touch in the base touch event.

Comment 6

[Michael Wu [:mwu]]

Comment on attachment 8559508 [details] [diff] [review]
Resample touch events based on the current touch

Review of attachment 8559508 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent.

Comment 7

[Mason Chang [Inactive] [:mchang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/4f1b69be79dd

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4f1b69be79dd

Comment 9

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8559508 [details] [diff] [review]
Resample touch events based on the current touch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Touch resampling, bug 1062331
User impact if declined: The phone could crash due to reading invalid memory.
Testing completed: Manual testing to verify the fix.
Risk to taking this patch (and alternatives if risky): Low, this corrects an invalid array access.
String or UUID changes made by this patch: None

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/462eda583dba