Android/frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Make Paint/Shader/ColorFilter#getNativeInstance conditionally thread-safe

Browse Source 
Bug: 159246162
Bug: 154358781
Test: PaintNativeInstanceTest.kt

Although the android.graphics package makes no promises regarding
thread-safety, it is not obvious to clients how they might call
getNativeInstance from multiple threads. If one thread sets up a Paint,
and then two threads use it, this sets up a race condition for setting
up the native SkPaint. This seems to have been observed in internal
apps. Put accesses and modifications to these native objects inside
a synchronized block, so that we don't end up with a situation where
two instances of mCleaner attempt to dereference the same native object.

Add tests in PaintNativeInstanceTest.kt

Change-Id: I10fa0958ea216bfbc79667c6217dd6570b3eb110
This commit is contained in:
Leon Scroggins III
2020-06-30 14:50:06 -04:00
parent d732652c9c
commit 69b95fa2a5
5 changed files with 186 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

161
core/tests/coretests/src/android/graphics/PaintNativeInstanceTest.kt Normal file
View File

@@ -0,0 +1,161 @@
/*
* Copyright 2020 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package android.graphics
import androidx.test.ext.junit.runners.AndroidJUnit4
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Test
import org.junit.runner.RunWith
import java.util.concurrent.Callable
import java.util.concurrent.CountDownLatch
import java.util.concurrent.Executors
import java.util.concurrent.TimeUnit
// Verify that various calls to getNativeInstance do not deadlock or otherwise fail.
@RunWith(AndroidJUnit4::class)
class PaintNativeInstanceTest {
// Force a GC after each test, so that if there was a double free, it would happen now, rather
// than later during other tests.
@After
fun runGcAndFinalizersSync() {
Runtime.getRuntime().gc()
Runtime.getRuntime().runFinalization()
val fence = CountDownLatch(1)
object : Any() {
@Throws(Throwable::class)
protected fun finalize() = fence.countDown()
}
try {
do {
Runtime.getRuntime().gc()
Runtime.getRuntime().runFinalization()
} while (!fence.await(100, TimeUnit.MILLISECONDS))
} catch (ex: InterruptedException) {
throw RuntimeException(ex)
}
}
private fun setupComposeShader(test: (Paint, ComposeShader, Shader, Shader) -> Unit) {
val size = 255f
val blue = LinearGradient(0f, 0f, size, 0f, Color.GREEN, Color.BLUE,
Shader.TileMode.MIRROR)
val red = LinearGradient(0f, 0f, 0f, size, Color.GREEN, Color.RED,
Shader.TileMode.MIRROR)
val compose = ComposeShader(blue, red, BlendMode.SCREEN)
val paint = Paint().apply {
shader = compose
}
test(paint, compose, blue, red)
}
// Change the matrix arbitrarily to invalidate the shader.
private fun Shader.changeMatrix() {
val matrix = Matrix().apply {
setScale(2f, 2f)
}
setLocalMatrix(matrix)
}
@Test
fun testUnchangedPaintNativeInstance() = setupComposeShader {
paint, compose, shaderA, shaderB ->
val nativeInstance = paint.nativeInstance
for (shader in listOf(compose, shaderA, shaderB)) {
shader.changeMatrix()
// Although the shader is invalidated, the Paint's nativeInstance remains the same.
assertEquals(nativeInstance, paint.nativeInstance)
}
}
@Test
fun testInvalidateSubShader() = setupComposeShader {
paint, compose, shaderA, shaderB ->
// Trigger the creation of native objects.
shaderA.nativeInstance
compose.nativeInstance
val instanceB = shaderB.nativeInstance
// Changing shaderA's matrix invalidates shaderA and compose. A new instance will be lazily
// created for each of them. We cannot assert that the new nativeInstance does not match,
// since it might be allocated at the same location. But we can verify that shaderB did not
// change, and that there was no deadlock.
shaderA.changeMatrix()
assertEquals(instanceB, shaderB.nativeInstance)
paint.nativeInstance
}
@Test
fun testInvalidateSubShaderDraw() = setupComposeShader {
paint, _, _, shaderB ->
val original = PaintTask(paint).call()
// Change one of the subshaders and verify that the paint now draws differently.
shaderB.changeMatrix()
val changed = PaintTask(paint).call()
assertFalse(changed.sameAs(original))
}
/*
* This task will trigger the creation of native objects, if they have not already been
* created.
*/
class PaintTask(private val mPaint: Paint) : Callable<Bitmap> {
private val size = 255 // matches size of gradients in setupComposeShader
override fun call(): Bitmap = Bitmap.createBitmap(size, size,
Bitmap.Config.ARGB_8888).apply {
val canvas = Canvas(this)
canvas.drawPaint(mPaint)
}
}
@Test
fun testMultiThreadShader() = setupComposeShader {
paint, _, _, _ ->
// Create an arbitrary number of tasks and try to start them at approximately the same time.
// They will race to create the native objects, but this should be safe.
val tasks = List(5) { PaintTask(paint) }
val results = Executors.newCachedThreadPool().invokeAll(tasks)
var expectedBitmap: Bitmap? = null
for (result in results) {
if (expectedBitmap == null) {
expectedBitmap = result.get()
} else {
assertTrue(expectedBitmap.sameAs(result.get()))
}
}
}
@Test
fun testMultiThreadColorFilter() {
val paint = Paint().apply {
color = Color.MAGENTA
colorFilter = LightingColorFilter(Color.BLUE, Color.GREEN)
}
// Create an arbitrary number of tasks and try to start them at approximately the same time.
// They will race to create the native objects, but this should be safe.
val tasks = List(5) { PaintTask(paint) }
val results = Executors.newCachedThreadPool().invokeAll(tasks)
for (result in results) {
assertEquals(Color.CYAN, result.get().getPixel(0, 0))
}
}
}

