Skip to content

Commit

Permalink
Proper container children cleanup (#477)
Browse files Browse the repository at this point in the history 
This change attempts to fix a problem when screen fragment views aren't properly detached from their parents despite `recycleView` being invoked on them.

The final outcome of the issue was that the app would crash with an error that view it tries to attach already has a parent. We observed this issue happening in production but with no clear repro steps the problem appeared to be a bit cryptic. Our investigation shown that `recycleView` does not reset the parent because the view is not listed in `mDisappearingChildren` in `ViewGroup`'s implementation.

We suspect that the above was due to us triggering `clearDisappearingChildren` directly in `Screen` and hence this PR updates that. 

The second change we make here is that we no longer attempt to reuse the main fragment view (crated by `onCreateView`). We used a custom container for stack fragments anyways and in this PR we adapt the same approach for `ScreenFragment` and make it so that the container view is not recycled (only the contents of the container that is managed by React Native is what we recycle).

Next, we update detach handler in containers. As we don't know the exact source of the parent reference cleanup we suspect that what is happening is that some children views remain attached to container past the child fragment lifecycle. This is due to the fact that fragments may perform transitions which may delay the process of child views being detached. As when the container is detached, its children are no longer visible we can safely drop all the children there. We restore fragment hierarchy when the container is attached anyways.
 
Lastly, we make small refactor in `ScreenContainer.java` which aims to extract fragment selection logic out of on attach handler. We may consider moving it elsewhere in the future or consider making it "asynchronous". This leads to some nullability check changes in that file as well as a new methods `setFragmentManager` being added.
  • Loading branch information
@kmagiera
kmagiera authored Apr 24, 2020
1 parent c0fb507 commit 36fb635
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 80 deletions.
6 changes: 0 additions & 6 deletions android/src/main/java/com/swmansion/rnscreens/Screen.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,12 +106,6 @@ public void runGuarded() {
}
}

@Override
protected void onDetachedFromWindow() {
super.onDetachedFromWindow();
clearDisappearingChildren();
}

