Creating a new script crashes latest 64-bit Notepad++ release #64
Comments
|
Thank you Robert. Let me check if the problem can be avoided from cs-script plugin. Marking it as a defect |
|
Hm, not sure what is happening. In my environment, it was an older version of N++. 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). |
|
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 The hosting model's equivalent type, There's an open PR at the hosting model's parent repo to address multi-arch memory safety; e.g., preferring I can confirm that a patched plugin can be safely loaded by even a pre-release editor once the
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. (*) <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> |
rdipardo
changed the title
|
First of all, thank you for such a comprehensive investigation. |
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 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. |
The crucial revision to From what I see, the only affected module should be 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 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 (*) <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> |
A quick margin note:
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 |
|
Agree. |
|
Actually, it was not only "dead code" but a "completely dead code" 😄 |
|
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 |
|
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 cs-script.npp/src/CSScriptNpp/Interop/UnmanagedExports.cs Lines 24 to 32 in 1bd6c45 It is possible a UNICODE compiler definition wasn't set during the build? (*) |
|
Rookie mistake 😊
For reference, all of the following need unblocking as well: |
|
Great, 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. |
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: jokedst/CsvQuery#33 (comment) 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. |
|
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: notepad-plus-plus/nppPluginList#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 notepad-plus-plus/notepad-plus-plus#11198 |
…g on otherwise incompatible versions of N++ - Some code cleanup
---- v1.7.28 - Patched NppPluginNet: `CharacterRange.cpMin` and `CharacterRange.cpMax` changed from `int` to `IntPtr` This addresses the problems triggered by this the third-party component (NppPluginNet): - Issue #64: Creating a new script crashes latest 64-bit Notepad++ release - Issue #65: Intermittent crashes (and the culprit is CS-Script) - Added transparent switching to the new NPP API depending on the version: v8.2.1<->v8.3.1 (triggered by #64 and #65) - Added logging syntaxer internal errors to the plugin log dir.
|
@rdipardo I have just published the release v1.7.28 with the transparent dynamic API invoke for the new N++ API: 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 |
|
I've updated the tracking issue to recommend 1.7.28.
|
|
Great. Thank you. And I even managed to convince N++ team to embed the compatibility metadata in the notepad-plus-plus/notepad-plus-plus#11198 |
|
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: cs-script.npp/src/CSScriptIntellisense/Interop/NppExtensions.cs Lines 461 to 462 in cf6b68a 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 notepad-plus-plus/nppPluginList#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. |
|
Thank you Robert. Indeed it is a problem. There is a workaround - user can set I have asked @donho to give me a few hours. |
---- v1.7.28-29 - Patched NppPluginNet: `CharacterRange.cpMin` and `CharacterRange.cpMax` changed from `int` to `IntPtr` This addresses the problems triggered by this the third-party component (NppPluginNet): - Issue #64: Creating a new script crashes latest 64-bit Notepad++ release - Issue #65: Intermittent crashes (and the culprit is CS-Script) - Added transparent switching to the new NPP API depending on the version: v8.2.1<->v8.3.0 (triggered by #64 and #65) - Added logging syntaxer internal errors to the plugin log dir.
|
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: N++ 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 . . . |
|
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. |
- Improved Npp compatibility checking (Issue #64) Described: #64 (comment)
|
The PR for nppPluginList/ has been created: notepad-plus-plus/nppPluginList#441 |
This comment was marked as outdated.
This comment was marked as outdated.
|
LOL, am I surprised? This very line makes me puzzled about N++ configuration management maturity:
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 |
- Improved Npp compatibility checking (Issue #64) Described: #64 (comment)
- Improved Npp compatibility checking (Issue notepad-plus-plus#64) Described: oleg-shilo/cs-script.npp#64 (comment)
* Update CSScriptNpp v2.0.2.0 - Improved Npp compatibility checking (Issue #64) Described: oleg-shilo/cs-script.npp#64 (comment) * - CSScriptNpp: fixed x86 version typo
---- - Npp compatibility checking (Issue #64) is disabled in favor of hard-codded compatibility for new Scintille.TextRange API. The checking algorithm became fragile in the latest releases of NPP when the editor restarts itself in Admin mode. - Updated CS-Script embedded engine to v4.6.4 - Updated CS-Script.Syntaxer embedded executable to v3.1.2
Steps to reproduce
Plugins > Plugins AdminOK:Also reported upstream at kbilsted/NotepadPlusPlusPluginPack.Net#84
The text was updated successfully, but these errors were encountered: