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.37k stars 74 forks source link

AssertionError: The new scope should exist (in ScopeManager) #220

Closed valeriyo closed 4 years ago

valeriyo commented 4 years ago

With latest version 2.3.0 - got a single report in Crashlytics:

Fatal Exception: java.lang.AssertionError: The new 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:610)
       at com.zhuinden.simplestack.Backstack$1$1.stateChangeComplete(Backstack.java:165)
       at our.app.ViewStateChanger$handleStateChange$3.onCompleted(ViewStateChanger.java:86)
       at com.zhuinden.simplestack.navigator.changehandlers.AnimatorViewChangeHandler$1$1.onAnimationEnd(AnimatorViewChangeHandler.java:47)
       at android.animation.AnimatorSet.onChildAnimatorEnded(AnimatorSet.java:829)
       ...

not sure what's going on yet, maybe you know @Zhuinden ?

Zhuinden commented 4 years ago

I'm really sorry to hear this is still possible! This is an AssertionError specifically because it means you did nothing wrong, but clearly I'm doing something horribly wrong. :persevere:

It's especially problematic that I didn't include more info in that exception message, so we don't know the state change that triggered it, what scopes were actually created, and what scopes were supposed to be activated but wasn't actually a created scope (as the exception message says).

It's especially tricky because this is rare. Honest answer is I did get exactly 1 report of this before (also in the wild on Crashlytics), but I was hoping that I've caught its possibility when I fixed #215 but apparently not.

However, we know that I don't use any handler.post {s or timers or anything like that, so the right sequence of events should provide a consistent repro, and then I'll fix it, possibly by revising the way I calculate activated/inactivated scopes.


So to do that, I'll need to ask the following questions:

All of these scenarios should work (and in fact there are tests to verify them) but there is potentially some silly edge-case involving multiple setHistory calls or something like that which I'll need to repro. Then I'll fix it ASAP.

I'll try to figure out what can possibly go wrong at all in the meantime.

valeriyo commented 4 years ago

@Zhuinden I appreciate such a quick response :) Answers to your questions:

In that particular crash:

Does this help?

Zhuinden commented 4 years ago

Definitely helps, thanks!

I guess it happens sometimes and not always? Now that is even more odd... :thinking:

[. . . 15 minutes later . . .]

I seem to have a consistent repro. Thank you for the report. I figured if there is "randomness" involved as the issue is rare, it has to involve when the state changer is removed (the Activity is paused), and so the initialization state change is triggered, but in the meantime apparently multiple setHistory calls are triggered.

Just one setHistory call does not trigger the issue, it has to be 2 navigation events while the state changer is removed, and the second event has to kill the scope that would be created by the first one.

    @Test
    public void reproduceCrashIssue220() {
        Backstack backstack = new Backstack();
        Object key1 = new Object();

        ScopeKey key2 = new ScopeKey() {
            @Nonnull
            @Override
            public String getScopeTag() {
                return "key2";
            }
        };

        ScopeKey key3 = new ScopeKey() {
            @Nonnull
            @Override
            public String getScopeTag() {
                return "key3";
            }
        };

        backstack.setScopedServices(new ScopedServices() {
            @Override
            public void bindServices(@Nonnull ServiceBinder serviceBinder) {
                // just be there
            }
        });

        backstack.setup(History.of(key1));

        final StateChanger stateChanger = new StateChanger() {
            @Override
            public void handleStateChange(@Nonnull StateChange stateChange, @Nonnull Callback completionCallback) {
                completionCallback.stateChangeComplete();
            }
        };

        backstack.setStateChanger(stateChanger);

        backstack.removeStateChanger();

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

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

        backstack.setStateChanger(stateChanger); // <-- crash
    }

I'll try to figure out something smart for the fix. :thinking:

Thank you again for the report, sorry for the issue. :persevere:

Zhuinden commented 4 years ago

Should be fixed by https://github.com/Zhuinden/simple-stack/commit/d44bfc82fd9f1ae51dc67d38b4e3330a3d40b956, released in 2.3.1

Super thank you for the bug report, and sorry I couldn't catch it earlier.

EDIT: now I even know which version and which commit introduced this, despite my 256 (now 257) unit tests apparently this has been dormant for a while. Thank you for making simple-stack better.

Zhuinden commented 4 years ago

Can you verify that updating to 2.3.1 fixes your issue?

Though technically I'm pretty sure I fixed what could have caused it.

valeriyo commented 4 years ago

@Zhuinden just saw your updates - thank you so much for following through! I'm going to upgrade to 2.3.1 and will report if I see this issue re-occur. Thanks! 👍

P.S. the crash was definitely random, because that transition would happen all the time during normal app start-up. And I think you-re right -- the activity pausing/resuming is likely a factor.

Zhuinden commented 4 years ago

In that case, I'll close this for now, if anything arises just give me a shout :musical_note: