Skip to content
This repository has been archived by the owner on Dec 22, 2023. It is now read-only.

Strange behaviour of ScintillaNET and WeifenLuo.WinFormsUI.Docking #85

Closed
Aghlara opened this issue Jul 21, 2015 · 29 comments
Closed

Strange behaviour of ScintillaNET and WeifenLuo.WinFormsUI.Docking #85

Aghlara opened this issue Jul 21, 2015 · 29 comments
Labels

Comments

@Aghlara
Copy link

Aghlara commented Jul 21, 2015

Hi,

Has anyone integrated Scintilla with WeifenLuo dock control? When I drag a form and change its location to left/right from being a document the Sceintilla either:

  • Gets Access Violation Exception:

    at ScintillaNET.Scintilla.DirectMessage(IntPtr sciPtr, Int32 msg, IntPtr wParam, IntPtr lParam)
    at ScintillaNET.Scintilla.DirectMessage(Int32 msg, IntPtr wParam, IntPtr lParam)
    at ScintillaNET.Scintilla.DirectMessage(Int32 msg, IntPtr wParam)
    at ScintillaNET.Scintilla.OnHandleCreated(EventArgs e)
    at System.Windows.Forms.Control.WmCreate(Message& m)
    at System.Windows.Forms.Control.WndProc(Message& m)
    at ScintillaNET.Scintilla.WndProc(Message& m)
    at System.Windows.Forms.NativeWindow.DebuggableCallback(IntPtr hWnd, Int32 msg, IntPtr wparam, IntPtr lparam)

  • TextChanged is raise and text is deleted with no proper call stack!
    image

I even disabled all the logic for folding and numbering... I thought might be related to that, but still the same issue.

Can anyone help me here?
Before moving:
moving-form

After moving is done:
moving-finished

As you can see the right side is the form that in previous picture had contents.

Regards.

@jacobslusser
Copy link
Owner

In some rare cases I believe it is possible for Windows Forms to destroy a control's handle and recreate it. My guess is that's what is happening.

I'm pretty sure we did a fix for this issue in ScintillaNET v2... I just need to go back through the code to find it.

If you would like to confirm that this is the issue, try hooking the Scintilla.HandleCreated event and/or Control.RecreatingHandle property.

@Aghlara
Copy link
Author

Aghlara commented Jul 21, 2015

I can confirm that's the handle is being created on the move.

screen shot 2015-07-21 at 11 02 53 am

And the content is already at this point.

@Aghlara
Copy link
Author

Aghlara commented Jul 21, 2015

I can handle both HandleDestroyed and HandleCreated and re-set the text after it is re-created. Is that the fix? Or you are going to do in a better way properly remove the root cause of it?

@Aghlara
Copy link
Author

Aghlara commented Jul 21, 2015

I just noticed, not only the Text will be deleted, but also, all the margins and other properties (Lexer) are gone as well. Hope that helps.

@Aghlara
Copy link
Author

Aghlara commented Jul 21, 2015

Just an update, the workaround is what I described, to listen on those events (HandleDestroyed , HandleCreated) and reinitialise the entire component again. Hope this will be fixed in future.

@Aghlara Aghlara closed this as completed Jul 21, 2015
@DaRumpel
Copy link

There is a another workaround on that. I had the same error when I reconstructed the window on startup from the XML file. To circumvent the error, you can put all windows involving scintilla controls in a kind of queue when you would normally load them in the reconstruction-call-back-function. Afterwards, catch the Shown event of the main form and process the queue, i.e. open the windows. Not beautiful, but it works (only at start-up, not when moving windows).

@jacobslusser
Copy link
Owner

So I've been studying this issue quite a bit and here is a brain dump of what I've found.

As I suspected, there are times when Windows Forms will force a handle change. See:

This is an issue for ScintillaNET (currently) because everything is associated with that handle. If the handle changes -- no more Scintilla.

This issue was also identified and fixed in previous version of ScintillaNET and detailed here:

The approach used in this fix is to swallow the WM_DESTROY Window message and instead change the control parent so that the handle doesn't actually change. To actually destroy the handle, some additional code has been added to the Dispose method. This seems to work, however, I'm a bit weary of it and feels hacky.

Another approach would be to do something similar to what @Aghlara has done and to save the state of the control when the handle is destroyed and then restore the state again when the handle is created. We could do this internally and seamlessly to ScintillaNET users. The drawback of this approach is that it is a bit more expensive. It requires copying out the document text and then restoring it again but that's probably not a significant hit.

Finally, we could leave this as an issue for ScintillaNET users... which I know would be an unpopular choice... but this issue is very specific to using ScintillaNET in an MDI configuration. When the visibility of an MDI window changes (or when moving an MDI window using the WeifenLuo library) the handle gets destroyed and created again. To prevent this from affecting the Scintilla control it should be as easy as removing the control from the MDI window and then adding it back again after the move/visible change.

If anyone has any opinions about this, please feel free to speak up. I'm going to let this churn a bit in my head and then make a decision.

@jacobslusser jacobslusser reopened this Jul 28, 2015
@theryan722
Copy link
Contributor

I think the first option would be the best to do, even if it isn't performed in the greatest of ways.

The option of reconfiguring the entire control sounds a little to expensive, and if it were to be the method used, should at least be done internally.

That is just my opinion however.

@HTD
Copy link

HTD commented Jul 28, 2015

Is it possible to make it configurable? I see it as new public property MdiSupport of enum MdiSupportType { None, Fast, Compatible }. Depending on this property ScintillaNET should apply one of these 2 workarounds, or none, when it's not necessary. It would be rock solid, no existing code could break (since it would be set to None by default), and each user would be able to decide which method would be best for her / him. For me it would depend on the file size - for large documents I would use the fast method, for average ones - compatible one. Of course if anything fails with fast method we can always fall back to compatible one.

@Aghlara
Copy link
Author

Aghlara commented Jul 28, 2015

I think re-initializing the control is safer although it feels expensive. The reason is safer is because of the fact that control handle is being destroyed by underlaying components, so swallowing the WM_DESTROY message and ignoring it may have some unknown side effects in the future.

I think this should be just handled by the ScintillaNet users.

@theryan722
Copy link
Contributor

I tried following these instructions which fix it in v2.x:

scint1

However what is the equivalent of Scintilla.WndProc in v3.x?

Also based on this, and from what I'm reading, the Dispose() method would have to be modified, but am not sure what to change. Would I have to reset the container controls handle? (https://scintillanet.codeplex.com/workitem/11811)

Thanks.

@jacobslusser
Copy link
Owner

@theryan722
Copy link
Contributor

The fix from v2 works, thanks. Do you think this is something we could implement?

@jacobslusser
Copy link
Owner

I like the suggestion that the fix be something developers can opt into. I'm working on it but it will take some time with my other priorities.

@Ahmad45123
Copy link
Contributor

This getting fixed anytime soon ? :/
Please set it as top priority.

@jacobslusser
Copy link
Owner

Sorry, I haven't forgotten. Been busy with my day job; but I confess that there is a part of me that is just ignoring it because I don't want to deal with it. :) I'm just not please with any of the options. In the meantime you are using one of the workaround, no?

@Krischkros
Copy link

Please fix it :)

@Petarian
Copy link

I ran into the same problem and ended up doing what Aghlara recommended. However, for some reason my HandleCreated is not getting called after I dock the MDI window. As a result, I check if my HandleDestroyed is getting called as a result of FormClose or Dock and then recreate my Scintilla control if necessary.

A question for Jacob, will this logic (HandleDestroyed/HandleCreated) have to be undone when you fix this code?

@Krischkros
Copy link

Yes, i've fixed it.
Try the AutoHide-property if you dont have the source.
Cheers

@Petarian
Copy link

@Krischkros I am at a loss - I don't see an "AutoHide" property in Scinitalla. I see a AutoCAutoHide but I am sure that is different. I am using version 3.5.1

Are you saying you have modified the code on your end?

@Krischkros
Copy link

Yes. And i found more:
Scintilla.cs

Loader.cs:
Loader.cs

and more.. if anyone need this, scream please :)

jacobslusser added a commit that referenced this issue Oct 14, 2015
…st fix.

