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.

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

WebViewDataTest fails on API 26+ after Android Profiler file injection (part 1: whitelist) #1842

Closed
mcomella opened this Issue Nov 29, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@mcomella
Contributor

mcomella commented Nov 29, 2017

Stack trace:

java.lang.AssertionError: Expected file 'perfd' to be in the app's data dir whitelist
at org.junit.Assert.fail(Assert.java:88)
at org.junit.Assert.assertTrue(Assert.java:41)
at org.mozilla.focus.activity.WebViewDataTest.DeleteWebViewDataTest(WebViewDataTest.java:205)
at java.lang.reflect.Method.invoke(Native Method)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)

It seems there are a few files in the data/ directory that don't exist on my API 23 emulator, which is why I didn't catch this sooner.

We should also verify that these files, if we whitelist them, aren't leaking browsing state. Otherwise, we should delete them!

@mcomella mcomella added this to the Focus Android V4.0 milestone Nov 29, 2017

@mcomella mcomella self-assigned this Nov 29, 2017

@mcomella

This comment has been minimized.

Show comment
Hide comment
@mcomella

mcomella Nov 30, 2017

Contributor

These may come from the new Android Studio 3.0 profiler. Three files I noticed are:

  • perfa.jar
  • perfd (binary file)
  • libperfa_x86.so

The contents of perfa.jar:

$ unzip perfa.jar
Archive:  perfa.jar
  inflating: META-INF/MANIFEST.MF
  inflating: classes.dex
   creating: com/
   creating: com/android/
   creating: com/android/tools/
   creating: com/android/tools/profiler/
   creating: com/android/tools/profiler/support/
   creating: com/android/tools/profiler/support/event/
   creating: com/android/tools/profiler/support/memory/
   creating: com/android/tools/profiler/support/network/
   creating: com/android/tools/profiler/support/network/httpurl/
   creating: com/android/tools/profiler/support/network/okhttp/
   creating: com/android/tools/profiler/support/network/okhttp/reflection/
   creating: com/android/tools/profiler/support/network/okhttp/reflection/okhttp2/
   creating: com/android/tools/profiler/support/network/okhttp/reflection/okhttp3/
   creating: com/android/tools/profiler/support/network/okhttp/reflection/okio/
   creating: com/android/tools/profiler/support/profilers/
   creating: com/android/tools/profiler/support/util/

I confirmed that these files are not injected in AS 2.3.3 but are in 3.0.1.

The new profiler description page mentions, "To show you advanced profiling data, Android Studio must inject monitoring logic into your compiled app." After following the directions to enable it, I see:
screen shot 2017-11-29 at 16 01 59

Which leads me to the conclusion: Android Studio 3.0+ is automatically injecting profiler related files on API 26+ and this can also be enabled for APIs below 26: I assume this only affects debug builds. Note that I did not extensively test this so this conclusion may be wrong in some cases.

Contributor

mcomella commented Nov 30, 2017

These may come from the new Android Studio 3.0 profiler. Three files I noticed are:

  • perfa.jar
  • perfd (binary file)
  • libperfa_x86.so

The contents of perfa.jar:

$ unzip perfa.jar
Archive:  perfa.jar
  inflating: META-INF/MANIFEST.MF
  inflating: classes.dex
   creating: com/
   creating: com/android/
   creating: com/android/tools/
   creating: com/android/tools/profiler/
   creating: com/android/tools/profiler/support/
   creating: com/android/tools/profiler/support/event/
   creating: com/android/tools/profiler/support/memory/
   creating: com/android/tools/profiler/support/network/
   creating: com/android/tools/profiler/support/network/httpurl/
   creating: com/android/tools/profiler/support/network/okhttp/
   creating: com/android/tools/profiler/support/network/okhttp/reflection/
   creating: com/android/tools/profiler/support/network/okhttp/reflection/okhttp2/
   creating: com/android/tools/profiler/support/network/okhttp/reflection/okhttp3/
   creating: com/android/tools/profiler/support/network/okhttp/reflection/okio/
   creating: com/android/tools/profiler/support/profilers/
   creating: com/android/tools/profiler/support/util/

I confirmed that these files are not injected in AS 2.3.3 but are in 3.0.1.

The new profiler description page mentions, "To show you advanced profiling data, Android Studio must inject monitoring logic into your compiled app." After following the directions to enable it, I see:
screen shot 2017-11-29 at 16 01 59

Which leads me to the conclusion: Android Studio 3.0+ is automatically injecting profiler related files on API 26+ and this can also be enabled for APIs below 26: I assume this only affects debug builds. Note that I did not extensively test this so this conclusion may be wrong in some cases.

