Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Remove static leaks #8825

Merged
dnfield merged 21 commits into from
May 3, 2019
Merged

Remove static leaks #8825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
d67a843
remove asserts
dnfield May 2, 2019
c31d7a6
remove comment
dnfield May 2, 2019
9af3fa5
format
dnfield May 2, 2019
43db149
Catch a few more missing asserts, update baseline
dnfield May 2, 2019
2b36e51
Guard logging calls using BuildConfig
dnfield May 2, 2019
e31d3ec
start removing static
dnfield May 2, 2019
a4200ac
missing imports
dnfield May 2, 2019
ef4dc20
Merge branch 'guard_log' into static_leak
dnfield May 2, 2019
1192be5
merge
dnfield May 2, 2019
8763b03
undo unintended ws change
dnfield May 2, 2019
1051752
sort imports
dnfield May 2, 2019
2fca0ee
Merge branch 'remove_assert' into static_leak
dnfield May 2, 2019
c4d196e
Remove static leaks
dnfield May 2, 2019
4c27404
merge
dnfield May 3, 2019
9e08955
cleanup
dnfield May 3, 2019
f66638e
stray import
dnfield May 3, 2019
2745766
merge
dnfield May 3, 2019
bc49269
merge
dnfield May 3, 2019
1b96d14
nonnull
dnfield May 3, 2019
50dd4de
more nonnull
dnfield May 3, 2019
e8d305e
annotate
dnfield May 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion shell/platform/android/io/flutter/view/FlutterMain.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,11 @@ private static void initResources(Context applicationContext) {
Log.e(TAG, "Unable to read application info", e);
}

sResourceExtractor = new ResourceExtractor(context);
final String dataDirPath = PathUtils.getDataDirectory(context);
final String packageName = context.getPackageName();
final PackageManager packageManager = context.getPackageManager();
final AssetManager assetManager = context.getResources().getAssets();
sResourceExtractor = new ResourceExtractor(dataDirPath, packageName, packageManager, assetManager);