Added a new opt-in method SetDestroyHandleBehavior(true) to work around
issues with the control being sent WM_DESTROY messages when we don't
want them.
@jacobslusser
Copy link
Owner

Thank you everyone for being patient on this. I just wanted to make sure I got this right, or if I didn't get it right, that I did it in a way which wouldn't impact users who don't have this issue.

Commit d632e6e adds a new method called SetDestroyHandleBehavior which will allow you to opt-in to a fix for this issue. I've decided to make it opt-in for now, but assuming everyone's testing goes well we can reverse that so that it is on by default and then give users a chance to opt-out.

To enable the workaround, simply call SetDestroyHandleBehavior(true) before creating any Scintilla controls. i.e.

static void Main()
{
    // Opt-in to WM_DESTROY workaround
    Scintilla.SetDestroyHandleBehavior(true);

    // Standard boilerplate
    Application.EnableVisualStyles();
    Application.SetCompatibleTextRenderingDefault(false);
    Application.Run(new FormDockPanel());
}

Internally I'm using the same fix that we did for ScintillaNET v2, but I've also addressed a memory leak that was present in the v2 fix.

Please report any issues you find.

@theryan722
Copy link
Contributor

I have been waiting for this for a long time, thank you. No more modifying the source every time an update is released 👍

@Krischkros
Copy link

thanks man! <3

normal? :)

/edit
oh shit -.-


System.AccessViolationException wurde nicht behandelt. HResult=-2147467261 Message=Es wurde versucht, im geschützten Speicher zu lesen oder zu schreiben. Dies ist häufig ein Hinweis darauf, dass anderer Speicher beschädigt ist. Source=System.Windows.Forms StackTrace: bei System.Windows.Forms.UnsafeNativeMethods.CallWindowProc(IntPtr wndProc, IntPtr hWnd, Int32 msg, IntPtr wParam, IntPtr lParam) bei System.Windows.Forms.NativeWindow.DefWndProc(Message& m) bei System.Windows.Forms.Control.DefWndProc(Message& m) bei System.Windows.Forms.Control.WmMouseMove(Message& m) bei System.Windows.Forms.Control.WndProc(Message& m) bei ScintillaNET.Scintilla.WndProc(Message& m) in

@jacobslusser
Copy link
Owner

Can you give me steps to reproduce? I didn't experience any AccessViolationExceptions in my testing.

@Krischkros
Copy link

Create a dockpanel instance (from http://dockpanelsuite.com/), create a scintillaNET instance and load files into the editor.
Also:
dockContent.Show(panel, dockState);
panel = DockPanel Instance
dockState = left, right, document etc.

                var loader = this.editor.CreateLoader(256);
                if (loader == null)
                    throw new ApplicationException("FileLoader bla bla.");

                var cts = new CancellationTokenSource();
                var document = await LoadFileAsync(loader, filePath, cts.Token);
                // Load Text and Release Document

etc..

so, open 10-30 files and close them quickly.
sometime you will get that exception.

i hope i can help

@jacobslusser
Copy link
Owner

I see what's going on... you have a race condition in your application.

If you want to load files on a background thread using the background loader you need to be mindful of when that operation completes. What I think is happening is that you are disposing of the Scintilla control before the load operation completes and sets the document. Said another way, your file doesn't finish loading until after the control is already disposed. Once the Scintilla control is disposed, trying to set the document will attempt to read/write a memory location which is no longer valid and the result is an AccessViolationException.

I would recommend using background threads (async/await) only if you are acutely aware of the challenges in synchronizing threads.

Oh, and be sure you are calling the new SetDestroyHandleBehavior(true) method at the start of your application.

@jacobslusser
Copy link
Owner

I'm going to close this issue for now because the fix was included in the 3.5.4 release. If we need to open it back up again I can.

@HTD
Copy link

HTD commented Oct 19, 2015

Works as charm. Thank you.

jacobslusser added a commit that referenced this issue Jul 24, 2016
The fix for #85, #93, #97 and #123 has been out and vetted by users for
quite a while now and so I'm making it the default behavior. Instead of
calling SetDestroyHandleBehavior(true) to enable it, it is now enabled
by default and can be disabled by calling
SetDestroyHandleBehavior(false).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

8 participants