Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calling backstackManager.finalizeScopes() twice results in an unexpected error message #150

Closed
jamolkhon opened this Issue Mar 4, 2019 · 9 comments

Comments

Projects
None yet
2 participants
@jamolkhon
Copy link

jamolkhon commented Mar 4, 2019

I'm getting the error: The previous scope should exist, but it doesn't! This shouldn't happen. If you see this error, this functionality is broken.


I'm not sure if it's a bug or a misconfiguration on my side.

Here's my RootActivity.kt:

class RootActivity : AppCompatActivity(), StateChanger {

  val backstackDelegate = BackstackDelegate(this)
  private val backstack by lazy { backstackDelegate.backstack }
  private val appComponent by lazy { (application as App).appComponent }
  private val stateChanger = FragmentStateChanger(supportFragmentManager, R.id.container)

  override fun onCreate(savedInstanceState: Bundle?) {
    backstackDelegate.setScopedServices(this, ScopeConfiguration(appComponent))
    backstackDelegate.onCreate(savedInstanceState, lastCustomNonConfigurationInstance,
        History.of(AuthKey()))
    backstackDelegate.registerForLifecycleCallbacks(this)

    super.onCreate(savedInstanceState)
    setContentView(R.layout.activity_root)

    backstackDelegate.setStateChanger(this)
  }

  override fun onPostResume() {
    super.onPostResume()
    backstackDelegate.onPostResume()
  }

  override fun onPause() {
    super.onPause()
    backstackDelegate.onPause()
  }

  override fun onDestroy() {
    super.onDestroy()
    backstackDelegate.onDestroy()
  }

  override fun onSaveInstanceState(outState: Bundle?) {
    super.onSaveInstanceState(outState)
    if (outState != null) {
      backstackDelegate.onSaveInstanceState(outState)
    }
  }

  override fun onRetainCustomNonConfigurationInstance(): Any =
      backstackDelegate.onRetainCustomNonConfigurationInstance()

  override fun onBackPressed() {
    if (!backstackDelegate.onBackPressed()) {
      super.onBackPressed()
    }
  }

  override fun handleStateChange(stateChange: StateChange, completionCallback: StateChanger.Callback) {
    if (stateChange.topNewState<Any>() == stateChange.topPreviousState()) {
      // no-op
      completionCallback.stateChangeComplete()
      return
    }

    stateChanger.handleStateChange(stateChange)
    completionCallback.stateChangeComplete()
  }

  fun navigateTo(key: BaseKey) {
    backstack.goTo(key)
  }
}

fun Fragment.navigateTo(key: BaseKey) {
  if (activity is RootActivity) {
    (activity as RootActivity).navigateTo(key)
    return
  }
  throw RuntimeException("I'm not your parent")
}

inline fun <reified T> Fragment.lookup(serviceTag: String = T::class.java.name): T {
  if (activity is RootActivity) {
    return (activity as RootActivity).backstackDelegate.lookupService(serviceTag)
  }
  throw RuntimeException("I'm not your parent")
}

inline fun <reified T> ScopedServices.ServiceBinder.addService(service: T,
    serviceTag: String = T::class.java.name) {
  add(serviceTag, service as Any)
}

AuthKey.kt:

//@Parcelize // not working :(
data class AuthKey(val tag: String = "AuthKey") : BaseKey(), ScopeKey {

  override fun createFragment() = AuthFragment()

  override fun getScopeTag() = AuthScope.SCOPE_TAG

  override fun writeToParcel(dest: Parcel?, flags: Int) {
  }

  override fun describeContents() = 0

  companion object {

    @JvmField
    val CREATOR = object : Parcelable.Creator<AuthKey> {

      override fun createFromParcel(source: Parcel?): AuthKey = AuthKey()

      override fun newArray(size: Int): Array<AuthKey> = emptyArray()
    }
  }
}

AuthScope.kt:

class AuthScope {

  companion object {
    const val AUTH_TAG = "AUTH"
  }
}

Inside AuthFragment.kt I have this:

private val controller by lazy { lookup<AuthController>() }

After pressing back button while on AuthFragment I get the error in the title. The exception happens at line backstackDelegate.onDestroy().

@jamolkhon

This comment has been minimized.

Copy link
Author

jamolkhon commented Mar 4, 2019

Happens on 1.13.1 and 1.13.3

@Zhuinden

This comment has been minimized.

Copy link
Owner

Zhuinden commented Mar 4, 2019

I was hoping I'd never see this error message 🤔

Theoretically I added this assertion to signal that there is a discrepancy between the scopes that the ScopeManager keeps track of, and the scopes that can be re-constructed from the Backstack's history stack.

