Zhuinden / simple-stack

[ACTIVE] Simple Stack, a backstack library / navigation framework for simpler navigation and state management (for fragments, views, or whatevers).
Apache License 2.0
1.36k stars 76 forks source link

Starting navigation from inside `ScopedService.Activated` is not safe #215

Closed Zhuinden closed 4 years ago

Zhuinden commented 4 years ago

During activation dispatch, calling backstack.setHistory(SomeTotallyOtherKey.create()) immediately destroys the scope hierarchy BEFORE the dispatch is complete.

Gives you this beautiful crash that I was totally hoping to never see, surprised to have actually triggered it myself.

    java.lang.AssertionError: The previous scope should exist, but it doesn't! This shouldn't happen. If you see this error, this functionality is broken.
        at com.zhuinden.simplestack.ScopeManager.dispatchActivation(ScopeManager.java:615)
        at com.zhuinden.simplestack.Backstack$1$1.stateChangeComplete(Backstack.java:161)
        at com.zhuinden.simplestacktutorials.utils.SimpleStateChanger.handleStateChange(SimpleStateChanger.kt:20)
        at com.zhuinden.simplestack.Backstack$1.handleStateChange(Backstack.java:95)
        at com.zhuinden.simplestack.NavigationCore.changeState(NavigationCore.java:652)
        at com.zhuinden.simplestack.NavigationCore.beginStateChangeIfPossible(NavigationCore.java:616)
        at com.zhuinden.simplestack.NavigationCore.enqueueStateChange(NavigationCore.java:598)
        at com.zhuinden.simplestack.NavigationCore.executeOrConsumeNavigationOp(NavigationCore.java:484)
        at com.zhuinden.simplestack.NavigationCore.setHistory(NavigationCore.java:473)
        at com.zhuinden.simplestack.Backstack.setHistory(Backstack.java:983)

A possible solution would be to detach and re-attach the StateChanger while the dispatching is active.

But this is quite a pain in the ass and I'll write unit tests for this when I can. It's kinda past 4 AM. :thinking:

The quick-fix solution (in the code where onActive() calls backstack.setHistory) will be a Handler.post {}.

This is not as scary as it looks though, I've built stuff on onScopeActive before and apparently the "race" is only between actually triggering navigation that destroys scopes right there and then. That navigation action should be enqueued, but currently it is not.

Zhuinden commented 4 years ago

Repro:

    @Test
    public void navigationIsPossibleAndEnqueuedDuringActivationDispatch() {
        final TestKey destination = new TestKey("destination");

        Backstack backstack = new Backstack();
        backstack.setScopedServices(new ServiceProvider());

        class MyService
                implements ScopedServices.Activated {
            private final Backstack backstack;

            public MyService(Backstack backstack) {
                this.backstack = backstack;
            }

            @Override
            public void onServiceActive() {
                backstack.setHistory(History.of(destination), StateChange.REPLACE);
            }

            @Override
            public void onServiceInactive() {

            }
        }

        final MyService service = new MyService(backstack);

        TestKeyWithOnlyParentServices beep = new TestKeyWithOnlyParentServices("beep", History.of("registration")) {
            @Override
            public void bindServices(ServiceBinder serviceBinder) {
                if(serviceBinder.getScopeTag().equals("registration")) {
                    serviceBinder.addService("SERVICE", service);
                }
            }
        };

        TestKeyWithScope boop = new TestKeyWithScope("boop") {
            @Override
            public void bindServices(ServiceBinder serviceBinder) {
            }
        };

        backstack.setup(History.of(boop));
        backstack.setStateChanger(stateChanger);

        backstack.setHistory(History.of(beep), StateChange.REPLACE);

        assertThat(backstack.getHistory()).containsExactly(destination);
    }

So to trigger the issue, you need:

The second navigation action ends, the scopes get cleaned up, then the first navigation action continues mid-activation-dispatch (as the second one started right there in place!) and by then, the second navigation action has cleared the scopes.

What a tricky bug. :thinking:

Zhuinden commented 4 years ago

Fixed in https://github.com/Zhuinden/simple-stack/commit/c7b88573fbed30bab6118e7b65e67d9f227968f3