sResourceExtractor
.addResource(fromFlutterAssets(sAotVmSnapshotData))
Expand Down
2 changes: 1 addition & 1 deletion shell/platform/android/io/flutter/view/ResourceCleaner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class ResourceCleaner {
private static final String TAG = "ResourceCleaner";
private static final long DELAY_MS = 5000;

private class CleanTask extends AsyncTask<Void, Void, Void> {
private static class CleanTask extends AsyncTask<Void, Void, Void> {
private final File[] mFilesToDelete;

CleanTask(File[] filesToDelete) {
Expand Down
161 changes: 95 additions & 66 deletions shell/platform/android/io/flutter/view/ResourceExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
import android.content.res.AssetManager;
import android.os.AsyncTask;
import android.os.Build;
import android.support.annotation.NonNull;
import android.support.annotation.WorkerThread;
import android.util.Log;

import io.flutter.BuildConfig;
Expand Down Expand Up @@ -39,7 +41,7 @@ class ResourceExtractor {
private static final String[] SUPPORTED_ABIS = getSupportedAbis();

@SuppressWarnings("deprecation")
static long getVersionCode(PackageInfo packageInfo) {
static long getVersionCode(@NonNull PackageInfo packageInfo) {
// Linter needs P (28) hardcoded or else it will fail these lines.
if (Build.VERSION.SDK_INT >= 28) {
return packageInfo.getLongVersionCode();
Expand All @@ -48,19 +50,40 @@ static long getVersionCode(PackageInfo packageInfo) {
}
}

private class ExtractTask extends AsyncTask<Void, Void, Void> {
ExtractTask() { }
private static class ExtractTask extends AsyncTask<Void, Void, Void> {
@NonNull
private final String mDataDirPath;
@NonNull
private final HashSet<String> mResources;
@NonNull
private final AssetManager mAssetManager;
@NonNull
private final String mPackageName;
@NonNull
private final PackageManager mPackageManager;

ExtractTask(@NonNull String dataDirPath,
@NonNull HashSet<String> resources,
@NonNull String packageName,
@NonNull PackageManager packageManager,
@NonNull AssetManager assetManager) {
mDataDirPath = dataDirPath;
mResources = resources;
mAssetManager = assetManager;
mPackageName = packageName;
mPackageManager = packageManager;
}

@Override
protected Void doInBackground(Void... unused) {
final File dataDir = new File(PathUtils.getDataDirectory(mContext));
final File dataDir = new File(mDataDirPath);

final String timestamp = checkTimestamp(dataDir);
final String timestamp = checkTimestamp(dataDir, mPackageManager, mPackageName);
if (timestamp == null) {
return null;
}

deleteFiles();
deleteFiles(mDataDirPath, mResources);

if (!extractAPK(dataDir)) {
return null;
Expand All @@ -76,23 +99,73 @@ protected Void doInBackground(Void... unused) {

return null;
}


/// Returns true if successfully unpacked APK resources,
/// otherwise deletes all resources and returns false.
@WorkerThread
private boolean extractAPK(@NonNull File dataDir) {
Copy link
Contributor

@sbaranov sbaranov May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this just be made static and kept in the outer class, instead of moving it here?

Copy link
Contributor Author

@dnfield dnfield May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could, you just then have to pass more references around to the method here (mResources, mAssetManager, mDataDirPath)

Copy link
Contributor

@sbaranov sbaranov May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit - It might be slightly better, to keep it symmetrical with deleteFiles and checkTimestamp.

Copy link
Contributor

@sbaranov sbaranov May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

for (String asset : mResources) {
try {
final String resource = "assets/" + asset;
final File output = new File(dataDir, asset);
if (output.exists()) {
continue;
}
if (output.getParentFile() != null) {
output.getParentFile().mkdirs();
}

try (InputStream is = mAssetManager.open(asset);
OutputStream os = new FileOutputStream(output)) {
copy(is, os);
}
if (BuildConfig.DEBUG) {
Log.i(TAG, "Extracted baseline resource " + resource);
}
} catch (FileNotFoundException fnfe) {
continue;

} catch (IOException ioe) {
Log.w(TAG, "Exception unpacking resources: " + ioe.getMessage());
deleteFiles(mDataDirPath, mResources);
return false;
}
}

return true;
}
}

private final Context mContext;
@NonNull
private final String mDataDirPath;
@NonNull
private final String mPackageName;
@NonNull
private final PackageManager mPackageManager;
@NonNull
private final AssetManager mAssetManager;
@NonNull
private final HashSet<String> mResources;
private ExtractTask mExtractTask;

ResourceExtractor(Context context) {
mContext = context;
ResourceExtractor(@NonNull String dataDirPath,
@NonNull String packageName,
@NonNull PackageManager packageManager,
@NonNull AssetManager assetManager) {
mDataDirPath = dataDirPath;
mPackageName = packageName;
mPackageManager = packageManager;
mAssetManager = assetManager;
mResources = new HashSet<>();
}

ResourceExtractor addResource(String resource) {
ResourceExtractor addResource(@NonNull String resource) {
mResources.add(resource);
return this;
}

ResourceExtractor addResources(Collection<String> resources) {
ResourceExtractor addResources(@NonNull Collection<String> resources) {
mResources.addAll(resources);
return this;
}
Expand All @@ -101,7 +174,7 @@ ResourceExtractor start() {
if (BuildConfig.DEBUG && mExtractTask != null) {
throw new AssertionError("Attempted to start resource extraction while another extraction was in progress.");
}
mExtractTask = new ExtractTask();
mExtractTask = new ExtractTask(mDataDirPath, mResources, mPackageName, mPackageManager, mAssetManager);
mExtractTask.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
return this;
}
Expand All @@ -114,11 +187,11 @@ void waitForCompletion() {
try {
mExtractTask.get();
} catch (CancellationException | ExecutionException | InterruptedException e) {
deleteFiles();
deleteFiles(mDataDirPath, mResources);
}
}

private String[] getExistingTimestamps(File dataDir) {
private static String[] getExistingTimestamps(File dataDir) {
return dataDir.list(new FilenameFilter() {
@Override
public boolean accept(File dir, String name) {
Expand All @@ -127,9 +200,9 @@ public boolean accept(File dir, String name) {
});
}

private void deleteFiles() {
final File dataDir = new File(PathUtils.getDataDirectory(mContext));
for (String resource : mResources) {
private static void deleteFiles(@NonNull String dataDirPath, @NonNull HashSet<String> resources) {
final File dataDir = new File(dataDirPath);
for (String resource : resources) {
final File file = new File(dataDir, resource);
if (file.exists()) {
file.delete();
Expand All @@ -144,50 +217,15 @@ private void deleteFiles() {
}
}

/// Returns true if successfully unpacked APK resources,
/// otherwise deletes all resources and returns false.
private boolean extractAPK(File dataDir) {
final AssetManager manager = mContext.getResources().getAssets();

for (String asset : mResources) {
try {
final String resource = "assets/" + asset;
final File output = new File(dataDir, asset);
if (output.exists()) {
continue;
}
if (output.getParentFile() != null) {
output.getParentFile().mkdirs();
}

try (InputStream is = manager.open(asset);
OutputStream os = new FileOutputStream(output)) {
copy(is, os);
}
if (BuildConfig.DEBUG) {
Log.i(TAG, "Extracted baseline resource " + resource);
}
} catch (FileNotFoundException fnfe) {
continue;

} catch (IOException ioe) {
Log.w(TAG, "Exception unpacking resources: " + ioe.getMessage());
deleteFiles();
return false;
}
}

return true;
}

// Returns null if extracted resources are found and match the current APK version
// and update version if any, otherwise returns the current APK and update version.
private String checkTimestamp(File dataDir) {
PackageManager packageManager = mContext.getPackageManager();
private static String checkTimestamp(@NonNull File dataDir,
@NonNull PackageManager packageManager,
@NonNull String packageName) {
PackageInfo packageInfo = null;

try {
packageInfo = packageManager.getPackageInfo(mContext.getPackageName(), 0);
packageInfo = packageManager.getPackageInfo(packageName, 0);
} catch (PackageManager.NameNotFoundException e) {
return TIMESTAMP_PREFIX;
}
Expand Down Expand Up @@ -225,22 +263,13 @@ private String checkTimestamp(File dataDir) {
return null;
}

private static void copy(InputStream in, OutputStream out) throws IOException {
private static void copy(@NonNull InputStream in, @NonNull OutputStream out) throws IOException {
byte[] buf = new byte[16 * 1024];
for (int i; (i = in.read(buf)) >= 0; ) {
out.write(buf, 0, i);
}
}

private String getAPKPath() {
try {
return mContext.getPackageManager().getApplicationInfo(
mContext.getPackageName(), 0).publicSourceDir;
} catch (Exception e) {
return null;
}
}

@SuppressWarnings("deprecation")
private static String[] getSupportedAbis() {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
Expand Down
33 changes: 0 additions & 33 deletions tools/android_lint/baseline.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,39 +100,6 @@
column="82"/>
</issue>

<issue
id="StaticFieldLeak"
message="Do not place Android context classes in static fields (static reference to `ResourceExtractor` which has field `mContext` pointing to `Context`); this is a memory leak (and also breaks Instant Run)"
errorLine1=" private static ResourceExtractor sResourceExtractor;"
errorLine2=" ~~~~~~">
<location
file="../../../flutter/shell/platform/android/io/flutter/view/FlutterMain.java"
line="77"
column="13"/>
</issue>

<issue
id="StaticFieldLeak"
message="This AsyncTask class should be static or leaks might occur (io.flutter.view.ResourceCleaner.CleanTask)"
errorLine1=" private class CleanTask extends AsyncTask&lt;Void, Void, Void> {"
errorLine2=" ~~~~~~~~~">
<location
file="../../../flutter/shell/platform/android/io/flutter/view/ResourceCleaner.java"
line="24"
column="19"/>
</issue>

<issue
id="StaticFieldLeak"
message="This AsyncTask class should be static or leaks might occur (io.flutter.view.ResourceExtractor.ExtractTask)"
errorLine1=" private class ExtractTask extends AsyncTask&lt;Void, Void, Void> {"
errorLine2=" ~~~~~~~~~~~">
<location
file="../../../flutter/shell/platform/android/io/flutter/view/ResourceExtractor.java"
line="52"
column="19"/>
</issue>

<issue
id="UseSparseArrays"
message="Use `new SparseArray&lt;SemanticsNode>(...)` instead for better performance"
Expand Down