Closed Bug 1307858 Opened 4 years ago Closed 4 years ago
Crash in Invalid
Array Index _CRASH | mozilla::hal::Notify Screen Configuration Change
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.
I think this will get more attention in a FxAndroid component rather than HAL.
Component: XPCOM → General
Product: Core → Firefox for Android
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 , resulting in index-out-of-bounds crashes. It's not apparent to me which code is modifying the observers list though.  https://dxr.mozilla.org/mozilla-central/rev/99a239e1866a57f987b08dad796528e4ea30e622/xpcom/glue/Observer.h#68
[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%
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).
Redirecting to snorp. This looks like a task for the mobile platform team.
Flags: needinfo?(s.kaspari) → needinfo?(snorp)
Looks like the code that actually broadcasts the change is here. 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?  https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/Observer.h#68
Assignee: nobody → droeh
This broadcasts from a copy of mObservers.
Attachment #8819398 - Flags: review?(snorp)
Attachment #8819398 - Flags: review?(snorp) → review?(nfroyd)
Comment on attachment 8819398 [details] [diff] [review] Proposed patch Review of attachment 8819398 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch!
Attachment #8819398 - Flags: review?(nfroyd) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/fc1d94a3195d Use a copy of the list of observers to broadcast. r=froydnj
tracking-fennec: ? → 51+
Dylan, could you fill an uplift request to aurora & beta? Thanks
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 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!
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.
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.
You need to log in before you can comment on or make changes to this bug.