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

Fix crash during Search result #11889

Closed
wants to merge 3 commits into from

Conversation

donho
Copy link
Member

@donho donho commented Jul 8, 2022

Fix #11883 partially.

@donho
Copy link
Member Author

donho commented Jul 8, 2022

@Yaron10

The second search-results are not lexed properly, and NPP may even crash.

I have reproduced the crash, and fixed it (hopefully).
Could you check the binary to validate this point (only crash) please?

@donho
Copy link
Member Author

donho commented Jul 8, 2022

@Yaron10
As soon as the confirmation of fix of crash issue is done,
I'll push the fix of Search Result's syntax highlighting issue.

@Yaron10
Copy link

Yaron10 commented Jul 8, 2022

@donho,

The crash was not consistent (about 1 of 10 NPP startups).
Unfortunately I've just encountered a crash with the new binary.

Thank you for looking into it.

@Yaron10
Copy link

Yaron10 commented Jul 8, 2022

Unfortunately I've just encountered a crash with the new binary.

Positive.

I might be wrong, but the crash seems to happen more frequently in NPP x32 (in x64 system).

@Yaron10
Copy link

Yaron10 commented Jul 8, 2022

@ArkadiuszMichalski,

Could you please try reproducing the crash on your machine?

@ArkadiuszMichalski
Copy link
Contributor

Crash still exist:

Notepad++ v8.4.3   (32-bit)
Build time : Jul  8 2022 - 15:52:51
Path : E:\Download\_chrome\npp.8.4.3.portable\notepad++.exe
Command Line : -multiInst
Admin mode : OFF
Local Conf mode : ON
Cloud Config : OFF
OS Name : Windows 11 (64-bit) 
OS Version : 21H2
OS Build : 22000.778
Current ANSI codepage : 1250
Plugins : 
    mimeTools (2.8)
    NppConverter (4.4)
    NppExport (0.4)

@Yaron10
Copy link

Yaron10 commented Jul 8, 2022

@ArkadiuszMichalski,

Thank you.

@donho
Copy link
Member Author

donho commented Jul 9, 2022

@Yaron10 @ArkadiuszMichalski
I cannot reproduce the crash at all with v8.4.3 x32 official portable version (neither with x32 Debug mode)

Could you provide a way to reproduce crash steadily please?

@ArkadiuszMichalski
Copy link
Contributor

@donho

  1. Open Notepad++
  2. Open some file, like license.txt
  3. Go to Find in Files (Ctrl+Shift+F) and mark Follow current doc.
  4. Find some text.

If crash not happend after two searches close and run NPP again and repeat above steps. Currently I get crash after first run.

@xomx
Copy link
Contributor

xomx commented Jul 9, 2022

@ArkadiuszMichalski
I can reproduce the crash with your steps, but on the 32-bit N++ only, x64 does not crash.

Edited:
And now I cannot reproduce it even on the very same binaries (I got the crash before on the v8.4.3 x32 official portable version and also on my 32-bit Debug version of the current GitHub master) , weird!
Thankfully, I did a screenshot of the crash before, so this is everything I have for now:
find_in_files_crash

@ArkadiuszMichalski
Copy link
Contributor

@xomx Try with version from this PR (looks like it breaks more often than the official one). If after catching a crash and after a few attempts, there is still no crash, try run system again (in the past I had a case that after a few crashes in NPP they stopped appearing until the system restarted).

@Yaron10
Copy link

Yaron10 commented Jul 9, 2022

@donho,

Could you provide a way to reproduce crash steadily please?

That's the problem: it's inconsistent and can not be reproduced steadily.

You're trying to separate the crash from the wrong lexing. - Your knowledge and understanding are better than mine.
Still, allow me to ask: did you find out why in "Find in Files" the lexing breaks after the first search? Perhaps it IS related to the random illusive crash?

@ArkadiuszMichalski has mentioned an important point:

If crash not happend after two searches close and run NPP again and repeat above steps.

And also, try it with the x32 build.

Thank you.

@donho
Copy link
Member Author

donho commented Jul 9, 2022

Try with version from this PR (looks like it breaks more often than the official one).

It should be more stable though. In anyway, I believe it doesn't contribute to the crash.

  • Open Notepad++

  • Open some file, like license.txt

  • Go to Find in Files (Ctrl+Shift+F) and mark Follow current doc.

  • Find some text.

Yes, now I can reproduce the crash with x86 official release. However, in debug mode (the same source code) I can see the syntax highlighting is lost, but no crash at all.

It seems it's rather the race condition on Scintilla - the execution on debug binary is much slower; since Scintilla is not thread safe, when the execution speeds up, there might be several accesses to Scintilla, which lead crash.

Since I cannot find the crash reason from the debug, I'll revert 0b5785c to see if the crash still happens.

@Yaron10
Copy link

Yaron10 commented Jul 9, 2022

I'll revert 0b5785c to see if the crash still happens.

We can all try it now with the previous commit.

Would you mind waiting a couple of days?
You or @xomx might still come up with a solution. :)

@donho
Copy link
Member Author

donho commented Jul 9, 2022

Found it!
I'll do a commit in 30 minutes!

@donho
Copy link
Member Author

donho commented Jul 9, 2022

The crash should be fixed in the latest commit.
There is still the colorizing problem though.
Once the crash-fix is confirmed, I will revert 0b5785c to fix the rest of problem.

@Yaron10
Copy link

Yaron10 commented Jul 9, 2022

@donho,

The crash seems to be fixed. 👍
Thank you.

I suppose you've noticed that with the recent commit the results in "Find in Files" are lexed but wrongly.

I will revert 0b5785c to fix the rest of problem.

Do you mean back to "no folding"?

@Yaron10
Copy link

Yaron10 commented Jul 9, 2022

And the lexing in "Find All in Current Document" is broken.

@donho
Copy link
Member Author

donho commented Jul 9, 2022

@Yaron10
Sorry my bad. I was too excited to commit the correct fix.
I just committed the correct one. Please check the latest commit. Sorry again for the error.

@ArkadiuszMichalski
Copy link
Contributor

Crash was fixed.

@Yaron10
Copy link

Yaron10 commented Jul 9, 2022

@donho,

Perfect! 👍
Thank you for you time and work.

I'm sharing your excitement. :)

So there's no need to revert 0b5785c, correct?

@donho
Copy link
Member Author

donho commented Jul 9, 2022

@ArkadiuszMichalski @Yaron10
Thank you for the confirmation!

So there's no need to revert 0b5785c, correct?

Indeed, it's not necessary to revert this PR.
OTOH, I remark there's still the folding performance issue (on the 2nd search) when "color" is searched in all file from root of Notepad++ project.
I may try to improve the method ScintillaEditView::collapse() in another PR.

@Yaron10
Copy link

Yaron10 commented Jul 9, 2022

Indeed, it's not necessary to revert this PR.

That's great.

OTOH, I remark there's still the folding performance issue (on the 2nd search) when "color" is searched in all file from root of Notepad++ project.

There's a performance issue in the first search too. :)
#11774.

I may try to improve the method ScintillaEditView::collapse() in another PR.

👍
Thank you and best of luck.

@donho donho closed this in e6fe568 Jul 9, 2022
@donho donho deleted the fix_search_result_crash branch July 9, 2022 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Regression] Wrong lexing in Search Results
4 participants