Fix SurfaceViewPositionListener race bugs
Bug: 29628138
1: Make windowPositionLost synchronous as that's
what the Java side was expecting
2: Make the listener ref counted as otherwise
there's a race condition with the GC, which could
end up with use-after-frees
3: Ensure that all position updates are invoked
prior to frame completion
Change-Id: Iedbc017f611ba2878a49b4586612f79249ca2fe3
diff --git a/core/jni/android_view_RenderNode.cpp b/core/jni/android_view_RenderNode.cpp
index 4fc546c..b0028e1 100644
--- a/core/jni/android_view_RenderNode.cpp
+++ b/core/jni/android_view_RenderNode.cpp
@@ -573,8 +573,9 @@
bounds.roundOut();
}
+ incStrong(0);
auto functor = std::bind(
- std::mem_fn(&SurfaceViewPositionUpdater::doUpdatePosition), this,
+ std::mem_fn(&SurfaceViewPositionUpdater::doUpdatePositionAsync), this,
(jlong) info.canvasContext.getFrameNumber(),
(jint) bounds.left, (jint) bounds.top,
(jint) bounds.right, (jint) bounds.bottom);
@@ -585,15 +586,18 @@
virtual void onPositionLost(RenderNode& node, const TreeInfo* info) override {
if (CC_UNLIKELY(!mWeakRef || (info && !info->updateWindowPositions))) return;
- if (info) {
- auto functor = std::bind(
- std::mem_fn(&SurfaceViewPositionUpdater::doNotifyPositionLost), this,
- (jlong) info->canvasContext.getFrameNumber());
-
- info->canvasContext.enqueueFrameWork(std::move(functor));
- } else {
- doNotifyPositionLost(0);
+ ATRACE_NAME("SurfaceView position lost");
+ JNIEnv* env = jnienv();
+ jobject localref = env->NewLocalRef(mWeakRef);
+ if (CC_UNLIKELY(!localref)) {
+ jnienv()->DeleteWeakGlobalRef(mWeakRef);
+ mWeakRef = nullptr;
+ return;
}
+
+ env->CallVoidMethod(localref, gSurfaceViewPositionLostMethod,
+ info ? info->canvasContext.getFrameNumber() : 0);
+ env->DeleteLocalRef(localref);
}
private:
@@ -605,36 +609,23 @@
return env;
}
- void doUpdatePosition(jlong frameNumber, jint left, jint top,
+ void doUpdatePositionAsync(jlong frameNumber, jint left, jint top,
jint right, jint bottom) {
ATRACE_NAME("Update SurfaceView position");
JNIEnv* env = jnienv();
jobject localref = env->NewLocalRef(mWeakRef);
if (CC_UNLIKELY(!localref)) {
- jnienv()->DeleteWeakGlobalRef(mWeakRef);
+ env->DeleteWeakGlobalRef(mWeakRef);
mWeakRef = nullptr;
- return;
+ } else {
+ env->CallVoidMethod(localref, gSurfaceViewPositionUpdateMethod,
+ frameNumber, left, top, right, bottom);
+ env->DeleteLocalRef(localref);
}
- env->CallVoidMethod(localref, gSurfaceViewPositionUpdateMethod,
- frameNumber, left, top, right, bottom);
- env->DeleteLocalRef(localref);
- }
-
- void doNotifyPositionLost(jlong frameNumber) {
- ATRACE_NAME("SurfaceView position lost");
-
- JNIEnv* env = jnienv();
- jobject localref = env->NewLocalRef(mWeakRef);
- if (CC_UNLIKELY(!localref)) {
- jnienv()->DeleteWeakGlobalRef(mWeakRef);
- mWeakRef = nullptr;
- return;
- }
-
- env->CallVoidMethod(localref, gSurfaceViewPositionLostMethod, frameNumber);
- env->DeleteLocalRef(localref);
+ // We need to release ourselves here
+ decStrong(0);
}
JavaVM* mVm;
diff --git a/libs/hwui/RenderNode.h b/libs/hwui/RenderNode.h
index f80be5e..47fef6d 100644
--- a/libs/hwui/RenderNode.h
+++ b/libs/hwui/RenderNode.h
@@ -232,7 +232,7 @@
// the frameNumber to appropriately batch/synchronize these transactions.
// There is no other filtering/batching to ensure that only the "final"
// state called once per frame.
- class ANDROID_API PositionListener {
+ class ANDROID_API PositionListener : public VirtualLightRefBase {
public:
virtual ~PositionListener() {}
// Called when the RenderNode's position changes
@@ -247,7 +247,7 @@
// before the RenderNode is used for drawing.
// RenderNode takes ownership of the pointer
ANDROID_API void setPositionListener(PositionListener* listener) {
- mPositionListener.reset(listener);
+ mPositionListener = listener;
}
// This is only modified in MODE_FULL, so it can be safely accessed
@@ -366,7 +366,7 @@
// mDisplayList, not mStagingDisplayList.
uint32_t mParentCount;
- std::unique_ptr<PositionListener> mPositionListener;
+ sp<PositionListener> mPositionListener;
}; // class RenderNode
} /* namespace uirenderer */
diff --git a/libs/hwui/renderthread/CanvasContext.cpp b/libs/hwui/renderthread/CanvasContext.cpp
index d4956be..ceef9c7 100644
--- a/libs/hwui/renderthread/CanvasContext.cpp
+++ b/libs/hwui/renderthread/CanvasContext.cpp
@@ -784,6 +784,7 @@
}
sp<FuncTask> task(new FuncTask());
task->func = func;
+ mFrameFences.push_back(task);
mFrameWorkProcessor->add(task);
}
