1373964 - Startup Crash in java.lang.NullPointerException: Attempt to get length of null array at org.mozilla.gecko.GeckoApp$DeferredCleanupTask.run(GeckoApp.java)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Kevin Brosnan [Ex-Mozilla]]

This bug was filed from the Socorro interface and is 
report bp-71f0481c-7e49-4070-8c0d-85bd60170616.
=============================================================

Only happens on specific devices.

Mfg.        Model             And. API    CPU ABI    #      
agm      A8                   24 (REL)  armeabi-v7a  86  54.4%
hisense  Hisense C30          24 (REL)  armeabi-v7a  34  21.5%
hisense  Hisense F32          25 (REL)  armeabi-v7a  27  17.1%
kruger   Kruger&Matz Drive 5  24 (REL)  armeabi-v7a  11  7.0%
matz     Kruger&Matz Drive 5  24 (REL)  armeabi-v7a  11  7.0%

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Reported from https://support.mozilla.org/en-US/questions/1164460

Comment 2

[Jeroen Tietema]

My AGM A8 has this issue as well. I you need help testing fixes, I am happy to help out.

Comment 3

[Wesly Huang (EPM)]

[triage@july 5th] may worth look into it in the future, while monitoring if this is seen in more (&popular) devices. set P2 for now.

Comment 4

[Julian Chu [:walkingice]]

Wesly updated the importance, set tracking flag as well

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

[triage] #50 top crasher in 58.0.1 release: 119 crashes in 7 days. Will set non-critical and revive it by triaging top crashers if it's an issue.

Comment 6

[runerd]

Quick note:
On my AGM X2, all versions from 50.0 crash after 16 seconds. In 60.0a1 nightly from GPlay (build 20180217100057) I saw it crashed in GeckoApp.java:2228 (report bp-8a0e260a-c64b-46b1-bf4e-412de0180217). Managed to reproduce this in my own build of nightly. Then I changed ./mobile/android/base/java/org/mozilla/gecko/GeckoApp.java, line 2221 from "if (cleanupVersion < 1)" to "if (cleanupVersion < 0)", just to see if this would force it to never get to line 2228. Recompiled - Voila, NO more crashes!
Disclaimer: This is my first post, sorry in advance for breaking any netiquette here. I'm no developer, just desperate getting FF working on my X2. My code change will probably lead to Something Bad, eventually :)

Comment 7

[runerd]

61.0a1 nightly 2018-03-12 not crashing on my AGM X2 so this has maybe been fixed?

Comment 8

[db]

61.0a1 nightly 2018-03-15 still crashing on my AGM X2 

Comment 9

[runerd]

Can confirm it is not fixed. My comment 7 was about an upgrade of my "fixed" version in comment 6. After I clear app data it crashes again. Sorry for the confusion.

Comment 10

[runerd]

So bp-11eee922-df3e-4795-ba6b-250410180317 stack trace says:
"java.lang.NullPointerException: Attempt to get length of null array" at GeckoApp.java:2258.

Looks like the dir.listFiles() in line 2258 returns null? Maybe a problem with permissions on the "res/fonts" directory when there is no existing appdata stored?

