Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crashes NP++ when selecting a CSV tab #67

Closed
ThomasMcA opened this issue May 12, 2023 · 13 comments
Closed

Crashes NP++ when selecting a CSV tab #67

ThomasMcA opened this issue May 12, 2023 · 13 comments
Labels
bug Something isn't working

Comments

@ThomasMcA
Copy link

Using NP++ 8.5.2 64bit and CSVLint v0.4.6.4

The crash happens with these steps:

  1. Launch NP++
  2. If a CSV is not already active when NP++ launches, then select any CSV tab
  3. Select a non-CSV tab
  4. Reselect the CSV tab from step 2
  5. NP++ crashes with no errors or popups

SETTINGS REQUIRED TO RECREATE THE CRASH

Enable the Multi-Line setting for the Tab Bar (Settings / General / Tab Bar / Multi-line). When that is enabled, and the tabs wrap to 2+ lines, activating any CSV tab a 2nd time crashes NP++.

  • Enable multi-line, and only have one line of tabs - no crash.
  • Un-maximize the window, drag it small enough to create 2+ rows of tabs - activating a CSV tab a 2nd time crashes NP++.

This crash even happens on a fresh download of NP++ v8.5.2 portable.

@BdR76
Copy link
Owner

BdR76 commented May 13, 2023

You also posted this a while ago on the Notepad++ forum, but this was a bit difficult to reproduce because it requires a specific combination of settings. Your step-by-step description helped point in the right direction, much appreciated and thanks for your patience. I think this actually has to do with the transparant cursor line setting.

I was able to reproduce it like this:

  1. in CSV Lint settings, set TransparantCursor to true
  2. In Notepad++ in menu Settings > Preferences, set General > Tab bar > Multi-line to checked/enabled.
  3. Have at least 2 lines of tabs opened
  4. Have at least 1 csv file open (or just all csv files)
  5. click to switch between the different tabs

After a few switches in step 5 it will crash Notepad++. When you set TransparantCursor to false, restart Notepad++ and then try again starting with step 2) then it doesn't crash.

csv_lint_transparant_multiline

@BdR76
Copy link
Owner

BdR76 commented May 13, 2023

It has to do with the transparant cursor line, so I'll have to look in to that. Issue #68 is probably related. Specifically, it has to do with these lines (when I disable them it doesn't crash). Although I can't explain why it doesn't crash when there is just a single line of tabs opened.

For the moment the work-around is to set the CSV Lint setting TransparantCursor to false

@BdR76 BdR76 added the bug Something isn't working label May 13, 2023
@ThomasMcA
Copy link
Author

Thanks. Changing TransparantCursor to false fixes the issue for me.

BdR76 added a commit that referenced this issue May 16, 2023
Synchronize with NotepadPlusPlusPluginPack.Net to eliminate the possibility that code difference was causing the crashing issue #67 (it was not)
@BdR76
Copy link
Owner

BdR76 commented May 16, 2023

I'm 100% certain that this has to do with these two lines i.e. when I disabled them then it doesn't crash anymore. However I can't figure out why that is. The two lines were introduced in this commit

I had updated some code to eliminate the possibility that this was causing the issue, although I suspect it might have to do with this "Updated Position types to work also on 64 bit" update

@rdipardo
Copy link
Contributor

I'm 100% certain that this has to do with these two lines i.e. when I disabled them then it doesn't crash anymore.

It looks like the editor object may be null under the circumstances the OP described.

However I can't figure out why that is

Start notepad++.exe from the debugger with CSVLint.pdb inside the plugin folder alongside a debug DLL, follow the repro steps, then watch if Visual Studio breaks on a NullPointerException in the suspected code path.

If that finds the bug, you can guard the API calls with a null check, or use the null-conditional operator instead of blindly accessing the editor object's methods.

@BdR76
Copy link
Owner

BdR76 commented May 18, 2023

It's not a null pointer error, the editor variable is valid. I tried just setting a try..catch around the but that didn't change anything. Also, when I step through the code using Visual Studio debugger, I can step through the functions but somewhere after these function calls it just stops debugging and goes back to the code editor (same as pressing Stop Debug), there is no error message or null pointer that pops up. So I think it has to do with variabled that go out of scope in Scintille

Yesterday I updated the .NET Scintilla interface using these tools. which although very helpful, was still a lot of work. But then I was able to use the new SetElementColour() and ColourAlpha etc that v5 supports. However, at the end of it all I changed the Lex() function like this:

    old:
    editor.SetCaretLineBackAlpha((Alpha)64);
    editor.SetCaretLineBack(new Colour(0xFF00C0));

    new:
    editor.SetElementColour(Element.CARET_LINE_BACK, new ColourAlpha(255, 0, 192, 64));

In the new situation it compiles and runs fine, but there is no alpha/transparency, see screenshot below.

csv_lint_transparency

While debugging I used a different caret background color to more clearly see what it's doing. I checked the ColourAlpha.Value is a 32bit int and it does contain the alpha value part in the most-significant 24 bits.

And on top of that it didn't solve the crash problem. 😐

@BdR76
Copy link
Owner

BdR76 commented May 18, 2023

I think it has to do more with the fact the CSV Lint is setting and resetting the caret background color when switching between tabs, in combination with one of the parameters going out of scope, somehow maybe.

I tried to rule this out, by using only hardcoded values and passing a new Colour() whenever calling the SetCaretLineBack* or SetElement* functions, but that also doesn't help.

I also tried in the Lex() function to only set the Alpha background after checking if it is currently Alpha.NOALPHA (using SCI_GETCARETLINEBACKALPHA) and if it is already transparent then do nothing, plus in the Main.OnNotification just do nothing at all to reset it. Then the CSV Lint plugin works and doesn't crash when switching tabs, but the cursor line is now always transparent in every file (not ideal but might be workable). However now it crashes when using the Compare plugin, when comparing or clearing the compare results. So yeah that's not a solution either.

