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

fix: crash on Android Tab-View #6466 #6467

Merged
merged 10 commits into from Nov 14, 2018

Conversation

Projects
None yet
4 participants
@DickSmith
Contributor

DickSmith commented Oct 27, 2018

This fix has been tested in production with no new issues occurring.

Long-term solution would be one of the following, though:

  1. reintroduce overrides for saveState/restoreState in the current PagerAdapter implementation (removed on this commit ac04ede#diff-f1459d509d1432b432c29bcd30e462fbL97)
  2. use FragmentPagerAdapter
  3. use FragmentStatePagerAdapter

Both 2 and 3 manage the save/restore cycles. The main difference between 2 and 3 is that 2 uses more memory, but allows for quicker switching between Fragments than 3. Since tabs should usually be limited to 5 or less, this may be the best choice to maintain performance, which is important for top level navigation tabs.

When I have more time I may experiment with these options myself to see what the difference to performance and memory consumption is for each.

PR Checklist

What is the current behavior?

~2% Crash Rate on Crashlytics:

Caused by java.lang.IllegalArgumentException
No view found for id 0xa (unknown) for fragment Fragment_script_37_1236368_r{1a141ad #11 id=0xa android:viewpager:10:2}
android.support.v4.app.FragmentManagerImpl.moveToState (FragmentManager.java:1454)
android.support.v4.app.FragmentManagerImpl.moveFragmentToExpectedState (FragmentManager.java:1784)
android.support.v4.app.FragmentManagerImpl.moveToState (FragmentManager.java:1852)
android.support.v4.app.BackStackRecord.executeOps (BackStackRecord.java:802)
android.support.v4.app.FragmentManagerImpl.executeOps (FragmentManager.java:2625)
android.support.v4.app.FragmentManagerImpl.executeOpsTogether (FragmentManager.java:2411)
android.support.v4.app.FragmentManagerImpl.removeRedundantOperationsAndExecute (FragmentManager.java:2366)
android.support.v4.app.FragmentManagerImpl.execPendingActions (FragmentManager.java:2273)
android.support.v4.app.FragmentController.execPendingActions (FragmentController.java:391)
android.support.v4.app.FragmentActivity.onResume (FragmentActivity.java:517)
android.app.Instrumentation.callActivityOnResume (Instrumentation.java:1361)
android.app.Activity.performResume (Activity.java:7344)
android.app.ActivityThread.performResumeActivity (ActivityThread.java:3763)
android.app.ActivityThread.handleResumeActivity (ActivityThread.java:3828)
android.app.ActivityThread$H.handleMessage (ActivityThread.java:1746)
android.os.Handler.dispatchMessage (Handler.java:105)
android.os.Looper.loop (Looper.java:164)
android.app.ActivityThread.main (ActivityThread.java:6938)
java.lang.reflect.Method.invoke (Method.java)
com.android.internal.os.Zygote$MethodAndArgsCaller.run (Zygote.java:327)
com.android.internal.os.ZygoteInit.main (ZygoteInit.java:1374)

What is the new behavior?

No crash.

Fixes/Implements/Closes #6466.

fix: crash on Android Tab-View #6466
This fix has been tested in production with no new issues occurring.

Long-term solution would be one of the following, though:
1. reintroduce overrides for `saveState`/`restoreState` in the current PagerAdapter implementation (removed on this commit ac04ede#diff-f1459d509d1432b432c29bcd30e462fbL97)
2. use FragmentPagerAdapter
3. use FragmentStatePagerAdapter

Both 2 and 3 manage the save/restore cycles. The main difference between 2 and 3 is that 2 uses more memory, but allows for quicker switching between Fragments than 3. Since tabs should usually be limited to 5 or less, this may be the best choice to maintain performance, which is important for top level navigation tabs.

When I have more time I may experiment with these options myself to see what the difference to performance and memory consumption is for each.

@ns-bot ns-bot added the cla: yes label Oct 27, 2018

@vakrilov

This comment has been minimized.

Member

vakrilov commented Nov 2, 2018

Hi and thanks for the PR!

The team have been quite busy baking the upcoming NativeScript 5.0 release. We hope you love it as much as we do! The major release is the reason we taking longer with PR reviews. We will resume active reviewing in the following weeks.

Thanks for your patience and for being awesome contributor!

@manoldonev

This comment has been minimized.

Contributor

manoldonev commented Nov 5, 2018

@DickSmith I went through a lot of threads that discuss potenial causes/solutions for the "No view found for id [...] (unknown) for fragment" exception like this and this (the latter looks promising but we still need a repro to work on) and I am not convinced that the proposed solution here addresses the root cause of the problem.

After the rework with commit ac04ede we started using a modified version of the stock Android FragmentPagerAdapter (main logic is the same) and the stock one does not have any similar safeguarding mechanisms. At the moment we are not planning to reintroduce the saveState / restoreState logic i.e. mimic the FragmentStatePagerAdapter.

I personally have seen the aforementioned error while sorting out various issues with frame/tab fragments over the past month (there are multiple fixes on fragments after rc was released). I am not sure how you are certain that no issues are occurring with the proposed fix having no reliable way to reproduce this but my first suggestion would be for you to roll out a version of your application that uses tns-core-modules@next (or wait a couple more days till tns-core-modules@5.0.2 is published but "next" essentially has all the fragment fixes now) and give it a go. If you still believe that the problem occurs in your app I would try the solution suggested in https://stackoverflow.com/a/19900206 and extend the org.nativescript.widgets.TabViewPager in tns-core-modules-widgets and test again.

@DickSmith

This comment has been minimized.

Contributor

DickSmith commented Nov 9, 2018

Hey @manoldonev ,

So I had discussed this issue with several members of the core-team when we met at jsMobileConf in Boston a few weeks back.

Not having a proper save/restore state is the problem here, since on resuming the activity, the mCurTransaction may still have transactions referencing objects which no longer exist due to garbage collection/backgrounding.

I have Crashlytics, which tells me the frequency and occurrence rate of a crash, and after making this hotfix, my crash rate improved. Crashlytics never provides repro.

This just prevents the crash, and does so 100%, I was getting hundreds of this crash, and now get none, with no other crash replacing it down the line. There is the possibility that the app may then be in an unresponsive state, and the user will need to manually quit and restart the app, but this is still infinitely better in a released production app with end users.

@vakrilov

This comment has been minimized.

Member

vakrilov commented Nov 12, 2018

Hey @DickSmith,
We just discussed this with @manoldonev - it doesn't seem that it might cause any side effects so it should be fairly safe to merge.

One thing that we wanted to figure out is wether it fixes an actual issue in the current codebase, as AFAIK - we have done some fixes since the version you are using. Do you think there is a way to do that? It will probably involve rolling out a version without the fix, but with updated version of NativeScript and monitoring if the errors will show up again in the analytics. I would understand if you are reluctant to do that though.

If you are out of ideas (we are) of how to further understand the actual problem and how to reproduce it - I would suggest just to merge the PR. Maybe, add an appropriate comment in the code (as it might be confusing to commit transaction on saveState).

@DickSmith

This comment has been minimized.

Contributor

DickSmith commented Nov 12, 2018

@vakrilov

At this point our tns-core-modules is 95% 5.0+ for Android (I last diff'd the packages on Monday or Tuesday after the official release). The only place it isn't is for text-base and its descendants, since so much of that is handled in common between iOS/Android and in iOS views still seems to have some quirks around the new lifecycle stuff.

Our, core-modules widgets is at 5.0+ (with some additional fixes from us). We also build with CLI 5.0. The only things at this point that aren't really flush with latest (other than additional bug fixes) are the tns-core-modules for iOS stuff I mentioned (due to the scrollview and other issues we've discussed) and the runtimes, mostly due to a compatibility issue with the latest runtime and Crashlytics.

I'll definitely add a comment with some of the stuff I mentioned at the top of this ticket, as I think we either need to reimplement save/restore, or one of the other Adapters that handle it, or find a way to guarantee that this component won't try and execute on potentially garbage-collected objects after restore (which I don't think is possible). Like I mentioned, this 100% prevents the crash, and in many cases won't lead to any adverse behaviour, but in some case may result in the tab being in a bad state due to the lack of restore logic (but still better than a much more frequent crash).

As an additional aside, the real solution here may also be to differentiate between Tabs and Bottom navigation buttons, which Google does, both in their API and their Material Design docs.

@manoldonev

This comment has been minimized.

Contributor

manoldonev commented Nov 12, 2018

test

vakrilov added some commits Nov 13, 2018

@NativeScript NativeScript deleted a comment from manoldonev Nov 13, 2018

@manoldonev

This comment has been minimized.

Contributor

manoldonev commented Nov 14, 2018

test ngapps api19 api21 api27 api28

@NativeScript NativeScript deleted a comment from vakrilov Nov 14, 2018

@vakrilov

This comment has been minimized.

Member

vakrilov commented Nov 14, 2018

test ngapps api19 api21 api27 api28

@vakrilov vakrilov merged commit db33cf3 into NativeScript:master Nov 14, 2018

1 of 2 checks passed

ci/jenkins/core-modules-tests FAILed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Nov 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment