[vlc-devel] [PATCH 02/14] qt: cancel preparse requests before destroying/changing model of NetworkMediaModel

[Pierre Lamot]

On 2020-02-06 16:45, Thomas Guillem wrote:
> On Thu, Feb 6, 2020, at 13:56, Pierre Lamot wrote:
>>   this caused a crash if the UI was destroyed when a request was 
>> pending
>> ---
>>  modules/gui/qt/network/networkmediamodel.cpp | 23 
>> +++++++++++++++++++-
>>  modules/gui/qt/network/networkmediamodel.hpp |  5 ++++-
>>  2 files changed, 26 insertions(+), 2 deletions(-)
>> 
>> diff --git a/modules/gui/qt/network/networkmediamodel.cpp
>> b/modules/gui/qt/network/networkmediamodel.cpp
>> index 1a24f3de91..dca7534438 100644
>> --- a/modules/gui/qt/network/networkmediamodel.cpp
>> +++ b/modules/gui/qt/network/networkmediamodel.cpp
>> @@ -40,10 +40,22 @@ enum Role {
>> 
>>  NetworkMediaModel::NetworkMediaModel( QObject* parent )
>>      : QAbstractListModel( parent )
>> +    , m_preparseSem(1)
>>      , m_ml( nullptr )
>>  {
>>  }
>> 
>> +NetworkMediaModel::~NetworkMediaModel()
>> +{
>> +    if (!m_preparseSem.tryAcquire())
>> +    {
>> +        auto libvlc = vlc_object_instance(m_ctx->getIntf());
>> +        vlc_media_tree_PreparseCancel( libvlc, this );
>> +        //wait for the callback call on cancel
>> +        m_preparseSem.acquire();
>> +    }
>> +}
>> +
>>  QVariant NetworkMediaModel::data( const QModelIndex& index, int role 
>> ) const
>>  {
>>      if (!m_ctx)
>> @@ -255,6 +267,11 @@ bool NetworkMediaModel::initializeMediaSources()
>>          emit isIndexedChanged();
>>      }
>> 
>> +    if (!m_preparseSem.tryAcquire())
>> +    {
>> +        vlc_media_tree_PreparseCancel( libvlc, this );
>> +        m_preparseSem.acquire();
>> +    }
> 
> What is the the goal of this semaphore ?
> I'm afraid that using tryAcquire() is racy. 2 threads could succeed in
> tryAcquire(), call vlc_media_tree_PreparseCancel() and
> m_preparseSem.acquire() at the same time.
> Anyway, I don't think it is needed. vlc_media_tree_PreparseCancel(id)
> will cancel every tasks that have this id.
> You could call it more than
> one time with the same id, the second time won't cancel any other
> tasks and do nothing.

This function (initializeMediaSources) can only be called from the UI 
thread.
Identically, the model can only be destroyed from the UI thread

The idea is that an instance of NetworkMediaModel can only have one 
preparse request at the same time. So I waited for the canceled task to 
return.
But yes, maybe it's unnecessary to tryAquire before doing the cancel as 
we returns in the callback when the status is Failed.

Though we still need to wait for the callback completion in the 
destructor as we don't want to run code on a destructed object.

>>      vlc_media_tree_Preparse( tree, libvlc, m_treeItem.media.get(),
>> this );
>>      m_parsingPending = true;
>>      emit parsingPendingChanged(m_parsingPending);
>> @@ -335,8 +352,12 @@ void
>> NetworkMediaModel::onItemRemoved(MediaSourcePtr, input_item_node_t *
>> node,
>>      }, Qt::QueuedConnection);
>>  }
>> 
>> -void NetworkMediaModel::onItemPreparseEnded(MediaSourcePtr,
>> input_item_node_t* node, enum input_item_preparse_status)
>> +void NetworkMediaModel::onItemPreparseEnded(MediaSourcePtr,
>> input_item_node_t* node, enum input_item_preparse_status status)
>>  {
>> +    m_preparseSem.release();
>> +    if (status != ITEM_PREPARSE_DONE)
>> +        return;
>> +
>>      InputItemPtr p_node { node->p_item };
>>      QMetaObject::invokeMethod(this, [this, 
>> p_node=std::move(p_node)]()
>> {
>>          if (p_node != m_treeItem.media)
>> diff --git a/modules/gui/qt/network/networkmediamodel.hpp
>> b/modules/gui/qt/network/networkmediamodel.hpp
>> index bc6c2ea8b5..969d085dc4 100644
>> --- a/modules/gui/qt/network/networkmediamodel.hpp
>> +++ b/modules/gui/qt/network/networkmediamodel.hpp
>> @@ -33,6 +33,8 @@
>>  #include <util/qml_main_context.hpp>
>>  #include "networksourcelistener.hpp"
>> 
>> +#include <QSemaphore>
>> +
>>  #include <memory>
>> 
>>  using MediaSourcePtr = vlc_shared_data_ptr_type(vlc_media_source_t,
>> @@ -98,6 +100,7 @@ public:
>> 
>>      explicit NetworkMediaModel(QObject* parent = nullptr);
>>      NetworkMediaModel( QmlMainContext* ctx, QString parentMrl,
>> QObject* parent = nullptr );
>> +    virtual ~NetworkMediaModel() override;
>> 
>>      QVariant data(const QModelIndex& index, int role) const override;
>>      QHash<int, QByteArray> roleNames() const override;
>> @@ -171,7 +174,7 @@ private:
>>      bool m_indexed = false;
>>      bool m_canBeIndexed  = false;
>>      bool m_parsingPending = false;
>> -
>> +    QSemaphore m_preparseSem;
>> 
>>      std::vector<Item> m_items;
>>      QmlMainContext* m_ctx = nullptr;
>> --
>> 2.17.1
>> 
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel