1289006 - Crash in java.lang.NullPointerException: storage == null at java.util.Arrays$ArrayList.<init>(Arrays.java)

Status: RESOLVED FIXED

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

Description

[Nicholas Nethercote [inactive]]

This bug was filed from the Socorro interface and is 
report bp-d28d059e-44ab-4fe0-904f-c27292160724.
=============================================================

This crash first appeared in Fennec Nightly 20160621030208. Since then it has occurred 21 times. The Java stack trace is this:

> java.lang.NullPointerException: storage == null
>   at java.util.Arrays$ArrayList.<init>(Arrays.java:38)
>   at java.util.Arrays.asList(Arrays.java:155)
>   at org.mozilla.gecko.telemetry.stores.TelemetryJSONFilePingStore.getAllPings(TelemetryJSONFilePingStore.java:132)
>   at org.mozilla.gecko.telemetry.stores.TelemetryJSONFilePingStore.getAllPings(TelemetryJSONFilePingStore.java:64)
>   at org.mozilla.gecko.telemetry.TelemetryUploadService.uploadPendingPingsFromStore(TelemetryUploadService.java:98)
>   at org.mozilla.gecko.telemetry.TelemetryUploadService.onHandleIntent(TelemetryUploadService.java:81)
>   at android.app.IntentService$ServiceHandler.handleMessage(IntentService.java:66)
>   at android.os.Handler.dispatchMessage(Handler.java:102)
>   at android.os.Looper.loop(Looper.java:148)
>   at android.os.HandlerThread.run(HandlerThread.java:61)

Michael, can you please investigate?

Comment 1

[u576187]

I am the guy responsible for most of the crash reports. If you need anything, ask me.

Comment 2

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

Digging in, the exception is thrown if the argument to ArrayList.<init> is null:
 http://androidxref.com/4.4.4_r1/xref/libcore/luni/src/main/java/java/util/Arrays.java#36

Looking at where the arg comes from, we see Arrays.asList:
 http://androidxref.com/4.4.4_r1/xref/libcore/luni/src/main/java/java/util/Arrays.java#154

Who gets its argument from the ping store, which calls listFiles:
 http://androidxref.com/4.4.4_r1/xref/libcore/luni/src/main/java/java/io/File.java#806

Which calls to list:
 http://androidxref.com/4.4.4_r1/xref/libcore/luni/src/main/java/java/io/File.java#766

And returns null when the native list method returns null or the filename filter is null. Since the filename filter is null, I assume the native list method is returning null, which I imagine might happen when the file in File.listFiles either does not exist or is a directory.

Quick fix is to return gracefully if the output is null.

In the interest of trying to figure out why this happened (because it's unexpected)...

(In reply to tobbi.bugs from comment #1)
> I am the guy responsible for most of the crash reports. If you need
> anything, ask me.

Do you have any special configuration for your phone/Firefox? Perhaps your disk is full or Firefox was not allowed to freely write to its data directory?

Comment 3

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

Attachment: Bug 1289006 - Return gracefully if listFiles returns null in telemetry store.

This changeset will correct the crash we're seeing in the bug.

The docs support that File.listFiles can return null.

Review commit: https://reviewboard.mozilla.org/r/66942/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66942/

Comment 4

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

Attachment: Bug 1289006 - Add code to assert potential crashing case never happens & tests.

I expect the crashes occurred because one of the following:
  * the store already existed as a file
  * the store was a directory that did not have the appropriate permissions

In the telemetry code, none of the cases above should happen so I assert that
they never do.

If the crashes did occur for these reasons, the user will unfortunately
continue to crash but at least we'll know where our assumptions are going wrong.

I originally intended to write a regression test for listFiles returning null
but it requires the application code to be modified in non-trivial ways (e.g.
accessor methods we might forget to use) so I decided against it.

Review commit: https://reviewboard.mozilla.org/r/66944/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66944/

Comment 5

[:Grisha Kruglov]

Comment on attachment 8774501 [details]
Bug 1289006 - Return gracefully if listFiles returns null in telemetry store.

https://reviewboard.mozilla.org/r/66942/#review63812

Comment 6

[:Grisha Kruglov]

Comment on attachment 8774502 [details]
Bug 1289006 - Add code to assert potential crashing case never happens & tests.

https://reviewboard.mozilla.org/r/66944/#review63814

Strange stuff; hopefully the added logging will help narrow this down.

Comment 7

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

https://hg.mozilla.org/integration/fx-team/rev/d5403a0e1c779912d79e14f7cd5d22e978653f89
Bug 1289006 - Return gracefully if listFiles returns null in telemetry store. r=grisha

https://hg.mozilla.org/integration/fx-team/rev/5a8b18f503baa996bcba9ea249e1762b7e536211
Bug 1289006 - Add code to assert potential crashing case never happens & tests. r=grisha

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/d5403a0e1c77
Return gracefully if listFiles returns null in telemetry store. r=grisha
https://hg.mozilla.org/integration/fx-team/rev/5a8b18f503ba
Add code to assert potential crashing case never happens & tests. r=grisha

Comment 9

[Nicholas Nethercote [inactive]]

Thank you for the fast fix.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5403a0e1c77
https://hg.mozilla.org/mozilla-central/rev/5a8b18f503ba