I'd also think this is not possible, but apparently it is.

The exact message claims that the History stack is able to retrieve a scope from the history that doesn't exist inside the ScopeManager. But I might need to add a better error message, I really wasn't expecting this to ever occur...


Do you have a sample project, or is this simple sample already reproducing it with backstack delegate + fragments?

We've been using lookupService heavily with Navigator+views, so I'm surprised to see this happen nonetheless.

@Zhuinden

This comment has been minimized.

Copy link
Owner

Zhuinden commented Mar 4, 2019

Theoretically if you need to revert, this seems to be an issue with dispatching .Activated, which came in with 1.12.1.

So the version without .Activated is 1.12.0.

....I really need to know how to reproduce this to proceed, though. I have a bunch of tests, this never happened before. >.<

@Zhuinden

This comment has been minimized.

Copy link
Owner

Zhuinden commented Mar 4, 2019

Actually, I have an idea.

Is there ANY chance that for SOME REASON you're calling backstackDelegate.onDestroy() twice?

For example, the fact that your sample code seems to register for automatic lifecycle callbacks, AND also invokes onPause, onPostResume, and most importantly onDestroy twice?

Triggering finalizeScopes() twice can theoretically cause this, as the previous history would try to access a scope that no longer exists.


I must admit that I didn't think of checking for scope finalization twice, this needs a better error message.


In your app though, the solution is to either remove registerForLifecycleCallbacks, or remove calls to onPause/onPostResume/onSaveInstanceState/onDestroy because that's what registerForLifecycleCallbacks handles internally.

@jamolkhon

This comment has been minimized.

Copy link
Author

jamolkhon commented Mar 4, 2019

Just checked with the scoping sample. The error doesn't happen. However the sample uses Navigator instead of BackstackDelegate. Also it uses explicit scopes I think. Not sure if that matters. I changed to implicit by removing ScopeKey.Child implementation from WordScope. I don't know if that's enough to make it implicit. Sorry, I'm still struggling to understand implicit and explicit scopes. But I got the following error:
The service [com.zhuinden.simplestackexamplescoping.WordController] does not exist in any scopes! Is the scope tag registered via a ScopeKey? If yes, make sure the StateChanger has been set by this time, and that you've bound and are trying to lookup the service with the correct service tag. Otherwise, it is likely that the scope you intend to inherit the service from does not exist.

I'll try to produce a sample later.

@jamolkhon

This comment has been minimized.

Copy link
Author

jamolkhon commented Mar 4, 2019

Yup. Your last comment is correct. Dumb me. Issue resolved by removing the call to registerForLifecycleCallbacks.

@jamolkhon

This comment has been minimized.

Copy link
Author

jamolkhon commented Mar 4, 2019

A quick question though. Is registerForLifecycleCallbacks equivalent to calling all (required) lifecycle callbacks manually?

@Zhuinden

This comment has been minimized.

Copy link
Owner

Zhuinden commented Mar 4, 2019

The only thing I can't track in there are onCreate and onRetainCustomNonConfigurationInstance.

onCreate is too late, and onRetainCustomNonConfigurationInstance isn't exposed there so I can't channel into it and therefore still needs to be called.

So it tracks onPause/onResume/onSaveInstanceState/onDestroy. I think I have this added to the Javadoc, though.


Dumb me.

From my perspective, it's a mistake of my API that I allow this. 🤔 I'll think about what can be done with it, because registerForLifecycleCallbacks is a "convenience method", primarily because it's easy to forget for example onSaveInstanceState (happened to me before, which is why the method exists...), but onPostResume can be useful if simple-stack is used to manage DialogFragments for some reason (and I can only detect onResume, but not onPostResume from the outside).

But BackstackDelegate exists if one cannot really trust a retained fragment (and man, I don't really trust them when other fragments are involved), because otherwise it'd just be Navigator which handles all of these internally.


Anyways, thanks for finding a bug. You should be getting a reasonable error message when this happens, instead of "hello the world exploded please hold".

@Zhuinden Zhuinden changed the title The previous scope should exist, but it doesn't! This shouldn't happen. If you see this error, this functionality is broken. Calling backstackManager.finalizeScopes() twice results in an unexpected error message Mar 4, 2019

@Zhuinden

This comment has been minimized.

Copy link
Owner

Zhuinden commented Mar 10, 2019

I had to figure out how I'd want to handle this, especially what to do with scopes after they're finalized; now I'll swallow multiple calls to finalizeScopes() in succession as no-op. Scopes also "stop being finalized" when they're triggered to be rebuilt.

So you will no longer get this cryptic error message in 1.13.4 (f0101f3).

@Zhuinden Zhuinden closed this Mar 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.