Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add extra shell file operation APIs #630

Merged
merged 3 commits into from Dec 30, 2022
Merged

Add extra shell file operation APIs #630

merged 3 commits into from Dec 30, 2022

Conversation

timsneath
Copy link
Contributor

@timsneath timsneath commented Dec 30, 2022

Also adds example\recycle_bin.dart, which demonstrates its usage.

While running tests (prior to adding this new code), I got a random test abort, which can be an indication of a use-after-free failure. I've therefore changed generate.cmd to pass extra flags to a) run tests single-threaded, and b) randomize test ordering, in the hope that this will occur again. If so, the last test that run should deterministically be the failing test.

Closes #629.

Copy link
Member

@halildurmus halildurmus left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@halildurmus
Copy link
Member

halildurmus commented Dec 30, 2022

While running tests (prior to adding this new code), I got a random test abort, which can be an indication of a use-after-free failure. I've therefore changed generate.cmd to pass extra flags to a) run tests single-threaded, and b) randomize test ordering, in the hope that this will occur again. If so, the last test that run should deterministically be the failing test.

Might be related to #463. I've been trying to reproduce this, with no luck so far :(.

@timsneath timsneath merged commit 91c562e into main Dec 30, 2022
@timsneath timsneath deleted the shell-apis branch December 30, 2022 17:18
@timsneath
Copy link
Contributor Author

Thanks! Although, I'm not certain this is the same thing, btw, @halildurmus. This was a segfault, where Dart just quits without warning, rather than a process exit as in #463. The last displayed test attempt was IReference<Int64>. Until the changes I made here, Dart tests ran with the default concurrency settings (two threads), so there's a 1 in 2 chance that it was this specific test that failed. But both tests running would have been from that test file, I think.

It might be worth setting a larger number of test runs on this test suite and seeing if we can flush this out.

@halildurmus
Copy link
Member

halildurmus commented Jan 4, 2023

This was a segfault, where Dart just quits without warning, rather than a process exit as in #463. The last displayed test attempt was IReference<Int64>. Until the changes I made here, Dart tests ran with the default concurrency settings (two threads), so there's a 1 in 2 chance that it was this specific test that failed. But both tests running would have been from that test file, I think.

I came across this today. You should be able to reproduce this with the following cmd script:

@echo off

rem Run the dart test command 30 times
for /l %%i in (1,1,30) DO (
  echo Run: %%i
  call dart test test/winrt_calendar_test.dart test/winrt_collections_test.dart test/winrt_ireference_test.dart
)

(You may need to run this script several times 😄)

While running this script, you should see either a segfault or an error like this in some random test:

  Error 0x80080008: Object server is stopping when OLE service contacts it
  package:win32/src/winrt_helpers.dart 140:21                ActivateClass
  package:win32/src/winrt/globalization/calendar.dart 37:15  new Calendar
  test\winrt_calendar_test.dart 18:18                        main.<fn>

I think this is caused by calls to the winrtUninitialize() helper function which calls RoUninitialize(). I can't reproduce this if I comment out the call to RoUninitialize() in the winrtUninitialize().

I have looked at the other WinRT projections to see if they call RoUninitialize() and I can confirm that they don't. In fact, they don't call RoInitialize() either. They support automatic initialization of the Windows Runtime by using CoIncrementMTAUsage API.

Here is the relevant snippet from cppwinrt:
https://github.com/microsoft/cppwinrt/blob/fe304096fa30583f3cc2ebfdbf564d2b4081e2ad/strings/base_activation.h#L37-L52

And this is from windows-rs:
https://github.com/microsoft/windows-rs/blob/a8b3b3841ae5f966dd5f86397a0a81eaa26470b6/crates/libs/windows/src/core/factory_cache.rs#L56-L73

Basically, if the call to RoGetActivationFactory fails with the error code CO_E_NOTINITIALIZED, they initialize a Multi-Threaded Apartment (MTA) on the current thread by calling CoIncrementMTAUsage() first and then try again.

I think we should do the same, Created #642 for this. Curious to hear your thoughts.

Note that I forgot to mention in #623, before we make NativeFinalizer call IUnknown's release() automatically on objects we would need to stop calling winrtUninitialize() because, by the time NativeFinalizer callbacks run, the WinRT would be uninitialized which would crash the app (I have experienced this multiple times while working on #623). So, even if we didn't have this problem, we would have to stop using winrtUninitialize at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Shell32's SHFileOperation() function
2 participants