search

sockeqwe / mosby

A Model-View-Presenter / Model-View-Intent library for modern Android apps
http://hannesdorfmann.com/mosby/
Apache License 2.0
5.49k stars 841 forks source link

DetachView retainInstance is true when activity is destroyed #256

Closed md3kung closed 6 years ago

md3kung commented 7 years ago

Mosby Version: 3.0.4

Expected behavior DetachView with retainInstance false when activity is destroyed

Actual behavior (include a stacktrace if crash) DetachView with retainInstance true when activity is destroyed (onDestroy triggered on fragment)

Steps to reproduce the behavior or link to a sample repository

Device : HTC One X - Android 5.1.1 Sample : MosbyTest.zip Logs : ... Activate "Don't keep activities" and launch App 06-20 12:49:47.799 4882-4882/com.example.mouda3.mosbytest D/FRAGMENT: CREATED 06-20 12:49:47.807 4882-4882/com.example.mouda3.mosbytest D/PRESENTER: CREATE 06-20 12:49:47.822 4882-4882/com.example.mouda3.mosbytest D/VIEWSTATE: NEW ... Config change portrait to landscape 06-20 12:49:55.857 4882-4882/com.example.mouda3.mosbytest D/TIMER: ALIVE 8 06-20 12:49:56.311 4882-4882/com.example.mouda3.mosbytest D/PRESENTER INSTANCE: true 06-20 12:49:56.420 4882-4882/com.example.mouda3.mosbytest D/VIEWSTATE RETAINED: true 06-20 12:49:56.857 4882-4882/com.example.mouda3.mosbytest D/TIMER: ALIVE 9 ... Config change landscape to portrait 06-20 12:50:01.857 4882-4882/com.example.mouda3.mosbytest D/TIMER: ALIVE 14 06-20 12:50:02.698 4882-4882/com.example.mouda3.mosbytest D/PRESENTER INSTANCE: true 06-20 12:50:02.786 4882-4882/com.example.mouda3.mosbytest D/VIEWSTATE RETAINED: true 06-20 12:50:02.857 4882-4882/com.example.mouda3.mosbytest D/TIMER: ALIVE 15 ... Leave App (Clicking on home button) 06-20 12:50:04.858 4882-4882/com.example.mouda3.mosbytest D/TIMER: ALIVE 17 06-20 12:50:05.269 4882-4882/com.example.mouda3.mosbytest D/PRESENTER INSTANCE: true 06-20 12:50:05.270 4882-4882/com.example.mouda3.mosbytest D/FRAGMENT: DESTROYED 06-20 12:50:05.858 4882-4882/com.example.mouda3.mosbytest D/TIMER: ALIVE 18

sockeqwe commented 7 years ago

Thanks for reporting and for providing this example to reproduce this bug! I'm investigating right now ...

sockeqwe commented 7 years ago

Ok, so the easiest way to solve this is to increase minSDK to API 17 because since Api 17 activity.isDestroyed() is provided. Otherwise, I have to build some work around which I would like to avoid if possible.

sockeqwe commented 7 years ago

Ok, I have to revert my previous comment ...

I did some research and played around a little bit with "Don't keep Activities" option. The Problem with "Don't keep Activities" is that your app will not behave like this in the real world.

For instance:

It turns out that this is not true. "Don't keep activities" just destroys the activity in a "strange" way by forcing the ActivityManager to recreate the activity. Again, this is not how a real app would behave:

  • I have assumed that "Don't keep activities" simulates the same behavior as destroying the Activity which is on the backstack while device is on low memory.

This is not true. Android kills the whole process without any callback at all. Therefore Presenter wont be "detached" (detachView() will not be called, whole process is killed).

  • Or that If my App has 2 Activities and I navigate from my apps Activity A to my apps Activity B, then on Activity A gets destroyed on low memory while Activity B is still in foreground. I thought that those use cases could be simulated with "Don't keep activities" enabled.

This is also not true. Android doesn't kill single Activities of an app. So on low memory, Activity A will not be killed if Activity B is in foreground. Rather the whole process is killed. https://commonsware.com/blog/2011/10/03/activities-not-destroyed-to-free-heap-space.html This seems like an implementation detail of the Android operating system, but it hasn't changed in years. Given the fact that RAM Memory built in a device is growing, I'm not expecting that Android OS will fall back to such fine and granular memory management (killing single activities) in the near future.

So again, "Don't keep activities" is not behaving nor simulating any real state your app can face in a real world scenario.

So, what about your reported issue? Well, I don't know how to fix it because this is really just the ActivityManager (part of the operating system) that destroys the Activity after all activity lifecycle callbacks of your Activity and Mosby went through. As far as I am concerned there is not much we can do.

I tend to say that this is "working as intended" or "wont fix". Any thoughts?

md3kung commented 7 years ago

So when memory is low, android kills the process that runs the activity and not the activity itself. Now that is something I wasn't aware of ...

The thing is, this misconception is spread all over the internet. As a result lot of production ready apps survive that use case. Although, after reading Dianne Hackborn's comment, I do not understand the purpose behind that option anymore.

Silly thing is that, I found out that some Android ROMs use that option for energy saving purposes by default : https://www.reddit.com/r/Xiaomi/comments/5guhh9/why_is_miui_8_so_aggressive_about_killing/davckqn

I think it would be better if Mosby reflects the right retainInstance value when the Activity is being destroyed in this case.

sockeqwe commented 7 years ago

I think it would be better if Mosby reflects the right retainInstance value when the Activity is being destroyed in this case.

Not sure how, as already said, this is something the operating system decides. As far as I know all Mosby can do is ask activity.isFinishing() which should return true (and does return true except if "don't keep Activities" enabled), but returns false if "dont keep Activities" is enabled and you press home.

I will take a look to check if architecture components ViewModel handle this case properly and if so how it is implemented.

sockeqwe commented 6 years ago

See #274. Feel free to reopen this issue if 3.1.0 (snapshot) doesn't solve this porblem.

chennyshan commented 6 years ago

Mosby Version 3.1.0 After splitting detachView(boolean retainInstance), it looks like that another problem appears when "Don't keep activities" is enable: maybe some presenter will never be removed. Suppose that we have the following Activities: A -> B -> C (C foreground). To Activity B, detachView() will be invoked, but destroy() will not, and it's presenter is retained in PresenterManager now. Then we call start the Activity A with FLAG_ACTIVITY_CLEAR_TOP from C. We know that Activity B won't be restored, so it's previous presenter will be left and looks like we have no chance to remove it later.

Any thoughts?

Thanks for this great library.

sockeqwe commented 6 years ago

Do you have a working demo example fir this that I could use for debugging?

chennyshan notifications@github.com schrieb am Fr., 12. Jan. 2018, 12:41:

Mosby Version 3.1.0 After splitting detachView(boolean retainInstance), it looks like that another problem appears when "Don't keep activities" is enable: maybe some presenter will never be removed. Suppose that we have the following Activities: A -> B -> C (C foreground). To Activity B, detachView() will be invoked, but destroy() will not, and it's presenter is retained in PresenterManager now. Then we call start the Activity A with FLAG_ACTIVITY_CLEAR_TOP from C. We know that Activity B won't be restored, so it's previous presenter will be left and looks like we have no chance to remove it later.

Any thoughts?

Thanks for this great library.

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/sockeqwe/mosby/issues/256#issuecomment-357217410, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjnrocC5KaEF3VqhoAu6qelxRPbROR-ks5tJ0TfgaJpZM4N_ka1 .

chennyshan commented 6 years ago

@sockeqwe .Thanks for your reply. I attached the demo and logcat output as following: image MosbyDemo.zip

chennyshan commented 6 years ago

@sockeqwe , I really want to know that what do you think about my question. Thanks a lot

sockeqwe commented 6 years ago

@chennyshan sorry for the long pause. This is working as intended: The Problem is that Don't keep activities is actually not emulating any real world behavior. So your users are never see this lifecycle. Actually Don't keep activites forces the ActivityManager to close the activity in such a "ugly" manner that Activity.isFinishing() returns false if Don't keep activities is enabled. In the real world it works just fine (if you test your example with "Don't keep activities" disabled it works properly).

So agian, Don't keep activities was introduced for us developer to test activity Activity.onSaveInstanceState() but it is not like the app behaves in the real world!

HanCheng commented 6 years ago

@sockeqwe In my experience, the Don't keep activities can easily spot real bugs/issues that are hard to reproduce in the real world. I have seen lots of weird crashes in some of the Samsung devices, after turn it on, its easy to reproduce. I suggest to save/restore presenter states in onSaveInstanceState and onRestoreInstanceState in activity.

sockeqwe commented 6 years ago

@HanCheng thanks for your feedback, but I don't understand how this is related to the original question / issue?

btw. Nothing is stoping you from saving the presenters state somehow...

lannyf77 commented 6 years ago

@sockeqwe the Don't keep activities alive is a great helper for spotting easily missed cases. When it is turned on, if you minimize the app the os will simulate that in some low resource case the os would 'kill' some app which is not in display. In that case (any case that os kills app because of low resource) it means os may use the savedInstanceState when the app is restored, and for that the isFinishing() will return false -- means the app is not really 'finishing'. but if activity is destroyed by calling the finish() regardless whether the 'dont keep activity alive' set or not the isFinishing will be true.

sockeqwe commented 6 years ago

simulate that in some low resource case the os would 'kill' some app which is not in display.

No it doesn't. On Low memory your app's process is killed without any notice and guarantees that any lifecycle callbacks is called.

All that Don't keep activites option is doing is it forces to call Activity.onSaveInstanceState() but it doesn't simulate any real world behavior.