fix(ios): Do not call setNeedsLayout() when another layout update request is in process #9875

CatchABus · 2022-04-15T14:08:24Z

PR Checklist

The PR title follows our guidelines: https://github.com/NativeScript/NativeScript/blob/master/tools/notes/CONTRIBUTING.md#commit-messages.
There is an issue for the bug/feature this PR is for. To avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it.
You have signed the CLA.
All existing tests are passing: https://github.com/NativeScript/NativeScript/blob/master/tools/notes/DevelopmentWorkflow.md#running-unit-tests-application.
Tests for the changes are included - https://github.com/NativeScript/NativeScript/blob/master/tools/notes/WritingUnitTests.md.

What is the current behavior?

There are cases that a layout contains children that often get updated because of bindings.
In my case, it was a list of GridLayouts with nested images, the visibility of which was changing through user interactivity.

Upon 2-3 uses, my app was crashing with an assertion failed message:

====== Assertion failed ======
Native stack trace:
1          0x103a1e7d8 tns::Assert(bool, v8::Isolate*) + 128
2          0x1039a6848 tns::ArgConverter::Invoke(v8::Local<v8::Context>, objc_class*, v8::Local<v8::Object>, tns::V8Args&, tns::MethodMeta const*, bool) + 100
3          0x1039f9658 tns::MetadataBuilder::InvokeMethod(v8::Local<v8::Context>, tns::MethodMeta const*, v8::Local<v8::Object>, tns::V8Args&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool) + 88
4          0x1039f8ec0 tns::MetadataBuilder::MethodCallback(v8::FunctionCallbackInfo<v8::Value> const&) + 220
5          0x103b300e4 v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) + 544
6          0x103b2f5e4 v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) + 524
7          0x103b2ed7c v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) + 228
8          0x10420a64c Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit + 108
9          0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
10         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
11         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
12         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
13         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
14         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
15         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
16         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
17         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
18         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
19         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
20         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
21         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
22         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
23         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
24         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
25         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
26         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
27         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
28         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
29         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
30         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
31         0x1041dd220 Builtins_StoreIC + 832
32         0x10428ab74 Builtins_StaNamedPropertyHandler + 148
33         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
34         0x1041a15ec Builtins_JSEntryTrampoline + 172
35         0x1041a1284 Builtins_JSEntry + 164
36         0x103c7bcf4 v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) + 2532
37         0x103c7b2dc v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) + 216
38         0x103e06f98 v8::internal::Object::SetPropertyWithAccessor(v8::internal::LookupIterator*, v8::internal::Handle<v8::internal::Object>, v8::Maybe<v8::internal::ShouldThrow>) + 864
39         0x103e0aa88 v8::internal::Object::SetPropertyInternal(v8::internal::LookupIterator*, v8::internal::Handle<v8::internal::Object>, v8::Maybe<v8::internal::ShouldThrow>, v8::internal::StoreOrigin, bool*) + 420
40         0x103e0a808 v8::internal::Object::SetProperty(v8::internal::LookupIterator*, v8::internal::Handle<v8::internal::Object>, v8::internal::StoreOrigin, v8::Maybe<v8::internal::ShouldThrow>) + 80
41         0x103ed8774 v8::internal::Runtime::SetObjectProperty(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, v8::internal::StoreOrigin, v8::Maybe<v8::internal::ShouldThrow>) + 272
42         0x103edbe58 v8::internal::Runtime_SetKeyedProperty(int, unsigned long*, v8::internal::Isolate*) + 88
43         0x10420a50c Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_NoBuiltinExit + 108
44         0x10428adec Builtins_StaKeyedPropertyHandler + 140
45         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
46         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
47         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
48         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
49         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
50         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
51         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
52         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
53         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
54         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
55         0x1041ada24 Builtins_StoreIC_NoFeedback + 4132
56         0x10428ab74 Builtins_StaNamedPropertyHandler + 148
57         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
58         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
59         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
60         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
61         0x1041a3818 Builtins_InterpreterEntryTrampoline + 248
62         0x1041a15ec Builtins_JSEntryTrampoline + 172
63         0x1041a1284 Builtins_JSEntry + 164
64         0x103c7bcf4 v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) + 2532
65         0x103c7b2dc v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) + 216
66         0x103ace780 v8::Function::Call(v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) + 448
67         0x1039a7f90 tns::ArgConverter::MethodCallback(ffi_cif*, void*, void**, void*) + 1156
68         0x103a99264 ffi_closure_SYSV_inner + 800
69         0x103a9c1b4 .Ldo_closure + 20
70         0x18624f42c <redacted> + 56
71         0x186218560 <redacted> + 116
72         0x1861e1260 <redacted> + 284
73         0x18621a910 <redacted> + 636
74         0x1861d2ad0 <redacted> + 1988
75         0x1862068c8 <redacted> + 784
76         0x186213a68 <redacted> + 4428
77         0x1863c3318 <redacted> + 828
78         0x1861e6c30 <redacted> + 7904
79         0x1861dba1c <redacted> + 6760
80         0x18701d7dc <redacted> + 176
81         0x18684c7d8 <redacted> + 84
82         0x186ec6008 <redacted> + 144
83         0x186ec55f8 <redacted> + 60
84         0x183c84020 <redacted> + 28
85         0x183c94ce0 <redacted> + 208
86         0x183bcefe8 <redacted> + 268
87         0x183bd47f4 <redacted> + 820
88         0x183be83b8 CFRunLoopRunSpecific + 600
89         0x19f57838c GSEventRunModal + 164
90         0x1865886a8 <redacted> + 1100
91         0x1863077f4 UIApplicationMain + 2092
92         0x103a9c044 ffi_call_SYSV + 68
93         0x103a98ac8 ffi_call_int + 968
94         0x103a3b9dc tns::Interop::CallFunctionInternal(tns::MethodCall&) + 428
95         0x1039fe65c std::__1::__function::__func<tns::MetadataBuilder::CFunctionCallback(v8::FunctionCallbackInfo<v8::Value> const&)::$_2, std::__1::allocator<tns::MetadataBuilder::CFunctionCallback(v8::FunctionCallbackInfo<v8::Value> const&)::$_2>, void ()>::operator()() + 560
96         0x103a5ec04 tns::Tasks::Drain() + 100
97         0x103a58fd0 -[NativeScript initWithConfig:] + 600
98         0x102dff430 98  mobile                              0x0000000102dff430 mobile + 29744
99         0x103901a24 99  dyld                                0x0000000103901a24 start + 520
JavaScript stack trace:
at requestLayout (file: node_modules/@nativescript/core/ui/core/view/index.ios.js:42:0)
at performLayout (file: node_modules/@nativescript/core/ui/core/view-base/index.js:412:0)
at requestLayout (file: node_modules/@nativescript/core/ui/core/view-base/index.js:419:0)
at requestLayout (file: node_modules/@nativescript/core/ui/core/view/view-common.js:735:0)
at requestLayout (file: node_modules/@nativescript/core/ui/core/view/index.ios.js:38:0)
at performLayout (file: node_modules/@nativescript/core/ui/core/view-base/index.js:412:0)
at requestLayout (file: node_modules/@nativescript/core/ui/core/view-base/index.js:419:0)
at requestLayout (file: node_modules/@nativescript/core/ui/core/view/view-common.js:735:0)
at requestLayout (file: node_modules/@nativescript/core/ui/core/view/index.ios.js:38:0)
at performLayout (file: node_modules/@nativescript/core/ui/core/view-base/index.js:412:0)

