Closed Bug 1314835 Opened 3 years ago Closed 11 months ago

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

Categories

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

50 Branch
Unspecified
Android
defect

Tracking

()

RESOLVED WONTFIX
Firefox 53
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- affected

People

(Reporter: ritu, Unassigned)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

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.
It jumped from rank #16 to currently at rank #7
Sebastian, this is a top crash in 50
Flags: needinfo?(s.kaspari)
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.
Flags: needinfo?(walkingice0204)
Flags: needinfo?(s.kaspari)
Flags: needinfo?(max)
Flags: needinfo?(cnevinchen)
Assignee: nobody → walkingice0204
Status: NEW → ASSIGNED
Flags: needinfo?(walkingice0204)
Flags: needinfo?(max)
Flags: needinfo?(cnevinchen)
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.
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?
Attachment #8810738 - Flags: review?(s.kaspari)
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
Attachment #8810738 - Flags: review?(s.kaspari)
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 on attachment 8810738 [details]
Bug 1314835: Add more debug information to TelemetryPingStore

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

LGTM.
Attachment #8810738 - Flags: review?(s.kaspari) → review+
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]: -
Attachment #8810738 - Flags: approval-mozilla-aurora?
Julian, is there a particular reason this hasn't landed on m-c yet?
Flags: needinfo?(walkingice0204)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=222f126fa6eb0692a66165d1bfe17639f0f1ba5c

Test again and passed, please check in.
Flags: needinfo?(walkingice0204)
Keywords: checkin-needed
Julien, sorry that's my mistake. I didn't aware of that I should land it into m-c first.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4c969c7c1c22
Add more debug information to TelemetryPingStore r=sebastian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4c969c7c1c22
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Depends on: 1321799
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?
Attachment #8810738 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Julien you are right!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Hey Julian, did the added debugging information ever turn up anything useful?
Flags: needinfo?(walkingice0204)
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
Flags: needinfo?(walkingice0204)
Too late for firefox 52, mass-wontfix.
Duplicate of this bug: 1411330
Duplicate of this bug: 1413354
See Also: → 1411330
[triage] Trivial crash count.
Assignee: walkingice0204 → nobody
Priority: -- → P3
Closing because no crashes reported for 12 weeks.
Status: REOPENED → RESOLVED
Closed: 3 years ago11 months ago
Resolution: --- → WONTFIX
Closing because no crashes reported for 12 weeks.
You need to log in before you can comment on or make changes to this bug.