Skip to content

Conversation

@0x5bfa
Copy link
Member

@0x5bfa 0x5bfa commented Mar 8, 2025

Resolved / Related Issues

General improvements for code that uses raw unmanaged pointers.

Steps used to test these changes

  1. Build the Files.
  2. Open a preview pane with a PDF or a DOCX, etc.

CC @hez2010

@0x5bfa
Copy link
Member Author

0x5bfa commented Mar 8, 2025

@hez2010 What and where exactly was the issue that caused the crash in Preview Pane? Looks like this PR has a crash when disposing CEOL again. Perhaps should I dispose these com objects manually?

@0x5bfa 0x5bfa marked this pull request as draft March 8, 2025 14:45
@0x5bfa 0x5bfa closed this Mar 12, 2025
@hez2010
Copy link
Member

hez2010 commented Mar 12, 2025

Looks like this PR has a crash when disposing CEOL again. Perhaps should I dispose these com objects manually?

Some resources shouldn't be released eagerly when the visual content is still visible.

@0x5bfa 0x5bfa reopened this Mar 12, 2025
@0x5bfa
Copy link
Member Author

0x5bfa commented Mar 13, 2025

I disposed an object (pChildVisual) that shouldn't be. Thanks, it looks to be working now.

@0x5bfa 0x5bfa force-pushed the 5bfa/CQ-ShellPreviewComPtr branch from aa719f3 to b0b0a2b Compare March 13, 2025 14:00
@0x5bfa
Copy link
Member Author

0x5bfa commented Mar 13, 2025

@yaira2 Ready. I'm gonna remove Vanara from this area later when i take a break for Omnibar.

@0x5bfa 0x5bfa marked this pull request as ready for review March 13, 2025 14:10
@yaira2 yaira2 added the ready for review Pull requests that are ready for review label Mar 13, 2025
@yaira2 yaira2 requested a review from hez2010 March 13, 2025 14:47
using ComPtr<IDXGIDevice> pDXGIDevice = default;
using ComPtr<IDCompositionDevice> pDCompositionDevice = default;
using ComPtr<IUnknown> pControlSurface = default;
ComPtr<IDCompositionVisual> pChildVisual = default; // Don't dispose this one, it's used by the compositor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that we never release it and would cause memory leaking. I think we should do something around this to free it eventually when user switched or closed the preview pane.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was what it was though. however, we can make this to a class-level field and dispose when disposing CEOL?

@yaira2 yaira2 requested a review from hez2010 March 16, 2025 15:22
Copy link
Member

@hez2010 hez2010 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@yaira2 yaira2 merged commit 79b7621 into files-community:main Mar 16, 2025
6 checks passed
@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed ready for review Pull requests that are ready for review labels Mar 16, 2025
@0x5bfa 0x5bfa deleted the 5bfa/CQ-ShellPreviewComPtr branch March 16, 2025 15:46
@0x5bfa
Copy link
Member Author

0x5bfa commented Mar 16, 2025

@hez2010 I just tried to dispose pChildVisual but it seems to be already disposed by CEOL. What should we do, shoul we leave as was?

image

image

FWIW, swapping the line of disposing CEOL and that of disposing pChildVisual causes the same crash in CEOL.Dispose() instead.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready to merge Pull requests that are approved and ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants