From a848ce8efd7ef24041a4d5e0ae466dbb981f069a Mon Sep 17 00:00:00 2001 From: Seth Kirby Date: Thu, 21 Jul 2016 18:17:31 -0700 Subject: [PATCH] Fix race conditions in DrawView. Summary: Currently we have race conditions in DrawView related to isViewGroupClipped, where we create a copy of the DrawView before we update the clipping state, and think the view is unclipped in the next iteration. Also we are sometimes creating a DrawView with a reactTag of 0. This fixes both, and is part of the upcoming DrawView bounds change, but is a separate issue that is live in current source. Reviewed By: ahmedre Differential Revision: D3598499 --- .../com/facebook/react/flat/DrawView.java | 29 +++++++- .../facebook/react/flat/FlatShadowNode.java | 19 ++++- .../facebook/react/flat/FlatViewGroup.java | 74 ++++++++++++++----- 3 files changed, 98 insertions(+), 24 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/DrawView.java b/ReactAndroid/src/main/java/com/facebook/react/flat/DrawView.java index 207acef53..530c19478 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/flat/DrawView.java +++ b/ReactAndroid/src/main/java/com/facebook/react/flat/DrawView.java @@ -14,15 +14,38 @@ import android.graphics.Canvas; /* package */ final class DrawView extends AbstractClippingDrawCommand { /* package */ final int reactTag; - /* package */ boolean isViewGroupClipped; + // Indicates if the DrawView is frozen. If it is frozen then any setting of the clip bounds + // should create a new DrawView. + private boolean mFrozen; + // Indicates whether this DrawView has been previously drawn. If it has been drawn, then we know + // that the bounds haven't changed, as a bounds change would trigger a new DrawView, which will + // set this to false for the new DrawView. Leaving this as package for direct access, but this + // should only be set from draw in DrawView, to avoid race conditions. + /* package */ boolean mPreviouslyDrawn; - public DrawView(int reactTag, float clipLeft, float clipTop, float clipRight, float clipBottom) { + public DrawView(int reactTag) { this.reactTag = reactTag; - setClipBounds(clipLeft, clipTop, clipRight, clipBottom); + } + + public DrawView collectDrawView( + float clipLeft, + float clipTop, + float clipRight, + float clipBottom) { + if (mFrozen) { + return clipBoundsMatch(clipLeft, clipTop, clipRight, clipBottom) ? + this : + new DrawView(reactTag).collectDrawView(clipLeft, clipTop, clipRight, clipBottom); + } else { + mFrozen = true; + setClipBounds(clipLeft, clipTop, clipRight, clipBottom); + return this; + } } @Override public void draw(FlatViewGroup parent, Canvas canvas) { + mPreviouslyDrawn = true; if (mNeedsClipping) { canvas.save(Canvas.CLIP_SAVE_FLAG); applyClipping(canvas); diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/FlatShadowNode.java b/ReactAndroid/src/main/java/com/facebook/react/flat/FlatShadowNode.java index 10ee7fb93..28cc4fa45 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/flat/FlatShadowNode.java +++ b/ReactAndroid/src/main/java/com/facebook/react/flat/FlatShadowNode.java @@ -43,6 +43,9 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper; private static final String PROP_REMOVE_CLIPPED_SUBVIEWS = ReactClippingViewGroupHelper.PROP_REMOVE_CLIPPED_SUBVIEWS; private static final Rect LOGICAL_OFFSET_EMPTY = new Rect(); + // When we first initialize a backing view, we create a view we are going to throw away anyway, + // so instead initialize with a shared view. + private static final DrawView EMPTY_DRAW_VIEW = new DrawView(0); private DrawCommand[] mDrawCommands = DrawCommand.EMPTY_ARRAY; private AttachDetachListener[] mAttachDetachListeners = AttachDetachListener.EMPTY_ARRAY; @@ -455,7 +458,9 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper; } if (mDrawView == null) { - mDrawView = new DrawView(getReactTag(), 0, 0, 0, 0); + // Create a new DrawView, but we might not know our react tag yet, so set it to 0 in the + // meantime. + mDrawView = EMPTY_DRAW_VIEW; invalidate(); // reset NodeRegion to allow it getting garbage-collected @@ -464,10 +469,16 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper; } /* package */ final DrawView collectDrawView(float left, float top, float right, float bottom) { - if (!Assertions.assumeNotNull(mDrawView).clipBoundsMatch(left, top, right, bottom)) { - mDrawView = new DrawView(getReactTag(), left, top, right, bottom); + Assertions.assumeNotNull(mDrawView); + if (mDrawView.reactTag == 0) { + // This is the first time we have collected this DrawView, but we have to create a new + // DrawView anyway, as reactTag is final. + mDrawView = new DrawView(getReactTag()).collectDrawView(left, top, right, bottom); + } else { + // We have collected the DrawView before, so the react tag is correct, but we may need a new + // copy with updated bounds. If the bounds match, the same view is returned. + mDrawView = mDrawView.collectDrawView(left, top, right, bottom); } - return mDrawView; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/flat/FlatViewGroup.java b/ReactAndroid/src/main/java/com/facebook/react/flat/FlatViewGroup.java index 45dda6a1c..02a9088ab 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/flat/FlatViewGroup.java +++ b/ReactAndroid/src/main/java/com/facebook/react/flat/FlatViewGroup.java @@ -178,6 +178,18 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper; mAndroidDebugDraw = true; } + private void clip(int id, View view) { + mClippedSubviews.put(id, view); + } + + private void unclip(int id) { + mClippedSubviews.remove(id); + } + + private boolean isClipped(int id) { + return mClippedSubviews.containsKey(id); + } + @Override public void dispatchDraw(Canvas canvas) { mAndroidDebugDraw = false; @@ -186,7 +198,7 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper; if (mRemoveClippedSubviews) { for (DrawCommand drawCommand : mDrawCommands) { if (drawCommand instanceof DrawView) { - if (!((DrawView) drawCommand).isViewGroupClipped) { + if (!isClipped(((DrawView) drawCommand).reactTag)) { drawCommand.draw(this, canvas); } // else, don't draw, and don't increment index @@ -219,7 +231,7 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper; private void debugDraw(Canvas canvas) { for (DrawCommand drawCommand : mDrawCommands) { if (drawCommand instanceof DrawView) { - if (!((DrawView) drawCommand).isViewGroupClipped) { + if (!isClipped(((DrawView) drawCommand).reactTag)) { drawCommand.debugDraw(this, canvas); } // else, don't draw, and don't increment index @@ -672,6 +684,29 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper; mNodeRegions = nodeRegions; } + /** + * Mount a list of views to add, and dismount a list of views to detach. Ids will not appear in + * both lists, aka: + * set(viewsToAdd + viewsToDetach).size() == viewsToAdd.length + viewsToDetach.length + * + * Every time we get any change in the views in a FlatViewGroup, we detach all views first, then + * reattach / remove them as needed. viewsToAdd is odd in that the ids also specify whether + * the view is new to us, or if we were already the parent. If it is new to us, then the id has + * a positive value, otherwise we are already the parent, but it was previously detached, since we + * detach everything when anything changes. + * + * The reason we detach everything is that a single detach is on the order of O(n), as in the + * average case we have to move half of the views one position to the right, and a single add is + * the same. Removing all views is also on the order of O(n), as you delete everything backward + * from the end, while adding a new set of views is also on the order of O(n), as you just add + * them all back in order. ArrayLists are weird. + * + * @param viewResolver Resolves the views from their id. + * @param viewsToAdd id of views to add if they weren't just attached to us, or -id if they are + * just being reattached. + * @param viewsToDetach id of views that we don't own anymore. They either moved to a new parent, + * or are being removed entirely. + */ /* package */ void mountViews(ViewResolver viewResolver, int[] viewsToAdd, int[] viewsToDetach) { for (int viewToAdd : viewsToAdd) { if (viewToAdd > 0) { @@ -681,11 +716,20 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper; View view = ensureViewHasNoParent(viewResolver.getView(-viewToAdd)); if (mRemoveClippedSubviews) { DrawView drawView = Assertions.assertNotNull(mDrawViewMap.get(-viewToAdd)); - // If the view is clipped, we don't need to attach it. - if (!drawView.isViewGroupClipped) { + if (!drawView.mPreviouslyDrawn) { + // The DrawView has not been drawn before, which means the bounds changed and triggered + // a new DrawView when it was collected from the shadow node. We have a view with the + // same id temporarily detached, but we no longer know the bounds. + unclip(drawView.reactTag); + attachViewToParent(view, -1, ensureLayoutParams(view.getLayoutParams())); + } else if (!isClipped(drawView.reactTag)) { + // The DrawView has been drawn before, and is not clipped. Attach it, and it will get + // removed if we update the clipping rect. attachViewToParent(view, -1, ensureLayoutParams(view.getLayoutParams())); } + // The DrawView has been previously drawn and is clipped, so don't attach it. } else { + // We aren't clipping, so attach all the things. attachViewToParent(view, -1, ensureLayoutParams(view.getLayoutParams())); } } @@ -694,13 +738,13 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper; for (int viewToDetach : viewsToDetach) { View view = viewResolver.getView(viewToDetach); if (view.getParent() != null) { - removeViewInLayout(view); + throw new RuntimeException("Trying to remove view not owned by FlatViewGroup"); } else { removeDetachedView(view, false); } if (mRemoveClippedSubviews) { - mClippedSubviews.remove(viewToDetach); + unclip(viewToDetach); } } @@ -830,7 +874,7 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper; } // Returns true if a view is currently animating. - static boolean animating(View view, Rect clippingRect) { + static boolean animating(View view) { Animation animation = view.getAnimation(); return animation != null && !animation.hasEnded(); } @@ -876,23 +920,19 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper; if (view == null) { // Not clipped, visible view = getChildAt(index++); - if (!animating(view, clippingRect) && !withinBounds(view, clippingRect)) { + if (!animating(view) && !withinBounds(view, clippingRect)) { // Now off the screen. Don't invalidate in this case, as the canvas should not be // redrawn unless new elements are coming onscreen. - mClippedSubviews.put(view.getId(), view); + clip(drawView.reactTag, view); detachViewFromParent(--index); - drawView.isViewGroupClipped = true; } } else { - // Clipped, invisible. + // Clipped, invisible. We obviously aren't animating here, as if we were then we would not + // have clipped in the first place. if (withinBounds(view, clippingRect)) { // Now on the screen. Invalidate as we have a new element to draw. - attachViewToParent( - view, - index++, - ensureLayoutParams(view.getLayoutParams())); - mClippedSubviews.remove(view.getId()); - drawView.isViewGroupClipped = false; + unclip(drawView.reactTag); + attachViewToParent(view, index++, ensureLayoutParams(view.getLayoutParams())); needsInvalidate = true; } }