1314835 - Crash in java.lang.IllegalStateException: Expected file to exist at org.mozilla.gecko.telemetry.stores.TelemetryJSONFilePingStore.lockAndReadJSONFromFile(TelemetryJSONFilePingStore.java)

Status: RESOLVED WONTFIX

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

Description

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

This bug was filed from the Socorro interface and is 
report bp-20e8b33b-12f5-4d67-be08-63fd12161102.
=============================================================

There is a spike on 50.0b10 of this Fennec crash.

Comment 1

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

It jumped from rank #16 to currently at rank #7

Comment 2

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

Sebastian, this is a top crash in 50

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

This is a bit funky: We get the list of files from the file system and when reading one of the files it doesn't seem to exist anymore (in the profile directory!).

Nevin, Julian, Max: Does anyone of you have time to look into this bug? Otherwise I'll have a look tomorrow.

Comment 4

[Julian Chu [:walkingice]]

Still trying to reproduce. If user use 'New Guest Session', then exits it, the guest profile directory will be deleted. I am looking at this part to see whether it cause any problem.

Comment 5

[Julian Chu [:walkingice]]

In the beginning I assume there are two possible reasons

1. File permission cause exception
2. Read non-existed files cause exception

For 1, I think it should be OK. The profile files directory is owned by same-app/same-user. No matter how we change session(ie. 'New Guest Session'), it still be same-user, so there should be no permission problem.

For 2, it might happen if there are two threads manipulates the same directory. But every manipulation is wrapped into Runnable and post into one background thread(GeckoBackgroundThread) in sequence. It seems no race condition problem.

Could we put more debug message into the exception to get more hints?

Comment 6

[Julian Chu [:walkingice]]

Attachment: Bug 1314835: Add more debug information to TelemetryPingStore

So far still cannot find the reason of causing exception. To add more
debug information to make sure it is not permssion problem, and it
access the correct directory.

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

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8810738 [details]
Bug 1314835: Add more debug information to TelemetryPingStore

https://reviewboard.mozilla.org/r/93052/#review93696

Did you test with your own builds? By default telemetry is disabled for those builds. You might need to test in Nightly or build with telemetry enabled.

I think for guest mode we have telemetry disabled too - so it might not be able to cause this.

::: mobile/android/base/java/org/mozilla/gecko/telemetry/stores/TelemetryJSONFilePingStore.java:199
(Diff revision 1)
> +            throw new IllegalStateException(String.format("Expected file %s to exist. Existence: %b",
> +                    file.getAbsolutePath(),
> +                    file.exists()));

This path contains the profile path. We usually try to not leak this path to the log. Would it be enough to log the file name in this case?

I wonder if the absolute path will be helful, after all we know that it will be:
profile directory + "telemetry_java" + random UUID

Comment 8

[Julian Chu [:walkingice]]

Comment on attachment 8810738 [details]
Bug 1314835: Add more debug information to TelemetryPingStore

https://reviewboard.mozilla.org/r/93052/#review93696

> This path contains the profile path. We usually try to not leak this path to the log. Would it be enough to log the file name in this case?
> 
> I wonder if the absolute path will be helful, after all we know that it will be:
> profile directory + "telemetry_java" + random UUID

OK. So just checking file existence is enough for here. (To disitinguish it is not permission problem).

Comment 9

[Julian Chu [:walkingice]]

Comment on attachment 8810738 [details]
Bug 1314835: Add more debug information to TelemetryPingStore

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93052/diff/1-2/

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8810738 [details]
Bug 1314835: Add more debug information to TelemetryPingStore

https://reviewboard.mozilla.org/r/93052/#review94456

LGTM.

Comment 11

[Julian Chu [:walkingice]]

Comment on attachment 8810738 [details]
Bug 1314835: Add more debug information to TelemetryPingStore

Approval Request Comment
[Feature/regressing bug #]:The patch will provide more debug information in crash report
[User impact if declined]: -
[Describe test coverage new/current, TreeHerder]: Manually trigger the code to print debug message.
[Risks and why]: Low. It doesn't change any behavior
[String/UUID change made/needed]: -

Comment 12

[Julien Cristau [:jcristau]]

Julian, is there a particular reason this hasn't landed on m-c yet?

Comment 13

[Julian Chu [:walkingice]]

Comment on attachment 8810738 [details]
Bug 1314835: Add more debug information to TelemetryPingStore

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93052/diff/2-3/

Comment 14

[Julian Chu [:walkingice]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=222f126fa6eb0692a66165d1bfe17639f0f1ba5c

Test again and passed, please check in.

Comment 15

[Julian Chu [:walkingice]]

Julien, sorry that's my mistake. I didn't aware of that I should land it into m-c first.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c969c7c1c22
Add more debug information to TelemetryPingStore r=sebastian

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4c969c7c1c22

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 8810738 [details]
Bug 1314835: Add more debug information to TelemetryPingStore

Add some debugging info to help figure out a crash, aurora52+

I guess this bug should have been left open, as it's about the crash, which this patch won't fix on its own?

Comment 19

[Julian Chu [:walkingice]]

Julien you are right!

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2ac543baa492

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Hey Julian, did the added debugging information ever turn up anything useful?

Comment 22

[Julian Chu [:walkingice]]

In the bug report[1], it says file do exists. As per comment 5, we can discard the second potential cause.

I think the file should be writable but it says not. Sebastian, do you have any idea?

[1] https://crash-stats.mozilla.com/report/index/50c9277c-6764-4e64-ad2b-8fd972170119

Comment 23

[Julien Cristau [:jcristau]]

Too late for firefox 52, mass-wontfix.

Comment 26

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

[triage] Trivial crash count.

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.

Comment 28

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.