Crash in InvalidArrayIndex_CRASH | mozilla::hal::NotifyScreenConfigurationChange

RESOLVED FIXED in Firefox 51

Status

()

Firefox for Android
General
--
critical
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: marcia, Assigned: droeh)

Tracking

({crash, topcrash-android-armv7})

50 Branch
Firefox 53
Unspecified
Android
crash, topcrash-android-armv7
Points:
---

Firefox Tracking Flags

(fennec51+, firefox50 wontfix, firefox51+ verified, firefox52+ fixed, firefox53+ fixed)

Details

(crash signature)

Attachments

(1 attachment)

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)

Comment 1

2 years ago
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: --- → ?
status-firefox53: --- → affected
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
Keywords: topcrash-android-armv7
Tracking 53+ for this topcrash.
tracking-firefox53: ? → +
tracking 52+ for the same reason
tracking-firefox52: ? → +
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).
tracking-firefox51: ? → +
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)
(Assignee)

Comment 9

a year ago
Created attachment 8819398 [details] [diff] [review]
Proposed patch

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+

Comment 11

a year ago
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+

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fc1d94a3195d
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Dylan, could you fill an uplift request to aurora & beta? Thanks
status-firefox50: affected → wontfix
Flags: needinfo?(droeh)
(Assignee)

Comment 14

a year ago
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+

Comment 16

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/0eeed34e22e4
status-firefox52: affected → fixed

Comment 17

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/fed1d53a9e04
status-firefox51: affected → fixed
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)
status-firefox51: fixed → verified
You need to log in before you can comment on or make changes to this bug.