Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DiskReadViolation triggered by setCrashlyticsCollectionEnabled() #3265

Closed
DanielNovak opened this issue Dec 23, 2021 · 6 comments
Closed

DiskReadViolation triggered by setCrashlyticsCollectionEnabled() #3265

DanielNovak opened this issue Dec 23, 2021 · 6 comments
Assignees
@mrichards
Labels
api: crashlytics

Comments

@DanielNovak
Copy link

Describe your environment

  • Android Studio version: 2020.3.1 Patch 4
  • Firebase Component: Crashlytics
  • Component version: 18.2.6

Describe the problem

Firebase Crashlytics is triggering Android StrictMode DiskReadViolation when setCrashlyticsCollectionEnabled(true) is called.

StrictMode policy violation; ~duration=351 ms: android.os.strictmode.DiskReadViolation
        at android.os.StrictMode$AndroidBlockGuardPolicy.onReadFromDisk(StrictMode.java:1659)
        at libcore.io.BlockGuardOs.access(BlockGuardOs.java:74)
        at libcore.io.ForwardingOs.access(ForwardingOs.java:131)
        at android.app.ActivityThread$AndroidOs.access(ActivityThread.java:7715)
        at java.io.UnixFileSystem.checkAccess(UnixFileSystem.java:281)
        at java.io.File.exists(File.java:813)
        at android.app.SharedPreferencesImpl.writeToFile(SharedPreferencesImpl.java:738)
        at android.app.SharedPreferencesImpl.access$900(SharedPreferencesImpl.java:59)
        at android.app.SharedPreferencesImpl$2.run(SharedPreferencesImpl.java:672)
        at android.app.SharedPreferencesImpl.enqueueDiskWrite(SharedPreferencesImpl.java:691)
        at android.app.SharedPreferencesImpl.access$100(SharedPreferencesImpl.java:59)
        at android.app.SharedPreferencesImpl$EditorImpl.commit(SharedPreferencesImpl.java:604)
        at com.google.firebase.crashlytics.internal.common.DataCollectionArbiter.storeDataCollectionValueInSharedPreferences(DataCollectionArbiter.java:206)
        at com.google.firebase.crashlytics.internal.common.DataCollectionArbiter.setCrashlyticsDataCollectionEnabled(DataCollectionArbiter.java:92)
        at com.google.firebase.crashlytics.internal.common.CrashlyticsCore.setCrashlyticsCollectionEnabled(CrashlyticsCore.java:257)
        at com.google.firebase.crashlytics.FirebaseCrashlytics.setCrashlyticsCollectionEnabled(FirebaseCrashlytics.java:448)

Steps to reproduce:

Enable strict mode:

        StrictMode.setThreadPolicy(
            ThreadPolicy.Builder()
                .detectAll()
                .penaltyLog()
                .build()

and call:

FirebaseCrashlytics.getInstance().setCrashlyticsCollectionEnabled(enableCrashlytics)

Relevant Code:

The problem is in Firebase DataCollectionArbitrer.java. It's using commit() instead of apply():

    private static void storeDataCollectionValueInSharedPreferences(SharedPreferences sharedPreferences, Boolean enabled) {
        Editor prefsEditor = sharedPreferences.edit();
        if (enabled != null) {
            prefsEditor.putBoolean("firebase_crashlytics_collection_enabled", enabled);
        } else {
            prefsEditor.remove("firebase_crashlytics_collection_enabled");
        }

        prefsEditor.commit();
    }

For some reason the developers also marked it with @SuppressLint({"ApplySharedPref"}). So maybe they had a reason to do it like this? But then this call is always blocking the main thread on many apps on start...
@argzdev
Copy link
Contributor

argzdev commented Dec 23, 2021

Hi @DanielNovak, thanks for reporting. We'll investigate this and see what we can find.

@argzdev
Copy link
Contributor

argzdev commented Dec 23, 2021 •
edited
Loading

@DanielNovak, I was unable to reproduce the issue with the given steps.

After enabling StrictMode policy. I've added the FirebaseCrashlytics.getInstance().setCrashlyticsCollectionEnabled(enableCrashlytics) with either true or false.

However, I didn't experience any issue on runtime on either Android 10.0, 11.0, or 12.0

Am I missing anything? If it's possible could you provide us a minimal reproducible example with the issue so I can investigate this further.

@DanielNovak
Copy link
Author

@argzdev There is no direct issue or crash - but you will see the log entry in the logs. You can also change the code to:

StrictMode.setThreadPolicy(
            ThreadPolicy.Builder()
                .detectAll()
                .penaltyDeath()
                .build()

to crash the app on strict mode violation. But make sure that you enable StrictMode before calling setCrashlyticsCollectionEnabled.

So this strict mode is more like a warning - that the Crashlytics is accessing the disk on main thread, but it's not causing any visible problems or crashes.

@argzdev
Copy link
Contributor

argzdev commented Dec 23, 2021 •
edited
Loading

Thanks for the clarification @DanielNovak, I see it now. I'll notify an engineer and see what we can do about this.

In the meantime, I'll create a pull request for the fix you provided. If this is approved by our engineers, we can include this on the next release.

@argzdev argzdev mentioned this issue Dec 23, 2021
Merged
@mrichards mrichards self-assigned this Jan 10, 2022
@mrichards
Copy link
Contributor

Thanks @DanielNovak, I merged the change and it will be available in our next release, 18.2.7, coming out in a couple weeks. I'll leave this ticket open until it ships.

@argzdev
Copy link
Contributor

argzdev commented Jan 26, 2022

Since this has been released in our latest version 18.2.7, I'll be closing this now. Thanks!

@argzdev argzdev closed this as completed Jan 26, 2022
@firebase firebase locked and limited conversation to collaborators Feb 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@mrichards mrichards

Labels
api: crashlytics
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@mrichards @DanielNovak @google-oss-bot @argzdev