Closed pjaholkowski closed 1 year ago
@pjaholkowski
memory leak in: IFACEMETHODIMP EditWithNppExplorerCommandHandler::Invoke(IShellItemArray psiItemArray, IBindCtx pbc) noexcept try memory for itemName is not released psi is not released
Could you do a PR for the memory leak please?
When regestering dll with regsvr32 I have assert at HRESULT NppShell::Installer::RegisterSparsePackage() line 272 auto deployResult = deploymentOperation.get(); Expression: !is_sta_thread()
... I generated my own certificate, installed it in system as trusted, I changed Publisher field in Identity tag of AppxManifest.dll to my generated certificate, built dll and msix packgae, signed both dll and msix package then from command line: regsvr32 NppShell.dll
I guess you have modified the source code of NppShell - does it work for you? If yes, could you do another PR for fixing the register problem?
Ok I managed to make it work but I don't know how. I suspected that it is fault of recent Windows update so I downloaded installer from notepad++ site. I installed Notepad++ and then clicked with rmb on file and there was no "Edit with notepad++" in context menu. So I did install WinMerge WinMerge installs context menu from powershell (command Add-AppxPackage) and they did something to refresh view in context menu because after installing WinMerge I can see all verbs that I registered.
@pjaholkowski the init_apartment is no longer needed. (Is what Microsoft explains in their WinRT 2.0 video - it is also missing from the WinRT projects you create in Visual Studio, so it is not needed anymore).
Regarding the name of the DLL, it is named like this, since the installer has multiple versions of it, and names it correctly when it is copied into the contextMenu directory.
Regarding the memory leak, the IShellItem* psi
shouldn't be a problem from what I understand, since it isn't owned by us (we just get a pointer to the object with GetItemAt - so it should be cleaned up by the system (from how I understand it, please correct me if I'm wrong)).
The itemName needs to be cleaned up yes, I'll take a look at that.
@pjaholkowski should this fix the leak?
if (!CreateProcessW(application, command, nullptr, nullptr, false, CREATE_NEW_PROCESS_GROUP, nullptr, nullptr, &si, &pi))
{
return S_OK;
}
free(itemName);
free(application);
free(command);
CloseHandle(pi.hProcess);
CloseHandle(pi.hThread);
I think the application
and command
should be free'd as well, since they are wchar_t* pointers.
Agree? 🙂
@pjaholkowski Please try Notepad++ v8.5.2 RC if it has not done yet: https://community.notepad-plus-plus.org/topic/24328/notepad-v8-5-2-release-candidate
A lot of issues in Notepad++ context menu (NppShell) of v8.5.1 have been fixed in v8.5.2.
@GurliGebis
Thank you for your work on the new NppShell.
Regarding the memory leak, the
IShellItem* psi
shouldn't be a problem
This is COM-world:
// at the end of your work decrement the reference count for the obtained interface on a COM object
// (the object itself will be automatically destroyed after its refcounter reaches 0 ...)
psi->Release();
and
if (itemName)
::CoTaskMemFree(itemName);
I think the
application
andcommand
should be free'd as well, since they are wchar_t* pointers.
Do not use free()
on pointers from c_str()
!
Your wchar_t* application
here is only a pointer to the 1st character in the null-terminated representation of the text content of the applicationName
wstring object.
Regarding the v8.5.2RC installer testing - mixed results: after installation on my Win10Pro notebook the context "Edit with Notepad++" is not there, on my another PC with Win10Ent is everything ok.
Edit: Maybe there is a problem with newer builds of Windows:
@xomx Thanks, I'll make a PR with those changes
@xomx like this: #19 ? 🙂
@GurliGebis
Thank you for your work on the new NppShell.
Regarding the memory leak, the
IShellItem* psi
shouldn't be a problemThis is COM-world:
// at the end of your work decrement the reference count for the obtained interface on a COM object // (the object itself will be automatically destroyed after its refcounter reaches 0 ...) psi->Release();
and
if (itemName) ::CoTaskMemFree(itemName);
I think the
application
andcommand
should be free'd as well, since they are wchar_t* pointers.Do not use
free()
on pointers fromc_str()
! Yourwchar_t* application
here is only a pointer to the 1st character in the null-terminated representation of the text content of theapplicationName
wstring object.Regarding the v8.5.2RC installer testing - mixed results: after installation on my Win10Pro notebook the context "Edit with Notepad++" is not there, on my another PC with Win10Ent is everything ok.
Edit: Maybe there is a problem with newer builds of Windows:
* my working one is the older 21H2 (Windows 10 Enterprise LTSC 2021 (64-bit)) * the nonworking one is the latest 22H2 (Windows 10 Professional (64-bit))
Sorry, now I realized that I did not properly address this issue. When I wrote that I do not see "Edit with Notepad++" in context menu I was thinking about context menu in Windows 11. Windows 10 has old context menu which is enlisted when you click "Show more options" on Windows 11. We are not thinking about the same context menu because HRESULT NppShell::Installer::Install() detects if code is running on Windows 11 and does not register old menu context.
from HRESULT NppShell::Installer::Install(): if (isWindows11) { // We need to unregister the old menu on Windows 11 to prevent double entries in the old menu. UnregisterOldContextMenu(); UnregisterSparsePackage();
result = RegisterSparsePackage();
} else { result = RegisterOldContextMenu(); }
In my opinion this is mistake because some people turn off Windows 11 context menu through register because most application do not support it and they need to do one click more.
Regarding your issue with not seeing it on Windows 10
HRESULT NppShell::Installer::RegisterOldContextMenu() { const wstring installationPath = GetContextMenuPath(); const wstring guid = GetCLSIDString();
CreateRegistryKey(HKEY_LOCAL_MACHINE, ShellKey, L"ExplorerCommandHandler", guid.c_str());
CreateRegistryKey(HKEY_LOCAL_MACHINE, ShellKey, L"", L"Notepad++ Context menu");
CreateRegistryKey(HKEY_LOCAL_MACHINE, ShellKey, L"NeverDefault", L"");
CreateRegistryKey(HKEY_LOCAL_MACHINE, L"Software\\Classes\\CLSID\\" + guid, L"", L"notepad++");
CreateRegistryKey(HKEY_LOCAL_MACHINE, L"Software\\Classes\\CLSID\\" + guid + L"\\InProcServer32", L"", installationPath + L"\\NppShell.dll");
CreateRegistryKey(HKEY_LOCAL_MACHINE, L"Software\\Classes\\CLSID\\" + guid + L"\\InProcServer32", L"ThreadingModel", L"Apartment");
return S_OK;
}
Maybe that is because registry keys are added to HKEY_LOCAL_MACHINE and not in HKEY_CURRENT_USER. If I'm thinking right adding to HKEY_LOCAL_MACHINE will add it to context menu for all users which you can't do if you are not administrator. On my system normal user don't have permission to write to "HKEY_LOCAL_MACHINE\Software\Classes\CLSID"
@pjaholkowski we are working on that in here: https://github.com/notepad-plus-plus/notepad-plus-plus/issues/13399 - hopefully we can get it verified, so a new RC can be released, which should fix this as well, if I understand the issue correctly.
@pjaholkowski we are working on that in here: notepad-plus-plus/notepad-plus-plus#13399 - hopefully we can get it verified, so a new RC can be released, which should fix this as well, if I understand the issue correctly.
For me adding "Edit with Notepad++" in Windows 11 context menu does not duplicate it in old context menu. I will clearly describe how it works on my Windows 11. I unistalled WinMerge and Notepad++ and there is no entries for Notepad++ or WinMerge. I installed Notepad 8.5.5.1 and there is no "Edit with Notepad++" in new or old context menu but if after that WinMerge is installed it refreshes new context menu without restarting explorer and then "Edit with Notepad++" is visible in new context menu but not in old context menu. I checked it a few times it always works that way. WinMerge installer does something to refresh that context menu. You wrote earlier that it is not needed to make call to winrt::init_apartment(); anymore but this function specifies if WinRT is initalized in STA or MTA mode. From that assert ((!is_sta_thread) it expects thread to run in MTA mode so in my case problem involves using incorrect mode, by default winrt::init_apartment initializes thread in MTA mode, so on my Windows 11 either thread of explorer.exe is initialized in STA mode or you are testing on release build where asserts are disabled by default and it works despite assert not being fullfilled.
Hey I have working version which fixes all problems mentioned before:
void registerThreadFunction() { winrt::init_apartment();
HRESULT hr = registerSparsePackage();
}
void unregisterThreadFunction() { winrt::init_apartment();
HRESULT hr = unregisterSparsePackage();
}
HRESULT install() { const bool isWindows11 = isWindows11Installation();
HRESULT result;
if (isWindows11)
{
bool registerExtension = true;
result = CExplorerCommandVerb_RegisterUnRegister(registerExtension, L"EditWithNotepadPlusPlus", L"Edit with Notepad++");
std::thread registerThread(registerThreadFunction);
registerThread.join();
}
else
{
bool registerExtension = true;
result = CExplorerCommandVerb_RegisterUnRegister(registerExtension, L"EditWithNotepadPlusPlus", L"Edit with Notepad++");
}
moveFileToTempAndScheduleDeletion(getApplicationPath() + L"\\NppShell.dll");
moveFileToTempAndScheduleDeletion(getApplicationPath() + L"\\NppShell.msix");
moveFileToTempAndScheduleDeletion(GetApplicationPath() + L"\\NppShell.dll");
moveFileToTempAndScheduleDeletion(GetApplicationPath() + L"\\NppShell.msix");
SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST, 0, 0);
return result;
}
HRESULT uninstall() { const bool isWindows11 = isWindows11Installation();
HRESULT result = S_OK;
if (isWindows11)
{
bool registerExtension = false;
result = CExplorerCommandVerb_RegisterUnRegister(registerExtension, L"EditWithNotepadPlusPlus", L"Edit with Notepad++");
std::thread unregisterThread(unregisterThreadFunction);
unregisterThread.join();
}
else
{
bool registerExtension = false;
result = CExplorerCommandVerb_RegisterUnRegister(registerExtension, L"EditWithNotepadPlusPlus", L"Edit with Notepad++");
}
SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST, 0, 0);
return S_OK;
}
I will test that on few Windows 11 machines that I have and then I will try to make pull request.
@pjaholkowski I have another branch awaiting with some changes to fix the other issue. Can you hold until it has been merged, and then base your PR on that?
Those registrations should be the same as we do now with PR #20 . Regarding the threading, would it be enough to do this?
In Install():
if (isWindows11)
{
// We need to unregister the old menu on Windows 11 to prevent double entries in the old menu.
UnregisterOldContextMenu();
thread unregisterThread(UnregisterSparsePackage);
unregisterThread.join();
thread registerThread(RegisterSparsePackage);
registerThread.join();
}
In the top of both the UnregisterSparsePackage and RegisterSparsePackage, adding winrt::init_apartment();
From what I can see from the code above, the should solve the threading issue - the rest should be taken care of with #20, since that registers both on Windows 11, and adds logic to handle when both are being shown at the same time in the "classic" context menu.
Those registrations should be the same as we do now with PR #20 . Regarding the threading, would it be enough to do this?
In Install():
if (isWindows11) { // We need to unregister the old menu on Windows 11 to prevent double entries in the old menu. UnregisterOldContextMenu(); thread unregisterThread(UnregisterSparsePackage); unregisterThread.join(); thread registerThread(RegisterSparsePackage); registerThread.join(); }
In the top of both the UnregisterSparsePackage and RegisterSparsePackage, adding
winrt::init_apartment();
From what I can see from the code above, the should solve the threading issue - the rest should be taken care of with #20, since that registers both on Windows 11, and adds logic to handle when both are being shown at the same time in the "classic" context menu.
Sure, but UnregisterSparspePackage and RegisterSparsePackage can be executed by one thread which calls winrt::init_apartment(). It solves crash on my side, will wait for that fix #20.
You are right, I will create a PR with that and send you the link for it after testing.
@pjaholkowski see PR #23 . The other install stuff you posted does the same as the current code that has been merged. We now register both the sparse package (msix) and the "classic" menu using the registry - so now it should show for everyone, no matter what.
@pjaholkowski
When I wrote that I do not see "Edit with Notepad++" in context menu I was thinking about context menu in Windows 11. Windows 10 has old context menu which is enlisted when you click "Show more options" on Windows 11. We are not thinking about the same
Could you try http://download.notepad-plus-plus.org/repository/MISC/nppShell.TEST14/
These new built (signed) packages should fix (fixed by @GurliGebis) your issue because it installs both W11 modern context menu and classic legacy menu.
Please let us know your test result.
@pjaholkowski can you explain to me, since I don't totally understand the reason for it, why we need to move the registration of the package into a separate thread? (since it seems to be working fine for everyone else - so I'm not able to hit the bug you are seeing)
@pjaholkowski
When I wrote that I do not see "Edit with Notepad++" in context menu I was thinking about context menu in Windows 11. Windows 10 has old context menu which is enlisted when you click "Show more options" on Windows 11. We are not thinking about the same
Could you try http://download.notepad-plus-plus.org/repository/MISC/nppShell.TEST14/
These new built (signed) packages should fix (fixed by @GurliGebis) your issue because it installs both W11 modern context menu and classic legacy menu.
Please let us know your test result.
After installation I can see "Edit with Notepad++" in new context menu but when I click "Show more options" there is no entry.
In simple words when I'm running debug build in function registerSparcePackage where line: "auto deployResult = deploymentOperation.get();" is executed, assert (!is_sta_thread) is called. This assert is located at file Windows.Foundation.h in function:
inline void check_sta_blocking_wait() noexcept { // Note: A blocking wait on the UI thread for an asynchronous operation can cause a deadlock. // See https://docs.microsoft.com/windows/uwp/cpp-and-winrt-apis/concurrency#block-the-calling-thread WINRT_ASSERT(!is_sta_thread()); }
I believe someone put it there for a reason. I also did not have crash when I installed Notepad 8.5.1. but it is release build (and compiler doesn't use assert checks in realease build). If you can't reproduce my problem it can be explained by two possible reasons.
Maybe build debug version of dll which works for you, send it to me, I will check it and if it crashes I will send you crash dump of explorer.exe, if not that means I did something wrong.
@pjaholkowski thank you for the explanation - I haven't been running debug builds on my test machine, since I don't build it there, so no debugger to attach anyway. (Yay for printf's) Regarding the old classic menu, lets take a look at that in another issue once we have 8.5.2 out the door.
Why do you need to detect what system is running Windows 11 or Windows 10 because how the context menu works on Windows 11 is app identify so it would work regardless if not on Windows 10 so it will work without adding Windows 11
On Windows 10, we shouldn't try registering the msix, since that context menu only works on Windows 11.
And no, the old classic menu does not show up in the new right click on Windows 11, which is why this entire thing was implemented 🙂
The context menu will still show up like normal under Windows 10 regardless if system is Windows 11 or not I tried both Windows 11 and Windows 10 still works with removing the Windows 11 checks why not register the msix on Windows 10
In Windows 11 you have two context menus:
There is no point of adding entry to new context menu on Windows 10 by registering msix package because such context menu does not exist on Windows 10. I'm not sure if you can now register old context menu with msix package on Windows 10 but I'm certain that it did not work on previous Windows 10 releases and won't work on Windows 7,8 for sure. So either way you will need to make detection between Windows 10,11 and Windows 7,8 if you want register context menu with msix package.
Registering msix package on Windows 11 adds entry to new context menu and old context menu. If you register msix package and add entry to old context menu just like in (Windows 7,8,10) on some machines you will get duplicated entry in old context menu. I remember that I tested this case on diffrent releases of Windows 11 and there were case when registering msix package and adding registry key just worked and few which I got duplicated entry in old context menu. This is why there is detection between Windows 10,8,7 and Windows 11.
This is that fix you did not see in the code which you wrote about here: https://github.com/notepad-plus-plus/notepad-plus-plus/issues/14040
Can you show where it got fixed and I know if Install msix package on Windows without the windows 11 detection it will still show up
Okay, I was half wrong. In previous version of nppShell.dll it worked as I described: https://github.com/notepad-plus-plus/nppShell/blob/9efecd71bad1d5a0278aa5fce401a1f44a3a8f2a/Installer.cpp
But now on head in nppShell/Installer.cpp we have:
HRESULT NppShell::Installer::Install() { const bool isWindows11 = IsWindows11Installation();
HRESULT result;
// Clean up the 8.5 Windows 11 hack if present.
CleanupHack();
if (isWindows11)
{
// We need to unregister the old menu on Windows 11 to prevent double entries in the old menu.
UnregisterOldContextMenu();
// To register the sparse package, we need to do it on another thread due to WinRT requirements.
thread reRegisterThread(ReRegisterSparsePackage);
reRegisterThread.join();
}
result = RegisterOldContextMenu();
// Ensure we schedule old files for removal on next reboot.
MoveFileToTempAndScheduleDeletion(GetApplicationPath() + L"\\NppShell_01.dll", false);
MoveFileToTempAndScheduleDeletion(GetApplicationPath() + L"\\NppShell_02.dll", false);
MoveFileToTempAndScheduleDeletion(GetApplicationPath() + L"\\NppShell_03.dll", false);
MoveFileToTempAndScheduleDeletion(GetApplicationPath() + L"\\NppShell_04.dll", false);
MoveFileToTempAndScheduleDeletion(GetApplicationPath() + L"\\NppShell_05.dll", false);
MoveFileToTempAndScheduleDeletion(GetApplicationPath() + L"\\NppShell_06.dll", false);
// This include the old NppModernShell and NppShell files from the main program directory.
MoveFileToTempAndScheduleDeletion(GetApplicationPath() + L"\\NppShell.dll", false);
MoveFileToTempAndScheduleDeletion(GetApplicationPath() + L"\\NppShell.msix", false);
MoveFileToTempAndScheduleDeletion(GetApplicationPath() + L"\\NppModernShell.dll", false);
MoveFileToTempAndScheduleDeletion(GetApplicationPath() + L"\\NppModernShell.msix", false);
// Finally we notify the shell that we have made changes, so it refreshes the context menus.
SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST, 0, 0);
return result;
}
Which means that old context menu is always registered. By looking in history of Installer.cpp you can find commit https://github.com/notepad-plus-plus/nppShell/commit/8f26080a71594f4f24de43d06ee1c906a8bc0b1d which describes how dupicated entry in old context menu is avoided in Windows 11.
So basically your hiding the context menu under show more options and it will only show up in Windows 11 new context menu is this correct
No, the current version on head works diffrent way, as commit states: "This PR does the following:
By doing this, we can register both the new MSIX and the old registry on Windows 11, and ensure the following:
From a users point of view - it "just works", and they always only see one entry in the old menu."
So now on Windows 11 it adds entry to new context menu by registering msix package and adds entry to old context menu by creating registry key just like in Windows 7,8,10 but duplicated entries in old contexts menu on Windows 11 caused by registering msix package and adding entry to register on some Windows 11 releases are avoided by using shared counter.
At ClassicEditWithNppExplorerCommandHandler.cpp we have code:
using namespace NppShell::CommandHandlers;
const EXPCMDSTATE ClassicEditWithNppExplorerCommandHandler::State(IShellItemArray* psiItemArray) { UNREFERENCED_PARAMETER(psiItemArray);
int state = counter->GetValue();
if (state == 3 || state == 5)
{
// This is on Windows 11, with both the modern and classic being shows, so we hide this one.
return ECS_HIDDEN;
}
return ECS_ENABLED;
}
which hides it if it is already shown and this is why there are no duplicated entries in old context menu anymore.
So this will make it only show up in the new one is it going go be updated soon the source code
Just so you know on the windows 11 build latest version edit with notepad++ has been duplicated again under show more options and the top on the context menu
memory leak in: IFACEMETHODIMP EditWithNppExplorerCommandHandler::Invoke(IShellItemArray psiItemArray, IBindCtx pbc) noexcept try memory for itemName is not released psi is not released
When regestering dll with regsvr32 I have assert at HRESULT NppShell::Installer::RegisterSparsePackage() line 272 auto deployResult = deploymentOperation.get(); Expression: !is_sta_thread() I'm not sure if it could be done from not WINRT thread when I execute regsvr32 one of thread from explorer.exe executes DllRegisterServer Does explorer.exe initializes WinRT by default ? https://learn.microsoft.com/en-us/windows/uwp/cpp-and-winrt-apis/get-started this sample calls winrt::init_apartment(); before using winrt functions. I'm not sure that it is safe to do that from explorer.exe
I tried uncommenting RegisterSparsePackage and did that manually with: powershell -c Add-AppxPackage "NppShell.msix" -ExternalLocation "C:\Shared\PJ\ContextHandler\win11" but after manually registering package it did not work either
I noticed few weird things: Built dll name from Visual Studio project is NppShell.x64.dll but in AppManifest.xml there is "NppShell.dll". I tried changing that it did not work. In ExplorerCommandBase.cpp function IFACEMETHODIMP ExplorerCommandBase::GetCanonicalName(GUID* pguidCommandName) fills GUID with GUID_NULL instead of verb GUID "dd2a27fa-3131-4b50-9b54-836af42fb64d" I noticed that WinMerge (https://github.com/WinMerge/winmerge/blob/master/ShellExtension/WinMergeContextMenu/dllmain.cpp) does the same thing. Is it supposed to work that way ? I'm asking because sample from Microsoft returns that GUID https://github.com/microsoft/Windows-classic-samples/blob/main/Samples/Win7Samples/winui/shell/appshellintegration/ExplorerCommandVerb/ExplorerCommandVerb.cpp
I generated my own certificate, installed it in system as trusted, I changed Publisher field in Identity tag of AppxManifest.dll to my generated certificate, built dll and msix packgae, signed both dll and msix package then from command line: regsvr32 NppShell.dll powershell -c Add-AppxPackage "NppShell.msix" -ExternalLocation "C:\Shared\PJ\ContextHandler\win11"