From 3c08b1c99559a3485fb8661ca98ce256db59adb8 Mon Sep 17 00:00:00 2001 From: Guy Carmeli Date: Sun, 26 May 2019 16:11:22 +0300 Subject: [PATCH] Fix setStackRoot crash when called with the same id (#5154) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a Stack’s root was set with an id of one of the Stack’s current children, there was a crash since the wrong elements were removed from the stack. This commit fixes this by creating a new stack when setStackRoot is called, and destroying all ViewControllers from the previous Stack. Fixes #5117 --- e2e/Stack.test.js | 5 ++++ .../viewcontrollers/IdStack.java | 10 ++----- .../stack/StackController.java | 27 +++++++++---------- .../stack/StackControllerTest.java | 27 ++++++++++++++++--- playground/src/screens/StackScreen.js | 15 +++++++++-- playground/src/testIDs.js | 1 + scripts/test-e2e.js | 2 +- 7 files changed, 58 insertions(+), 29 deletions(-) diff --git a/e2e/Stack.test.js b/e2e/Stack.test.js index 931e552cc26..10590a31e4f 100644 --- a/e2e/Stack.test.js +++ b/e2e/Stack.test.js @@ -80,6 +80,11 @@ describe('Stack', () => { await expect(elementById(TestIDs.STACK_SCREEN_HEADER)).toBeVisible(); }); + it('does not crash when setting the stack root to an existing component id', async () => { + await elementById(TestIDs.SET_STACK_ROOT_WITH_ID_BTN).tap(); + await elementById(TestIDs.SET_STACK_ROOT_WITH_ID_BTN).tap(); + }); + it(':ios: set stack root component should be first in stack', async () => { await elementById(TestIDs.PUSH_BTN).tap(); await expect(elementByLabel('Stack Position: 1')).toBeVisible(); diff --git a/lib/android/app/src/main/java/com/reactnativenavigation/viewcontrollers/IdStack.java b/lib/android/app/src/main/java/com/reactnativenavigation/viewcontrollers/IdStack.java index 9465db85e73..7f70396b728 100644 --- a/lib/android/app/src/main/java/com/reactnativenavigation/viewcontrollers/IdStack.java +++ b/lib/android/app/src/main/java/com/reactnativenavigation/viewcontrollers/IdStack.java @@ -29,17 +29,11 @@ public void set(String id, E item, int index) { } public E peek() { - if (isEmpty()) { - return null; - } - return map.get(last(deque)); + return isEmpty() ? null : map.get(last(deque)); } public E pop() { - if (isEmpty()) { - return null; - } - return map.remove(removeLast(deque)); + return isEmpty() ? null : map.remove(removeLast(deque)); } public boolean isEmpty() { diff --git a/lib/android/app/src/main/java/com/reactnativenavigation/viewcontrollers/stack/StackController.java b/lib/android/app/src/main/java/com/reactnativenavigation/viewcontrollers/stack/StackController.java index 3ebde66e142..a38f64adffe 100644 --- a/lib/android/app/src/main/java/com/reactnativenavigation/viewcontrollers/stack/StackController.java +++ b/lib/android/app/src/main/java/com/reactnativenavigation/viewcontrollers/stack/StackController.java @@ -14,7 +14,6 @@ import com.reactnativenavigation.presentation.Presenter; import com.reactnativenavigation.presentation.StackPresenter; import com.reactnativenavigation.react.Constants; -import com.reactnativenavigation.utils.CollectionUtils; import com.reactnativenavigation.utils.CommandListener; import com.reactnativenavigation.utils.CommandListenerAdapter; import com.reactnativenavigation.viewcontrollers.ChildControllersRegistry; @@ -32,10 +31,11 @@ import java.util.List; import static android.view.ViewGroup.LayoutParams.MATCH_PARENT; +import static com.reactnativenavigation.utils.CollectionUtils.*; public class StackController extends ParentController { - private final IdStack stack = new IdStack<>(); + private IdStack stack = new IdStack<>(); private final NavigationAnimator animator; private TopBarController topBarController; private BackButtonHelper backButtonHelper; @@ -181,20 +181,22 @@ private void addChildToStack(ViewController child, View view, Options resolvedOp public void setRoot(List children, CommandListener listener) { animator.cancelPushAnimations(); + IdStack stackToDestroy = stack; + stack = new IdStack<>(); if (children.size() == 1) { - backButtonHelper.clear(CollectionUtils.last(children)); - push(CollectionUtils.last(children), new CommandListenerAdapter() { + backButtonHelper.clear(last(children)); + push(last(children), new CommandListenerAdapter() { @Override public void onSuccess(String childId) { - removeChildrenBellowTop(); + destroyStack(stackToDestroy); listener.onSuccess(childId); } }); } else { - push(CollectionUtils.last(children), new CommandListenerAdapter() { + push(last(children), new CommandListenerAdapter() { @Override public void onSuccess(String childId) { - removeChildrenBellowTop(); + destroyStack(stackToDestroy); for (int i = 0; i < children.size() - 1; i++) { stack.set(children.get(i).getId(), children.get(i), i); children.get(i).setParentController(StackController.this); @@ -210,14 +212,9 @@ public void onSuccess(String childId) { } } - private void removeChildrenBellowTop() { - Iterator iterator = stack.iterator(); - while (stack.size() > 1) { - ViewController controller = stack.get(iterator.next()); - if (!stack.isTop(controller.getId())) { - stack.remove(iterator, controller.getId()); - controller.destroy(); - } + private void destroyStack(IdStack stack) { + for (String s : (Iterable) stack) { + ((ViewController) stack.get(s)).destroy(); } } diff --git a/lib/android/app/src/test/java/com/reactnativenavigation/viewcontrollers/stack/StackControllerTest.java b/lib/android/app/src/test/java/com/reactnativenavigation/viewcontrollers/stack/StackControllerTest.java index e51459d101e..8964c0de200 100644 --- a/lib/android/app/src/test/java/com/reactnativenavigation/viewcontrollers/stack/StackControllerTest.java +++ b/lib/android/app/src/test/java/com/reactnativenavigation/viewcontrollers/stack/StackControllerTest.java @@ -42,7 +42,11 @@ import org.json.JSONException; import org.json.JSONObject; import org.junit.Test; -import org.mockito.*; +import org.mockito.ArgumentCaptor; +import org.mockito.InOrder; +import org.mockito.Mockito; +import org.robolectric.Robolectric; +import org.robolectric.shadows.ShadowLooper; import java.util.ArrayList; import java.util.Arrays; @@ -65,6 +69,7 @@ public class StackControllerTest extends BaseTest { private ChildControllersRegistry childRegistry; private StackController uut; private ViewController child1; + private ViewController child1a; private ViewController child2; private ViewController child3; private ViewController child4; @@ -84,6 +89,7 @@ public void beforeEach() { renderChecker = spy(new RenderChecker()); presenter = spy(new StackPresenter(activity, new TitleBarReactViewCreatorMock(), new TopBarBackgroundViewCreatorMock(), new TopBarButtonCreatorMock(), ImageLoaderMock.mock(), renderChecker, new Options())); child1 = spy(new SimpleViewController(activity, childRegistry, "child1", new Options())); + child1a = spy(new SimpleViewController(activity, childRegistry, "child1", new Options())); child2 = spy(new SimpleViewController(activity, childRegistry, "child2", new Options())); child3 = spy(new SimpleViewController(activity, childRegistry, "child3", new Options())); child4 = spy(new SimpleViewController(activity, childRegistry, "child4", new Options())); @@ -257,6 +263,8 @@ public void onSuccess(String childId) { @Test public void setRoot_multipleChildren() { + Robolectric.getForegroundThreadScheduler().pause(); + activity.setContentView(uut.getView()); disablePushAnimation(child1, child2, child3, child4); disablePopAnimation(child4); @@ -264,6 +272,7 @@ public void setRoot_multipleChildren() { assertThat(uut.isEmpty()).isTrue(); uut.push(child1, new CommandListenerAdapter()); uut.push(child2, new CommandListenerAdapter()); + ShadowLooper.idleMainLooper(); assertThat(uut.getTopBar().getTitleBar().getNavigationIcon()).isNotNull(); uut.setRoot(Arrays.asList(child3, child4), new CommandListenerAdapter() { @Override @@ -275,6 +284,7 @@ public void onSuccess(String childId) { assertThat(uut.getCurrentChild()).isEqualTo(child4); uut.pop(Options.EMPTY, new CommandListenerAdapter()); + ShadowLooper.idleMainLooper(); assertThat(uut.getTopBar().getTitleBar().getNavigationIcon()).isNull(); assertThat(uut.getCurrentChild()).isEqualTo(child3); } @@ -286,14 +296,25 @@ public void setRoot_doesNotCrashWhenCalledInQuickSuccession() { disablePushAnimation(child1); uut.setRoot(Collections.singletonList(child1), new CommandListenerAdapter()); + ViewGroup c2View = child2.getView(); + ViewGroup c3View = child3.getView(); uut.setRoot(Collections.singletonList(child2), new CommandListenerAdapter()); uut.setRoot(Collections.singletonList(child3), new CommandListenerAdapter()); - animator.endPushAnimation(child2.getView()); - animator.endPushAnimation(child3.getView()); + animator.endPushAnimation(c2View); + animator.endPushAnimation(c3View); assertContainsOnlyId(child3.getId()); } + @Test + public void setRoot_doesNotCrashWhenCalledWithSameId() { + disablePushAnimation(child1, child1a); + uut.setRoot(Collections.singletonList(child1), new CommandListenerAdapter()); + uut.setRoot(Collections.singletonList(child1a), new CommandListenerAdapter()); + + assertContainsOnlyId(child1a.getId()); + } + @Test public synchronized void pop() { disablePushAnimation(child1, child2); diff --git a/playground/src/screens/StackScreen.js b/playground/src/screens/StackScreen.js index 04898d5eb80..454d60e2f33 100644 --- a/playground/src/screens/StackScreen.js +++ b/playground/src/screens/StackScreen.js @@ -3,7 +3,7 @@ const Root = require('../components/Root'); const Button = require('../components/Button'); const Screens = require('./Screens'); const Navigation = require('../services/Navigation'); -const {stack, component} = require('../commons/Layouts'); +const { stack, component } = require('../commons/Layouts'); const { PUSH_BTN, STACK_SCREEN_HEADER, @@ -12,7 +12,8 @@ const { PUSH_CUSTOM_BACK_BTN, CUSTOM_BACK_BTN, SEARCH_BTN, - SET_STACK_ROOT_BTN + SET_STACK_ROOT_BTN, + SET_STACK_ROOT_WITH_ID_BTN } = require('../testIDs'); class StackScreen extends React.Component { @@ -39,6 +40,7 @@ class StackScreen extends React.Component {