@Override
protected void onAttachedToWindow() {
super.onAttachedToWindow();
Expand Down
80 changes: 48 additions & 32 deletions android/src/main/java/com/swmansion/rnscreens/ScreenContainer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,12 @@ protected Screen getScreenAt(int index) {
return mScreenFragments.get(index).getScreen();
}

private FragmentManager findFragmentManager() {
private void setFragmentManager(FragmentManager fm) {
mFragmentManager = fm;
updateIfNeeded();
}

private void setupFragmentManager() {
ViewParent parent = this;
// We traverse view hierarchy up until we find screen parent or a root view
while (!(parent instanceof ReactRootView || parent instanceof Screen) && parent.getParent() != null) {
Expand All @@ -132,7 +137,9 @@ private FragmentManager findFragmentManager() {
// If parent is of type Screen it means we are inside a nested fragment structure.
// Otherwise we expect to connect directly with root view and get root fragment manager
if (parent instanceof Screen) {
return ((Screen) parent).getFragment().getChildFragmentManager();
Fragment screenFragment = ((Screen) parent).getFragment();
setFragmentManager(screenFragment.getChildFragmentManager());
return;
}

// we expect top level view to be of type ReactRootView, this isn't really necessary but in order
Expand All @@ -153,7 +160,7 @@ private FragmentManager findFragmentManager() {
throw new IllegalStateException(
"In order to use RNScreens components your app's activity need to extend ReactFragmentActivity or ReactCompatActivity");
}
return ((FragmentActivity) context).getSupportFragmentManager();
setFragmentManager(((FragmentActivity) context).getSupportFragmentManager());
}

protected FragmentTransaction getOrCreateTransaction() {
Expand Down Expand Up @@ -211,23 +218,7 @@ protected void onAttachedToWindow() {
super.onAttachedToWindow();
mIsAttached = true;
mNeedUpdate = true;
mFragmentManager = findFragmentManager();
updateIfNeeded();
}

@Override
protected void onDetachedFromWindow() {
// if there are pending transactions and this view is about to get detached we need to perform
// them here as otherwise fragment manager will crash because it won't be able to find container
// view. We also need to make sure all the fragments attached to the given container are removed
// from fragment manager as in some cases fragment manager may be reused and in such case it'd
// attempt to reattach previously registered fragments that are not removed
if (mFragmentManager != null && !mFragmentManager.isDestroyed()) {
removeMyFragments();
mFragmentManager.executePendingTransactions();
}
super.onDetachedFromWindow();
mIsAttached = false;
setupFragmentManager();
}

/**
Expand All @@ -236,6 +227,7 @@ protected void onDetachedFromWindow() {
private void removeMyFragments() {
FragmentTransaction transaction = mFragmentManager.beginTransaction();
boolean hasFragments = false;

for (Fragment fragment : mFragmentManager.getFragments()) {
if (fragment instanceof ScreenFragment && ((ScreenFragment) fragment).mScreenView.getContainer() == this) {
transaction.remove(fragment);
Expand All @@ -247,6 +239,31 @@ private void removeMyFragments() {
}
}

@Override
protected void onDetachedFromWindow() {
// if there are pending transactions and this view is about to get detached we need to perform
// them here as otherwise fragment manager will crash because it won't be able to find container
// view. We also need to make sure all the fragments attached to the given container are removed
// from fragment manager as in some cases fragment manager may be reused and in such case it'd
// attempt to reattach previously registered fragments that are not removed
if (mFragmentManager != null && !mFragmentManager.isDestroyed()) {
removeMyFragments();
mFragmentManager.executePendingTransactions();
}
super.onDetachedFromWindow();
mIsAttached = false;
// When fragment container view is detached we force all its children to be removed.
// It is because children screens are controlled by their fragments, which can often have a
// delayed lifecycle (due to transitions). As a result due to ongoing transitions the fragment
// may choose not to remove the view despite the parent container being completely detached
// from the view hierarchy until the transition is over. In such a case when the container gets
// re-attached while tre transition is ongoing, the child view would still be there and we'd
// attept to re-attach it to with a misconfigured fragment. This would result in a crash. To
// avoid it we clear all the children here as we attach all the child fragments when the container
// is reattached anyways.
removeAllViews();
}

@Override
protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
super.onMeasure(widthMeasureSpec, heightMeasureSpec);
Expand All @@ -256,25 +273,24 @@ protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
}

private void updateIfNeeded() {
if (!mNeedUpdate || !mIsAttached) {
if (!mNeedUpdate || !mIsAttached || mFragmentManager == null) {
return;
}
mNeedUpdate = false;
onUpdate();
}

private final void onUpdate() {
if (mFragmentManager != null) {
// We double check if fragment manager have any pending transactions to run.
// In performUpdate we often check whether some fragments are added to
// manager to avoid adding them for the second time (which result in crash).
// By design performUpdate should be called at most once per frame, so this
// should never happen, but in case there are some pending transaction we
// need to flush them here such that Fragment#isAdded checks reflect the
// reality and that we don't have enqueued fragment add commands that will
// execute shortly and cause "Fragment already added" crash.
mFragmentManager.executePendingTransactions();
}
// We double check if fragment manager have any pending transactions to run.
// In performUpdate we often check whether some fragments are added to
// manager to avoid adding them for the second time (which result in crash).
// By design performUpdate should be called at most once per frame, so this
// should never happen, but in case there are some pending transaction we
// need to flush them here such that Fragment#isAdded checks reflect the
// reality and that we don't have enqueued fragment add commands that will
// execute shortly and cause "Fragment already added" crash.
mFragmentManager.executePendingTransactions();

performUpdate();
}

Expand Down
20 changes: 7 additions & 13 deletions android/src/main/java/com/swmansion/rnscreens/ScreenFragment.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import android.view.View;
import android.view.ViewGroup;
import android.view.ViewParent;
import android.widget.FrameLayout;

import androidx.annotation.Nullable;
import androidx.fragment.app.Fragment;
Expand All @@ -23,12 +24,6 @@ protected static View recycleView(View view) {
((ViewGroup) parent).endViewTransition(view);
((ViewGroup) parent).removeView(view);
}
// the below check is here to help determine if crashes due to IllegalStateException with
// "child already has a parent" caused when view is created are there because for some reason
// the above code fails detach the view from its parent
if (view.getParent() != null) {
throw new IllegalStateException("Recycled fragment view is not detached from its parent");
}

// view detached from fragment manager get their visibility changed to GONE after their state is
// dumped. Since we don't restore the state but want to reuse the view we need to change visibility
Expand All @@ -53,7 +48,12 @@ public ScreenFragment(Screen screenView) {
public View onCreateView(LayoutInflater inflater,
@Nullable ViewGroup container,
@Nullable Bundle savedInstanceState) {
return recycleView(mScreenView);
FrameLayout wrapper = new FrameLayout(getContext());
FrameLayout.LayoutParams params = new FrameLayout.LayoutParams(
ViewGroup.LayoutParams.MATCH_PARENT, ViewGroup.LayoutParams.MATCH_PARENT);
mScreenView.setLayoutParams(params);
wrapper.addView(recycleView(mScreenView));
return wrapper;
}

public Screen getScreen() {
Expand All @@ -74,12 +74,6 @@ public void onViewAnimationEnd() {
dispatchOnAppear();
}

@Override
public void onDestroyView() {
super.onDestroyView();
recycleView(getView());
}

@Override
public void onDestroy() {
super.onDestroy();
Expand Down
49 changes: 20 additions & 29 deletions android/src/main/java/com/swmansion/rnscreens/ScreenStackFragment.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ protected void onAnimationEnd() {
private AppBarLayout mAppBarLayout;
private Toolbar mToolbar;
private boolean mShadowHidden;
private CoordinatorLayout mScreenRootView;

@SuppressLint("ValidFragment")
public ScreenStackFragment(Screen screenView) {
Expand Down Expand Up @@ -82,31 +81,6 @@ public void onStackUpdate() {
}
}

private CoordinatorLayout configureView() {
CoordinatorLayout view = new NotifyingCoordinatorLayout(getContext(), this);
CoordinatorLayout.LayoutParams params = new CoordinatorLayout.LayoutParams(
LinearLayout.LayoutParams.MATCH_PARENT, LinearLayout.LayoutParams.MATCH_PARENT);
params.setBehavior(new AppBarLayout.ScrollingViewBehavior());
mScreenView.setLayoutParams(params);
view.addView(mScreenView);

mAppBarLayout = new AppBarLayout(getContext());
// By default AppBarLayout will have a background color set but since we cover the whole layout
// with toolbar (that can be semi-transparent) the bar layout background color does not pay a
// role. On top of that it breaks screens animations when alfa offscreen compositing is off
// (which is the default)
mAppBarLayout.setBackgroundColor(Color.TRANSPARENT);
mAppBarLayout.setLayoutParams(new AppBarLayout.LayoutParams(
AppBarLayout.LayoutParams.MATCH_PARENT, AppBarLayout.LayoutParams.WRAP_CONTENT));
view.addView(mAppBarLayout);

if (mToolbar != null) {
mAppBarLayout.addView(mToolbar);
}

return view;
}

@Override
public void onViewAnimationEnd() {
super.onViewAnimationEnd();
Expand Down Expand Up @@ -135,11 +109,28 @@ private void notifyViewAppearTransitionEnd() {
public View onCreateView(LayoutInflater inflater,
@Nullable ViewGroup container,
@Nullable Bundle savedInstanceState) {
if (mScreenRootView == null) {
mScreenRootView = configureView();
CoordinatorLayout view = new NotifyingCoordinatorLayout(getContext(), this);
CoordinatorLayout.LayoutParams params = new CoordinatorLayout.LayoutParams(
LinearLayout.LayoutParams.MATCH_PARENT, LinearLayout.LayoutParams.MATCH_PARENT);
params.setBehavior(new AppBarLayout.ScrollingViewBehavior());
mScreenView.setLayoutParams(params);
view.addView(recycleView(mScreenView));

mAppBarLayout = new AppBarLayout(getContext());
// By default AppBarLayout will have a background color set but since we cover the whole layout
// with toolbar (that can be semi-transparent) the bar layout background color does not pay a
// role. On top of that it breaks screens animations when alfa offscreen compositing is off
// (which is the default)
mAppBarLayout.setBackgroundColor(Color.TRANSPARENT);
mAppBarLayout.setLayoutParams(new AppBarLayout.LayoutParams(
AppBarLayout.LayoutParams.MATCH_PARENT, AppBarLayout.LayoutParams.WRAP_CONTENT));
view.addView(mAppBarLayout);

if (mToolbar != null) {
mAppBarLayout.addView(recycleView(mToolbar));
}

return recycleView(mScreenRootView);
return view;
}

public boolean isDismissable() {
Expand Down

0 comments on commit 36fb635

Please sign in to comment.