Closed Bug 1307858 Opened 4 years ago Closed 4 years ago

Crash in InvalidArrayIndex_CRASH | mozilla::hal::NotifyScreenConfigurationChange

Categories

(Firefox for Android :: General, defect)

50 Branch
Unspecified
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
fennec 51+ ---
firefox50 --- wontfix
firefox51 + verified
firefox52 + fixed
firefox53 + fixed

People

(Reporter: marcia, Assigned: droeh)

Details

(Keywords: crash, topcrash-android-armv7)

Crash Data

Attachments

(1 file)

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.
Flags: needinfo?(nchen)
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 [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
Flags: needinfo?(nchen)
[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%
tracking-fennec: --- → ?
Tracking 53+ for this topcrash.
tracking 52+ for the same reason
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).
Flags: needinfo?(s.kaspari)
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[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
Assignee: nobody → droeh
Flags: needinfo?(snorp)
Attached patch Proposed patchSplinter Review
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 droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc1d94a3195d
Use a copy of the list of observers to broadcast. r=froydnj
tracking-fennec: ? → 51+
https://hg.mozilla.org/mozilla-central/rev/fc1d94a3195d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Dylan, could you fill an uplift request to aurora & beta? Thanks
Flags: needinfo?(droeh)
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]:
Flags: needinfo?(droeh)
Attachment #8819398 - Flags: approval-mozilla-beta?
Attachment #8819398 - Flags: approval-mozilla-aurora?
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!
Flags: needinfo?(mozillamarcia.knous)
Attachment #8819398 - Flags: approval-mozilla-beta?
Attachment #8819398 - Flags: approval-mozilla-beta+
Attachment #8819398 - Flags: approval-mozilla-aurora?
Attachment #8819398 - Flags: approval-mozilla-aurora+
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.
Flags: needinfo?(mozillamarcia.knous)
You need to log in before you can comment on or make changes to this bug.