bluelinelabs / Conductor

A small, yet full-featured framework that allows building View-based Android applications
Apache License 2.0
3.9k stars 343 forks source link

NPE when Conductor tries to unregister from Activity results #107

Closed gabrielittner closed 8 years ago

gabrielittner commented 8 years ago

Unfortunately I'm not able to reproduce this issue. It seems like onDetach() in LifecycleHandler can be called before the view gets detached.

Fatal Exception: java.lang.NullPointerException: Attempt to invoke virtual method 'void com.bluelinelabs.conductor.internal.LifecycleHandler.unregisterForActivityResults(java.lang.String)' on a null object reference
       at com.bluelinelabs.conductor.ActivityHostedRouter.unregisterForActivityResults(ActivityHostedRouter.java:78)
       at com.bluelinelabs.conductor.Controller.performDestroy(Controller.java:854)
       at com.bluelinelabs.conductor.Controller.removeViewReference(Controller.java:795)
       at com.bluelinelabs.conductor.Controller.detach(Controller.java:765)
       at com.bluelinelabs.conductor.Controller$7.onViewDetachedFromWindow(Controller.java:834)
       at android.view.View.dispatchDetachedFromWindow(View.java:13588)
       at android.view.ViewGroup.dispatchDetachedFromWindow(ViewGroup.java:2838)
       at android.view.ViewGroup.dispatchDetachedFromWindow(ViewGroup.java:2835)
       at android.view.ViewGroup.dispatchDetachedFromWindow(ViewGroup.java:2835)
       at android.view.ViewGroup.dispatchDetachedFromWindow(ViewGroup.java:2835)
       at android.view.ViewGroup.dispatchDetachedFromWindow(ViewGroup.java:2835)
       at android.view.ViewGroup.dispatchDetachedFromWindow(ViewGroup.java:2835)
       at android.view.ViewGroup.dispatchDetachedFromWindow(ViewGroup.java:2835)
       at android.view.ViewGroup.dispatchDetachedFromWindow(ViewGroup.java:2835)
       at android.view.ViewRootImpl.dispatchDetachedFromWindow(ViewRootImpl.java:2984)
       at android.view.ViewRootImpl.doDie(ViewRootImpl.java:5502)
       at android.view.ViewRootImpl.die(ViewRootImpl.java:5479)
       at android.view.WindowManagerGlobal.removeViewLocked(WindowManagerGlobal.java:369)
       at android.view.WindowManagerGlobal.removeView(WindowManagerGlobal.java:324)
       at android.view.WindowManagerImpl.removeViewImmediate(WindowManagerImpl.java:116)
       at android.app.ActivityThread.handleDestroyActivity(ActivityThread.java:3775)
       at android.app.ActivityThread.access$1500(ActivityThread.java:154)
       at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1375)
       at android.os.Handler.dispatchMessage(Handler.java:102)
       at android.os.Looper.loop(Looper.java:135)
       at android.app.ActivityThread.main(ActivityThread.java:5294)
       at java.lang.reflect.Method.invoke(Method.java)
       at java.lang.reflect.Method.invoke(Method.java:372)
       at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:904)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:699)
EricKuck commented 8 years ago

Interesting. Didn't expect this to be possible. I think I have a fix ready, but haven't been able to reproduce the issue to be able to actually test it. Any idea which version of Android this happened on?

gabrielittner commented 8 years ago

According to Crashlytics on 5.0.2, 5.1.1, 6.0.1 and 7.0, the latter was on a Nexus 5X.

On Mon, 5 Sep 2016 at 19:12 Eric Kuck notifications@github.com wrote:

Interesting. Didn't expect this to be possible. I think I have a fix ready, but haven't been able to reproduce the issue to be able to actually test it. Any idea which version of Android this happened on?

— You are receiving this because you authored the thread.