@mcomella

This comment has been minimized.

Show comment
Hide comment
@mcomella

mcomella Nov 30, 2017

Contributor

We should also verify that these files, if we whitelist them, aren't leaking browsing state. Otherwise, we should delete them!

I don't want to delete them because this may break our ability to correctly profile the app so I'm going to whitelist them.

Contributor

mcomella commented Nov 30, 2017

We should also verify that these files, if we whitelist them, aren't leaking browsing state. Otherwise, we should delete them!

I don't want to delete them because this may break our ability to correctly profile the app so I'm going to whitelist them.

@mcomella mcomella added the bug label Nov 30, 2017

@mcomella

This comment has been minimized.

Show comment
Hide comment
@mcomella

mcomella Nov 30, 2017

Contributor

I'm having second thoughts – if our intent is to ensure there is no browsing data left on disk and the profiler caches browsing data on disk, shouldn't we delete the profiler's files? I'm going to investigate if these files are added in:

  1. Release builds
  2. Gradle-only builds (i.e. not from Android Studio)

If so:

  • We should delete the profiler's files on clean-up
  • To prevent the profiler from breaking, in local builds we can have a branch to avoid deleting those files

If not:

  • The simple solution is to whitelist the files. It could be insecure if these files are added to release builds later
  • We could delete on cleanup and add a branch that doesn't delete the profile, as per the above.

fwiw, perfd contained "localhost" (or, at least the binary data in the file matched "localhost") and broke the test even when I whitelist the files; perhaps this is not surprising because it has a network monitor.

Contributor

mcomella commented Nov 30, 2017

I'm having second thoughts – if our intent is to ensure there is no browsing data left on disk and the profiler caches browsing data on disk, shouldn't we delete the profiler's files? I'm going to investigate if these files are added in:

  1. Release builds
  2. Gradle-only builds (i.e. not from Android Studio)

If so:

  • We should delete the profiler's files on clean-up
  • To prevent the profiler from breaking, in local builds we can have a branch to avoid deleting those files

If not:

  • The simple solution is to whitelist the files. It could be insecure if these files are added to release builds later
  • We could delete on cleanup and add a branch that doesn't delete the profile, as per the above.

fwiw, perfd contained "localhost" (or, at least the binary data in the file matched "localhost") and broke the test even when I whitelist the files; perhaps this is not surprising because it has a network monitor.

@mcomella

This comment has been minimized.

Show comment
Hide comment
@mcomella

mcomella Nov 30, 2017

Contributor

It appears data these profiler files will only be injected if you build via Android Studio and you have opened the "Android profiler" tab at least once since this current Android Studio process has been launched.

I think we should whitelist these so we don't break local builds and I think it's unlikely that these files will be added for release builds so it should be safe.

Contributor

mcomella commented Nov 30, 2017

It appears data these profiler files will only be injected if you build via Android Studio and you have opened the "Android profiler" tab at least once since this current Android Studio process has been launched.

I think we should whitelist these so we don't break local builds and I think it's unlikely that these files will be added for release builds so it should be safe.

mcomella added a commit to mcomella/focus-android that referenced this issue Nov 30, 2017

Closes #1842: Whitelist AS profiler files.
Given the conditions under which I've noticed these files are added, it's only
likely to occur in local builds. However, if we wanted to be extra safe that
these files aren't kept on disk in release builds, we could explicitly delete
them but then we'd presumably have to add a branch to allow developer builds to
use these files without problems. For simplicity, I've opted for the whitelist.

@mcomella mcomella closed this in 78981d7 Nov 30, 2017

@mcomella mcomella changed the title from WebViewDataTest fails on API 26 Pixel emulator to WebViewDataTest fails on API 26+ after Android Profiler file injection Nov 30, 2017

@mcomella mcomella changed the title from WebViewDataTest fails on API 26+ after Android Profiler file injection to WebViewDataTest fails on API 26+ after Android Profiler file injection (part 1) Dec 1, 2017

@mcomella mcomella changed the title from WebViewDataTest fails on API 26+ after Android Profiler file injection (part 1) to WebViewDataTest fails on API 26+ after Android Profiler file injection (part 1: whitelist) Dec 1, 2017

riyaz pushed a commit to riyaz/focus-android that referenced this issue Dec 14, 2017

Closes #1842: Whitelist AS profiler files.
Given the conditions under which I've noticed these files are added, it's only
likely to occur in local builds. However, if we wanted to be extra safe that
these files aren't kept on disk in release builds, we could explicitly delete
them but then we'd presumably have to add a branch to allow developer builds to
use these files without problems. For simplicity, I've opted for the whitelist.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment