Closed rdipardo closed 9 months ago
oleg-shilo
commented
2 years ago Thank you Robert. While indeed kbilsted/NotepadPlusPlusPluginPack.Net might be the cause the chance of addressing it is arguably less.
Let me check if the problem can be avoided from cs-script plugin. Marking it as a defect
oleg-shilo
commented
2 years ago Hm, not sure what is happening. In my environment, it was an older version of N++. Updated it to the latest version. And everything works just fine. Then noticed that you are using v8.2.2 (pre-release).
So I suggest you try the latest stable release 8.2.1. At least we know it works.
BTW I suggest when you get it working switch CS-Script to the .NET 6 runtime instead of .NET Framework as MS has discontinued it. The plugin still targets .NET Framework by default to allow the users some transition time. But in the next release, I will need to completely phase it out and target .NET Core family (e.g. ,NET 6).
rdipardo
commented
2 years ago Thank you for looking into this.
You are absolutely right the crash is not reproducible in Notepad++ 8.2.1. In fact, I would advise users to resist upgrading their editors, or else use only 32-bit versions for the time being. It has happened again in the now generally available 8.3 release. The runtime version of the assembly that faulted in 8.2.2 and now 8.3 was reported as "4.0.30319" by the Windows Event Viewer(*); the same runtime target as the NppPlugin.Host assemblies:
As I tried to convey in the linked issue, this is not a bug in either the plugin or the editor. The developers of N++ are well aware that the new release will be an unstable host for 64-bit plugins.
The problem goes back to a feature request against N++ to make massive files of >2GB editable. It was just barely achieved (for x64 builds only) by retyping Scintilla's Sci_PostionCR message as intptr_t, which expands to eight bytes on x86_64 architecture.
The hosting model's equivalent type, CharacterRange, declares fields that are currently too small to safely handle the 64-bit values that calling into SCI_GETTEXTRANGE will now return from 64-bit editors.
There's an open PR at the hosting model's parent repo to address multi-arch memory safety; e.g., preferring IntPtr to int for types exposed to unmanaged interactions with Scintilla's API. As you rightly suggested earlier, this will be no help, assuming it ever gets merged. There is no drop-in replacement for cs-script's heavily customized hosting model.
I can confirm that a patched plugin can be safely loaded by even a pre-release editor once the struct declaration near line 214 of NppPlugin.Host/PluginInfrastructure/GatewayDomain.cs is changed to use IntPtr for the cpmin and cpmax fields, with the other type changes that entails. Notice the type and bit width of each member in the watch list below:
These will be sized accordingly in 32-bit builds, so backward compatibility isn't affected:
I can't predict if there will be regressions; that should be weighed against the current risk of losing data to an application crash.
Notepad++ v8.3 (64-bit)
Build time : Feb 5 2022 - 19:10:24
Path : C:\Program Files\Notepad++\notepad++.exe
Command Line :
Admin mode : OFF
Local Conf mode : OFF
Cloud Config : OFF
OS Name : Windows 11 (64-bit)
OS Version : 2009
OS Build : 22000.469
Current ANSI codepage : 65001
Plugins : CSScriptNpp.dll mimeTools.dll NppConverter.dll NppExport.dll(*)
<Event xmlns="http://schemas.microsoft.com/win/2004/08/events/event">
<System>
<Provider Name=".NET Runtime" />
<EventID Qualifiers="0">1026</EventID>
<Version>0</Version>
<Level>2</Level>
<Task>0</Task>
<Opcode>0</Opcode>
<Keywords>0x80000000000000</Keywords>
<TimeCreated SystemTime="2022-02-06T20:37:13.4781435Z" />
<EventRecordID>20975</EventRecordID>
<Correlation />
<Execution ProcessID="9132" ThreadID="0" />
<Channel>Application</Channel>
<Computer>AcerNotebook</Computer>
<Security />
</System>
<EventData>
<Data>Application: notepad++.exe
Framework Version: v4.0.30319
Description: The process was terminated due to an unhandled exception.
Exception Info: exception code c0000005, exception address 00007FF6BE92EFFF
Stack:
at Kbg.NppPluginNET.PluginInfrastructure.Win32.SendMessage(IntPtr, UInt32, IntPtr, IntPtr)
at Kbg.NppPluginNET.PluginInfrastructure.Win32.SendMessage(IntPtr, UInt32, IntPtr, IntPtr)
at Kbg.NppPluginNET.PluginInfrastructure.ScintillaGateway.GetTextRange(Kbg.NppPluginNET.PluginInfrastructure.TextRange)
at CSScriptIntellisense.NppExtensions.GetTextBetween(Kbg.NppPluginNET.PluginInfrastructure.ScintillaGateway, Int32, Int32)
at CSScriptNpp.CodeMapPanel.RefreshContent()
at CSScriptNpp.Plugin.OnCurrentFileChanged()
at CSScriptNpp.UnmanagedExports.beNotified(IntPtr)
at Kbg.NppPluginNET.UnmanagedExports.beNotified(IntPtr)
at Kbg.NppPluginNET.PluginInfrastructure.Win32.SendMessage(IntPtr, UInt32, IntPtr, System.String)
at Kbg.NppPluginNET.PluginInfrastructure.Win32.SendMessage(IntPtr, UInt32, IntPtr, System.String)
at CSScriptNpp.ProjectPanel.newBtn_Click(System.Object, System.EventArgs)
at System.Windows.Forms.ToolStripItem.RaiseEvent(System.Object, System.EventArgs)
at System.Windows.Forms.ToolStripButton.OnClick(System.EventArgs)
at System.Windows.Forms.ToolStripItem.HandleClick(System.EventArgs)
at System.Windows.Forms.ToolStripItem.HandleMouseUp(System.Windows.Forms.MouseEventArgs)
at System.Windows.Forms.ToolStrip.OnMouseUp(System.Windows.Forms.MouseEventArgs)
at System.Windows.Forms.Control.WmMouseUp(System.Windows.Forms.Message ByRef, System.Windows.Forms.MouseButtons, Int32)
at System.Windows.Forms.Control.WndProc(System.Windows.Forms.Message ByRef)
at System.Windows.Forms.ToolStrip.WndProc(System.Windows.Forms.Message ByRef)
at System.Windows.Forms.NativeWindow.Callback(IntPtr, Int32, IntPtr, IntPtr)</Data>
</EventData>
</Event>First of all, thank you for such a comprehensive investigation. You have done the bulk of heavy lifting. My task is very simple now :). Can you please review the a7db619 to verify that I understood you correctly.
oleg-shilo
commented
2 years ago There is no drop-in replacement for cs-script's heavily customized hosting model.
I am glad you noticed the difference between the original NppPlugin.Host architecture and the approach CS-Script.Npp has taken.
I was not satisfied with the idea of building two versions of a plugin assembly and duplicating probably 80% of the codebase. Instead, I wanted to utilize .NET offers a very natural any CPU model.
Thus I have created two functionally identical plugin hosting assemblies (x86 and x64) and a single AnyCPU plugin assembly to be loaded by the hosting assembly dynamically. That's it. :)
I even contacted the developer of the NppPlugin.Host and shared the approach. Even offered assistance with incorporating it. But it did not go any further.
That's why this plugin also maintains the customized fork of NppPlugin.Host.
rdipardo
commented
2 years ago Can you please review the a7db619 to verify that I understood you correctly.
The crucial revision to CharacterRange has been made here. The integration details really depend on how far you want to baby-proof the plugin's interaction with N++.
From what I see, the only affected module should be src/CSScriptIntellisense/Interop/NppExtensions.cs, where the high-level API methods GetText and friends need syncing with the new TextRange definition. At the moment, I can build the topic branch only after some explicit type conversions:
diff --git a/src/CSScriptIntellisense/Interop/NppExtensions.cs b/src/CSScriptIntellisense/Interop/NppExtensions.cs
index 0c49003..fbd91e7 100644
--- a/src/CSScriptIntellisense/Interop/NppExtensions.cs
+++ b/src/CSScriptIntellisense/Interop/NppExtensions.cs
@@ -74,7 +74,7 @@ namespace CSScriptIntellisense
if (end == -1)
end = document.GetLength();
- using (var tr = new TextRange(start, end, end - start + 1)) //+1 for null termination
+ using (var tr = new TextRange(new IntPtr(start), new IntPtr(end), end - start + 1)) //+1 for null termination
{
document.GetTextRange(tr);
return tr.lpstrText;
@@ -92,7 +92,7 @@ namespace CSScriptIntellisense
if (size > 0)
{
- using (var tr = new TextRange(startPos, currentPos, bufCapacity))
+ using (var tr = new TextRange(new IntPtr(startPos), new IntPtr(currentPos), bufCapacity))
{
document.GetTextRange(tr);
return tr.lpstrText;
@@ -460,7 +460,7 @@ namespace CSScriptIntellisense
if (size > 0)
{
- using (var tr = new TextRange(startPos, endPos, bufCapacity))
+ using (var tr = new TextRange(new IntPtr(startPos), new IntPtr(endPos), bufCapacity))
{
document.GetTextRange(tr);
return tr.lpstrText;
After that, I can use it in 64-bit N++ without crashes.
Note that I added the int -> IntPTr conversions at method local scope for the sake of covenvience. Now that 64-bit N++ can open files >2Gb, a document could someday pass a character position in excess of Int32.MaxValue to GetTextBetween or similar, in which case the conversion to IntPtr comes too late to avert disaster.
I can think of one way to mitigate that risk: be idle when the active file is not C# source or related file type. (There won't be many C# files >2Gb.) To cause a crash right now, the plugin only needs to be loaded while the buffer contains a file with active syntax highlight (e.g. a Markdown page does it for me). Selecting any region of text will bring down the application. These crashes don't leave a stack trace in the event log (*), but the VS debugger shows that TextBeforePosition is called at every text selection event in any source file. I haven't looked closely enough to see how or if that can be avoided.
(*)
<Event xmlns="http://schemas.microsoft.com/win/2004/08/events/event">
<System>
<Provider Name=".NET Runtime" />
<EventID Qualifiers="0">1026</EventID>
<Version>0</Version>
<Level>2</Level>
<Task>0</Task>
<Opcode>0</Opcode>
<Keywords>0x80000000000000</Keywords>
<TimeCreated SystemTime="2022-02-09T14:46:45.5796994Z" />
<EventRecordID>21683</EventRecordID>
<Correlation />
<Execution ProcessID="2704" ThreadID="0" />
<Channel>Application</Channel>
<Computer>AcerNotebook</Computer>
<Security />
</System>
<EventData>
<Data>
Application: notepad++.exe
Framework Version: v4.0.30319
Description: The process was terminated due to an unhandled exception.
Exception Info: exception code c0000005, exception address 00007FF6E26AEFFF
Stack:
</Data>
</EventData>
</Event>Can you please review the a7db619 . . .
A quick margin note: git grep reveals additional (unpatched) definitions of CharacterRange in two sources:
These look like dead code, but it may be better to remove or mark them up with comments, just to eliminate any chance of instantiating a flawed CharacterRange in the future
oleg-shilo
commented
2 years ago Agree. Thank you. Will patch it and do proceed with the release. Though I will need to release new version of CS-Script first. It's getting some deployment/compatibility improvements I would like to deliver. But it should not take more than a few days.
oleg-shilo
commented
2 years ago Actually, it was not only "dead code" but a "completely dead code" 😄 I removed the files completely
oleg-shilo
commented
2 years ago I am not very optimistic about https://github.com/kbilsted/NotepadPlusPlusPluginPack.Net addressing this hosting problem long term. So did the patch by myself.
BTW, you made it possible. I did not have the bandwidth for any investigation. Really appreciate your help.
Please test the prerelease: https://github.com/oleg-shilo/cs-script.npp/releases/tag/1.7.27.0
rdipardo
commented
2 years ago Something strange about the prerelease: both DLLs are failing N++'s Unicode compatibility check, causing an exception at load time:
pi->_pFuncIsUnicode = (PFUNCISUNICODE)GetProcAddress(pi->_hLib, "isUnicode");
if (!pi->_pFuncIsUnicode || !pi->_pFuncIsUnicode())
throw generic_string(TEXT("This ANSI plugin is not compatible with your Unicode Notepad++."));
I can see the obligatory isUnicode function is properly defined and exported(*):
It is possible a UNICODE compiler definition wasn't set during the build?
(*)
Microsoft (R) COFF/PE Dumper Version 14.29.30140.0
Copyright (C) Microsoft Corporation. All rights reserved.
Dump of file CSScriptNpp.dll
File Type: DLL
Section contains the following exports for \NppPlugin.Host.dll
00000000 characteristics
62104030 time date stamp Fri Feb 18 19:56:16 2022
0.00 version
0 ordinal base
6 number of functions
6 number of names
ordinal hint RVA name
5 0 00028CEA beNotified
2 1 00028CC6 getFuncsArray
4 2 00028CDE getName
0 3 00028CAE isUnicode
3 4 00028CD2 messageProc
1 5 00028CBA setInfoRookie mistake 😊 I just had to override the security block that Windows applies to executables from "untrusted" sources:
For reference, all of the following need unblocking as well:
CSScriptNpp/
|__
CSScriptNpp.dll
CSScriptNpp/
|__
CSScriptIntellisense.dll
CSScriptLibrary.dll
CSScriptNpp.asm.dll
ICSharpCode.NRefactory.CSharp.dll
ICSharpCode.NRefactory.dll
Intellisense.Common.dll
Mono.Cecil.dllGreat, I did not even notice that when I tested it Notepad++ v8.3.1. I use 7z, which does not trigger silent locking executables by OS as other archive managers such as WinZip do.
I'll wait a day or two for potential failure reports and then republish this prerelease as a proper one. Adding it to the nppPluginList is problematic as it will be dispatched even to the N++ v8.2* versions that it will not be compatible with :(
rdipardo
commented
2 years ago Adding it to the nppPluginList is problematic as it will be dispatched even to the N++ v8.2* versions that it will not be compatible with :(
Unfortunately I don't have any ideas for backporting the new data type (apart from maintaining two mutually incompatible hosting models). You may want to collaborate with the developer of the (.NET-based) CsvQuery plugin, who plans to make the next release backward compatible with 8.2.x: https://github.com/jokedst/CsvQuery/issues/33#issuecomment-1044622510
As a last resort, you could set up a hook that checks N++'s version at runtime and flashes a warning dialog. I'm planning to do that with the orphaned plugin I ended up inheriting during the ABI kerfuffle.
oleg-shilo
commented
2 years ago Yep, that's my plan B. I can check the version of the editor and ask the user to upgrade N++ or downgrade the plugin.
Unfortunately, N++ team has missed the compatibility check in the PluginsAdmin completely. I have raised it with them: https://github.com/notepad-plus-plus/nppPluginList/issues/416
A backward compatible plugin is possible based on the detected version of the editor and choosing one or another struct for the underlying Scintilla API. Though this would require very precise knowledge of what is changed in the API.
N++ teal allowing specifying target N++ version in the plugin definition is the most standard, industry-accepted way. The majority of package managers do it (e.g. Nuget, VS extension manager).
BTW, kudos on your https://github.com/notepad-plus-plus/notepad-plus-plus/issues/11198
oleg-shilo
commented
2 years ago @rdipardo I have just published the release v1.7.28 with the transparent dynamic API invoke for the new N++ API: https://github.com/oleg-shilo/cs-script.npp/releases/tag/v1.7.28.0
I accepted that there is close to no chance N++ team would do anything about it. The proposal for the solution is not even reviewed. The same as yours.
So I took that matter into my hands and implemented not elegant but working solution for detecting the N++ version (v8.3.1 and above) and switching to a new vs legacy API dynamically.
It turned out that the impacted codebase was relatively small so the dynamic switch was not as hard as I was expecting.
It is a proper release but just in case I will let it some time for the user feedback. And if none will push it to PluginAdmin for the normal distribution to the users.
Again, thank you for your help
rdipardo
commented
2 years ago I've updated the tracking issue to recommend 1.7.28. Works fine, even with .NET 6.0.200 🎉
oleg-shilo
commented
2 years ago Great. Thank you. And I even managed to convince N++ team to embed the compatibility metadata in the https://github.com/notepad-plus-plus/notepad-plus-plus/issues/11198
rdipardo
commented
2 years ago I'm sorry to bump this thread after all the good news, but Notepad++ has run headlong with your idea and there may be unintended regressions.
I note that cs-script looks for N++ 8.3.1 or greater to decide on a safe interface:
However, a proposed plugin manifest would make 8.3 the critical release. That version was withdrawn from general availability some weeks ago, but someone appears to think that it's few remaining users should set the standard for everyone else.
If you foresee a problem there, please request a revision at https://github.com/notepad-plus-plus/nppPluginList/pull/424 (as I have already unsuccessfully done)
The PR author has been in quite the hurry lately, so make it a priority if you can.
oleg-shilo
commented
2 years ago Thank you Robert.
Indeed it is a problem. There is a workaround - user can set CSSCRIPT_NPP_NEW_NPP_API envariable. Though it does not stop the problem from hitting the users but simply allows users to fix it for them specifically. Thus I still need to solve it by releasing the plugin with the updated API switching algorithm updated to be triggered no longer by N++ v8.3.1 but v8.3.0.
I have asked @donho to give me a few hours.
rdipardo
commented
2 years ago Sorry to bump this thread again, but N++'s upcoming 8.4 release could be another show stopper if the plugin's version check remains as is.
Consider how the "minor version" segment should be evaluated, in terms of ABI compatibility:
Major Minor Compatible?
----- ----- ------------
8 0 False
8 1 False
8 11 False
8 12 False
8 13 False
8 14 False
8 15 False
8 16 False
8 17 False
8 18 False
8 19 False
8 191 False
8 192 False
8 193 False
8 2 False
8 21 False
8 3 True
8 31 True
8 32 True
8 33 True
8 4 TrueN++ 8.4 should be compatible, but 4 < 31. (I should also point out that, while N++ 8.1.9.1 is not compatible, 191 >= 31)
The (forgivable) mistake is testing the minor version's integer value without considering its position in the series. I was recently bitten by this myself, as you can read elsewhere.
The root issue is a critical oversight in Notepad++'s plugin API "design", wherein a version string like "8.4" is naïvely parsed as the numbers 8 and 4 — not 40, as a downstream project should be able to expect. Until Notepad++ gets its act together, plugins have to implement some tortured logic like this:
// collect the "minor" versions of 8.1.9.1, etc.
var patchReleases = new int[] { 191, 192, 193 };
// ...
else if (version[0] == 8)
newNppVersion =
// 1. allow "short" versions from 8.3 to 8.10 (only 8.1 and 8.2 are excluded);
// since version 8.11 would match 8.1.1, we just hope that 9.0 comes before then
((version[1] >= 3 && version[1] <= 10) ||
// 2. now we can be more inclusive, with the exception of the "long" versions
(version[1] >= 31 && !patchReleases.Any(v => v == version[1])));
// ...Hopefully a patched plugin can still get into whatever version of Plugins Admin will ship with 8.4, but things obviously move fast with the Notepad++ devs . . .
oleg-shilo
commented
2 years ago Hi Robert,
Thank you again for keeping an eye on it. Really appreciate it.
I have overlooked it. Interestingly enough I have also noted that N++ approach to many design challenges is rather naїve but the way they handle version info is plain childish. I just did not recognize the danger that is the future impact.
Will do the update asap.
oleg-shilo
commented
2 years ago The PR for nppPluginList/ has been created: https://github.com/notepad-plus-plus/nppPluginList/pull/441
rdipardo
commented
2 years ago Heads up: there's a truly bad proposal put forward that would mean even more updates to the plugin API: https://github.com/notepad-plus-plus/notepad-plus-plus/issues/11535#issuecomment-1120080767
If history is any guide, it's already been implemented . . .
oleg-shilo
commented
2 years ago LOL, am I surprised?
This very line makes me puzzled about N++ configuration management maturity:
Yes I confirm each number part of version will remain 1 digit (except the first one), 8.10.5 or 8.5.12 will never happen.
Anyway, handling it in the plugin will be relatively simple. If the new API is available (SendMessage returns non-zero ), it will be used. Otherwise, falls back to the old API.
Thank you for heads up
Steps to reproduce
Plugins > Plugins AdminOK:Also reported upstream at https://github.com/kbilsted/NotepadPlusPlusPluginPack.Net/issues/84