Page MenuHome

Fix reference count for COM handling
AbandonedPublic

Authored by Robert Guetzkow (rjg) on Apr 15 2022, 1:27 AM.

Details

Summary

This patch fixes T97338. The reference count for IShellItem *pSI is reduced, preventing the memory leak. For IFileOperation *pfo the decrement of the reference count is only attempted when the pointer is not NULL.

Diff Detail

Repository
rB Blender
Branch
2022-04-18-com-refcount-v2 (branched from master)
Build Status
Buildable 21673
Build 21673: arc lint + arc unit

Event Timeline

Robert Guetzkow (rjg) requested review of this revision.Apr 15 2022, 1:27 AM
Robert Guetzkow (rjg) created this revision.

Short term this will fix the issue, long term it'll probably be better to move the file to cpp and have something like (untested, for illustration purposes only, also @Julian Eisel (Severin) will likely have improvements on it)

#define CHECK_HR_DELETE_SOFT(hr, error_text) \
  if (FAILED(hr)) { \
    *error_message = error_text; \
    throw std::exception(); \
  }


static bool delete_soft(const wchar_t *path_16, const char **error_message)
{
  /* Deletes file or directory to recycling bin. The latter moves all contained files and
   * directories recursively to the recycling bin as well. */

  HRESULT hr = CoInitializeEx(NULL, COINIT_APARTMENTTHREADED | COINIT_DISABLE_OLE1DDE);

  if (FAILED(hr)) {
    *error_message = "Failed to initialize COM";
    return FAILED(hr);
  }

  try {
    Microsoft::WRL::ComPtr<IShellItem> pSI;
    Microsoft::WRL::ComPtr<IFileOperation> fileOperation;

    hr = CoCreateInstance(CLSID_FileOperation, nullptr, CLSCTX_ALL, IID_PPV_ARGS(&fileOperation));
    CHECK_HR_DELETE_SOFT(hr, "Failed to create FileOperation instance");

    /* Flags for deletion:
     * FOF_ALLOWUNDO: Enables moving file to recycling bin.
     * FOF_SILENT: Don't show progress dialog box.
     * FOF_WANTNUKEWARNING: Show dialog box if file can't be moved to recycling bin. */
    hr = fileOperation->SetOperationFlags(FOF_ALLOWUNDO | FOF_SILENT | FOF_WANTNUKEWARNING);
    CHECK_HR_DELETE_SOFT(hr, "Failed to set operation flags");

    hr = SHCreateItemFromParsingName(path_16, NULL, IID_IShellItem, (void **)&pSI);
    CHECK_HR_DELETE_SOFT(hr, "Failed to parse path");

    hr = fileOperation->DeleteItem(pSI.Get(), NULL);
    CHECK_HR_DELETE_SOFT(hr, "Failed to prepare delete operation");

    hr = fileOperation->PerformOperations();
    CHECK_HR_DELETE_SOFT(hr, "Failed to delete file or directory");
  }
  catch (std::exception) {
  }

  CoUninitialize();
  return FAILED(hr);
}

cause i'll be honest, those goto's... oof.. :)

I agree, moving this to C++ would be a lot nicer. If we keep this in C for now, I could replace the gotos with nested ifs?

I think nested ifs or C++ would be better than this, either of those is fine with me.

I will update the patch on Monday (or earlier) using plain C with nested ifs. We can then port it to C++ at a later point in time.

Robert Guetzkow (rjg) updated this revision to Diff 50544.EditedApr 18 2022, 8:53 PM

The gotos from the original implementation were removed. In order to avoid 6 levels of nested if/else, I've decided to use booleans to indicate which resources require cleanup.

Edit: The error handling can be simlified, I'll update the patch again.

  • Remove redundant error variable

I also implemented a version with nested ifs, which can be found in D14681. Just in case you prefer one version over the other.

source/blender/blenlib/intern/fileops.c
313

can't you just return here and skip the com_initialized variable all together?

source/blender/blenlib/intern/fileops.c
313

The else-case is for when the COM initialization is successful and only then should COM be uninitialized later on.

source/blender/blenlib/intern/fileops.c
313

I may have misunderstood your comment. Did you mean you want an early return in the if-case?

source/blender/blenlib/intern/fileops.c
313

yes, i mean, if the com init call failed, unlikely any of the other stuff is going to execute? so you could bail out early in that case i think?

source/blender/blenlib/intern/fileops.c
313

Yes, you are right. In this particular case we could do a return in the if case and eliminate the bool. Technically you can do an early return in every step, as each one depends on the successful execution of the previous step, but then you have to duplicate code for the release and uninitialization in later parts.

I accepted the other patch, I think it's a bit more clear, but either is fine with me really.

The other variation of the patch in D14681 has been committed.