qt: use ComPtr with the rest of DirectComposition objects

changed milestone to %4.0

added Component::Interface: Qt label

added 1 commit

9d7e55cb - qt: don't keep a reference to the IDCompositionTarget

Compare with previous version

added MRStatus::NotCompliant label

added 1 commit

88f47266 - qt: don't keep a reference to the IDCompositionTarget

Compare with previous version

added MRStatus::Reviewable label and removed MRStatus::NotCompliant label

The owner of these objects is Qt itself, and it also does not use ComPtr. I'm not convinced if this would really be beneficial.

It's safer to keep them around for as a long as we need them.

That is a given, without the utility of incrementing the reference counter. The time we need them is limited by the validity of window surface and Qt RHI backend. Utilizing these members when scene graph is gone can be considered an undefined behavior, and as far as I know we don't do that anyway.

If the lifetime of these objects are prolonged, we then have other problems. For example, the content of dcompVisual is set to the swapchain which is managed by Qt. The visual becomes zombie, because the swapchain (it's content) would have been gone.

The current order of destruction by Qt appears to be:

The swap chain.
DComp Visual.
DComp Target.
DComp device.

There can be more releases in between that I have not noticed but these objects depend on.

COM objects can have multiple "owners" (or none if you prefer). They are reference counted.

And it's safer because we might reference each object even after Qt is done with them. Right now if we do, it would crash. With these changes we are free to call them, even though the result will be useless because Qt is not using them anymore.

Usually COM objects also don't need an order of release. Child elements keep a reference to their parent so if you release the parent first, the child still has a reference to keep it alive.

Right now if we do, it would crash

We should not do that, and becoming a shared owner of these objects would only mask a bad operation if in the future they are used at inappropriate times.

If it crashes, at least that would indicate that they are already gone.

In my opinion, we should use weak reference here so that it would not be a security concern.

I'm saying it should not crash and you prefer it to crash. Maybe it's fine during development but certainly not when a lot of users will use VLC 4.

Ideally, we should assert the existence whenever these objects are used. We already use assertion, and the default behavior of assertion failure is usually crash, is not it?

The main point of assertion is not to cause inconvenience to users, but rather aid debugging sessions to fix the bad code. Again, being a shared owner would mask bad code and let these objects occupy memory for more time for no real reason.

This MR doesn't affect the use of the assert. If you're using an object when it's not there it's still wrong and it should will crash.

This MR helps not dealing when we don't need the resource anymore.

There is a potential issue if an interface needs a specific call to destroy its resources and we would keep using the interface after Qt has done that call. But this is not the case for those interfaces:

The code in Qt:

    if (dcompDevice) {
        dcompDevice->Release();
        dcompDevice = nullptr;
    }

and

    if (dcompVisual) {
        dcompVisual->Release();
        dcompVisual = nullptr;
    }

    if (dcompTarget) {
        dcompTarget->Release();
        dcompTarget = nullptr;
    }

Inversely, this line is potentially crashing if Qt has released m_uiVisual before us.

Same thing with this line, if for some reason the windows disabling happens after the Qt gui is destroyed (or at least some internals).

All these uncertainties can be avoided.

This MR doesn't affect the use of the assert. If you're using an object when it's not there it's still wrong and it should will crash.

Yes, but I already tried to assert everywhere that the pointers are not null. And, that's my point, we should not be using an object that is not there. The solution of this is not to extend the lifetime of objects, but rather not use in inappropriate times.

I see your point, but I'm more focused on the security aspect of it. Dereferencing a dangling pointer is a security risk. That's why I proposed to use weak reference here, there are already pointer null-ness assertions everywhere.

If a weak reference is used, there would be assertion failure (which is okay). In debug sessions, with this information the developer can correct object lifecycle management, if there are issues right now at all, which we don't know.

While your suggestion, using a strong reference, would extend the lifetime of objects and mask object lifecycle issues.

this line is potentially crashing if Qt has released m_uiVisual before us.

I don't think Qt is allowed to do that, because they guarantee that when SurfaceAboutToBeDestroyed is signalled the swap chain (hence the DComp objects, since the swap chain is a dependency) is still valid. In fact, the swap chain is released in the handler of this event, however since an event filter is installed here we receive the event first.

if for some reason the windows disabling happens after the Qt gui is destroyed (or at least some internals).

In my opinion, the correct way of that is to not allow disabling the window when it can not be disabled. Window should be closed, not disabled when the interface is closing.

Yes, but I already tried to assert everywhere that the pointers are not null.

This is not what this is fixing. The values remain null until they are set. However Qt may release them behind your back (especially since it's a dirty hack, using things that should be private). In that case your pointers may not be null, but if you use them, you have a "use after free" issue.

And, that's my point, we should not be using an object that is not there.

When we have a reference, we can use them. Only it will not be displayed.

but rather not use in inappropriate times.

Then it should have stayed private to Qt. In other words, the hack should move a lot of the code we need in Qt and only expose the tiny bits we need, without knowing how it's done behind. But until we have that, we should make sure we use these objects safely.

If a weak reference is used, there would be assertion failure

I don't see how you could have a weak reference here. When Qt releases something, our code will never know.

mask object lifecycle issues.

That's the point of ComPtr. Having memory safety and not caring about the internals.

In that case your pointers may not be null, but if you use them, you have a "use after free" issue.

Yes, that's what I referred "Dereferencing a dangling pointer is a security risk". It is possible to know the existence with a reference.

We both think that we need a reference, but it appears that you want to have a strong reference (being an owner), while I argue that it should be a weak reference (watcher).

But until we have that, we should make sure we use these objects safely.

I agree, but my point is to know if they exist and don't use if not. Again, Qt should be the (sole) owner of these objects, not us.

Actually, with the changes here don't we become the sole owner of the object (I initially thought mutual ownership, but Qt's side don't use ComPtr)? What happens if Qt wants to access these after compositor_dcomp is gone? Don't you have the same problem, just the sides switched?

I don't see how you could have a weak reference here. When Qt releases something, our code will never know.

Maybe we should patch Qt to use ComPtr on their side, and then have a weak ref here. Maybe with ComPtr::AsWeak?

That's the point of ComPtr

ComPtr is noted to be one type of smart pointer, there are different types of smart pointers. I mean std::weak_ptr in this case.

while I argue that it should be a weak reference (watcher).

I don't see how we can have a weak pointer on a pointer that is managed outside our code.

Again, Qt should be the (sole) owner of these objects, not us.

No, that's not how COM objects work: !5712 (comment 445508) "COM objects can have multiple "owners" (or none if you prefer). They are reference counted."

What happens if Qt wants to access these after compositor_dcomp is gone? Don't you have the same problem, just the sides switched?

Without my change it would crash, with my changes it doesn't. We have more stable code as a result.

ComPtr is noted to be one type of smart pointer, there are different types of smart pointers. I mean std::weak_ptr in this case.

Except the underlying pointer is reference counted (see IUnknown).

Maybe with ComPtr::AsWeak

I'm not familiar with it. But that still seems like a broken concept if the pointer is managed outside of our code.

I'm not really familiar with COM. I tried to do some research, and it suggests that there is no such weak reference concept in COM [1]. In fact, there seems to be IWeakReference but it is said to be functional under WinRT.

Also, it appears that IUnknown itself is reference counted (as also you said), but also the counter is incremented through IUnknown::AddRef() when IUnknown::QueryInterface() is called. So my assumption "Don't you have the same problem, just the sides switched?" is actually false, and we should be safe in that regard.

[1] https://stackoverflow.com/questions/6120854/how-can-i-maintain-a-weak-reference-on-a-com-object-in-c

That being said, I maintain my argument.

Without my change it would crash, with my changes it doesn't. We have more stable code as a result.

That's what we have different opinions. I would like to keep the (safe, through assertion) crash here in order to not let bad code creep in in the future.

Program that does not crash does not necessarily mean that it is a well-coded program.

Nevertheless, in hindsight, we don't actually need to use a weak reference at all. We can directly use C++ references since Qt sets the member pointers to null after it releases them. I will try to create a merge request and notify you.

safe, through assertion

That concept doesn't exist. Assertions are only enabled in debug builds. In release builds you will just crash wherever you're using the pointer wrong, maybe inside the some code that is not ours.

Asserts are good to avoid traps during developments. It doesn't make the release code catch errors better. And my point is that I don't want the UI to crash randomly.

I did say we should remove the asserts (although calling null->whatever() will do the same as an assert).

My point is that crashes that are induced by assertion failure are safe. It is an indication to the developers that there is something going wrong in the code.

In release build, that would be null pointer dereference which is an undefined behavior. While it is not safe, I have always seen it treated as an exception. So, that is not safe, but still safer than dereferencing a dangling pointer. Still, it is developers' responsibility to fix the code so that NPD does not occur.

In my opinion, incrementing the reference counter and holding zombie objects for the sake of preventing crashes is not something we should be doing here.

If you insist going this way (I don't have blocking privilege), I think a third opinion would be nice to have here.

added MRStatus::InReview label and removed MRStatus::Reviewable label

mentioned in merge request !5733 (merged)

mentioned in merge request !5734 (merged)

Closing this as this has been going in circle for too long. In the end I'm not the one who will fix crashes in the UI when VLC 4 is released.

I think this merge request would be fine if you hold the device only, not the UI visual or the target.

I think this merge request would be fine if you hold the device only

Maybe this is also not so safe, what happens to the DXGI Device or D2D1 Device used for creating the DComp device? Does DComp device become an owner?

closed

mentioned in merge request !6205 (merged)

qt: use ComPtr with the rest of DirectComposition objects

Merge request reports

Activity