1307858 - Crash in InvalidArrayIndex_CRASH | mozilla::hal::NotifyScreenConfigurationChange

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-1b633168-a7d2-4941-8b55-5a1352161005.
=============================================================

Seen while looking at Beta crash data: http://bit.ly/2dxUDoo. Crash affects 51 and 52 but in small volume.

One comment mentions "Trying to use airtasker website"

ni on jchen for any ideas. I probably have this bucketed in the wrong component.

Comment 1

[Benjamin Smedberg]

I think this will get more attention in a FxAndroid component rather than HAL.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Seems like some code is modifying the screen orientation observers list, while screen change notifications are being broadcasted to those observers. ObserverList does not support modification during broadcasting [1], resulting in index-out-of-bounds crashes. It's not apparent to me which code is modifying the observers list though.

[1] https://dxr.mozilla.org/mozilla-central/rev/99a239e1866a57f987b08dad796528e4ea30e622/xpcom/glue/Observer.h#68

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: topcrash in 51+

Mfg      Model     And Ver       ABI       #   
samsung  SM-G930F  23 (REL)  armeabi-v7a  22  3.8%
huawei   ALE-L21   23 (REL)  armeabi-v7a  20  3.5%
samsung  SM-G920F  23 (REL)  armeabi-v7a  17  3.0%
samsung  SM-G935F  23 (REL)  armeabi-v7a  16  2.8%
samsung  SM-G900F  21 (REL)  armeabi-v7a  13  2.3%

Comment 4

[Marcia Knous [:marcia]]

Tracking 53+ for this topcrash.

Comment 5

[Julien Cristau [:jcristau]]

tracking 52+ for the same reason

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Sebastian, can you help find an owner for this bug? Is there anything actionable here? This signature is  1.75% of reported crashes for fennec in 50.0.2 (over 8000 crashes in the last week).

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Redirecting to snorp. This looks like a task for the mobile platform team.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Looks like the code that actually broadcasts the change is here[0]. My guess is that one of the observers then removes itself from the list in the callback, and we blow up. Probably best to just copy the list during the broadcast. Dylan, can you take care of that?

[0] https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/Observer.h#68

Comment 9

[Dylan Roeh (:droeh) (he/him)]

Attachment: Proposed patch

This broadcasts from a copy of mObservers.

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8819398 [details] [diff] [review]
Proposed patch

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

Thanks for the patch!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc1d94a3195d
Use a copy of the list of observers to broadcast. r=froydnj

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Dylan, could you fill an uplift request to aurora & beta? Thanks

Comment 14

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8819398 [details] [diff] [review]
Proposed patch

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: Potential for crashes.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: Very low-risk
[Why is the change risky/not risky?]: Simply copies the observers array before using it, so modifications to the original will not affect the loop.
[String changes made/needed]:

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8819398 [details] [diff] [review]
Proposed patch

Fix for a fennec topcrash in 50.1 (~10K crashes/week). Let's try it out for 51 beta 11. 
Marcia, can you check back on this crash next week? Thanks!

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0eeed34e22e4

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fed1d53a9e04

Comment 18

[u549602]

No crashes were encountered in today's beta testing session (51.0b11 / January 4th, 2017). 
I will keep this issue opened until we have more crash stats information after the build gets to more users, post playstore launch.

Comment 19

[Marcia Knous [:marcia]]

Crash-stats shows the last beta this crash appeared in was 51.0b9, so it appears as if the crash is no longer occuring on beta.