1340959 - Crash in res_countArrayItems_59

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect)

Description

[Nicholas Nethercote [inactive]]

This bug was filed from the Socorro interface and is 
report bp-75b24ccb-c5d3-48f7-8a7a-5dede2170219.
=============================================================

New crash, first showed up in Nightly 20170209110155, though it's infrequent enough that the root cause may have landed a couple of days before that.

I think ICU just got enabled on Android, presumably that's relevant. snorp, any ideas?

Comment 1

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

Nope, sorry, I have no knowledge of that stuff.

Comment 2

[Makoto Kato [:m_kato]]

(ICU will be turned off release build, so it is only nightly and aurora)

Comment 3

[Andrew McCreight [:mccr8]]

#4 crash on the 5-31 Android Nightly.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

We're about to turn on ICU in Android production (bug 1344625) and this crash may skyrocket in crash stats.
NI'ing :m_kato, :jfkthame and anrebargull hoping we could get some help here.

Comment 6

[Makoto Kato [:m_kato]]

My interesting is that this crash on android is Samsung and API level 17 only.

Comment 7

[Makoto Kato [:m_kato]]

(and, even if data address is valid, it will be unalignment access on some cases)

Comment 8

[Jonathan Kew [:jfkthame]]

I don't really know anything about this. Is it possible the JS engine is passing a null pointer as a locale argument to ICU somewhere it shouldn't? (In general, though, I thought NULL was permitted, meaning "default locale".)

Note that it's not unique to Android, there are also some crash reports from Windows.

Comment 9

[André Bargull [:anba]]

There are some reports (like https://crash-stats.mozilla.com/report/index/2b885441-11f1-417a-b911-9246d0170716) where we call an ICU function (ucal_openTimeZones) which doesn't accept any parameters, so I don't think this is caused by passing invalid parameters to ICU.

Comment 10

[Jonathan Kew [:jfkthame]]

True; I hadn't spotted that version of the stack. I wonder if the problem arises when we hit some kind of resource limitation (e.g. memory/address space) and a memory-allocation or resource-load within ICU fails, and isn't handled well at the immediate point of failure.

Comment 11

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

From early beta crash reports this is the #2 top crash for Fennec.

Comment 12

[Jonathan Kew [:jfkthame]]

I looked at a number of reports, and AFAICT (judging by the "telemetry environment" they show) all the ones I checked are coming from low-end phones that only have 1GB RAM. I'm guessing that's just not enough to be able to reliably load the data we need.

As of Firefox 55, we state (https://www.mozilla.org/en-US/firefox/android/55.0/system-requirements/) that 384MB RAM is required. I wonder if that's still a reasonable claim, or do we need to update it to something significantly more generous?

Comment 13

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

Jeff, should we consider changing our system requirements for Android?

Comment 14

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

Or, Joe , is this for you to decide?

Comment 15

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

Wontfix for 56 since are a week and a half away from the 56 release and the volume here is not release-blocking high. 

We should still consider changing our system requirements for the future.

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

I checked the last 10 reports and they all have memoryMB in telemetry data in the range of 816mb-880mb. This seems to confirm comment 12 hypothesis.

Comment 17

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

I think the memory thing is a red herring. Every report I've looked at for the res_countArrayItems_59 signature is on a single SoC, the Marvell PXA988. The vast majority of devices seem to be the Samsung GT-I8200N aka Galaxy S III Mini VE. Gandalf, maybe you or another ICU hacker can get ahold of one of those devices?

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

I'm not familiar with memory management in C++ at all :( Jonathan may be the right person to ask.

Comment 19

[Jonathan Kew [:jfkthame]]

I doubt I'm likely to have a chance to dig into this any time soon; I don't have a suitable device to try and reproduce or debug it here.

Comment 20

[serfreeman1337]

Hello. I have GT-I8200N Android 4.2.2

Crash always happens
* after type in password input field
* on about:downloads

Can i help to debug it somehow ?

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

Jamie, do you have a device that hits this?

Comment 22

[Marcia Knous [:marcia]]

This is the top browser crash in Fennec 57. Adding top crash keyword.

Comment 23

[Jamie Nicol [:jnicol]]

I don't, but I have submitted a servicenow request for the device with the most crashes currently, SM-T110 (galaxy tab 3 lite 7.0)

Comment 24

[Makoto Kato [:m_kato]]

No crash data on 58+.  Although we change toolchain from gcc to clang, is this compiler issue?

Comment 25

[Marcia Knous [:marcia]]

Some of the comments mention crashing while entering a password:

*Firefox crashes everytime a password is asked for 
*crashes every time I try to access Gmail account, during the entry of password 
*wont let me enter password confirmation.
*I can't sync Firefox in my Samsung Galaxy S3 mini because it crashes every time when trying to introduce the password.

Comment 26

[David Bolter [:davidb] (NeedInfo me for attention)]

Note some interesting data (pasting from "Bugzilla Crash Stop" addon)

res_countArrayItems_59 in FennecAndroid — beta

Version		57.0b9	57.0b11	57.0b13	57.0b15	58.0b3	58.0b5	58.0b7	58.0b9	58.0b11	58.0b12
Installs 	219	192	133	432	0	0	0	0	0	0
Crashes 		595	533	316	1107	0	0	0	0	0	0

What changed in 58? (fix or pref flip?)

Comment 27

[André Bargull [:anba]]

(In reply to David Bolter [:davidb] (NeedInfo me for attention) from comment #26)
> What changed in 58? (fix or pref flip?)

Hmm, interesting. Maybe it's due to the compiler change as mentioned in comment #24?

If it's not fixed by the compiler change, the signature will change in 59 to res_countArrayItems_60 (ICU update to version ICU60 in bug 1405993).

Comment 28

[Ryan VanderMeulen [:RyanVM]]

No res_countArrayItems_60 crashes ever appeared as far as I can see. Still a small trickle of crashes in 58, so I'm going to say this was fixed by the ICU update in 59.