What is the new behavior?

I concluded that an additional setNeedsLayout call was causing this crash in my app.
My patch checks if a layout update has already been requested, and prevents pointless setNeedsLayout calls.

Runtime version: 8.2
Core version: 8.2

… in process.

… validation issues.

CatchABus · 2022-04-15T17:53:54Z

Let me point out this problem never occured in versions older than 8.2.

farfromrefug · 2022-04-16T06:47:37Z

@dimitrisrk when you say older than 8.2 you are talking about the runtime or the core?

CatchABus · 2022-04-16T13:37:52Z

@dimitrisrk when you say older than 8.2 you are talking about the runtime or the core?

It seems it's core-specific.
I run an app with NS 8.2 core and runtime 8.1 but error persists.

farfromrefug · 2022-04-16T14:50:45Z

@dimitrisrk ok so if you say it was not happening with core 8.1 we should theorically be able to find the culprit commit

CatchABus · 2022-04-17T16:58:04Z

@dimitrisrk ok so if you say it was not happening with core 8.1 we should theorically be able to find the culprit commit

I managed to find the culprit and it seems the problem is deeper than it looks.
Strange though it may seem, my app gets crashed because of queueGC() calls inside core.

I have a list-view with items that also contain icons (using Image element which makes use of GC during disposal), and a popup view that has sorting options for this list.

Visual sorting is performed by sorting an ObservableArray of items. ListView gets notified for items change and attempts to destroy old children views in order to create new ones.

So, after I attempt to sort my list about 2-3 times, my app will throw the error posted above.

@NathanWalker I'm not sure why, but it seems iOS garbage collector has trouble when multiple image views are to be disposed which is my case. If I comment out code inside queueGC(), my app will stop crashing.

farfromrefug · 2022-04-17T19:31:01Z

@dimitrisrk you call queueGC() or is it the core calling it ?
you use N imageView?
if so i think they added some tricky garbage code in it. could be worse trying with another image component

CatchABus · 2022-04-17T19:56:21Z

@dimitrisrk you call queueGC() or is it the core calling it ? you use N imageView? if so i think they added some tricky garbage code in it. could be worse trying with another image component

I use original Image core component for all my icons.
Those queueGC() are also inside core.

farfromrefug · 2022-04-17T21:34:41Z

ok so i think it could be good to test with the jsc runtime. it could be v8 related.

CatchABus · 2022-04-17T22:44:27Z

ok so i think it could be good to test with the jsc runtime. it could be v8 related.

Perhaps. Few commits ago, GC was performed more often and a recent fix made it less frequent (I think problem starts there).
It seems this is caused by a single icon in my popup that's disposed during open, close, and selection change.

What I found so far:
The delay of GC is probably the main reason for my problem and I'm not sure why.
There is a delay of 900ms that is probably a bit long for GC now?

Changing queueGC call delay to 1ms here, solves my app crash problem.

    disposeImageSource() {
        var _a, _b, _c;
        if (((_a = this.nativeViewProtected) === null || _a === void 0 ? void 0 : _a.image) === ((_b = this.imageSource) === null || _b === void 0 ? void 0 : _b.ios)) {
            this.nativeViewProtected.image = null;
        }
        if ((_c = this.imageSource) === null || _c === void 0 ? void 0 : _c.ios) {
            this.imageSource.ios = null;
        }
        this.imageSource = null;
        queueGC(1); // Delay of 1 ms
    }

NathanWalker · 2022-04-17T22:56:22Z

@dimitrisrk lets decrease it to 1ms there - if you pr’d that we can put up an alpha that we can put against few projects next couple days and if good universally we can publish patch with that. Makes sense the delay should be less there. Could also include it in this PR.

edusperoni · 2022-04-18T11:59:59Z

a GC call shouldn't crash your app. If it does then we have some underlying issue. It might be that the view that's calculating the layout has been removed from the view tree and the GC call collected the native view from it, in which case there should be a guard around that.

farfromrefug · 2022-04-18T12:09:38Z

I agree with @edusperoni. Changing the delay does not seem like the right fix. We need to find the real source of the issue

CatchABus · 2022-04-18T15:29:02Z

a GC call shouldn't crash your app. If it does then we have some underlying issue. It might be that the view that's calculating the layout has been removed from the view tree and the GC call collected the native view from it, in which case there should be a guard around that.

I've been doing more tests since last night and I'm also suspecting it's related to a garbage-collected view.
I have also been looking for reasons why discarded view keeps requesting layout updates. I remove its binding context and don't change any of its properties, not to mention it's removed from view tree too early to explain this.
I also attempted to use a condition like this != getRootView() && this.parent == null to block requestLayout further execution but it didn't help at all.

I believe such issues should give more priority to solving the problem related to _tearDownUI patch for iOS, since I suspect the incomplete view lifecycle may have also played an importart role in this case.

CatchABus added 2 commits April 15, 2022 16:41

fix(ios): Do not call requestLayout() when another layout update is…

4803d61

… in process.

fix: Do not prevent call of super class method as it may cause layout…

35ebb5f

… validation issues.

cla-bot bot added the cla: yes label Apr 15, 2022

CatchABus marked this pull request as draft April 15, 2022 21:33

CatchABus mentioned this pull request Apr 23, 2022

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

Merged

5 tasks

CatchABus closed this Jun 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(ios): Do not call setNeedsLayout() when another layout update request is in process #9875

fix(ios): Do not call setNeedsLayout() when another layout update request is in process #9875

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fix(ios): Do not call setNeedsLayout() when another layout update request is in process #9875

fix(ios): Do not call setNeedsLayout() when another layout update request is in process #9875

Uh oh!

Conversation

Uh oh!

PR Checklist

What is the current behavior?

What is the new behavior?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!