4
graphics/java/android/graphics/ColorFilter.java
View File

@@ -48,7 +48,7 @@ public class ColorFilter {
return 0;
}
void discardNativeInstance() {
synchronized final void discardNativeInstance() {
if (mNativeInstance != 0) {
mCleaner.run();
mCleaner = null;
@@ -57,7 +57,7 @@ public class ColorFilter {
}
/** @hide */
public long getNativeInstance() {
public synchronized final long getNativeInstance() {
if (mNativeInstance == 0) {
mNativeInstance = createNativeInstance();

10
graphics/java/android/graphics/ComposeShader.java
View File

@@ -95,13 +95,9 @@ public class ComposeShader extends Shader {
/** @hide */
@Override
protected void verifyNativeInstance() {
if (mShaderA.getNativeInstance() != mNativeInstanceShaderA
|| mShaderB.getNativeInstance() != mNativeInstanceShaderB) {
// Child shader native instance has been updated,
// so our cached native instance is no longer valid - discard it
discardNativeInstance();
}
protected boolean shouldDiscardNativeInstance() {
return mShaderA.getNativeInstance() != mNativeInstanceShaderA
|| mShaderB.getNativeInstance() != mNativeInstanceShaderB;
}
private static native long nativeCreate(long nativeMatrix,

7
graphics/java/android/graphics/Paint.java
View File

@@ -674,10 +674,15 @@ public class Paint {
* Return the pointer to the native object while ensuring that any
* mutable objects that are attached to the paint are also up-to-date.
*
* Note: Although this method is |synchronized|, this is simply so it
* is not thread-hostile to multiple threads calling this method. It
* is still unsafe to attempt to change the Shader/ColorFilter while
* another thread attempts to access the native object.
*
* @hide
*/
@UnsupportedAppUsage
public long getNativeInstance() {
public synchronized long getNativeInstance() {
long newNativeShader = mShader == null ? 0 : mShader.getNativeInstance();
if (newNativeShader != mNativeShader) {
mNativeShader = newNativeShader;

21
graphics/java/android/graphics/Shader.java
View File

@@ -150,7 +150,12 @@ public class Shader {
/**
* @hide Only to be used by subclasses in the graphics package.
*/
protected final void discardNativeInstance() {
protected synchronized final void discardNativeInstance() {
discardNativeInstanceLocked();
}
// For calling inside a synchronized method.
private void discardNativeInstanceLocked() {
if (mNativeInstance != 0) {
mCleaner.run();
mCleaner = null;
@@ -159,11 +164,12 @@ public class Shader {
}
/**
* Callback for subclasses to call {@link #discardNativeInstance()} if the most recently
* constructed native instance is no longer valid.
* Callback for subclasses to specify whether the most recently
* constructed native instance is still valid.
* @hide Only to be used by subclasses in the graphics package.
*/
protected void verifyNativeInstance() {
protected boolean shouldDiscardNativeInstance() {
return false;
}
@@ -171,9 +177,10 @@ public class Shader {
* @hide so it can be called by android.graphics.drawable but must not be called from outside
* the module.
*/
public final long getNativeInstance() {
// verify mNativeInstance is valid
verifyNativeInstance();
public synchronized final long getNativeInstance() {
if (shouldDiscardNativeInstance()) {
discardNativeInstanceLocked();
}
if (mNativeInstance == 0) {
mNativeInstance = createNativeInstance(mLocalMatrix == null