• Login
Community
  • Login

Intermittent Notepad++ crash related to NPPM_GETOPENFILENAMES (only on 64-bit NPP)

Scheduled Pinned Locked Moved Notepad++ & Plugin Development
pluginscrash
11 Posts 4 Posters 950 Views
Loading More Posts
  • Oldest to Newest
  • Newest to Oldest
  • Most Votes
Reply
  • Reply as topic
Log in to reply
This topic has been deleted. Only users with topic management privileges can see it.
  • T
    Terry R @Mark Olson
    last edited by Dec 29, 2024, 5:40 AM

    @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

    1 Reply Last reply Reply Quote 1
    • C
      Coises @Mark Olson
      last edited by Dec 29, 2024, 5:49 AM

      @Mark-Olson

      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.

      M 1 Reply Last reply Dec 29, 2024, 7:48 AM Reply Quote 6
      • M
        Mark Olson @Coises
        last edited by Mark Olson Dec 29, 2024, 8:26 AM Dec 29, 2024, 7:48 AM

        @Coises

        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 and NPPM_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.

        A C 3 Replies Last reply Dec 29, 2024, 10:55 AM Reply Quote 4
        • A
          Alan Kilborn @Mark Olson
          last edited by Alan Kilborn Dec 29, 2024, 2:14 PM Dec 29, 2024, 10:55 AM

          @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().

          1 Reply Last reply Reply Quote 2
          • C
            Coises @Mark Olson
            last edited by Dec 29, 2024, 3:13 PM

            @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.

            Try NPPM_GETCURRENTDOCINDEX .

            Remember that it is possible for the second view to be open and the first view closed.

            M 1 Reply Last reply Dec 29, 2024, 6:58 PM Reply Quote 3
            • C
              Coises @Mark Olson
              last edited by Coises Dec 29, 2024, 4:11 PM Dec 29, 2024, 4:07 PM

              @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.

              M 1 Reply Last reply Dec 29, 2024, 6:09 PM Reply Quote 3
              • M
                Mark Olson @Coises
                last edited by Dec 29, 2024, 6:09 PM

                @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 the CLikeStringArray.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.

                1 Reply Last reply Reply Quote 0
                • M
                  Mark Olson @Coises
                  last edited by Mark Olson Dec 29, 2024, 7:07 PM Dec 29, 2024, 6:58 PM

                  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#.

                  A 1 Reply Last reply Dec 29, 2024, 7:33 PM Reply Quote 2
                  • A
                    Alan Kilborn @Mark Olson
                    last edited by Dec 29, 2024, 7:33 PM

                    @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”.

                    1 Reply Last reply Reply Quote 1
                    • M
                      Mark Olson
                      last edited by Jan 1, 2025, 8:14 AM

                      So it turns out that my plugin was at fault, as noted here . I was allocating MAX_PATH bytes for a buffer that could contain MAX_PATH wide chars, so any filename with length MAX_PATH / 2 or greater would cause a bug.

                      1 Reply Last reply Reply Quote 2
                      • C Coises referenced this topic on May 19, 2025, 7:12 PM
                      11 out of 11
                      • First post
                        11/11
                        Last post
                      The Community of users of the Notepad++ text editor.
                      Powered by NodeBB | Contributors