Fix context menu crash, improve DLL packaging, and upgrade Boost to 1.86 #33
Conversation
and now also add some other atomic protect
|
I admit that I implemented a rather foolish design at the log level. I will provide a final update in the near future, and I envision that it will no longer use the concept of threshold, but instead use optional masks. On the UI settings side, it appears that you can use checkboxes to select the desired combination of logs. I also added the output of stack information, although this feature has the same implementation in the debugger. Alternatively, simply discard these confusing excessive designs: No Level! no Verbosity! P.S : 最初设计这部分日志，是出于后面在进一步提出集成到 Shell，使得 RoboCopyEx 在资源管理器处理任何文件的移动/剪切，复制，和剪贴板的捕获方面获得优先权，这样就不必每次都打开右键菜单选择二级菜单，然后使用旁路的Copy/Paste 进行操作了。而上述设计是相当复杂的，预计可能会消耗我更多的业余时间。也需要较长的周期去调试，如果组件能够在源代码级别就使用一整套适合开发人员调试和观察的处理，那么将获得比每次附加 VS 的内部调试器获得更多的有趣信息：例如你可以不中断程序观察数据流和控制流的关键上下文信息，这可以发现超出你设计时预想的可能执行路径。为了避免侵入性，我采取了分级别、DEBUG宏和调试器探测模式相互结合的形式，并且允许一部分的类实例在所运行的线程上执行临时输出提升（让线程变得唠叨或停滞），这将在作用域过后恢复线程原来的管理级别（即默认情况下的全局级别[新的设计中我将改为使用选项的组合]，每个线程的临时级别可以覆盖全局级别）。 |
|
OK, this is a big one. Let's get it done, step by step.
<?xml version="1.0" encoding="utf-8"?>
<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<!--
Shared MSBuild task to copy missing DLLs from a source directory to a target directory.
Import this file in your .vcxproj to use the CopyMissingDllsTask.
Usage:
<CopyMissingDllsTask SourceDir="$(TargetDir)" TargetDir="$(TargetDir)AppX\" />
-->
<UsingTask TaskName="CopyMissingDllsTask" TaskFactory="CodeTaskFactory" AssemblyFile="$(MSBuildToolsPath)\Microsoft.Build.Tasks.Core.dll">
<ParameterGroup>
<SourceDir ParameterType="System.String" Required="true" />
<TargetDir ParameterType="System.String" Required="true" />
</ParameterGroup>
<Task>
<Using Namespace="System" />
<Using Namespace="System.IO" />
<Code Type="Fragment" Language="cs"><![CDATA[
if (!Directory.Exists(SourceDir))
{
Log.LogError("[CopyMissingDlls] SourceDir does not exist: {0}", SourceDir);
return false;
}
Log.LogMessage(MessageImportance.Normal, "[CopyMissingDlls] SourceDir: {0}", SourceDir);
Log.LogMessage(MessageImportance.Normal, "[CopyMissingDlls] TargetDir: {0}", TargetDir);
try
{
Directory.CreateDirectory(TargetDir);
}
catch (Exception ex)
{
Log.LogError("[CopyMissingDlls] Failed to create TargetDir: {0} (error: {1})", TargetDir, ex.Message);
return false;
}
int copiedCount = 0;
bool anyError = false;
foreach (var srcPath in Directory.GetFiles(SourceDir, "*.dll"))
{
string filename = Path.GetFileName(srcPath);
string dstPath = Path.Combine(TargetDir, filename);
if (File.Exists(dstPath))
continue;
try
{
File.Copy(srcPath, dstPath);
Log.LogMessage(MessageImportance.Normal, "[CopyMissingDlls] Copied: {0}", filename);
copiedCount++;
}
catch (Exception ex)
{
Log.LogError("[CopyMissingDlls] Failed to copy \"{0}\": {1}", filename, ex.Message);
anyError = true;
}
}
if (copiedCount == 0)
{
Log.LogMessage(MessageImportance.Normal, "[CopyMissingDlls] No missing DLLs to copy.");
}
else
{
Log.LogMessage(MessageImportance.Normal, "[CopyMissingDlls] Copied {0} DLL(s).", copiedCount);
}
return !anyError;
]]></Code>
</Task>
</UsingTask>
</Project>This is simple enough, copy all missing dlls from <Import Project="$(SolutionDir)Shared\CopyMissingDlls.targets" />
<!-- After build, copy missing DLLs from $(TargetDir) to the AppX directory -->
<Target Name="CopyRuntimeFilesToAppx" AfterTargets="Build" Condition="'$(Platform)'=='x64'">
<CopyMissingDllsTask SourceDir="$(TargetDir)" TargetDir="$(TargetDir)AppX\" />
</Target> |
|
i will check it later. @HO-COOH |
| <x:String x:Key="ThemeGlyph"></x:String> | ||
| <x:String x:Key="WindowBackgroundGlyph"></x:String> | ||
| <x:String x:Key="DevModeGlyph"></x:String> | ||
| <ScrollViewer |
There was a problem hiding this comment.
Don't. The settings window should ideally be simple enough. If you need scroling then you are complicating things. Just calculate the needed window size, and make it fixed.
| m_fs = _wfopen(filename.data(), L"wb"); | ||
| if (!m_fs) | ||
| throw std::runtime_error{ "Cannot open file" }; | ||
| auto& s = FastCopy::Settings::CommonSharedSettings::Instance(); |
There was a problem hiding this comment.
Rename s to be something meaningful
| }) != std::filesystem::directory_iterator{}; | ||
| try | ||
| { | ||
| auto& s = FastCopy::Settings::CommonSharedSettings::Instance(); |
| if (!folderOpt) { | ||
| FC_LOG_ERROR(L"LocalDataFolder unavailable, skip recording."); | ||
| return false; | ||
| } | ||
|
|
||
| auto const& folder = *folderOpt; | ||
|
|
||
| if (folder.empty()) | ||
| { | ||
| FC_LOG_ERROR(L"LocalDataFolder empty, skip recording."); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Unnecessary check. You are already in a try-catch block, just access the optional. It should throw if its value not set.
| for (auto it = std::filesystem::directory_iterator{ folder, ec }; | ||
| it != std::filesystem::directory_iterator{} && !ec; | ||
| ++it) | ||
| { | ||
| auto name = it->path().filename().wstring(); | ||
| if (!name.empty() && | ||
| (name.starts_with(L'C') || name.starts_with(L"M2") || name.starts_with(L'P'))) | ||
| { | ||
| FC_LOG_DEBUG(L" found '{}'", name); | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
What's bad about my original std::find_if 😂
| static bool HasRecord(); | ||
| private: | ||
| FILE* m_fs; | ||
| FILE* m_fs = nullptr; |
There was a problem hiding this comment.
Not necessary. wfopen returns NULL if not opened, but okay
| }; | ||
|
|
||
| // Get the current settings.ini path | ||
| inline std::filesystem::path GetFastCopyIniPath() |
There was a problem hiding this comment.
This function is not called anywhere. Is it written by AI? 😅
| } | ||
|
|
||
| // Returns true if a debugger is currently attached. | ||
| [[nodiscard]] inline bool is_debugger_attached() noexcept |
|
Please, explain the necessity of |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| { | ||
| DetourTransactionAbort(); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Bug: Invalid DetourTransactionAbort after DetourTransactionCommit returns
When DetourTransactionCommit() fails and returns an error, the code calls DetourTransactionAbort() in the else branch. However, once DetourTransactionCommit() has been called, the transaction is already terminated (it either commits successfully or aborts internally on failure). Calling DetourTransactionAbort() after DetourTransactionCommit() has returned is undefined behavior since there is no active transaction to abort. The DetourTransactionAbort() call on line 68 is incorrect and could cause unexpected behavior.
| strong.get()->OnSharedSettingsChangedNotifyUI(); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Bug: Use-after-free race in settings change callback
The OnSharedSettingsChanged callback receives a raw this pointer as ctx and accesses self->m_dispatcher and self->get_weak() without synchronization. The CommonSharedSettings::NotifyConfigChanged() copies the listener list under a lock, then releases the lock and iterates over the copy to call callbacks. If the destructor runs concurrently (calling UnregisterChangeListener() and destroying the object), the callback can access a freed object because unregistering only removes from the live list, not the already-copied list being iterated. This results in a use-after-free race condition.
Additional Locations (1)
| return false; | ||
| } | ||
|
|
||
| std::error_code ec; |
There was a problem hiding this comment.
Not necessary either. Just let it throw
| if (!cb) | ||
| return; | ||
|
|
||
| std::lock_guard lock(m_listenersMutex); |
| SetLastError(0); | ||
|
|
||
| DWORD len = GetPrivateProfileStringW( | ||
| std::wstring(section).c_str(), |
2 participants
Pull Request
Description
This pull request makes several improvements and fixes across the project:
DLL dependency handling and post-build configuration
Fix for context menu crash due to incorrect user data directory
Boost upgrade to 1.86 and process/thread handling
Debug output system for shell extension
Reliable DLL unload and host process termination handling (dllhost.exe)
DllCanUnloadNowwould always be called before the module is unloaded and that the following “ideal” sequence would occur:CoUninitializeRtlDllShutdownInProgressDllCanUnloadNow→ COM refcounts reach 0 →FreeLibraryDllMain(DLL_PROCESS_DETACH)dllhost.exemay callTerminateProcessafterCoUninitializewithout:CoFreeUnusedLibraries/DllCanUnloadNow, orDllMain(DLL_PROCESS_DETACH)for the shell extension.TerminateProcessis used.CoUninitializeandTerminateProcessviaHostProcessHook.DllMain(DLL_PROCESS_ATTACH)/DllHost!wWinMainthread) viaHostShutdownWatcher.CoUninitializecalls on the recorded owner thread as the “host COM shutdown” signal, avoiding interference from other modules callingCoUninitializeon background threads.CoUninitializehas been observed on the owner thread and no subsequentDllCanUnloadNowhas been called from our module, a dedicated cleanup callback (FastCopyCleanupCallback) is invoked from theTerminateProcessdetour.FastCopyCleanupCallbackis responsible for shutting down internal resources that must be cleaned up even on the hardTerminateProcesspath, such as:CommonSharedSettings.DLL_PROCESS_DETACHhave been updated to:dllhost.exe.FreeLibraryis handled byDllMain(DLL_PROCESS_DETACH)and object lifetimes, while theTerminateProcesspath is handled explicitly by the hooks and the cleanup callback.dllhost.exeis included in the documentation section below.Motivation and context:
dllhost.exe, even when the host terminates the process viaTerminateProcesswithout callingDllMain(DLL_PROCESS_DETACH).Fixes # (issue number)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.
dllhost.exe:CoUninitialize,DllCanUnloadNow,DllMain, andTerminateProcessvia debugger breakpoints.FastCopyCleanupCallbackis invoked on the intendedTerminateProcesspath and that background threads/handles (e.g.CommonSharedSettingsmonitor) are properly shut down.Example (adjust as needed):
Dependencies
Mention any dependencies this PR relies on or blocks.
Depends on # (PR)
Blocks # (PR)
Screenshots/Documentation
Include any relevant screenshots or gifs that demonstrate the UI changes. Include links to updated documentation if applicable.
Checklist for Reviewers
Request for Specific Feedback
Optional: Mention any particular aspects of the PR where you would like to receive specific feedback.
HostShutdownWatcher/HostProcessHookapproach is clear and maintainable.Additional Information
Anything else you want to add (links to documentation, style guides, etc.)
dllhost.exe(including theTerminateProcesspath).Note
Add rule-based post-build DLL packaging, shared settings + logging controls, and reliable shell extension shutdown; update UI and boost runtime to 1.86.
CopyFilesByRulestool andTools/AppxDllRules.txt; wire intoFastCopy.vcxprojto copy runtime DLLs to$(TargetDir)AppXpost-build.$(TargetDir)entries; addTools/CopyFilesSimple.batand ShellExtension post-build to copy its DLL.boost_filesystem-vc143-...-1_86.dll.Shared/CommonSharedSettings(INI-backed, change notifications),DebugHelper.hpp, andvar_init_once.h; include across projects.DebugPrint.*) with verbosity/breakpoint controls sourced from shared settings.HostProcessHook(Detours) andHostShutdownWatcherto handleCoUninitialize/TerminateProcessand ensure cleanup viaFastCopyCleanupCallback.dllmain,DllCanUnloadNow, and commands to log and use new Recorder; makeRecorderpathing robust via shared settings.SettingsViewModelto INotifyPropertyChanged; add logger settings (enable, verbosity, debug-break) synced viaCommonSharedSettings.SettingsWindow.xamlwith Developer Options expander; add resources/strings; mergeSettingsExpander_Resource.CreateSuspendhandler; obtain first thread handle for APC injection; minor helper and NTDLL tweaks.CopyFilesByRulesproject andSharedsolution items; adjust project include paths/toolsets and .gitignore.Written by Cursor Bugbot for commit 988d5e4. This will update automatically on new commits. Configure here.