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

REOPENED
Assigned to

Status

()

Firefox for Android
General
--
major
REOPENED
a year ago
3 months ago

People

(Reporter: ritu, Assigned: walkingice)

Tracking

({crash, leave-open})

50 Branch
Firefox 53
Unspecified
Android
crash, leave-open
Points:
---

Firefox Tracking Flags

(firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox53 affected)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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.
(Reporter)

Comment 1

a year ago
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)
(Assignee)

Comment 4

a year ago
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.
(Assignee)

Comment 5

a year ago
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 hidden (mozreview-request)
(Assignee)

Updated

a year ago
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)
(Assignee)

Comment 8

a year ago
mozreview-review-reply
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 hidden (mozreview-request)
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+
(Assignee)

Comment 11

a year ago
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)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

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

Test again and passed, please check in.
Flags: needinfo?(walkingice0204)
Keywords: checkin-needed
(Assignee)

Comment 15

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

Comment 16

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4c969c7c1c22
Add more debug information to TelemetryPingStore r=sebastian
Keywords: checkin-needed

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4c969c7c1c22
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
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+
(Assignee)

Comment 19

a year ago
Julien you are right!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
status-firefox50: --- → wontfix
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: fixed → affected
Keywords: leave-open

Comment 20

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/2ac543baa492
status-firefox52: affected → fixed
status-firefox52: fixed → affected
Hey Julian, did the added debugging information ever turn up anything useful?
status-firefox51: affected → wontfix
Flags: needinfo?(walkingice0204)
(Assignee)

Comment 22

a year ago
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.
status-firefox52: affected → wontfix

Updated

3 months ago
Duplicate of this bug: 1411330

Updated

3 months ago
Duplicate of this bug: 1413354

Updated

3 months ago
See Also: → bug 1411330
You need to log in before you can comment on or make changes to this bug.