[NUI][AT-SPI] Detach NUIViewAccessible in View.Dispose() #5646
Conversation
aswigon
force-pushed
the
aswigon-a11y-detach
branch
from
93c885e to
d51fb4c
Compare
| @@ -113,8 +113,6 @@ private static void InitializeAccessibilityDelegateAccessibleInterface() | |||
| private static ulong AccessibilityCalculateStatesWrapper(IntPtr self, ulong initialStates) | |||
| { | |||
| View view = GetViewFromRefObject(self); | |||
| if (view == null) | |||
There was a problem hiding this comment.
If view is null, then the following code is not accessible?
Or view cannot be null?
bitMask = view.AccessibilityCalculateStates().BitMask;
There was a problem hiding this comment.
It cannot be null, see below.
There was a problem hiding this comment.
Could you please throw exception than? If so, it might be helpful when some unknown error occured in future.
There was a problem hiding this comment.
Okay, added throwing ArgumentException in both GetViewFromRefObject and GetInterfaceFromRefObject.
| @@ -113,8 +113,6 @@ private static void InitializeAccessibilityDelegateAccessibleInterface() | |||
| private static ulong AccessibilityCalculateStatesWrapper(IntPtr self, ulong initialStates) | |||
| { | |||
| View view = GetViewFromRefObject(self); | |||
| if (view == null) | |||
There was a problem hiding this comment.
GetViewFromRefObject는 로직상 null 을 리턴할 수 있는데, null 리턴이 절대 일어나지 않아야 한다면 debug assert나 exception 또는 최소한 로그 정도는 남겨두어야 문제 상황을 추적하기 쉬울 것 같은데 어떻게 생각하시나요?
GetViewFromRefObject can return null. If you think it's not possible to get null in this case, how about adding debug.assert or throwing an exception or leave a debug log to notify the error state?
There was a problem hiding this comment.
There is already an error log reported in GetViewFromRefObject(). It should never be null.
I deleted the null check in AccessibilityCalculateStatesWrapper() because it is the only one, and there are 61 more Accessibility...Wrapper() functions in this file without a null check.
The null check in only one of 62 wrapper methods was added in #4370, as a workaround / hack. At that time it was not understood what the underlying cause was... This PR should address the underlying cause, so the workaround here should no longer be necessary.
aswigon
requested review from
hyunjushin,
dongsug-song,
Seoyeon2Kim,
taehyub,
everLEEst and
hinohie
as code owners
The application may crash if the View is disposed but the Accessibility infrastructure calls one of the View methods. Detaching the NUIViewAccessible proxy object in View.Dispose() should prevent that.
aswigon
force-pushed
the
aswigon-a11y-detach
branch
from
d51fb4c to
f9788d2
Compare
| @@ -53,7 +53,7 @@ private static View GetViewFromRefObject(IntPtr refObjectPtr) | |||
|
|
|||
| if (view is null) | |||
| { | |||
| NUILog.Error($"RefObject 0x{refObjectPtr:x} is not a View"); | |||
| throw new ArgumentException($"RefObject 0x{refObjectPtr:x} is not a View", nameof(refObjectPtr)); | |||
There was a problem hiding this comment.
I just worryed about some case of View's that NUI didn't created. (For example, Tizen.NUI.Scene3D.Model create Tizen.NUI.Scene3D.ModelNode internally, but they are automatically created only at dali-side. So NUI (actually, Registry) don't know that View. But ModelNode is also kind of View.
There was a problem hiding this comment.
This shouldn't be a problem. The methods in ViewAccessibilityWrappers.cs are entry points for NUIViewAccessible methods (a C++ subclass of ControlAccessible) and NUIViewAccessible is only created for (a subset of) C# Views.
Description of Change
The application may crash if the View is disposed but the Accessibility infrastructure calls one of the View methods. Detaching the NUIViewAccessible proxy object in View.Dispose() should prevent that.
Dependency: https://review.tizen.org/gerrit/#/c/platform/core/uifw/dali-csharp-binder/+/300277/
API Changes