Recently, it was found that SurfaceView crashed during the test on Android 10 system. The system tests before 10 will crash, but it is normal on devices after Android 11.
Error log
Native error message
signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x4
Cause: null pointer dereference
eax 00000000 ebx f017756c ecx 00000001 edx 00017af6
edi 00017af6 esi edbe9140
ebp c4c67938 esp c4c67920 eip f0169a71
backtrace:
#00 pc 0000fa71 /system/lib/libutils.so (android::RefBase::incStrong(void const*) const+33) (BuildId: 288ba3aff5b46dbd7e74be954af88b83)
#01 pc 00114620 /system/lib/libandroid_runtime.so (android::nativeDeferTransactionUntilSurface(_JNIEnv*, _jclass*, long long, long long, long long, long long)+96) (BuildId: 3643bee2c4fb7899d7781c565843060b)
#02 pc 002a2325 /system/framework/x86/boot-framework.oat (art_jni_trampoline+213) (BuildId: 38176ebc9c3cce5f657a723b08d40d487952c484)
#03 pc 0204d399 /memfd:/jit-cache (deleted) (android.view.SurfaceControl.access$2700+89)
#04 pc 020421aa /memfd:/jit-cache (deleted) (android.view.SurfaceControl$Transaction.deferTransactionUntilSurface+202)
#05 pc 02036db1 /memfd:/jit-cache (deleted) (android.view.SurfaceView.applySurfaceTransforms+177)
#06 pc 0204979f /memfd:/jit-cache (deleted) (android.view.SurfaceView.setParentSpaceRectangle+95)
#07 pc 020546cb /memfd:/jit-cache (deleted) (android.view.SurfaceView.access$200+59)
#08 pc 0204ae12 /memfd:/jit-cache (deleted) (android.view.SurfaceView$3.positionChanged+434)
#09 pc 0203c004 /memfd:/jit-cache (deleted) (android.graphics.RenderNode$CompositePositionUpdateListener.positionChanged+148)Error messages displayed in the application layer log
Fatal signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x10 in tid 6711 (hwuiTask0), pid 6669 (.crash)
Exception from repositionChild
java.lang.NullPointerException: mNativeObject is null. Have you called release() already?
at android.view.SurfaceControl.checkNotReleased(SurfaceControl.java:945)
at android.view.SurfaceControl.access$800(SurfaceControl.java:77)
at android.view.SurfaceControl$Transaction.setPosition(SurfaceControl.java:2170)
at android.view.SurfaceView.applySurfaceTransforms(SurfaceView.java:1025)
at android.view.SurfaceView.setParentSpaceRectangle(SurfaceView.java:1038)
at android.view.SurfaceView.access$200(SurfaceView.java:99)
at android.view.SurfaceView$1.positionChanged(SurfaceView.java:1081)Find a relevant Google patch on the Internet according to the error information
commit 005c63e8c6e6e5b312dc9c4631c57fb12224df30 [log] [tgz]
author Robert Carr <[email protected]> Fri Sep 20 14:31:24 2019 -0700
committer Robert Carr <[email protected]> Fri Sep 20 14:48:40 2019 -0700
tree 939eaeca0159a44bca7b5abf44f41f5096c1fb90
parent c48da419931a3a9272910f2631f815cecf05c9ba [diff]
SurfaceView: Destroy SurfaceControl from RenderThread
Currently there is a race where the UI thread can destroy
the SurfaceControl (free the C++ object) while the RenderThread
still has pending operations on it. To fix have the positionLost callback handle the destruction.
Bug: 139111930
Bug: 136808018
Test: go/wm-smoke
Change-Id: Id5225e1e47046d928f8298de38ff61662debe360
diff --git a/core/java/android/view/SurfaceView.java b/core/java/android/view/SurfaceView.java
index 90e69f3..262b9e5 100644
--- a/core/java/android/view/SurfaceView.java
+++ b/core/java/android/view/SurfaceView.java
@@ -136,6 +136,7 @@
@UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.P, trackingBug = 115609023)
boolean mIsCreating = false;
private volatile boolean mRtHandlingPositionUpdates = false;
+ private volatile boolean mRtReleaseSurfaces = false;
private final ViewTreeObserver.OnScrollChangedListener mScrollChangedListener =
this::updateSurface;
@@ -658,7 +659,14 @@
}
private void releaseSurfaces() {
+ mSurfaceAlpha = 1f;
+
synchronized (mSurfaceControlLock) {
+ if (mRtHandlingPositionUpdates) {
+ mRtReleaseSurfaces = true;
+ return;
+ }
+
if (mSurfaceControl != null) {
mTmpTransaction.remove(mSurfaceControl);
mSurfaceControl = null;
@@ -669,7 +677,6 @@
}
mTmpTransaction.apply();
}
- mSurfaceAlpha = 1f;
}
/** @hide */
@@ -1084,7 +1091,9 @@
// the synchronization would violate the rule that RT must never block
// on the UI thread which would open up potential deadlocks. The risk of
// a single-frame desync is therefore preferable for now.
- mRtHandlingPositionUpdates = true;
+ synchronized(mSurfaceControlLock) {
+ mRtHandlingPositionUpdates = true;
+ }
if (mRTLastReportedPosition.left == left
&& mRTLastReportedPosition.top == top
&& mRTLastReportedPosition.right == right
@@ -1126,6 +1135,18 @@
frameNumber);
}
mRtTransaction.hide(mSurfaceControl);
+
+ synchronized (mSurfaceControlLock) {
+ if (mRtReleaseSurfaces) {
+ mRtReleaseSurfaces = false;
+ mRtTransaction.remove(mSurfaceControl);
+ mRtTransaction.remove(mBackgroundControl);
+ mSurfaceControl = null;
+ mBackgroundControl = null;
+ }
+ mRtHandlingPositionUpdates = false;
+ }
+
mRtTransaction.apply();
}
};After updating, the SurfaceView getViewRootImpl() will always be empty. No patches for other updates were found
Because it is normal to test on Android 11, Android 11 solves this problem, so directly copy the code of Android 11’s SurfaceView. Because Android 11 has updated a lot of things, select relevant modifications to update.
diff --git a/frameworks/base/core/java/android/view/SurfaceView.java b/frameworks/base/core/java/android/view/SurfaceView.java
index d11548d687..3d1a5a3549 100644
--- a/frameworks/base/core/java/android/view/SurfaceView.java
+++ b/frameworks/base/core/java/android/view/SurfaceView.java
@@ -118,8 +118,7 @@ public class SurfaceView extends View implements ViewRootImpl.WindowStoppedCallb
boolean mDrawFinished = false;
final Rect mScreenRect = new Rect();
- SurfaceSession mSurfaceSession;
-
+ private final SurfaceSession mSurfaceSession = new SurfaceSession();
SurfaceControl mSurfaceControl;
// In the case of format changes we switch out the surface in-place
// we need to preserve the old one until the new one has drawn.
@@ -135,6 +134,7 @@ public class SurfaceView extends View implements ViewRootImpl.WindowStoppedCallb
@UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.P, trackingBug = 115609023)
boolean mIsCreating = false;
private volatile boolean mRtHandlingPositionUpdates = false;
+ private volatile boolean mRtReleaseSurfaces = false;
private final ViewTreeObserver.OnScrollChangedListener mScrollChangedListener =
this::updateSurface;
@@ -428,9 +428,16 @@ public class SurfaceView extends View implements ViewRootImpl.WindowStoppedCallb
}
private void performDrawFinished() {
+ if (mDeferredDestroySurfaceControl != null) {
+ synchronized (mSurfaceControlLock) {
+ mTmpTransaction.remove(mDeferredDestroySurfaceControl).apply();
+ mDeferredDestroySurfaceControl = null;
+ }
+ }
if (mPendingReportDraws > 0) {
mDrawFinished = true;
if (mAttachedToWindow) {
+ mParent.requestTransparentRegion(SurfaceView.this);
notifyDrawFinished();
invalidate();
}
@@ -658,7 +665,15 @@ public class SurfaceView extends View implements ViewRootImpl.WindowStoppedCallb
}
private void releaseSurfaces() {
+ mSurfaceAlpha = 1f;
+
synchronized (mSurfaceControlLock) {
+ mSurface.release();
+ if (mRtHandlingPositionUpdates) {
+ mRtReleaseSurfaces = true;
+ return;
+ }
+
if (mSurfaceControl != null) {
mTmpTransaction.remove(mSurfaceControl);
mSurfaceControl = null;
@@ -669,7 +684,6 @@ public class SurfaceView extends View implements ViewRootImpl.WindowStoppedCallb
}
mTmpTransaction.apply();
}
- mSurfaceAlpha = 1f;
}
/** @hide */
@@ -680,12 +694,21 @@ public class SurfaceView extends View implements ViewRootImpl.WindowStoppedCallb
}
return;
}
- ViewRootImpl viewRoot = getViewRootImpl();
- if (viewRoot == null || viewRoot.mSurface == null || !viewRoot.mSurface.isValid()) {
+ final ViewRootImpl viewRoot = getViewRootImpl();
+ if (viewRoot == null) {
+ if (DEBUG) {
+ Log.d(TAG, System.identityHashCode(this)
+ + " updateSurface: viewRoot null");
+ }
+ return;
+ }
+ if (viewRoot.mSurface == null || !viewRoot.mSurface.isValid()) {
if (DEBUG) {
Log.d(TAG, System.identityHashCode(this)
+ " updateSurface: no valid surface");
}
+ notifySurfaceDestroyed();
+ releaseSurfaces();
return;
}
@@ -708,9 +731,14 @@ public class SurfaceView extends View implements ViewRootImpl.WindowStoppedCallb
final boolean sizeChanged = mSurfaceWidth != myWidth || mSurfaceHeight != myHeight;
final boolean windowVisibleChanged = mWindowVisibility != mLastWindowVisibility;
boolean redrawNeeded = false;
-
- if (creating || formatChanged || sizeChanged || visibleChanged || (mUseAlpha
- && alphaChanged) || windowVisibleChanged) {
+ getLocationInSurface(mLocation);
+ final boolean positionChanged = mWindowSpaceLeft != mLocation[0]
+ || mWindowSpaceTop != mLocation[1];
+ final boolean layoutSizeChanged = getWidth() != mScreenRect.width()
+ || getHeight() != mScreenRect.height();
+ if (creating || formatChanged || sizeChanged || visibleChanged ||
+ (mUseAlpha && alphaChanged) || windowVisibleChanged ||
+ positionChanged || layoutSizeChanged) {
getLocationInWindow(mLocation);
if (DEBUG) Log.i(TAG, System.identityHashCode(this) + " "
@@ -744,7 +772,6 @@ public class SurfaceView extends View implements ViewRootImpl.WindowStoppedCallb
if (creating) {
viewRoot.createBoundsSurface(mSubLayer);
- mSurfaceSession = new SurfaceSession();
mDeferredDestroySurfaceControl = mSurfaceControl;
updateOpaqueFlag();
@@ -853,28 +880,7 @@ public class SurfaceView extends View implements ViewRootImpl.WindowStoppedCallb
final boolean surfaceChanged = creating;
if (mSurfaceCreated && (surfaceChanged || (!visible && visibleChanged))) {
mSurfaceCreated = false;
- if (mSurface.isValid()) {
- if (DEBUG) Log.i(TAG, System.identityHashCode(this) + " "
- + "visibleChanged -- surfaceDestroyed");
- callbacks = getSurfaceCallbacks();
- for (SurfaceHolder.Callback c : callbacks) {
- c.surfaceDestroyed(mSurfaceHolder);
- }
- // Since Android N the same surface may be reused and given to us
- // again by the system server at a later point. However
- // as we didn't do this in previous releases, clients weren't
- // necessarily required to clean up properly in
- // surfaceDestroyed. This leads to problems for example when
- // clients don't destroy their EGL context, and try
- // and create a new one on the same surface following reuse.
- // Since there is no valid use of the surface in-between
- // surfaceDestroyed and surfaceCreated, we force a disconnect,
- // so the next connect will always work if we end up reusing
- // the surface.
- if (mSurface.isValid()) {
- mSurface.forceScopedDisconnect();
- }
- }
+ notifySurfaceDestroyed();
}
if (creating) {
@@ -933,7 +939,6 @@ public class SurfaceView extends View implements ViewRootImpl.WindowStoppedCallb
} finally {
mIsCreating = false;
if (mSurfaceControl != null && !mSurfaceCreated) {
- mSurface.release();
releaseSurfaces();
}
}
@@ -944,48 +949,6 @@ public class SurfaceView extends View implements ViewRootImpl.WindowStoppedCallb
TAG, "Layout: x=" + mScreenRect.left + " y=" + mScreenRect.top
+ " w=" + mScreenRect.width() + " h=" + mScreenRect.height()
+ ", frame=" + mSurfaceFrame);
- } else {
- // Calculate the window position in case RT loses the window
- // and we need to fallback to a UI-thread driven position update
- getLocationInSurface(mLocation);
- final boolean positionChanged = mWindowSpaceLeft != mLocation[0]
- || mWindowSpaceTop != mLocation[1];
- final boolean layoutSizeChanged = getWidth() != mScreenRect.width()
- || getHeight() != mScreenRect.height();
- if (positionChanged || layoutSizeChanged) { // Only the position has changed
- mWindowSpaceLeft = mLocation[0];
- mWindowSpaceTop = mLocation[1];
- // For our size changed check, we keep mScreenRect.width() and mScreenRect.height()
- // in view local space.
- mLocation[0] = getWidth();
- mLocation[1] = getHeight();
-
- mScreenRect.set(mWindowSpaceLeft, mWindowSpaceTop,
- mWindowSpaceLeft + mLocation[0], mWindowSpaceTop + mLocation[1]);
-
- if (translator != null) {
- translator.translateRectInAppWindowToScreen(mScreenRect);
- }
-
- if (mSurfaceControl == null) {
- return;
- }
-
- if (!isHardwareAccelerated() || !mRtHandlingPositionUpdates) {
- try {
- if (DEBUG_POSITION) {
- Log.d(TAG, String.format("%d updateSurfacePosition UI, "
- + "position = [%d, %d, %d, %d]",
- System.identityHashCode(this),
- mScreenRect.left, mScreenRect.top,
- mScreenRect.right, mScreenRect.bottom));
- }
- setParentSpaceRectangle(mScreenRect, -1);
- } catch (Exception ex) {
- Log.e(TAG, "Exception configuring surface", ex);
- }
- }
- }
}
}
@@ -994,12 +957,6 @@ public class SurfaceView extends View implements ViewRootImpl.WindowStoppedCallb
Log.i(TAG, System.identityHashCode(this) + " "
+ "finishedDrawing");
}
-
- if (mDeferredDestroySurfaceControl != null) {
- mTmpTransaction.remove(mDeferredDestroySurfaceControl).apply();
- mDeferredDestroySurfaceControl = null;
- }
-
runOnUiThread(this::performDrawFinished);
}
@@ -1015,9 +972,8 @@ public class SurfaceView extends View implements ViewRootImpl.WindowStoppedCallb
}
private void applySurfaceTransforms(SurfaceControl surface, Rect position, long frameNumber) {
- if (frameNumber > 0) {
- final ViewRootImpl viewRoot = getViewRootImpl();
-
+ final ViewRootImpl viewRoot = getViewRootImpl();
+ if (frameNumber > 0 && viewRoot != null ) {
mRtTransaction.deferTransactionUntilSurface(surface, viewRoot.mSurface,
frameNumber);
}
@@ -1036,10 +992,10 @@ public class SurfaceView extends View implements ViewRootImpl.WindowStoppedCallb
final ViewRootImpl viewRoot = getViewRootImpl();
applySurfaceTransforms(mSurfaceControl, position, frameNumber);
-
- applyChildSurfaceTransaction_renderWorker(mRtTransaction, viewRoot.mSurface,
+ if (frameNumber > 0 && viewRoot != null ) {
+ applyChildSurfaceTransaction_renderWorker(mRtTransaction, viewRoot.mSurface,
frameNumber);
-
+ }
mRtTransaction.apply();
}
@@ -1062,7 +1018,9 @@ public class SurfaceView extends View implements ViewRootImpl.WindowStoppedCallb
// the synchronization would violate the rule that RT must never block
// on the UI thread which would open up potential deadlocks. The risk of
// a single-frame desync is therefore preferable for now.
- mRtHandlingPositionUpdates = true;
+ synchronized(mSurfaceControlLock) {
+ mRtHandlingPositionUpdates = true;
+ }
if (mRTLastReportedPosition.left == left
&& mRTLastReportedPosition.top == top
&& mRTLastReportedPosition.right == right
@@ -1096,14 +1054,27 @@ public class SurfaceView extends View implements ViewRootImpl.WindowStoppedCallb
if (mSurfaceControl == null) {
return;
}
+ final ViewRootImpl viewRoot = getViewRootImpl();
- if (frameNumber > 0) {
- final ViewRootImpl viewRoot = getViewRootImpl();
- mRtTransaction.deferTransactionUntilSurface(mSurfaceControl, viewRoot.mSurface,
- frameNumber);
+ synchronized (mSurfaceControlLock) {
+ if (frameNumber > 0 && viewRoot != null) {
+ if (viewRoot.mSurface.isValid()) {
+ mRtTransaction.deferTransactionUntilSurface(mSurfaceControl, viewRoot.mSurface,
+ frameNumber);
+ }
+ }
+ mRtTransaction.hide(mSurfaceControl);
+ if (mRtReleaseSurfaces) {
+ mRtReleaseSurfaces = false;
+ mRtTransaction.remove(mSurfaceControl);
+ mRtTransaction.remove(mBackgroundControl);
+ mSurfaceControl = null;
+ mBackgroundControl = null;
+ }
+ mRtHandlingPositionUpdates = false;
}
- mRtTransaction.hide(mSurfaceControl);
+
mRtTransaction.apply();
}
};
@@ -1348,4 +1319,29 @@ public class SurfaceView extends View implements ViewRootImpl.WindowStoppedCallb
public SurfaceControl getSurfaceControl() {
return mSurfaceControl;
}
+
+ private void notifySurfaceDestroyed() {
+ if (mSurface.isValid()) {
+ if (DEBUG) Log.i(TAG, System.identityHashCode(this) + " "
+ + "surfaceDestroyed");
+ SurfaceHolder.Callback[] callbacks = getSurfaceCallbacks();
+ for (SurfaceHolder.Callback c : callbacks) {
+ c.surfaceDestroyed(mSurfaceHolder);
+ }
+ // Since Android N the same surface may be reused and given to us
+ // again by the system server at a later point. However
+ // as we didn't do this in previous releases, clients weren't
+ // necessarily required to clean up properly in
+ // surfaceDestroyed. This leads to problems for example when
+ // clients don't destroy their EGL context, and try
+ // and create a new one on the same surface following reuse.
+ // Since there is no valid use of the surface in-between
+ // surfaceDestroyed and surfaceCreated, we force a disconnect,
+ // so the next connect will always work if we end up reusing
+ // the surface.
+ if (mSurface.isValid()) {
+ mSurface.forceScopedDisconnect();
+ }
+ }
+ }
}
After modification, the test is normal, and the application will not crash, but an error log message will pop up from time to time
E/Layer: [SurfaceView – com. crash/com. crash. MainActivity # 3] No local sync point found
Check Layer.cpp when encountering this exception is to skip the logic, so it does not care, it is not clear if there are any other problems with this change.
741 bool Layer::applyPendingStates(State* stateToCommit) {
742 bool stateUpdateAvailable = false;
743 while (!mPendingStates.empty()) {
744 if (mPendingStates[0].barrierLayer_legacy != nullptr) {
745 if (mRemoteSyncPoints.empty()) {
746 // If we don't have a sync point for this, apply it anyway. It
747 // will be visually wrong, but it should keep us from getting
748 // into too much trouble.
749 ALOGE("[%s] No local sync point found", mName.string());
750 popPendingState(stateToCommit);
751 stateUpdateAvailable = true;
752 continue;
753 }
754
755 if (mRemoteSyncPoints.front()->getFrameNumber() !=
756 mPendingStates[0].frameNumber_legacy) {
757 ALOGE("[%s] Unexpected sync point frame number found", mName.string());
758
759 // Signal our end of the sync point and then dispose of it
760 mRemoteSyncPoints.front()->setTransactionApplied();
761 mRemoteSyncPoints.pop_front();
762 continue;
763 }
764
765 if (mRemoteSyncPoints.front()->frameIsAvailable()) {
766 ATRACE_NAME("frameIsAvailable");
767 // Apply the state update
768 popPendingState(stateToCommit);
769 stateUpdateAvailable = true;
770
771 // Signal our end of the sync point and then dispose of it
772 mRemoteSyncPoints.front()->setTransactionApplied();
773 mRemoteSyncPoints.pop_front();
774 } else {
775 ATRACE_NAME("!frameIsAvailable");
776 break;
777 }
778 } else {
779 popPendingState(stateToCommit);
780 stateUpdateAvailable = true;
781 }
782 }
783
784 // If we still have pending updates, wake SurfaceFlinger back up and point
785 // it at this layer so we can process them
786 if (!mPendingStates.empty()) {
787 setTransactionFlags(eTransactionNeeded);
788 mFlinger->setTransactionFlags(eTraversalNeeded);
789 }
790
791 mCurrentState.modified = false;
792 return stateUpdateAvailable;
793 }
