[vlc-devel] [PATCH 2/3] ios: uiview: add new vout_window module

[Steve Lhomme]

On 2020-11-05 9:43, Alexandre Janniaux wrote:
> Hi,
> 
> Thank you for the thoroughful review.
> 
> Answers inline. ;)
> 
> On Thu, Nov 05, 2020 at 07:59:58AM +0100, Steve Lhomme wrote:
>> Comments below.
>>
>> On 2020-11-04 17:54, Alexandre Janniaux wrote:
>>> Refactor code from the ios vout display to create a dedicated UIView
>>> which handle the touch events and window resizing.
>>>
>>> The resizing events are emitted after any children UIView layer is added
>>> to the UIView vout window so that it doesn't block the main thread under
>>> display lock while the display module is being created. Indeed, it would
>>> result in a lock inversion and thus potential deadlocks.
>>> ---
>>>    modules/video_output/Makefile.am    |  15 +-
>>>    modules/video_output/apple/uiview.m | 390 ++++++++++++++++++++++++++++
>>>    modules/video_output/ios.m          |  66 ++---
>>>    3 files changed, 421 insertions(+), 50 deletions(-)
>>>    create mode 100644 modules/video_output/apple/uiview.m
>>>

>>> +- (void)detachFromParent
>>> +{
>>> +    vlc_mutex_lock(&_mutex);
>>> +    _wnd = NULL;
>>
>> Using a atomic_uintptr_t would save you a lock between the vout thread and
>> the UI/main thread during the Close.
> 
> No, it won't work, same TOCTOU as explained by Rémi:
> 
>      vout_window_t *wnd = atomic_load(&atomic_wnd);
>      // the real window object might be destroyed here
>      do_something_with_wnd(wnd);
>      // UB because wnd might have been freed.

I was just talking about these particular lines. The rest of the code 
should still use the mutex to do actions on the wnd. So the atomic_load 
would be done under the mutex. It's not pretty but less lock in the 
Close would be good.