Skip to content

fix(ios): proper disposal and recreation of iOS native views #9879

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

Merged
merged 8 commits into from
Apr 30, 2022

Conversation

CatchABus
Copy link
Contributor

@CatchABus CatchABus commented Apr 23, 2022

PR Checklist

What is the current behavior?

Native iOS views are not disposed and cannot be recreated when their view instances are re-added to the view tree.

What is the new behavior?

Native View Disposal

This PR was initially created for the sole purpose of destroying old native views (until more problems showed up in the process).
This is based on Martin's @farfromrefug PR #9671 which was reverted because of few problems.

@rigor789 If I'm not mistaken, the most common error case was this one:

***** Fatal JavaScript exception - application has been terminated. *****
NativeScript encountered a fatal error: Uncaught TypeError: Cannot set property 'delegate' of null
at
onUnloaded(file: src/webpack:/node_modules/@nativescript/core/ui/text-field/index.ios.js:114:0)
at (file: src/webpack:/node_modules/@nativescript/core/ui/core/view-base/index.js:321:0)
at callFunctionWithSuper(file: src/webpack:/node_modules/@nativescript/core/ui/core/view-base/index.js:312:0)
at callUnloaded(file: src/webpack:/node_modules/@nativescript/core/ui/core/view-base/index.js:321:0)
at unloadView(file: src/webpack:/node_modules/@nativescript/core/ui/core/view-base/index.js:466:0)
at (file: src/webpack:/node_modules/@nativescript/core/ui/core/view-base/index.js:249:0)
at eachChildView(file: src/webpack:/node_modules/@nativescript/core/ui/layouts/layout-base-common.js:101:0)
at eachChild(file: src/webpack:/node_modules/@nativescript/core/ui/core/view/view-common.js:772:0)
at onUnloaded(file: src/webpack:/node_modules/@nativescript/core/ui/core/view-base/index.js:248:0)
at (file: src/webpack:/node_modules/@nativescript/core/ui/core/view-base/index.js:321:0)
at callFunctionWithSuper(file: src/webpack:/node_modules/@nativescript/core/ui/core/view-base/index.js:312:0)
at callUnloaded(file: src/webpack:/node_modules/@nativescript/core/ui/core/view-base/index.js:321:0)
at unloadView(file: src/webpack:/node_modules/@nativescript/core/ui/core/view-base/index.js:466:0)
at (file: src/webpack:/node_modules/@nativescript/core/ui/core/view-base/index.js:249:0)
at eachChildView(file: src/webpack:/node_modules/@nativescript/core/ui/layouts/layout-base-common.js:101:0)
at eachChild(file: src/webpack:/node_modules/@nativescript/core/ui/core/view/view-common.js:772:0)
at onUnloaded(file: src/webpack:/node_modules/@nativescript/core/ui/core/view-base/index.js:248:0)
at (file: src/webpack:/node_modules/@nativescript/core/ui/core/view-base/index.js:321:0)
at callFunctionWithSuper(file: src/webpack:/node_modules/@nativescript/core/ui/core/view-base/index.js:312:0)
at callUnloaded(file:///app/vend<…>

After a bit of research, I concluded the root of this cause is _tearDownUI calls being called before the view gets unloaded.
So, what are the cases for a view to get unloaded too late when it's already destroyed?

One case is view controllers which have a viewDidDisappear method that is called once owner view is disposed. This method will call callUnloaded which will attempt to unload view and all its children in view tree. Let's keep in mind that if a view is disposed by means other than removeChild, it's still in view hierarchy.

One primary suspect for this crime is method destroyNode in View class. I'm not familiar with NS Angular, but its renderer seems to wrap this method inside another destroyNode. One of errors mentioned in #9671 has also occured in an Angular app.

I couldn't locate any other error-prone cases and I'm not completely sure all errors have occured because of the flaw in that certain method, but for start I added a missing callUnloaded call inside destroyNode.
If there are more causes that trigger this issue, I believe we can track them upon further app usage and solve them immediately, since we know what can cause this now. Core has got a dozen of _tearDownUI calls that are supposed to get called after view is unloaded but who knows!

Just as @farfromrefug pointed out, perhaps we should make sure those two calls are done in proper order in the future, like adding callUnloaded inside _tearDownUI for example which needs some refactoring.

So, all these details refer to https://github.com/NativeScript/NativeScript/pull/9879/files#diff-86ce61f77399a980c83c5a14ee2ac54b8c672d375d0280597c217eb17811a076, the change for disposing native views for iOS.

Re-using iOS view instance (non-reusable native view)

What if one wants to reuse the same NS instance even though its native view is not reusable? Upon re-adding an already removed view instance to the view tree, it should generate a new native view. This leads us to new problems which I'll use an example to describe below.

I'll use a non-reusable animated drawer view of mine embedded in RootLayout, in a plain NS app. This is a good example since there is a constant use of insertChild and removeChild methods, and view will also get transformed it gets hidden to the left side of the screen.

In short, attempting to open drawer will result in calling insertChild, while trying to close it will result in calling removeChild.

This is how my drawer originally looks like when opening it for first time:
image

If it's removed and re-added to view tree in current NS core version, it will look like this:
image

It looks like this because images undergo a cleanup on disposal since NS 8.2. As view is not currently disposed properly, native view will not be created anew and this results in images not being reloaded.

Now, the following screenshot displays the result of using changes from PR #9671 alone.
image

View layout seems completely messed up and that is so confusing. This problem occured because view instances keep cache of last used native frame.
And here comes the addition of disposeNativeView in the iOS version of View class: https://github.com/NativeScript/NativeScript/pull/9879/files#diff-025d8125f2c2388468769424324a18b7a896921f7b44f720db8014eb653b5f84R67

This comes with really good results:
image

Yet, it seems few children are not aligned properly, and there's no good explanation.
I concluded that the cause of this is the native view translateX transform occuring too early. That transform messes up view position in native window, resulting in padding and inset miscalculations.

That is why we should prevent transforms from happening when layout is still invalid, and apply them once view content layout is ready.
See here: https://github.com/NativeScript/NativeScript/pull/9879/files#diff-025d8125f2c2388468769424324a18b7a896921f7b44f720db8014eb653b5f84R132

The final result:
image

The proper disposal of native views works quite well with the few recent additions of GC inside core.
About reuse, I believe this is a really good step for re-using iOS views even when their native instances are not reusable. This has always been possible in android, but it seems we lacked the proper view cleanup to make this work in iOS and that created a bit of confusion.

This PR is a possible candidate for fixing the iOS part of #9820, and its view cleanup is a good solution for my issue described in #9875

@cla-bot cla-bot bot added the cla: yes label Apr 23, 2022
@CatchABus CatchABus marked this pull request as ready for review April 23, 2022 18:10
@cla-bot
Copy link

cla-bot bot commented Apr 25, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign the CLA at https://www.nativescript.org/cla.
CLA has not been signed by users: @CatchABus.
After signing the CLA, you can ask me to recheck this PR by posting @cla-bot check as a comment to the PR.

@cla-bot cla-bot bot removed the cla: yes label Apr 25, 2022
@cla-bot
Copy link

cla-bot bot commented Apr 25, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign the CLA at https://www.nativescript.org/cla.
CLA has not been signed by users: @CatchABus.
After signing the CLA, you can ask me to recheck this PR by posting @cla-bot check as a comment to the PR.

@CatchABus
Copy link
Contributor Author

@cla-bot check

@cla-bot cla-bot bot added the cla: yes label Apr 25, 2022
@cla-bot
Copy link

cla-bot bot commented Apr 25, 2022

The cla-bot has been summoned, and re-checked this pull request!

@NathanWalker NathanWalker changed the base branch from master to alpha April 30, 2022 17:59
@NathanWalker NathanWalker changed the title fix: Proper disposal and recreation of iOS native views fix(ios): proper disposal and recreation of iOS native views Apr 30, 2022
@NathanWalker NathanWalker merged commit 96a575d into NativeScript:alpha Apr 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants