Intermittent Notepad++ crash related to NPPM_GETOPENFILENAMES (only on 64-bit NPP)
-
Today, when I was running tests for my JsonTools plugin, I noticed that Notepad++ kept crashing unrecoverably for no apparent reason. This crash caused the application to hard exit without even being caught by the Visual Studio debugger. Eventually I was able to track the crash down to
NPPM_GETOPENFILENAMES
; it looks like there’s some kind of memory corruption going on. In addition, when I run a plugin command in CSharpPluginPack that callsNPPM_GETOPENFILENAMES
, this also causes a crash.EDIT: I forgot to mention that this is only a problem on 64-bit Notepad++; 32-bit Notepad++ is not affected.
The weirdest thing about this crash issue is that it does not appear to be linked to a specific version of Notepad++ or a specific version of JsonTools. For example, a few days ago I was able to run all my tests for v8.3.1 of JsonTools on Notepad++ 8.7.4, but this evening, running tests for the same version of JsonTools on the same version of Notepad++ causes a crash. This is in spite of the fact that I haven’t updated my OS in the interim.
EDIT: I previously showed debug info, but I’m removing it, because the same thing happens on every 64-bit version of Notepad++ I’ve tested from 8.3.3 to 8.7.4 (yes, I have a lot of Notepad++ versions installed on my machine).
-
@Mark-Olson
Sounds like you done quite a bit of testing to define the environment. The only thing I’d say is missing is conducting the same tests on another PC. Do you have ability, and of course time to set up a similar environment?Terry
-
Oy… that code is a disaster waiting to happen.
I don’t know if you can read C++: the code expects you to pass it an array of pointers to buffers; the pointers must already be set to point to the buffers, and the buffers must be large enough to contain the file name strings.
It uses lstrcpy; the Microsoft documentation for that function says, “Warning Do not use.”
I suggest pretending NPPM_GETOPENFILENAMES does not exist, and instead using NPPM_GETNBOPENFILES and NPPM_GETBUFFERIDFROMPOS to enumerate buffer ids, then NPPM_GETFULLPATHFROMBUFFERID to get the file names.
-
Thank you so much for identifying the badness of the
NPPM_GETOPENFILENAMES
code in Notepad++. 🎉 I may submit an issue in the NPP GitHub repo tomorrow.I reimplemented the code in my plugin so that it uses almost the approach you suggested, with the difference being that I iterate over the two views and use
NPPM_GETNBOPENFILES
andNPPM_GETBUFFERIDFROMPOS
in each view. Now there is no crash when I run the tests in 64-bit Notepad++, and they still work in 32-bit Notepad++.Now to implement this fix in NppCSharpPluginPack…EDIT: This issue has been fixed in the aforementioned plugins in JsonTools v8.3.1.2 and NppCSharpPluginPack v0.0.3.16.
The one wrinkle with this approach is that, if the second view is not open, the list of open file names will always have
new 1
at the end, because that’s the name of the ghost placeholder buffer that a closed second view always has for mysterious internal reasons. AFAICT there’s no exposed API for determining if the second view is open, so for now I just have to accept that this is the way things are. -
@Coises said:
Oy… that code is a disaster waiting to happen
Oy…indeed. :-(
@Mark-Olson said:
AFAICT there’s no exposed API for determining if the second view is open, …
I looked up how the PythonScript plugin provides the
notepad.isSingleView()
function; from what I found, HERE, I would guess that you are right that Notepad++ does not directly provide an API for getting this information. :-(…so for now I just have to accept that this is the way things are.
I suppose you could take a similar approach to what the PS plugin is doing (regard the view info)…and plow onward from there…
FWIW, HERE is how PS retrieves info via
notepad.getFiles()
. -
@Mark-Olson said in Intermittent Notepad++ crash related to NPPM_GETOPENFILENAMES (only on 64-bit NPP):
AFAICT there’s no exposed API for determining if the second view is open, so for now I just have to accept that this is the way things are.
Remember that it is possible for the second view to be open and the first view closed.
-
@Mark-Olson said in Intermittent Notepad++ crash related to NPPM_GETOPENFILENAMES (only on 64-bit NPP):
I may submit an issue in the NPP GitHub repo tomorrow.
It hasn’t been touched for a long time. (Git Blame shows the last change as from 2015, and that appears to be just reformatting as part of a clean-up effort.)
The problem is more the interface itself than the way it is implemented. No doubt the interface has been and will be retained for compatibility with any older plugin that uses it.
I’m not sure where or how one would request this, but I suggest the appropriate course of action would be to deprecate the message. I believe that could only consist of putting note in the header file and the online documentation.
Were you able to set a breakpoint here and check whether cStrArray was as expected, and whether the crash occurred during the Notepad++ call or after it returned? I notice that the ClikeStringArray constructor uses Marshal.AllocHGlobal with a string capacity; I think you might be passing MAX_PATH to it, which is a count of characters, while AllocHGlobal is expecting bytes and the file paths are surely in Unicode.
In any case, I think MAX_PATH is not a reliable maximum on 64-bit Windows.
-
@Coises said in Intermittent Notepad++ crash related to NPPM_GETOPENFILENAMES (only on 64-bit NPP):
Were you able to set a breakpoint here and check whether cStrArray was as expected, and whether the crash occurred during the Notepad++ call or after it returned? I notice that the ClikeStringArray constructor uses Marshal.AllocHGlobal with a string capacity; I think you might be passing MAX_PATH to it, which is a count of characters, while AllocHGlobal is expecting bytes and the file paths are surely in Unicode.
As far as I could tell, the crash seemed to be occurring during a call to
Marshal.FreeHGlobal
in theCLikeStringArray.Dispose
method. I didn’t try to poke around in there too much, because it seemed like the method was being called asynchronously when the garbage collector collected the CLikeStringArray.I suggest the appropriate course of action would be to deprecate the message
I agree with this. From my search for calls to
lstrcpy
it looked to me like there might be other plugin messages that could be deprecated. -
I looked at PythonScript’s approach (as @Alan-Kilborn suggested) to determining if there’s a single view, and I decided to use NPPM_GETCURRENTDOCINDEX, as @Coises suggested. It’s a little roundabout, but it feels much less hacky than the approach using
FindWindowEx
to me, at least in C#. -
@Mark-Olson said:
I decided to use NPPM_GETCURRENTDOCINDEX , as @Coises suggested. It’s a little roundabout, but it feels much less hacky than the approach using FindWindowEx
I agree. I don’t know why PS didn’t use that approach as well. I’d make a suggest that PS be changed, but it’s probably a case of “ain’t broke, so don’t fix it”.
-
So it turns out that my plugin was at fault, as noted here. I was allocating
MAX_PATH
bytes for a buffer that could containMAX_PATH
wide chars, so any filename with lengthMAX_PATH / 2
or greater would cause a bug.