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

[Alexandre Janniaux]

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.

The point is that if you're using wnd, you cannot let the display close
and potentially the underlying code to destroy the window, so you have
to wait, meaning mutex. That's actually the same discussion as with the
macosx.m vout but without the display_lock issue.

Regards,
--
Alexandre Janniaux
Videolabs