Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 414ebc6a authored by Robert Carr's avatar Robert Carr
Browse files

ViewRoot: More surgically fix child-life time. 

A more targeted version of "ViewRootImpl: Fix child lifetime".
In this version we avoid hijacking the window visibility callback
and introduce a new path.
Our contract has been that at any time after returning from
onStop, the WindowManager may destroy the clients Surface. We
see in setWindowStopped, we set the stopped state on the hardware
renderer in order to prevent further use of the Surface. However
there is nothing synchronously dispatching this change to child views
and so SurfaceView is not necessarily informed to release it's Surface
in time. Typically SurfaceView will be informed through the
onWindowVisibilityChanged callback. The signal producing this
is emitted from SystemServer prior to onStop but has to pass
through the clients handler thread. It seems this has always been
broken, and we have always had intermittent reports of EGL_BAD_ALLOC
scenarios. I speculate that elevation of WindowManager scheduling during
app close scenarios is perhaps making this more severe, as it seems to
ensure the WindowManger will win the race to destroy the Surface before
the client handler thread process onWindowVisibilityChanged (unless
the FIFO timeout is surpassed).

Test: Put Chrome in PiP. Turn screen off. No Crash!
Bug: 36561071
Change-Id: I9233ba8151c7f577b1d1044afc0c2ac3a65be4b4
parent 068b429c

core/java/android/view/SurfaceView.java

+24 −4
Original line number Diff line number Diff line
@@ -93,7 +93,7 @@ import java.util.concurrent.locks.ReentrantLock; 
 * artifacts may occur on previous versions of the platform when its window is

 * positioned asynchronously.</p>

 */

public class SurfaceView extends View {

public class SurfaceView extends View implements ViewRootImpl.WindowStoppedCallback {

    private static final String TAG = "SurfaceView";

    private static final boolean DEBUG = false;
@@ -169,6 +169,8 @@ public class SurfaceView extends View { 
    boolean mWindowVisibility = false;

    boolean mLastWindowVisibility = false;

    boolean mViewVisibility = false;

    boolean mWindowStopped = false;



    int mRequestedWidth = -1;

    int mRequestedHeight = -1;

    /* Set SurfaceView's format to 565 by default to maintain backward
@@ -226,12 +228,27 @@ public class SurfaceView extends View { 
        return mSurfaceHolder;

    }



    private void updateRequestedVisibility() {

        mRequestedVisible = mViewVisibility && mWindowVisibility && !mWindowStopped;

    }



    /** @hide */

    @Override

    public void windowStopped(boolean stopped) {

        mWindowStopped = stopped;

        updateRequestedVisibility();

        updateSurface();

    }



    @Override

    protected void onAttachedToWindow() {

        super.onAttachedToWindow();



        getViewRootImpl().addWindowStoppedCallback(this);



        mParent.requestTransparentRegion(this);

        mViewVisibility = getVisibility() == VISIBLE;

        mRequestedVisible = mViewVisibility && mWindowVisibility;

        updateRequestedVisibility();



        mAttachedToWindow = true;

        if (!mGlobalListenersAdded) {
@@ -246,7 +263,7 @@ public class SurfaceView extends View { 
    protected void onWindowVisibilityChanged(int visibility) {

        super.onWindowVisibilityChanged(visibility);

        mWindowVisibility = visibility == VISIBLE;

        mRequestedVisible = mWindowVisibility && mViewVisibility;

        updateRequestedVisibility();

        updateSurface();

    }
@@ -254,7 +271,7 @@ public class SurfaceView extends View { 
    public void setVisibility(int visibility) {

        super.setVisibility(visibility);

        mViewVisibility = visibility == VISIBLE;

        boolean newRequestedVisible = mWindowVisibility && mViewVisibility;

        boolean newRequestedVisible = mWindowVisibility && mViewVisibility && !mWindowStopped;

        if (newRequestedVisible != mRequestedVisible) {

            // our base class (View) invalidates the layout only when

            // we go from/to the GONE state. However, SurfaceView needs
@@ -278,6 +295,8 @@ public class SurfaceView extends View { 


    @Override

    protected void onDetachedFromWindow() {

        getViewRootImpl().removeWindowStoppedCallback(this);



        mAttachedToWindow = false;

        if (mGlobalListenersAdded) {

            ViewTreeObserver observer = getViewTreeObserver();
@@ -299,6 +318,7 @@ public class SurfaceView extends View { 
        mSurfaceControl = null;



        mHaveFrame = false;



        super.onDetachedFromWindow();

    }

core/java/android/view/ViewRootImpl.java

+17 −0
Original line number Diff line number Diff line
@@ -1249,6 +1249,19 @@ public final class ViewRootImpl implements ViewParent, 
        mIsAmbientMode = ambient;

    }



    interface WindowStoppedCallback {

        public void windowStopped(boolean stopped);

    }

    private final ArrayList<WindowStoppedCallback> mWindowStoppedCallbacks =  new ArrayList<>();



    void addWindowStoppedCallback(WindowStoppedCallback c) {

        mWindowStoppedCallbacks.add(c);

    }



    void removeWindowStoppedCallback(WindowStoppedCallback c) {

        mWindowStoppedCallbacks.remove(c);

    }



    void setWindowStopped(boolean stopped) {

        if (mStopped != stopped) {

            mStopped = stopped;
@@ -1264,6 +1277,10 @@ public final class ViewRootImpl implements ViewParent, 
                    renderer.destroyHardwareResources(mView);

                }

            }



            for (int i = 0; i < mWindowStoppedCallbacks.size(); i++) {

                mWindowStoppedCallbacks.get(i).windowStopped(stopped);

            }

        }

    }