To be clear, when a CSV file is opend the lexer Lex() sets transparency here and when switching to a different file/tab the transparency resets here

@rdipardo
Copy link
Contributor

I think it has to do more with the fact the CSV Lint is setting and resetting the caret background color when switching between tabs, in combination with one of the parameters going out of scope, somehow maybe.

I'm not sure how .NET encodes RGBA colors, but I'm certain that Scintilla's format is different. Read the documentation and make sure the alpha value is in the right bit position. To be absolutely safe, use a method that shifts a color int like NppDarkMode.BGRToColor does, but with logic added for the alpha bits.

now it crashes when using the Compare plugin, when comparing or clearing the compare results. So yeah that's not a solution either.

It's unfortunate that comparePlus manipulates the caret line background style, but I can't see how that would lead to a crash.

It could be the color int is becoming negative and overflowing in the SendMessage wrapper. IIRC the .NET infrastructure blindly casts everything to a (signed) IntPtr, but the underlying Win32 function expects the WPARAM to be an unsigned type.

The alpha variable is passed as the WPARAM by the SetCaretLineBackAlpha method. You could try overloading Win32.SendMessage as "(IntPtr hWnd, UInt32 Msg, UIntPtr wParam, IntPtr lParam)", then use the debugger to evaluate (UIntPtr) alpha when the method gets called.

@BdR76
Copy link
Owner

BdR76 commented May 18, 2023

I'm aware of the RGBA encoding as a 32bit int, but this has nothing to do with that. Because it only happens when there are 2 or more lines of tabs, see the screenshot with the red arrows.

When you resize the Notepad++ screen, so that it has one single line of tabs, then it doesn't crash(!?) really weird.

fyi this is the implementation of the ColourAlpha I used. I'll try to also submit this as a PR for the main NotepadPlusPlus PluginPack.Net project

public class ColourAlpha
{
    public readonly int Red, Green, Blue, Alpha;

    public ColourAlpha(Int64 rgba)
    {
        Red = (byte)rgba & 0xFF;
        Green = (byte)(rgba >> 8) & 0xFF;
        Blue = (byte)(rgba >> 16) & 0xFF;
        Alpha = (byte)(rgba >> 24) & 0xFF;
    }

    public ColourAlpha(byte red, byte green, byte blue, byte alpha)
    {
        Red = red;
        Green = green;
        Blue = blue;
        Alpha = alpha;
    }

    public int Value => Red + (Green << 8) + (Blue << 16) + (Alpha << 24);
}

@rdipardo
Copy link
Contributor

I think I've found the code path where the crash really happens, here:

var editor = new ScintillaGateway(PluginBase.GetCurrentScintilla());
editor.SetCaretLineBackAlpha(sAlpha);// Alpha.NOALPHA); // default
editor.SetCaretLineBack(sCaretLineColor);

The cause is definitely a null pointer exception; the Windows error log reports it it as an access violation (error code 0xC0000005), which is basically the same thing:

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 00007FF7D0A39047

The reason goes back to a basic principle of object initialization. You have a static readonly member called sAlpha.

static readonly Alpha sAlpha = editor.GetCaretLineBackAlpha();

static members are initialized as soon as execution starts, i.e., before the plugin has been loaded by Npp. Every time sAlpha is accessed, it has the same value it had before the plugin was loaded. There is no caret line at that early stage of initialization.

When debugging I found sAlpha was constantly 256 (i.e. Alpha.NOALPHA), but release builds optimize away any residual data, so sAlpha is most likely null when the plugin starts up on end users' PCs. Every crash I observed happened after editor.SetCaretLineBackAlpha was called with sAlpha. No crash after removing those lines from Main.cs. The lexer still styles the caret line every time you type a key or switch buffers, so no extra callback in the Main class is even necessary.

The caret line still goes black when comparePlus is installed alongside, but that, like I said before, is just a nuisance, not something that would crash the editor.

BdR76 added a commit that referenced this issue May 19, 2023
Attemp to fix issue #67 by using new/fixed values instead of variables, however this still has same result and crashes when switching between multiple stacked tabs
@BdR76
Copy link
Owner

BdR76 commented May 19, 2023

I get what you're saying, except by that logic it would be fixed by using a new Color() and the standard Alpha.NOALPHA value, instead of the variables that are potentially not initialised. However this doesn't fix it and still has the same result, see commit 3077f2d

@BdR76
Copy link
Owner

BdR76 commented May 19, 2023

Besides the crashing with stacked tabs, the plugin would also crash at startup in very specific circumstances. I had a reproducable situation with two files, but then when I copied those files to another folder then it wasn't reproducable anymore with those files. Even renaming one file by one character (both the file and in filename in sessions.xml) and then it wouldn't crash anymore, it is really weird. See also issues #69 I spend quite some time on this, I even viewed the file and the copy/renamed file with powershell format-hex to verify that the files are byte-for-byte exactly the same.

Anyway, I made an update to the plugin, see commit 241a6ee now the SetCaretLineBackAlpha is called only once when the plugin first starts, so not when opening a file or changing between files.

This fixes both cases, so the stacked tabs crash and the sessions/startup crash, only downside is that the transparent cursor line now stays transparent for the rest of the Notepad++ session also in other files. But the next release of the plugin should at least not have any unexplained edge-case crashes anymore.

@BdR76
Copy link
Owner

BdR76 commented Jun 4, 2023

The cursor line is set to transparent and is then not changed anymore, see previous post. So this issue is fixed in the latest version v0.4.6.5, see the releases page. You can download it manually and it will be available in the next Notepad++ update in the Plugin Manager.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants