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

[Steve Lhomme]

On 2020-11-05 10:49, Alexandre Janniaux wrote:
> Hi,
> 
> On Thu, Nov 05, 2020 at 10:35:59AM +0100, Steve Lhomme wrote:
>> 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.
> 
> That doesn't change the issue, if elsewhere you do:
> 
>      vlc_mutex_lock()
>      if (wnd)
>          do_something_with_wnd()
>      vlc_mutex_unlock()
> 
> whereas you do this in close:
> 
>      atomic_store(&wnd, NULL);
> 
> You´ll break the contract vlc_mutex_lock() was supposed to protect
> since wnd could be destroyed inside the lock.

Ah yes, my bad.