From 28f1a5875e5cadcef923d9407365f7507f46faf0 Mon Sep 17 00:00:00 2001 From: Hristo Hristov Date: Fri, 10 Nov 2017 13:40:38 +0200 Subject: [PATCH] Fix crash on android where we queue few back navigations an exception is thrown. Fix https://github.com/NativeScript/NativeScript/issues/4986 --- tests/app/TKUnit.ts | 6 +- tests/app/ui/frame/frame-tests-common.ts | 139 ++++++++++++++++++--- tests/app/ui/frame/frame-tests.android.ts | 2 + tests/app/ui/frame/frame-tests.ios.ts | 1 + tns-core-modules/ui/frame/frame-common.ts | 68 +++++++--- tns-core-modules/ui/frame/frame.android.ts | 4 +- tns-core-modules/ui/frame/frame.d.ts | 2 +- tns-core-modules/ui/frame/frame.ios.ts | 1 - tns-core-modules/ui/page/page.ios.ts | 13 +- 9 files changed, 190 insertions(+), 46 deletions(-) diff --git a/tests/app/TKUnit.ts b/tests/app/TKUnit.ts index 77683f9d9..f7645a978 100644 --- a/tests/app/TKUnit.ts +++ b/tests/app/TKUnit.ts @@ -114,17 +114,17 @@ function runAsync(testInfo: TestInfoEntry, recursiveIndex: number, testTimeout?: duration = time() - testStartTime; testInfo.duration = duration; if (isDone) { - write(`--- [${testInfo.testName}] OK, duration: ${duration}`, trace.messageType.info); + write(`--- [${testInfo.testName}] OK, duration: ${duration.toFixed(2)}`, trace.messageType.info); testInfo.isPassed = true; runTests(testsQueue, recursiveIndex + 1); } else if (error) { - write(`--- [${testInfo.testName}] FAILED: ${error.message}, duration: ${duration}`, trace.messageType.error); + write(`--- [${testInfo.testName}] FAILED: ${error.message}, duration: ${duration.toFixed(2)}`, trace.messageType.error); testInfo.errorMessage = error.message; runTests(testsQueue, recursiveIndex + 1); } else { const testEndTime = time(); if (testEndTime - testStartTime > timeout) { - write(`--- [${testInfo.testName}] TIMEOUT, duration: ${duration}`, trace.messageType.error); + write(`--- [${testInfo.testName}] TIMEOUT, duration: ${duration.toFixed(2)}`, trace.messageType.error); testInfo.errorMessage = "Test timeout."; runTests(testsQueue, recursiveIndex + 1); } else { diff --git a/tests/app/ui/frame/frame-tests-common.ts b/tests/app/ui/frame/frame-tests-common.ts index e1bca0fdd..3c3563820 100644 --- a/tests/app/ui/frame/frame-tests-common.ts +++ b/tests/app/ui/frame/frame-tests-common.ts @@ -1,43 +1,150 @@ // >> frame-require -import * as frameModule from "tns-core-modules/ui/frame"; -var topmost = frameModule.topmost(); +import { topmost, NavigationEntry } from "tns-core-modules/ui/frame"; // << frame-require -import * as labelModule from "tns-core-modules/ui/label"; -import * as pagesModule from "tns-core-modules/ui/page"; +import { Label } from "tns-core-modules/ui/label"; +import { Page } from "tns-core-modules/ui/page"; +import * as helper from "../helper"; +import * as TKUnit from "../../TKUnit"; -export var ignore_test_DummyTestForSnippetOnly0 = function () { +export function ignore_test_DummyTestForSnippetOnly0() { // >> frame-navigating - topmost.navigate("details-page"); + const frame = topmost(); + frame.navigate("details-page"); // << frame-navigating } -export var ignore_test_DummyTestForSnippetOnly1 = function () { +export function ignore_test_DummyTestForSnippetOnly1() { // >> frame-factory-func - var factoryFunc = function () { - var label = new labelModule.Label(); + const func = function () { + const label = new Label(); label.text = "Hello, world!"; - var page = new pagesModule.Page(); + const page = new Page(); page.content = label; return page; }; - topmost.navigate(factoryFunc); + const frame = topmost(); + frame.navigate(func); // <> frame-naventry - var navigationEntry = { + const navigationEntry = { moduleName: "details-page", context: { info: "something you want to pass to your page" }, animated: false }; - topmost.navigate(navigationEntry); + const frame = topmost(); + frame.navigate(navigationEntry); // << frame-naventry } -export var ignore_test_DummyTestForSnippetOnly3 = function () { +export function ignore_test_DummyTestForSnippetOnly3() { + // >> frame-naventrycontext + const navigationEntry: NavigationEntry = { + moduleName: "details-page", + bindingContext: { info: "something you want to pass as binding context to your page" }, + animated: false + }; + const frame = topmost(); + frame.navigate(navigationEntry); + // << frame-naventrycontext +} + +export function ignore_test_DummyTestForSnippetOnly4() { // >> frame-back - topmost.goBack(); + const frame = topmost(); + frame.goBack(); // << frame-back } + +export function test_can_go_back() { + const frame = topmost(); + + frame.navigate({ create: () => new Page(), clearHistory: true }); + TKUnit.waitUntilReady(() => frame.navigationQueueIsEmpty()); + + frame.navigate(() => new Page()); + frame.navigate(() => new Page()); + frame.navigate({ create: () => new Page(), backstackVisible: false }); + frame.navigate(() => new Page()); + + TKUnit.assertTrue(frame.canGoBack(), '1'); + frame.goBack(); + + TKUnit.assertTrue(frame.canGoBack(), '2'); + frame.goBack(); + + TKUnit.assertTrue(frame.canGoBack(), '3'); + frame.goBack(); + + TKUnit.assertFalse(frame.canGoBack(), '4'); + frame.goBack(); + + frame.navigate({ create: () => new Page(), backstackVisible: false }); + frame.navigate(() => new Page()); + + TKUnit.assertTrue(frame.canGoBack(), '5'); + frame.goBack(); + + TKUnit.assertFalse(frame.canGoBack(), '6'); + frame.goBack(); + + frame.navigate(() => new Page()); + frame.navigate({ create: () => new Page(), clearHistory: true }); + + TKUnit.assertFalse(frame.canGoBack(), '7'); + frame.goBack(); + + frame.navigate(() => new Page()); + frame.navigate({ create: () => new Page(), backstackVisible: false }); + + TKUnit.assertTrue(frame.canGoBack(), '8'); + frame.goBack(); + + TKUnit.assertTrue(frame.canGoBack(), '9'); + frame.goBack(); + + TKUnit.assertFalse(frame.canGoBack(), '10'); + frame.goBack(); + + frame.navigate(() => new Page()); + frame.navigate({ create: () => new Page(), clearHistory: true }); + frame.navigate({ create: () => new Page(), backstackVisible: false }); + + TKUnit.assertTrue(frame.canGoBack(), '11'); + frame.goBack(); + + TKUnit.assertFalse(frame.canGoBack(), '12'); + frame.goBack(); + + frame.navigate({ create: () => new Page(), clearHistory: true }); + frame.navigate({ create: () => new Page(), backstackVisible: false }); + frame.navigate(() => new Page()); + + TKUnit.assertTrue(frame.canGoBack(), '13'); + frame.goBack(); + + TKUnit.assertFalse(frame.canGoBack(), '14'); + frame.goBack(); + TKUnit.waitUntilReady(() => frame.navigationQueueIsEmpty()); +} + +export function test_go_back_to_backstack_entry() { + const frame = topmost(); + frame.navigate(() => new Page()); + TKUnit.waitUntilReady(() => frame.navigationQueueIsEmpty()); + + frame.navigate(() => new Page()); + frame.navigate(() => new Page()); + frame.navigate({ create: () => new Page(), backstackVisible: false }); + frame.navigate(() => new Page()); + + TKUnit.assertTrue(frame.canGoBack(), '1'); + frame.goBack(frame.backStack[0]); + + TKUnit.assertFalse(frame.canGoBack(), '2'); + frame.goBack(); + TKUnit.waitUntilReady(() => frame.navigationQueueIsEmpty()); +} \ No newline at end of file diff --git a/tests/app/ui/frame/frame-tests.android.ts b/tests/app/ui/frame/frame-tests.android.ts index dc76a52c7..5e0ac94a2 100644 --- a/tests/app/ui/frame/frame-tests.android.ts +++ b/tests/app/ui/frame/frame-tests.android.ts @@ -2,6 +2,8 @@ import * as frameModule from "tns-core-modules/ui/frame"; import * as TKUnit from "../../TKUnit"; import { unsetValue, PercentLength } from "tns-core-modules/ui/core/view"; +export * from "./frame-tests-common"; + export function test_percent_width_and_height_set_to_page_support() { let topFrame = frameModule.topmost(); let currentPage = topFrame.currentPage; diff --git a/tests/app/ui/frame/frame-tests.ios.ts b/tests/app/ui/frame/frame-tests.ios.ts index e69de29bb..ef08ea72d 100644 --- a/tests/app/ui/frame/frame-tests.ios.ts +++ b/tests/app/ui/frame/frame-tests.ios.ts @@ -0,0 +1 @@ +export * from "./frame-tests-common"; \ No newline at end of file diff --git a/tns-core-modules/ui/frame/frame-common.ts b/tns-core-modules/ui/frame/frame-common.ts index 8da7314d5..82a35dea9 100644 --- a/tns-core-modules/ui/frame/frame-common.ts +++ b/tns-core-modules/ui/frame/frame-common.ts @@ -199,7 +199,42 @@ export class FrameBase extends CustomLayoutView implements FrameDefinition { } public canGoBack(): boolean { - return this._backStack.length > 0; + let backstack = this._backStack.length; + let previousForwardNotInBackstack = false; + this._navigationQueue.forEach((item, i, array) => { + if (item.entry !== this._currentEntry) { + if (item.isBackNavigation) { + previousForwardNotInBackstack = false; + if (!item.entry) { + backstack--; + } else { + const backstackIndex = this._backStack.indexOf(item.entry); + if (backstackIndex !== -1) { + backstack = backstackIndex; + } else { + // NOTE: We don't search for entries in navigationQueue because there is no way for + // developer to get reference to BackstackEntry unless transition is completed. + // At that point the entry is put in the backstack array. + // If we start to return Backstack entry from navigate method then + // here we should check also navigationQueue as well. + backstack--; + } + } + } else if (item.entry.entry.clearHistory) { + previousForwardNotInBackstack = false; + backstack = 0; + } else { + backstack++; + if (previousForwardNotInBackstack) { + backstack--; + } + + previousForwardNotInBackstack = item.entry.entry.backstackVisible === false; + } + } + }); + + return backstack > 0; } /** @@ -232,8 +267,7 @@ export class FrameBase extends CustomLayoutView implements FrameDefinition { if (this._navigationQueue.length === 1) { this._processNavigationContext(navigationContext); - } - else { + } else { if (traceEnabled()) { traceWrite(`Going back scheduled`, traceCategories.Navigation); } @@ -321,14 +355,22 @@ export class FrameBase extends CustomLayoutView implements FrameDefinition { this._currentEntry = entry; } + public _updateBackstack(entry: BackstackEntry): void { + if (entry.entry.clearHistory) { + this._backStack.length = 0; + } else if (FrameBase._isEntryBackstackVisible(this._currentEntry)) { + this._backStack.push(this._currentEntry); + } + } + public _processNavigationQueue(page: Page) { if (this._navigationQueue.length === 0) { // This could happen when showing recreated page after activity has been destroyed. return; } - let entry = this._navigationQueue[0].entry; - let currentNavigationPage = entry.resolvedPage; + const entry = this._navigationQueue[0].entry; + const currentNavigationPage = entry.resolvedPage; if (page !== currentNavigationPage) { // If the page is not the one that requested navigation - skip it. return; @@ -388,21 +430,9 @@ export class FrameBase extends CustomLayoutView implements FrameDefinition { } } - public _clearBackStack(): void { - this._backStack.length = 0; - } - @profile private performNavigation(navigationContext: NavigationContext) { - let navContext = navigationContext.entry; - - // TODO: This should happen once navigation is completed. - if (navigationContext.entry.entry.clearHistory) { - // Don't clear backstack immediately or we can't remove pages from frame. - } else if (FrameBase._isEntryBackstackVisible(this._currentEntry)) { - this._backStack.push(this._currentEntry); - } - + const navContext = navigationContext.entry; this._onNavigatingTo(navContext, navigationContext.isBackNavigation); this._navigateCore(navContext); } @@ -419,7 +449,7 @@ export class FrameBase extends CustomLayoutView implements FrameDefinition { this._backStack.pop(); this._removeBackstackEntries(removed); } - + this._onNavigatingTo(backstackEntry, true); this._goBackCore(backstackEntry); } diff --git a/tns-core-modules/ui/frame/frame.android.ts b/tns-core-modules/ui/frame/frame.android.ts index 8ff9c1d93..1e02faa3f 100644 --- a/tns-core-modules/ui/frame/frame.android.ts +++ b/tns-core-modules/ui/frame/frame.android.ts @@ -89,6 +89,9 @@ export class Frame extends FrameBase { public setCurrent(entry: BackstackEntry): void { if (this._currentEntry !== entry) { + if (!this._isBack) { + this._updateBackstack(entry); + } this.changeCurrentPage(entry); } @@ -172,7 +175,6 @@ export class Frame extends FrameBase { _setAndroidFragmentTransitions(animated, navigationTransition, currentFragment, newFragment, transaction, manager); if (clearHistory) { destroyPages(this.backStack, true); - this._clearBackStack(); } if (currentFragment && animated && !navigationTransition) { diff --git a/tns-core-modules/ui/frame/frame.d.ts b/tns-core-modules/ui/frame/frame.d.ts index fc0cc17d9..3071408b4 100644 --- a/tns-core-modules/ui/frame/frame.d.ts +++ b/tns-core-modules/ui/frame/frame.d.ts @@ -139,7 +139,7 @@ export class Frame extends View { /** * @private */ - _clearBackStack(): void; + _updateBackstack(entry: BackstackEntry): void; /** * @private */ diff --git a/tns-core-modules/ui/frame/frame.ios.ts b/tns-core-modules/ui/frame/frame.ios.ts index 13c53119f..0eb317ca4 100644 --- a/tns-core-modules/ui/frame/frame.ios.ts +++ b/tns-core-modules/ui/frame/frame.ios.ts @@ -50,7 +50,6 @@ export class Frame extends FrameBase { let clearHistory = backstackEntry.entry.clearHistory; if (clearHistory) { - this._clearBackStack(); navDepth = -1; } navDepth++; diff --git a/tns-core-modules/ui/page/page.ios.ts b/tns-core-modules/ui/page/page.ios.ts index 1a0903846..adee5a027 100644 --- a/tns-core-modules/ui/page/page.ios.ts +++ b/tns-core-modules/ui/page/page.ios.ts @@ -102,8 +102,6 @@ class UIViewControllerImpl extends UIViewController { if (!owner.parent) { if (!frame._currentEntry) { frame._currentEntry = newEntry; - } else { - frame._navigateToEntry = newEntry; } owner._frame = frame; @@ -142,14 +140,19 @@ class UIViewControllerImpl extends UIViewController { // Skip navigation events if modal page is shown. if (!owner._presentedViewController && frame) { const newEntry = this[ENTRY]; - let isBack = isBackNavigationTo(owner, newEntry); + + let isBack: boolean; // We are on the current page which happens when navigation is canceled so isBack should be false. if (frame.currentPage === owner && frame._navigationQueue.length === 0) { isBack = false; + } else { + isBack = isBackNavigationTo(owner, newEntry); } - frame._navigateToEntry = null; - frame._currentEntry = newEntry; + if (!isBack) { + frame._updateBackstack(newEntry); + } + frame.setCurrent(newEntry); owner.onNavigatedTo(isBack); // If page was shown with custom animation - we need to set the navigationController.delegate to the animatedDelegate.