Reply to this email directly, view it on GitHub https://github.com/bluelinelabs/Conductor/issues/107#issuecomment-244788138, or mute the thread https://github.com/notifications/unsubscribe-auth/ABS5GUFJb_D8dv6XZv5W3A1aiL3mJ2fwks5qnE1vgaJpZM4Ju420 .

dimsuz commented 7 years ago

Note that this still is reproducible. I got one crash reported in Firebase with the same stack trace.

Also notice that you've probably inserted a null check in the wrong place: you are checking router != null, but what stack trace actually reports is that lifecycleHandler was null in the following call:

    void unregisterForActivityResults(String instanceId) {
        lifecycleHandler.unregisterForActivityResults(instanceId);
    }

in ActivityHostedRouter.java.

dimsuz commented 7 years ago

I wasn't able to reproduce this myself, but I can describe what my app was doing: I have an onboarding screen with animation. It runs in a Controller.

  private fun onWelcomeAnimationEnded() {
    if (conductorRouter.hasRootController()) {
      val transaction = RouterTransaction.with(MainAppController()))
        .pushChangeHandler(HorizontalChangeHandler(200))
        .popChangeHandler(HorizontalChangeHandler(200))
      conductorRouter.replaceTopController(transaction)
    } else {
      Timber.d("welcome animation is finished, but back stack is empty, user must have exited the app")
    }
  }

I added an explicit check for hasRootController() for the case when user presses back (pops controller) during the animation, in that case there's no need to replaceTopController as activity will get destroyed soon anyway. But the crash occurs inside the first if branch, so controller is still there. Trace is a bit different in my case, but ends in the same way:

Exception java.lang.NullPointerException: Attempt to invoke virtual method 'void com.bluelinelabs.conductor.internal.LifecycleHandler.unregisterForActivityResults(java.lang.String)' on a null object reference
com.bluelinelabs.conductor.ActivityHostedRouter.unregisterForActivityResults (ActivityHostedRouter.java:78)
com.bluelinelabs.conductor.Controller.destroy (Controller.java:884)
com.bluelinelabs.conductor.Controller.destroy (Controller.java:877)
com.bluelinelabs.conductor.Backstack.pop (Backstack.java:55)
com.bluelinelabs.conductor.Router.replaceTopController (Router.java:152)
ru.appkode.utair.ui.main.MainActivity.onWelcomeAnimationEnded (MainActivity.kt:127)
ru.appkode.utair.ui.main.MainActivity.access$onWelcomeAnimationEnded (MainActivity.kt:33)
ru.appkode.utair.ui.main.MainActivity$setupRootController$1.invoke (MainActivity.kt:114)
ru.appkode.utair.ui.main.MainActivity$setupRootController$1.invoke (MainActivity.kt:33)
ru.appkode.utair.ui.welcome_screen.WelcomeController.onAnimationEnded (WelcomeController.kt:150)

So maybe it is related to the situation when user presses back during the animation and activity somehow gets destroyed under the hood somehow. But note that hasRootController() is still true and router is definetely not null! lifecycleHandler is however.

Should this issue be reopened or should I create a new one?

dimsuz commented 7 years ago

Another user was bitten by this crash, according to my crash reporting tool. I am still not able to reproduce, similarly to OP...

@EricKuck I think it should be reopened. See my explanation above, having a null check around router is not enough, it should also be added to a lifecycleHandler...

cachapa commented 7 years ago

I just got this crash, unfortunately can't reproduce it. Google Pixel on Android 7.1.2

ChristineKelly commented 7 years ago

I also have this. Reproducible always.

One Plus X running Android 6.0.1 (OxygenOS 3.1.4) and Google Pixel on Android 7.1.2

Similar to dimsuz above, I have a splash screen with loading animation. If I press back button while animation running (before leaving splash screen/replacing top controller) I get the crash every time.

sockeqwe commented 7 years ago

@ChristineKelly can you provide a little sample demo so that we can reproduce it in isolation and track the issue down?

Which version of Conductor do you use?