markzhai / AndroidPerformanceMonitor

A transparent ui-block detection library for Android. (known as BlockCanary)
Apache License 2.0
6.66k stars 1.02k forks source link

java.io.FileNotFoundException #80

Open Piasy opened 7 years ago

Piasy commented 7 years ago
E/LogWriter: save: 
 java.io.FileNotFoundException: /data/com.github.piasy.bootstrap/performance/looper-2016-12-11_23-48-25.844.log (No such file or directory)
     at java.io.FileOutputStream.open(Native Method)
     at java.io.FileOutputStream.<init>(FileOutputStream.java:221)
     at java.io.FileOutputStream.<init>(FileOutputStream.java:140)
     at com.github.moduth.blockcanary.LogWriter.save(LogWriter.java:108)
     at com.github.moduth.blockcanary.LogWriter.save(LogWriter.java:56)
     at com.github.moduth.blockcanary.BlockCanaryInternals$1.onBlockEvent(BlockCanaryInternals.java:63)
     at com.github.moduth.blockcanary.LooperMonitor$1.run(LooperMonitor.java:74)
     at android.os.Handler.handleCallback(Handler.java:751)
     at android.os.Handler.dispatchMessage(Handler.java:95)
     at android.os.Looper.loop(Looper.java:154)
     at android.os.HandlerThread.run(HandlerThread.java:61)
markzhai commented 7 years ago

Does the file really exist?And do you really have permission to write file in that directory?

Normally the log file should be in sdcard if sdcard exists

Piasy commented 7 years ago

It seems /data/com.github.piasy.bootstrap/performance/ does not locate in external storage, so I can't verify whether this file exist, but as this exception is thrown, it obviously doesn't exist.

This exception doesn't cause crash, it's just printed in logcat, but when I run my app next time, I can't see it again. I just want to verify that does the log writing logic is correct :)

And I haven't request for permission manually, so do I need request it by myself?

zjupure commented 7 years ago

@Piasy this problem is produced when the external storage is not available or not writable. For example, the external storage is unmounted and it is really exist in some devices. When external storage is not exist, the library with choose the /data/{your_provided_dir}, but this directory is not accessible by normal app due to permission limit. the code in BlockCanaryInternals.getPath() have some drawback in design.

static String getPath() {
        String state = Environment.getExternalStorageState();
        String logPath = BlockCanaryInternals.getContext()
                == null ? "" : BlockCanaryInternals.getContext().providePath();

        if (Environment.MEDIA_MOUNTED.equals(state)
                && Environment.getExternalStorageDirectory().canWrite()) {
            return Environment.getExternalStorageDirectory().getPath() + logPath;
        }
        return Environment.getDataDirectory().getAbsolutePath() + BlockCanaryInternals.getContext().providePath();
    }

There is another permission problem, the external storage is not writeable default except the app-specifical directory due to the dynamic permission mechanism since Android 6.0. So i suggest BlockCanary library can modify the implementation of BlockCanaryInternals.getPath(), there are two choice:

  1. the path is provided entirely by user through BlockCanaryContext.providePath(), the path validation is decided by user.
  2. change the external storage directory to app-specifical directory, when this path is not avaiable, use the internal storage directory instead.

App-specifical directory usually locates in external storage /Android/data/{pacakage_name}, you can use Context.getExternalCacheDir() or Context.getExternalFilesDir() to obtain a cache directory(/cache) or a files directory(/files). Those directory do not need to declare permission in Android Mainifest since Kitkat. If abvoe path is not avaiable (external storage is unmount), use Context.getCacheDir() or Context.getFilesDir() instead. The result are usually located in /data/data/{pacakage_name} in internal storage and always available to normal app.

A reference implementation for choice 2

static String getPath() {
        String state = Environment.getExternalStorageState();
        String logPath = BlockCanaryInternals.getContext()
                == null ? "" : BlockCanaryInternals.getContext().providePath();

        Context context = BlockCanaryInternals.getContext().provideContext();
        File appCache = context.getExternalCacheDir();
        if (appCache != null && Environment.MEDIA_MOUNTED.equals(state)) {
            return  appCache.getAbsolutePath() + logPath;
        }
        return context.getCacheDir() + logPath;
    }
markzhai commented 7 years ago

Wow, cool, thanks @zjupure

initialjie commented 7 years ago

@markzhai @zjupure Yes, Android's external storage is really a complicated thing to deal with, like permission or path etc,. I totally agree that the path should be provided entirely by user.

markzhai commented 7 years ago

@initialjie @zjupure thanks, I adopt that the path should be provided entirely by user, the library itself should not do any magic to it.