So I added a test for null with a change in line 2257, from this:
if (dir.exists() && dir.isDirectory()) {
to this:
if (dir.exists() && dir.isDirectory() && dir.listFiles() != null) {

No crash so this works. Hopefully a nicer fix than comment 6.

Comment 11

[Padacore]

Hi There,
Do you notice that FF, FF beta, FF nightly crash every time but not FF focus / klar ?
https://crash-stats.mozilla.com/report/index/994a03db-0e23-44b9-87d8-a4c2b0180324
https://support.mozilla.org/en-US/questions/1198709
Maybe this can help you

Comment 12

[runerd]

Hi Padacore! Can confirm that Focus does not crash. I find it too stripped down to be of any use for me and I'm covered anyway by my own fixed build of nightly. But thank you for the suggestion! :)

Comment 13

[Kevin Brosnan [Ex-Mozilla]]

Focus and Klar are irrelevant they currently use the Android WebKit/Blink engine (this may change in the future). Whereas Firefox for Android uses Gecko.

Comment 14

[db]

Runerd, any chance you could upload an APK somewhere for those of us tired of waiting for Mozilla?
I'd sure appreciate it ☺️

Comment 15

[runerd]

Hi db! I don't think it's a good idea for me to distribute my builds. But anyone can build their own version! :) I made a howto here: https://pastebin.com/zzd8mpCs

Comment 16

[Kevin Brosnan [Ex-Mozilla]]

Comment 6 has a proposed fix. Mike does this make sense?

Comment 17

[runerd]

I think comment 6 should be ignored as this will prevent the clean-up routine of .ttf files from running at all. Comment 10 on the other hand adds an extra test to check that the dir.listFiles() does not return null, which prevents the for-loop from running on a null-array (and crash).

Comment 18

[runerd]

Been playing around more with GeckoApp.java:
https://dxr.mozilla.org/mozilla-beta/source/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java

I think the first problem is this test in the cleanup routine:
if (dir.exists() && dir.isDirectory()) {

On my AGM X2 this returns TRUE.
On two Samsung phones I have this returns FALSE (so they won't get to the point where the for-loop runs on a null array).

Prior to this the directory is defined as "res/fonts":
File dir = new File("res/fonts");

By using dir.getAbsolutePath() I managed to write the absolute path to a text file in the Fennec data directory and then look at this in Firefox by pointing it to /data/data/org.mozilla.fennec_user/file.txt
The result is this on all my phones:
/res/fonts

So what does this mean? Maybe the /res/fonts/ folder exists by default on the devices in question, AGM etc? Even if this is outside where the Fennec app has access, Android is for some reason happy to return: "Yes, /res/fonts exists, and yes, it is a directory".

But then it gets here:
for (File file : dir.listFiles()) {

The dir.listFiles() array returns null because Fennec is actually not allowed to list files in that part of the file system, and the for-statement crashes.

Does this sound plausible?

Comment 19

[runerd]

To test theory in comment 18:
Installed Cyanogenmod 14.1 (Android 7.1) on a Samsung A5. Enabled root access. Created /res/fonts/ directory. Disabled root access. Installed Firefox 59.0.2 APK from Mozilla ftp. This crashes the same way as on my AGM X2: bp-aca497e5-f853-42b1-aa99-eb8ad0180408

And as a second test: Changed dir directory in GeckoApp.java to "res/asdf" which is highly unlikely to exist on any phone. This build does NOT crash on my AGM X2 or SamsungA5/CM14.1.

Conclusion: Firefox will crash if directory /res/fonts/ exists.

So why is Firefox trying to access the root file system at all? Shouldn't it have stayed within /data/data/org.mozilla.firefox/ and add "res/fonts" to this? Are Android apps supposed to prefix all file operations with the its data dir, and this is missing in the cleanup routine in GeckoApp.java?

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

> The dir.listFiles() array returns null because Fennec is actually not allowed to
> list files in that part of the file system, and the for-statement crashes.
>
> Does this sound plausible?

Yep!

> Conclusion: Firefox will crash if directory /res/fonts/ exists.

> So why is Firefox trying to access the root file system at all? Shouldn't it have stayed within /data/data/org.mozilla.firefox/ 
> and add "res/fonts" to this? Are Android apps supposed to prefix all file operations with the its data dir, and this is missing 
> in the cleanup routine in GeckoApp.java?

Yep! Your analysis makes sense to me! Thanks for helping out.

From bug 878674 comment 0, it also looks like we're supposed to be looking at res/fonts in the data dir: I guess the original implementer did not do this correctly.

runerd, if you wanted to submit a patch to fix this issue, I'd be happy to review it! Please add a needinfo flag on me if not so I can get someone else to put forward a patch.

Comment 21

[runerd]

(In reply to Michael Comella (:mcomella) from comment #20)
> runerd, if you wanted to submit a patch to fix this issue, I'd be happy to
> review it! Please add a needinfo flag on me if not so I can get someone else
> to put forward a patch.
Thank you Michael! Really wish I could help by submitting a patch, but I think it's best to excuse myself and leave it to you experts. But I'll be happy of course to do testing once it finds its way into nightly!

One last note: Just to confirm the existence of the /res/fonts/ folder on my (non-rooted) AGM phone I figured out how to use USB debugging and adb:
~$ adb shell getprop | grep model
[ro.hmct.internal.model]: [AGM X2]
[ro.product.model]: [AGM X2]

~$ adb shell ls -ld /res
drwxr-xr-x 4 root root 0 1970-01-01 01:00 /res

~$ adb shell ls -l /res
total 0
drwxr-xr-x 2 root root 0 1970-01-01 01:00 fonts
drwxr-xr-x 3 root root 0 1970-01-01 01:00 images

~$ adb shell ls /res/fonts
ls: /res/fonts/Clockopia.ttf: Permission denied

Comment 22

[emveka]

I'm just a regular year with a AGM X2, so I have the same problem. Great to know that you people are puzzelling on this problem!

As far as I understand there is solution yet.

Keep up the good work!

Comment 23

[db]

Something seems to have changed, and not for the better.
In Nightly I am no longer even able to submit a crash report. The app dies in a really weird way that's hard to describe and which seems to put a strain on the entire OS. Once I even had to reboot my phone.
If I didn't know better I would say it was probably an infinite loop or stack overflow or something like that.

Runerd, are you SURE you can't submit a patch? It really seems like you're more than qualified and just being unnecessarily humble ☺️

Comment 24

[Padacore]

is there any chance the bug could ever be fixed?
I love firefox too much to let it go that easily.

Comment 25

[db]

I gave up eventually - and have discovered that Opera / Opera Mini covers my needs quite well.

Comment 26

[Padacore]

(In reply to db from comment #25)
> I gave up eventually - and have discovered that Opera / Opera Mini covers my
> needs quite well.
Maybe, but since i use firefox sync I would have to install opera on all my pcs and opera in not open-source.
I can't imagine a model phone related bug being so hard to fix. But I guess everyone has to say that.

Comment 27

[db]

The frustrating part is that runerd has already found a working fix but for some reason didn't feel comfortable submitting it. And it's too far down Mozilla's own pile for them to notice it apparently.
I tried filing a crash report every day for a while in the hope that it would push the issue up their list, but got tired after a few weeks.

Comment 28

[runerd]

(In reply to db from comment #27)
> The frustrating part is that runerd has already found a working fix but for
> some reason didn't feel comfortable submitting it. And it's too far down
> Mozilla's own pile for them to notice it apparently.
> I tried filing a crash report every day for a while in the hope that it
> would push the issue up their list, but got tired after a few weeks.

Hi, sorry for my late response, been busy months, then a long vacation. I agree it's frustrating. I actually tried to figure out the process of submitting a patch, but I couldn't find the specifics, what commands to run etc. I guess it's easy for a developer, but I have no prior experience with this. I don't know the commands or syntax for creating a patch, using a revision control system, or submitting a patch. I will give it another shot, but if someone could point me to more practical "howtos" than the general guides at mozilla.org it would be much appreciated :)

Comment 29

[emveka]

I switched to chrome. Not that I like it most, but I get used to it. An advantage is that it's really easy to log in to my personalised chrome at any workstation.

Comment 30

[runerd]

Attachment: Prevent for-loop running on null array in GeckoApp.java

Adds a check that listfiles() does not return null on the "res/fonts" directory, (which is actually the protected /res/fonts system directory). This prevents the for-loop running on a null array and crash on devices where this directory exists.

Comment 31

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8996964 [details] [diff] [review]
Prevent for-loop running on null array in GeckoApp.java

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

Thanks taking the time to figure out how to submit a patch, runerd! We really appreciate it! Sorry it took me so long to get back to you.

It looks like your patch will prevent the crash - good work! However, after investigating this since our original discussion, it's a little more complicated than that: we actually want to delete the files from `<data-dir>/res/fonts` rather than `/res/fonts`. I apologize if you wanted to continue working on this (after my review) but given that I had already done the necessary investigation and the urgency I felt from others in this bug, I thought I'd throw together a patch together myself.

If you want to dig into more code, let me know with a needinfo!

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1373964: On profile migrate, delete <dir>/res/fonts.

Hey Vlad, do you mind reviewing this? :)

Comment 33

[Vlad Baicu]

Comment on attachment 9000145 [details] [diff] [review]
Bug 1373964: On profile migrate, delete <dir>/res/fonts.

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

Seems like a pretty straightforward fix to me, r+

Comment 34

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Please NI me if there are any difficulties landing this.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40e4222861a5
On profile migrate, delete <dir>/res/fonts. r=vladbaicu

Comment 36

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/40e4222861a5

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 38

[runerd]

Awesome, thank you all! I can confirm that Firefox Nightly 2018-08-17, that just got available in Play, does NOT crash on my AGM X2. I will keep running my own builds though, since it's so much fun! :)

Comment 39

[Padacore]

Congratulations,
Firefox Nightly 63.0.a1 2018-08-17 from ftp works like a charm on AGM X2. Little issue with sync but it's another story...
This helps me to understand more the process of debugging.
Thanks for all.

Comment 40

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 9000145 [details] [diff] [review]
Bug 1373964: On profile migrate, delete <dir>/res/fonts.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 878674
[User impact if declined]: Users on certain devices crash on startup and there's no workaround. These devices have a /res/fonts directory but don't provide the user read access. On rooted devices, .ttf files will unfortunately be removed from this directory (assuming Firefox also runs as root) but there will be no crash.

[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: By users: see comments.
[Needs manual test from QE? If yes, steps to reproduce]: Ideal. It'd be great if someone can test the migration case: 
- Open a new profile on Firefox 24
- Verify there are *ttf files in `<data-dir>/res/fonts`
- Upgrade to a release with this patch
- Verify the *ttf files have been deleted

It may also be good to verify:
- We don't crash on the devices users have specified create crashes (can also be reproduced on rooted devices by creating the directories ourselves); note that users have already done this though (thank you!)
- we don't save any other *ttf files into `<data-dir>/res/fonts` that we may accidentally delete (but I don't see why we would).

[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Small to medium but only because I'm missing context.

[Why is the change risky/not risky?]: Originally, it appears we unpackaged our fonts from the APK dir to <data-dir>/res/fonts but, at some point, this became unnecessary. To save users space, this code was created to remove these files. However, there was a bug: we attempted to remove font files from `/res/fonts` instead of `<data-dir>/res/fonts`. This patch fixes the incorrect paths. However, if I misunderstood the issue and the context, or we have since started saving more files in `<data-dir>/res/fonts`, we may end up deleting font files (in our app's data directory) we did not intend to!

[String changes made/needed]: None

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9000145 [details] [diff] [review]
Bug 1373964: On profile migrate, delete <dir>/res/fonts.

Approved for Fennec 62.0b21.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/76136f7b8f3a

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Not sure how easily this can be verified beyond watching crash-stats, but it would be nice to do so if possible.

Comment 44

[db]

As an affected AGM X2 user let me just chime in and say Nightly is no longer crashing for me after the fix.

Comment 45

[Ioana Chiorean]

 (In reply to Ryan VanderMeulen [:RyanVM] from comment #43)
> Not sure how easily this can be verified beyond watching crash-stats, but it
> would be nice to do so if possible.

Oana is handling the Bug verification for the next beta and will take care of this.

Comment 46

[Bogdan Surd, QA [:bsurd, NI]]

Devices:
 - Huawei MediaPad M2 (Android 5.1.1);
 - Nexus 9 (Android 6.0.1);
 - Sony Xperia Z5 Premium (Android 6.0.1);
 - Samsung Galaxy S8 (Android 8).

Hello,

Could not find a beta version of FF 24 in order to test this. I did, however, managed to find and install the Nightly version of FF 24 (that would not even open on any of the devices I've tested this with).

Accessing the app files via ADB is a dead end after accessing the app. I can navigate inside the app but have no access to any other files after that. I would also like to mention that none of the devices were rooted.

Checked with a file explorer as well and there is no fonts folder visible in '<data-dir>/res/'.

I can see that [e-mail] tested this on his AGM X2 and is no longer experiencing this issue as mentioned in Comment 44. @[e-mail] thank you for confirming that the fix works.

Comment 47

[runerd]

New beta 62.0b21 is also fixed. Tested on AGM X2. Version from Google Play and all apks from download (api-16/aarch64 + en-us/multi).

Comment 48

[Oana Horvath [:ohorvath]]

Closing based on comment 47. Thanks @runerd!