fix: possible crash in UBThumbnailScene

[letsfindaway]

• fix access to still not loaded thumbnails
  • either check for nullptr
  • or load thumbnail before accessing

Fixes letsfindaway#209

[kaamui]

I don't know if related, and I didn't find a reproducible scenario, but I have a segfault error on the UBDocumentThumbnailsView::resizeEvent, on line 435 (ensureVisible(mLastSelectedThumbnail))

Maybe mLastSelectedThumbnail should be stored as a shared_ptr to avoir accessing a thumbnail that could have been deleted ?

It's happening during tests on deletion, duplication of scenes, after moving them, etc.

edit : So it's not related at all to a deleted scene, nor the issue you identified. Not fully reproducible, but I'm able to reproduce it by simply selecting a tuhmbnail of a document A, then a thumbnail of a document B. No need to go in Board Mode or to do some operations.

So the issue is really related to mLastSelectedThumbnail which is probably pointing to a non existing thumbnail.

[letsfindaway]

I don't know if related, and I didn't find a reproducible scenario, but I have a segfault error on the UBDocumentThumbnailsView::resizeEvent, on line 435 (ensureVisible(mLastSelectedThumbnail))

Thanks for reporting. However, I could not find a scenario to reproduce it.

Maybe mLastSelectedThumbnail should be stored as a shared_ptr to avoir accessing a thumbnail that could have been deleted ?

I don't want to do this. The thumbnails are owned by the UBThumbnailScene. So the problem is more that somebody else keeps a reference to it without knowing whether this object still exists. Unfortunately, a UBThumbnail is not a QObject, so also a QPointer cannot help.

It's happening during tests on deletion, duplication of scenes, after moving them, etc.

edit : So it's not related at all to a deleted scene, nor the issue you identified. Not fully reproducible, but I'm able to reproduce it by simply selecting a tuhmbnail of a document A, then a thumbnail of a document B. No need to go in Board Mode or to do some operations.

Do you need to do some resize operation after this to trigger the resizeEvent?

So the issue is really related to mLastSelectedThumbnail which is probably pointing to a non existing thumbnail.

Yes. I will look into this and ask: can we avoid storing a pointer to a UBThumbnail here? Or can we invalidate the mLastSelectedThumbnail when switching documents? Indeed switching documents without opening in Board mode deletes the associated UBDocument and therefore all the UBThumbnails of that. This might cause this problem.

Edit: The easiest way to invalidate this variable would be to set it to nullptr in UBDocumentThumbnailsView::setDocument. I think this should be sufficient to fix this issue. It also makes no sense to keep it after we have switched to another document.

[letsfindaway]

I added another commit to this PR superseding the previous one. I now introduced a new class UBPointer managing a guarded pointer to non-QObject classes. Under the hood it uses the QPointer mechanism to a helper QObject, but it does not require the target object to be a QObject.

This new class is a guarded pointer and not a shared pointer. Thumbnails are still owned and managed by the UBThumbnailScene and ownership is not distributed as std::shared_ptr does. But users of UBPointer can now check whether the pointed-to object still exists.

The advantage over the previous fix is that this does not require explicit invalidation. It now covers all, even unforeseen cases when a thumbnail could be deleted.

Additionally the UBPointer template class may in the future also be used for similar cases, when a pointer to a non-QObject class has to be handed over to some other class without influencing the lifetime of that object.

[kaamui]

So the problem is more that somebody else keeps a reference to it without knowing whether this object still exists. Unfortunately, a UBThumbnail is not a QObject, so also a QPointer cannot help.

You're right for ownership, I should have written smart pointer instead of shared pointer. Also, a simple index of the last selected scene, and calling of thumbnailAt should avoid the issue and be a way simpler fix.

I added another commit to this PR superseding the previous one. I now introduced a new class UBPointer managing a guarded pointer to non-QObject classes. Under the hood it uses the QPointer mechanism to a helper QObject, but it does not require the target object to be a QObject.

It's much simpler to store an index instead isn't it ? If we want to use smart pointers, can't we use weak_ptr for this case (shared_pointer but with no ownership) instead of creating a custom one ? https://en.cppreference.com/w/cpp/memory/weak_ptr

I don't think it is a good idea to introduce this UBPointer class. I'm also not a big fan of the non-natural language it introduces (meaning of lastSelectedThumbnail->target is not obvious), so even if we had to do so, I would like it to be as close as possible of C++ smart pointers.

Please reconsider your last fix to use an index instead, unless you think it cannot solve the current issue.

Do you need to do some resize operation after this to trigger the resizeEvent?

Nope. Just clicking on a doc A with no PDF, click on thumbnail 1, click on document B containing a PDF -> crash (I'm on a Ubuntu machinw with Qt 6.7.2)

Additionally the UBPointer template class may in the future also be used for similar cases, when a pointer to a non-QObject class has to be handed over to some other class without influencing the lifetime of that object.

I'm not against the idea itself. I remember when C++11 was discussed, Loïc Joly wanted it to introduce a smart pointer to encapsulate raw pointers, as he considered it was a mistake to let devs maintaining their direct use of raw/unguarderd pointers as-is.

That said, I don't think it's necessary to fix this issue

[letsfindaway]

So the problem is more that somebody else keeps a reference to it without knowing whether this object still exists. Unfortunately, a UBThumbnail is not a QObject, so also a QPointer cannot help.

You're right for ownership, I should have written smart pointer instead of shared pointer. Also, a simple index of the last selected scene, and calling of thumbnailAt should avoid the issue and be a way simpler fix.

At the situation causing the crash, the UBDocument does no longer exist so we cannot call thumbnailAt().

I added another commit to this PR superseding the previous one. I now introduced a new class UBPointer managing a guarded pointer to non-QObject classes. Under the hood it uses the QPointer mechanism to a helper QObject, but it does not require the target object to be a QObject.

It's much simpler to store an index instead isn't it ? If we want to use smart pointers, can't we use weak_ptr for this case (shared_pointer but with no ownership) instead of creating a custom one ? https://en.cppreference.com/w/cpp/memory/weak_ptr

No. The index changes when a page is added, moved or deleted in between. So we would have to be very careful whether the index is still valid when we use it later. This also does not cover the case when the page indicated by the index is deleted. And a weak_ptr only works together with a shared_ptr. It cannot be used standalone.

I don't think it is a good idea to introduce this UBPointer class. I'm also not a big fan of the non-natural language it introduces (meaning of lastSelectedThumbnail->target is not obvious), so even if we had to do so, I would like it to be as close as possible of C++ smart pointers.

C++ smart pointers do not have a guarded pointer. And even a std::shared_ptr would require to call get() in this case. Ok, I could rename this function.

Please reconsider your last fix to use an index instead, unless you think it cannot solve the current issue.

As I said, Indexes do not solve the issue. They also do not carry the information to which document they belong. So if the document is changed, the index would still be the same.

Do you need to do some resize operation after this to trigger the resizeEvent?

Nope. Just clicking on a doc A with no PDF, click on thumbnail 1, click on document B containing a PDF -> crash (I'm on a Ubuntu machinw with Qt 6.7.2)

Seems that resize is triggered internally by some means.

Additionally the UBPointer template class may in the future also be used for similar cases, when a pointer to a non-QObject class has to be handed over to some other class without influencing the lifetime of that object.

I'm not against the idea itself. I remember when C++11 was discussed, Loïc Joly wanted it to introduce a smart pointer to encapsulate raw pointers, as he considered it was a mistake to let devs maintaining their direct use of raw/unguarderd pointers as-is.

That said, I don't think it's necessary to fix this issue

IMHO the alternative is to find ALL possibilities where the mLastSelectedThumbnail could become invalid. And I assume you will always forget one, either now or later with some change, where you do not think of that requirement.

[kaamui]

At the situation causing the crash, the UBDocument does no longer exist so we cannot call thumbnailAt().

UBDocumentThumbnailsView has a mDocument that is a share_ptr of a UBDocument. If the document has been deleted or is being deleted, we should not pass the first if statement of the resizeEvent, so maybe the issue comes with the fact that mDocument should be set to null before anything else ?

void UBDocumentThumbnailsView::resizeEvent(QResizeEvent *event)
{
    Q_UNUSED(event);

    if (document())
    {
        document()->thumbnailScene()->arrangeThumbnails();

        if (mLastSelectedThumbnail)
        {
            //qDebug() << "mLastSelectedThumbnail" << mLastSelectedThumbnail;
            ensureVisible(mLastSelectedThumbnail);
        }
    }

    emit resized();
}

[letsfindaway]

I now improved the UBPointer so that it has minimal impact to UBDocumentThumbnailsView and UBThumbnail. Only minimal changes are necessary to use this guarded pointer.

I still think some kind of smart pointer is the best idea, if you want to store and use a pointer owned and managed by somebody else. You need a safe way to know whether the object still exists.

[letsfindaway]

At the situation causing the crash, the UBDocument does no longer exist so we cannot call thumbnailAt().

UBDocumentThumbnailsView has a mDocument that is a share_ptr of a UBDocument. If the document has been deleted or is being deleted, we should not pass the first if statement of the resizeEvent, so maybe the issue comes with the fact that mDocument should be set to null before anything else ?

I should have been more precise: the UBDocument is no longer the document to which the last selected thumbnail belongs.

The UBDocumentThumbnailsView is just switching from one document to another, so there is no phase in-between where it has to be nullptr.

I analyzed the crash and here is what happens:

• When we click on the document containing the PDF, the document changes and UBDocumentThumbnailsView::setDocument() is called.
• This causes the previous document's reference count going to zero and it is deleted.
• This also causes all thumbnails of that document to be deleted
• The mLastSelectedThumbnail however still points to this deleted object.
• Then selecting the first page of the new document causes resizeEvent() to be called, which is using the pointer to the deleted thumbnail.

With the guarded pointer, the mLastSelectedThumbnail is already invalidated at the second stage. And it is totally correct that document() returns a non-null result, as we already have switched to the new document when the crash occurs.

[letsfindaway]

What I do not want to use to solve the problem:

• Explicit invalidation on all possible events causing this, because you always forget one. And yes, I'm quite sure that my previous approach to set mLastSelectedThumbnail to nullptr when switching documents missed some other situations.
• A std::shared_ptr, as it introduces shared ownership and might keep a thumbnail alive even when the thumbnail scene and the document are already deleted.
• An index, as it might change when copying/adding/removing/moving pages and it does not indicate to which document it belongs.

[kaamui]

if you want to store and use a pointer owned and managed by somebody else.

But do we actually want that ? I think it's better that the view asks the scene for what thumbnail is the last one selected. So we architecturally avoid situations like that.

When you look at mLastSelectedThumbnail use, it's for three things :

• ensure mLastSelectedThumbnail is visible (so we only need to get access to the last selected thumbnail here, no need to store it or an access to it)
• get mLastSelectedThumbnail thumbnail's scene index
• set mLastSelectedThumbnail value (and we call ThumbnailScene::thumbnailAt for that)

I don't see any good reason to store a pointer to the thumbnail in the view. What I think should be done (not tested, just to give an idea of what I try to say) :

ensureVisible calls become

    if (document() && document()->thumbnailScene())
    {
        ensureVisible(document()->thumbnailScene()->lastSelectedThunbnail();
    }

for view's mouse and key events handling selection :

UBThumbnail* sceneItem = dynamic_cast<UBThumbnail*>(itemAt(mMousePressPos));

//...
if (!sceneItem->isSelected())
{
        selectItemAt(sceneItem->sceneIndex(), Qt::ControlModifier & event->modifiers());
}

void UBDocumentThumbnailsView::selectItemAt(int pIndex, bool extend)
{
    document()->thumbnailScene()->selectItemAt(pIndex, extend); //rename highlightItem 
    ensureVisible(thumbnailScene->thumbnailAt(pIndex));
}

and store mLastSelectedThumbnailSceneIndex in UBThumbnailScene

int mLastSelectedThumbnailSceneIndex{0}

/*public*/ lastSelectedThunbnail()
{ //we can add safety checks of course
   return thumbnailAt(mLastSelectedThumbnailSceneIndex);
}

/*private*/ setLastSelectedThumnail(index)
{
    mLastSelectedThumbnailSceneIndex = index;
}

void UBThumbnailScene::hightlightItem(int index, bool only)
{
    if (only)
    {
        for (auto item : selectedItems())
        {
            auto thumbnail = dynamic_cast<UBThumbnail*>(item);

            if (thumbnail && thumbnail->sceneIndex() != index)
            {
                thumbnail->setSelected(false);
            }
        }
    }

    if (index >= 0 && index < mThumbnailItems.size())
    {
        thumbnailAt(index)->setSelected(true);
        setLastSelectedThumnail(index);
    }
}

By giving the responsibility to the thumbnailScene to handle that, I don't think we can end in the selection of a wrong index.

Of course what I wrote is probably not perfect, it's to illustrate my point.

What I do not want to use to solve the problem:

• Explicit invalidation on all possible events causing this, because you always forget one. And yes, I'm quite sure that my previous approach to set mLastSelectedThumbnail to nullptr when switching documents missed some other situations.
• A std::shared_ptr, as it introduces shared ownership and might keep a thumbnail alive even when the thumbnail scene and the document are already deleted.

I agree

• An index, as it might change when copying/adding/removing/moving pages and it does not indicate to which document it belongs.

I agree if we suppose the index is handled by the view, disagree as we can give the thumbnail scene the responsibility to handle that information

[letsfindaway]

But do we actually want that ? I think it's better that the view asks the scene for what thumbnail is the last one selected. So we architecturally avoid situations like that.

Very good point!!! Yes, I agree. Let me see whether the scene always has the information which thumbnail was selected last, and if not, it should get that information.

I will create another approach in this direction (and save my guarded pointer for some later usage, if required ;).

[letsfindaway]

Here the approach where management of the last selected thumbnail is moved to the UBThumbnailScene. Indeed, this is a much cleaner architecture.

In contrast to your sketch above I'm still storing the pointer to the thumbnail instead of the index as this moves automatically when thumbnails are added/removed/moved. I only have to invalidate this pointer when exactly that thumbnail is deleted.

[kaamui]

missing "h" ;)