search

dwmkerr / sharpshell

SharpShell makes it easy to create Windows Shell Extensions using the .NET Framework.
MIT License
1.51k stars 260 forks source link

Explorer crash... again. #233

Closed ElektroStudios closed 5 years ago

ElektroStudios commented 5 years ago

Hi.

I'm sorry to bring bad news but the Explorer crash problem is not solved, I have found a bug that causes Explorer to crash in specific circumstances. I'm not sure whether it is the same bug or different.

Here below I'll explain how to reproduce it, or at least how I think you will be able to reproduce it, this is how my user-interface (my property page) is composed, I don't have any code running in background and the explorer just crash after some time after the property page is initialized...

  1. Add a TableLayoutPanel with 0 columns and 2 rows.
  2. Add a TabControl on the top row.
  3. Add a Panel on the bottom row.
  4. Add a Button inside the Panel.
  5. Add 3 additional tab pages to the TabControl , for a total of 4 tab pages.
  6. Add a ListView in each tab page.
  7. Set the "Dock" property value to "Fill" for: TableLayoutPanel, TabControl, Panel and all the ListViews. Or in other words, for all the controls except the Button.

Result would be something like this:

  1. Compile, register the shell-extension and open/initialize the property page.
  2. Wait like 1 minute or so until the Explorer crash occur. I have the feeling that the crash will occur faster if the property page lose input focus, or if you hide the window by putting another window to foreground, and then you return the input focus to the property page it will hang and finally Explorer will crash.

Originally I just had put one Listview control in the UI (nothing more) and all worked perfect after #222 fix, but now after doing this table layout and tabbed structure I'm having the Explorer crash again. I disabled/commented every line of a method block and method call and everything in my source-code to ensure that nothing in my source-code is causing the crash, the problem is with the changes in the UI, with the controls that I added, I don't know exactly what or why.

Oh my god, how desperate is this thing...

Anyway if needed to analyze, this is the exact source-code that I compiled:

#Region " Option Statements "

Option Strict On
Option Explicit On
Option Infer Off

#End Region

#Region " Imports "

Imports System.ComponentModel
Imports System.Globalization
Imports System.IO
Imports System.Reflection
Imports System.Text
Imports System.Windows.Forms

Imports SharpShell.SharpPropertySheet

#End Region

#Region " PropertyPage "

Partial Public Class PropertyPage : Inherits SharpPropertyPage

#Region " Private Fields "

    Private filePath As String
    Private currentListView As ListView

    Private ReadOnly lvItemsProductDict As New Dictionary(Of String, ListViewItem)(20, StringComparer.OrdinalIgnoreCase) From {
            {"File Name", New ListViewItem("File Name")},
            {"Original Name", New ListViewItem("Original Name")},
            {"Product Name", New ListViewItem("Product Name")},
            {"Product Description", New ListViewItem("Product Description")},
            {"Company Name", New ListViewItem("Company Name")},
            {"Copyright", New ListViewItem("Copyright")},
            {"File Version", New ListViewItem("File Version")},
            {"Product Version", New ListViewItem("Product Version")},
            {"Language", New ListViewItem("Language")}
        }

    Private ReadOnly lvItemsCompilationDict As New Dictionary(Of String, ListViewItem)(20, StringComparer.OrdinalIgnoreCase) From {
            {"PE File Kind", New ListViewItem("PE File Kind")},
            {"Platform", New ListViewItem("Platform")},
            {"Build Configuration", New ListViewItem("Build Configuration")},
            {"CLR Version", New ListViewItem("CLR Version")},
            {"Target .NET Framework", New ListViewItem("Target .NET Framework")},
            {"Entry-Point Address", New ListViewItem("Entry-Point Address")},
            {"Root Namespace", New ListViewItem("Root Namespace")},
            {"Is Strong-Name Signed", New ListViewItem("Is Strong-Name Signed")},
            {"Public Key Token", New ListViewItem("Public Key Token")},
            {"Compilation TimeStamp", New ListViewItem("Compilation TimeStamp")}
        }

    Private ReadOnly lvItemsAttributesDict As New Dictionary(Of String, ListViewItem)(20, StringComparer.OrdinalIgnoreCase) From {
            {"Dll Import Search Paths", New ListViewItem("Dll Import Search Paths")},
            {"Is CLS Compliant", New ListViewItem("Is CLS Compliant")},
            {"Is COM Visible", New ListViewItem("Is COM Visible")},
            {"Is Installed in GAC", New ListViewItem("Is Installed in GAC")},
            {"Security Permission", New ListViewItem("Security Permission")}
        }

    Private ReadOnly lvItemsUniqueIdsDict As New Dictionary(Of String, ListViewItem)(20, StringComparer.OrdinalIgnoreCase) From {
            {"Assembly GUID", New ListViewItem("Assembly GUID")},
            {"MD5 Hash", New ListViewItem("MD5 Hash")},
            {"SHA-1 Hash", New ListViewItem("SHA-1 Hash")},
            {"SHA-256 Hash", New ListViewItem("SHA-256 Hash")}
        }

#End Region

#Region " Constructors "

    Public Sub New()
        MyClass.InitializeComponent()

        Me.Name = My.Application.Info.AssemblyName
        Me.PageTitle = Me.Name
        ' Me.PageIcon = My.Resources.Visual_Studio
    End Sub

#End Region

#Region " Event Invocators "

    Protected Overrides Sub OnPropertyPageInitialised(ByVal e As SharpPropertySheet)
        Me.filePath = e.SelectedItemPaths.Single()
        MyBase.OnPropertyPageInitialised(e)
    End Sub

#End Region

#Region " Event Handlers "

    Private Sub TabControlListViews_SelectedIndexChanged(ByVal sender As Object, ByVal e As EventArgs) Handles TabControlListViews.SelectedIndexChanged
    End Sub

    Private Sub CopyToolStripMenuItem_Click(ByVal sender As Object, ByVal e As EventArgs) Handles CopyToolStripMenuItem.Click
    End Sub

    Private Sub ButtonSave_Click(sender As Object, e As EventArgs) Handles ButtonSave.Click
    End Sub

#End Region

#Region " Private Methods "

    Private Sub LoadAssemblyInfo()
    End Sub

    Private Sub LoadListViewItems(lv As ListView)
    End Sub

    Private Sub SaveAsTextFile(filePath As String)
    End Sub

#End Region

End Class

#End Region
dwmkerr commented 5 years ago

Can you provide a copy of the logs @ElektroStudios?

ElektroStudios commented 5 years ago

@dwmkerr

Sorry but which logs do you mean?, where are stored or how do I generate them?. The only thing I can give by the moment is what the WER event says: "EventId: 1002 Source: Application Hang". It says that Explorer closed (crashed) due a hang (that we know it was caused by something related to the shell-extension). It does not say anything about an exception occured, curious, so its a hang and not an exception.

@Countryen

If you read this, please, could you share with me a link to see the answer that you gave me days ago on which you explained me how to manage logging after registered the shell-extension? I remember you mentioned a program to do so. I really can't find your answer in the topics/issues that I opened, maybe it was deleted or something else?.

dwmkerr commented 5 years ago

Hi @ElektroStudios,

Here's the docs on how to log:

https://github.com/dwmkerr/sharpshell/blob/master/docs/logging/logging.md

If you can enable file logging, then run the operation to failure, then include the logs, that'd be great!

ElektroStudios commented 5 years ago

Here is the log but it seem nothing special has been logged about the problem:

One silly question: the reference count logged here it is right?.

2018-11-05 07:03:03.230Z - ServerManager - SharpShell Diagnostics Initialised. Process ServerManager.
2018-11-05 07:03:03.233Z - ServerManager - .NET Assembly Info: Constructing property sheet.
2018-11-05 07:03:07.225Z - ServerManager - Preparing to register SharpShell Server PropertySheet as OS64Bit
2018-11-05 07:03:07.230Z - ServerManager - Registration of PropertySheet completed
2018-11-05 07:03:09.406Z - explorer - SharpShell Diagnostics Initialised. Process explorer.
2018-11-05 07:03:09.409Z - explorer - .NET Assembly Info: Constructing property sheet.
2018-11-05 07:03:09.412Z - explorer - .NET Assembly Info: Initializing shell extension...
2018-11-05 07:03:09.416Z - explorer - .NET Assembly Info: Shell extension initialised.
Parent folder: <none>
Items: 
P:\-=Temporal=-\Visual Studio Projects\NET Assembly Info ShellEx\NET Assembly Info ShellEx\bin\Release\AssemblyInfo.dll
2018-11-05 07:03:09.421Z - explorer - .NET Assembly Info: Adding Pages...
2018-11-05 07:03:09.426Z - explorer - NativeBridge: Preparing to load 'SharpShell.NativeBridge.SharpShellNativeBridge64.dll' into 'C:\Users\Administrador\AppData\Local\Temp\12b94bd1-8e2d-4125-84bd-41ab2b7c710d.dll'...
2018-11-05 07:03:09.439Z - explorer - NativeBridge: Loaded bridge successfully
2018-11-05 07:03:09.500Z - explorer - .NET Assembly Info (Proxy 00000000 for 'AssemblyInfo' Page): Creating property page handle via bridge.
2018-11-05 07:03:09.505Z - explorer - .NET Assembly Info (Proxy 00000000 for 'AssemblyInfo' Page): Add Internal Ref 0 -> 1
2018-11-05 07:03:09.507Z - explorer - .NET Assembly Info (Proxy 00000000 for 'AssemblyInfo' Page): IShellPropSheetExt: Add Ref 3 -> 4
2018-11-05 07:03:09.509Z - explorer - .NET Assembly Info: Created Page Proxy, handle is 0a7f9960
2018-11-05 07:03:09.511Z - explorer - .NET Assembly Info: Adding Pages (Done)
2018-11-05 07:03:11.386Z - explorer - .NET Assembly Info (Proxy 0a7f9960 for 'AssemblyInfo' Page): Create Callback
2018-11-05 07:03:11.390Z - explorer - .NET Assembly Info ('AssemblyInfo' Page): Page initialised
2018-11-05 07:03:11.394Z - explorer - .NET Assembly Info ('AssemblyInfo' Page): Page activated

---> AT THIS POINT I CLICKED INSIDE THE PROPERTY PAGE, I MEAN, THE DIALOG WINDOW HAS INPUT FOCUS,
     I SET TO FOREGROUND ANOTHER (WHICHEVER) WINDOW, SO THE PROPERTY PAGE LOSE FOCUS,  
     I WAIT AROUND 20-30 SECS AND THEN I CLICK AGAIN IN THE PROPERTY PAGE TO GIVE INPUT FOCUS
     BUT THE DIALOG WINDOW HANGS AND FINALLY EXPLORER CRASH.
     and I proceed to unregister the extension as logged at the next lines...

2018-11-05 07:03:49.726Z - ServerManager - Preparing to unregister SharpShell Server PropertySheet as OS64Bit
2018-11-05 07:03:49.730Z - ServerManager - Unregistration of PropertySheet completed
dwmkerr commented 5 years ago

OK, looks tricky. When I get home I'm going to share a way to get better debugging information, will update shortly

ElektroStudios commented 5 years ago

Any news to this problem?.

I discovered that the problem will only occur when you put a control inside the control collection of a TabPage (of a TabControl). Note that the hang / Explorer crash will NOT occur if the TabControl is empty (I mean, if there is no control added into the control collection of their tab pages).

I didn't tested what would happen if I replace the TabControl for a Panel, and then I add a control inside the control collection of the Panel.

New improved steps to reproduce the problem:

  1. Add a TabControl, and insert any control into the first tab page (like a Button, or a ListView).
  2. Compile, register the shell-extension and open/initialize the property page.
  3. Give mouse input to the control that you added. Just click in the control / Button / ListView.
  4. Make the property page lose input focus, just by clicking outside the window, or putting another window above in the Z window order.
  5. Go back to the property page window, now it should be hanged.

A demonstration:

dwmkerr commented 5 years ago

Great, thanks for the detailed notes! I'll be on this shortly, I'm just trying to close the registration issues. I think that for the registration issues I have finished the tests which demonstrate the failure:

https://github.com/dwmkerr/sharpshell/pull/240/files#diff-2b093ddbaab14cd3a857fabd7130bac6R95

Although interestingly I cannot actually get the error to manifest on my machine. When I've got a fix, would you be able to retest the code before I merge it? Then I'll get back onto this.

ElektroStudios commented 5 years ago

When I've got a fix, would you be able to retest the code before I merge it? Then I'll get back onto this.

Yes, of course. And good luck fixing the registration issues, and this.

dwmkerr commented 5 years ago

Thanks @ElektroStudios - there's a fix ready to test at #240 now

ElektroStudios commented 5 years ago

Confirmed that the fix for the registration issue is working (at least in my O.S., I insist). Thank you for the fix.

Now the difficult part remains: the "intermittent" Explorer hang/crash ~ part two

I really hope if you can manifest (and fix) the problem on the operating systems on which you are doing your tests.

Cheers!

dwmkerr commented 5 years ago

Thanks @ElektroStudios, this is the next issue I'll get onto 😄

Countryen commented 5 years ago

Hey all, I can verify/reproduce, problem happens on my system (win10x64 as always), too. Created my Server using precompiled SharpShell v2.7.0 with a form as stated here: https://github.com/dwmkerr/sharpshell/issues/233#issuecomment-437516334

Countryen commented 5 years ago

Well after reproducing it I digged a bit and maybe found something.

PropertyPageProxy is using the Method WindowProc to communicate between the shell and the UI. So, I inspected the executing calls to this method and made note of the sent uMessage's. -> Using Debug > Attach to Process > explorer.exe

I noticed that the uMessage that hits the WindowProcwhen leaving the Property Sheet with a focus on the inner TextBox (I used a TextBox inside the TabControl) is 6 (0x0006).

6 (aka. WM_ACTIVATE) wasn't handled yet, so I just added a case -> picture: image

I just tried it with Target.Focus() to just "steal" the focus from the inner control right before leaving the Property Sheet, and for me this stops the crashing.

I used information about WM_ACTIVATE from here: MSDN

Note: If used, maybe needs to be checked if activated/deactivated via L/W according to MSDN.

(Oh yeah, I used the SharpShell@master/trunk, compiled by myself (Windows SDK 10), but w/o the change I can still reproduce the crashing issue).

Inform me if this helped you in any shape or form.

ElektroStudios commented 5 years ago

Hi. Thanks for comment @Countryen. I've downloaded source-code of SharpShell 2.7.0 I added the next case in the WindowProc of PropertyPageProxy class

case WindowsMessages.WM_ACTIVATE:
   // if (!Target.Focused) {
        Target.Focus();
   //}
   break;

and I used the resulting SharpShell.dll with this change applied to compile my shell-extension, but the problem persists, same problem as in the GIF image that I shared, a hang occurs on the property sheet window, and finally a Explorer crash.

Countryen commented 5 years ago

@ElektroStudios You could list which uMessage's are sent when the crash occurs - maybe it's different for you. I will try with a different Control inside the TabControl (like in your GIF).

Countryen commented 5 years ago

Okay just added a ListViewand it still works (and crashes without my modification). Does it only happen when the Control is inside a TabControl? Or with other "Boxes", too?

ElektroStudios commented 5 years ago

Before listing uMessages, here is my full VS solution using the SharpShell.dll with the WM_Activation case applied:

( the shell-extension to be registered is the "AssemblyInfo.dll" file inside the "bin\Release" folder. )

maybe could you please check whether it is working for you without any hang/crash?.

dwmkerr commented 5 years ago

This is looking awesome guys, I think we are very close. I will jump in on this one too, just slammed today tomorrow so might be a little behind. I'm trying in the meantime to see if I can find official docs which reference whether certain winproc messages must be handled to safely implement the prop sheet

Countryen commented 5 years ago

@ElektroStudios Downloaded your solution and built it with my SharpShell v2.7 WITH my modification for WM_ACTIVATE and it runs perfectly fine. Without the Target.Focus() call it crashes/hangs as expected.

This is my log output when using Target.Focus() on leaving the property sheet after focusing the TabControl:

2018-11-13 19:10:43.346Z - explorer - .NET Assembly Info (Proxy 1dc35ee0 for 'AssemblyInfo' Page): H:590506, U:6, W:0, L:0
2018-11-13 19:10:43.379Z - explorer - .NET Assembly Info (Proxy 1dc35ee0 for 'AssemblyInfo' Page): Target.Focus() on WM_ACTIVATE
2018-11-13 19:10:43.390Z - explorer - .NET Assembly Info (Proxy 1dc35ee0 for 'AssemblyInfo' Page): H:590506, U:28, W:0, L:18716

And this is it w/o the Target.Focus() call:

2018-11-13 19:12:31.288Z - explorer - .NET Assembly Info (Proxy 1dea7640 for 'AssemblyInfo' Page): H:4852368, U:6, W:0, L:0

For some reason the second WindowProc is not hit (28, WM_ACTIVATEAPP)

Note when logging this you will see multiple uMessage 32 when hovering the Controls. I omitted these.

ElektroStudios commented 5 years ago

@Countryen

( please next time if you can share the code rather than a picture, to be able copy it as is. )

Ok so I have used this, which is the exact code that you shared:

private IntPtr WindowProc(IntPtr hWnd, uint uMessage, IntPtr wParam, IntPtr lParam)
{
    Log($"H:{hWnd}, U:{uMessage}, W: {wParam}, L: {lParam}");
    switch (uMessage)
    {
        case WindowsMessages.WM_ACTIVATE:
            Target.Focus();
            Log("Target.Focus() on WM_ACTIVATE");
            break;
...

Ok, now listen to this weird facts that I discovered:

@Countryen It's evident that you can always reproduce the same issues as I can, and for that reason I'm sure you will be able to reproduce this. Just remove the lines that you used to call "Log", and it will hang again, add the calls to "Log", and it will not hang anymore, no matter how absurd it sounds, it's like that, I've tested several builds for many minutes to be sure about.

In other words, this will NOT fix the problem:

private IntPtr WindowProc(IntPtr hWnd, uint uMessage, IntPtr wParam, IntPtr lParam)
{
    switch (uMessage)
    {
        case WindowsMessages.WM_ACTIVATE:
            Target.Focus();
            break;
...

but this other, will fix it:

private IntPtr WindowProc(IntPtr hWnd, uint uMessage, IntPtr wParam, IntPtr lParam)
{
    Log($"H:{hWnd}, U:{uMessage}, W: {wParam}, L: {lParam}");
    switch (uMessage)
    {
        case WindowsMessages.WM_ACTIVATE:
            Target.Focus();
            Log("Target.Focus() on WM_ACTIVATE");
            break;
...

I think that some kind of race condition bug could be happening here, that with the timing added by the "Log" calls, it fixes it (but that is not a proper solution), but of course I could be wrong, for me it could be a thousand things, but @dwmkerr is the gurú about shell-extension internals.

Countryen commented 5 years ago

@ElektroStudios You don't think I tested that^^? I was sure that the logging does not infere - because for me it doesnt. I can remove all the logging (just did) and it still works with your Server / Property Sheet.

As soon as I remove Target.Focus() it crashes

(But next time I'll include the snippet for extra clarity.)

I agree that my "solution" is rather magically and the actual problem is probably not the missing Target.Focus() / Log call.

@dwmkerr I researched some and inspected others Property Sheets (example from Microsoft and TortoiseGit (which I use frequently) for example) and could not find any difference in using WindowProc than the PropertyPageProxy is. They don't react to neiter 0x0006 nor 0x001C (28).

But yet again, they are not managed and there is little to none information about Managed Property Page Servers out there.

@ElektroStudios can you confirm that it happens with other Controls (maybe multiple layers), too? Maybe some System.Winddows.Forms Controls work and help searching for the actual issue here. I will try some myself. too. Maybe then we can just use different Components/Controls instead until the issue is fixed completely.

ElektroStudios commented 5 years ago

@ElektroStudios You don't think I tested that^^? I was sure that the logging does not infere - because for me it doesnt. I can remove all the logging (just did) and it still works with your Server / Property Sheet.

Oh, then... sorry, this leaves me without words. The problem is already very strange and weird for me to see that when I add the calls to "Log" method the hang never (NEVER) occurs, but when I remove them, the hang occurs as always following the same steps of the GIF image above.

EDIT: by the way, when compiling NativeBridge of SharpShell 2.7 did you used Windows SDK 8.1, or 10.0?, I used (only) 8.1 to test your fix.

It's very tedious to test this thing. Now I will test a couple of builds doing changes with the WM_ACTIVATE and other window messages to see if I can find any clues that can help me to solve this. I think tomorrow I will test your suggested fix with other controls. If you have any suggestion of a specific container control just say it.

Thanks for read.

Countryen commented 5 years ago

@ElektroStudios Windows SDK v10.0.17134.0

ElektroStudios commented 5 years ago

This is fixing my problem:

private IntPtr WindowProc(IntPtr hWnd, uint uMessage, IntPtr wParam, IntPtr lParam)
{
    switch (uMessage)
    {
        case WindowsMessages.WM_ACTIVATE:
            return new IntPtr(-1);

return a value different than zero instead of returning zero as the Microsoft docs says. It is working for me, the window is activated and no hang will occur anymore, does not matter If I activate the window by a mouse-click or by ALT+TAB.

I also keep an eye on WM_MOUSEACTIVATE message. Before discovering the workaround above, I tried to return all available values (1, 2, 3 and 4) when processing WM_MOUSEACTIVATE message, but it always hangs. I need to test this with other controls as suggested by @Countryen

Countryen commented 5 years ago

...instead of returning zero as the Microsoft docs says. It is working for me, the Window is being activated, and no hang will occur anymore.

Same for me.

Also I looked at others examples, they don't return IntPtr, but (I guess) boolean or other BOOL CGitPropertyPage::PageProc (HWND /*hwnd*/, UINT uMessage, WPARAM wParam, LPARAM lParam) https://github.com/TortoiseGit/TortoiseGit/blob/master/src/TortoiseShell/GITPropertyPage.cpp

Researching I found that some Windows.Forms Controls also react to WndProc https://docs.microsoft.com/en-us/dotnet/api/system.windows.forms.control?view=netframework-4.7.2 https://docs.microsoft.com/en-us/dotnet/api/system.windows.forms.scrollablecontrol?view=netframework-4.7.2 https://docs.microsoft.com/en-us/dotnet/api/system.windows.forms.containercontrol?redirectedfrom=MSDN&view=netframework-4.7.2

Using method protected virtual void Component.WndProc (ref System.Windows.Forms.Message m) with a Message

Maybe we can inspect how .NET is handling messages and learn from that.

Countryen commented 5 years ago

Okay, I now tested various different Controls. Result: Only if I nest a Control inside a TabControl it crashes. GroupBoxes, FlowLayoutPanel, Panel, etc. all fine with nested controls - just the TabControl is bad for me. Even multi-nesting works fine. I am curious what your result will be.

(Tested w/o any modifcation to the PropertyPageProxy)

So I guess we can focus on the actual TabControl and it's difference. Maybe because the actual Property Sheet is already a Tab?

For now I'd suggest just building a custom TabControl with panels and buttons or other Controls as a workaround.

Note: If the result is the same for ElektroStudios it would be good to add "TabControl" that to the title of the issue - for future reference.

ElektroStudios commented 5 years ago

Sorry for the delay.

Okay, I now tested various different Controls. Result: Only if I nest a Control inside a TabControl it crashes. GroupBoxes, FlowLayoutPanel, Panel, etc. all fine with nested controls - just the TabControl is bad for me. Even multi-nesting works fine. I am curious what your result will be.

(Tested w/o any modifcation to the PropertyPageProxy)

Same experience here. It only crashes when adding a control inside a tabpage of a tabcontrol. I tested groupboxes, panels, flowlayoutpanel and with multi-nesting too.

In short, the results are the same for me.

ElektroStudios commented 5 years ago

I sub-classed the default TabControl class to override the window procedure (WndProc) to try investigate what window message is sent to the tabcontrol's window exactly at the moment before the hang/crash. I have these two candidates which could be very related with the hang:

WM_IME_SETCONTEXT (0x281)

Sent to an application when a window is activated.

WM_GETDLGCODE (0x87)

Sent to the window procedure associated with a control. By default, the system handles all keyboard input to the control; the system interprets certain types of keyboard input as dialog box navigation keys. To override this default behavior, the control can respond to the WM_GETDLGCODE message to indicate the types of input it wants to process itself.

I hope this info could be helpful to debug it. I'll try too some things with this info.

EDIT: ok I think WM_GETDLGCODE could be the issue, after the tabcontrol window lose focus and until it get focus again, the window gets crazy receiving that message repeteadly and non stopping until the hang occurs, with lparam=0 wparam=0 and return value=0...

If helpful, note that the message sent just before WM_GETDLGCODE is OCM_DRAWITEM (0x202b) / WM_DRAWITEM (0x002b)

ElektroStudios commented 5 years ago

I have come to the simplest and easiest fix that does not imply to fix SharpShell source-code, however, SharpShell source-code should be updated to fix this because we could not depend on sub-classing controls to avoid issues like this...

The temporal solution is to set the ControlStyles.ContainerControl like in the example below:

Imports System.Windows.Forms

Public Class TabControlFix : Inherits TabControl

    Public Sub New()
        MyBase.SetStyle(ControlStyles.ContainerControl, True)
    End Sub

End Class

Don't ask me why it works, I just was testing control styles and noticed that enabling that one the problem is gone, no more hangs.

This discovery also helps to determine the difference about why the hang does not occur with other container controls like a panel, a groupbox or a tablelayoutpanel, because the tabcontrol "is not" a container control, while the others are (their ControlStyles.ContainerControl flag is True).

Countryen commented 5 years ago

because the tabcontrol "is not" a container control, while the others are (their ControlStyles.ContainerControl flag is True).

Weird to me.

Yes seems like the best workaround right now. I am glad we've got the same result for once ;)

Maybe dwmkerr knows more about the WindowProc's and how to fully fix this in the SharpShell code. For a simple fix the SharpPropertyPage could probably set the style of all containing TabControls by itself. Or it should deny usage of TabControls until it's fixed.

ElektroStudios commented 5 years ago

For a simple fix the SharpPropertyPage could probably set the style of all containing TabControls by itself. Or it should deny usage of TabControls until it's fixed.

In case of there is no other better way to fix it, then I think it would be easy for @dwmkerr to determine a TabControl window by its class name which is "SysTabControl32", and then apply a control style/window extended style or whatever other thing to do with that window to fix this issue, however, what I really want to say is that I think any solution should care about custom user controls and/or 3rd party controls too, that maybe are not a "SysTabControl32" but maybe could have implemented the same factors that makes a TabControl hangs a property sheet.

Anyway, and just to provide more detailed info: I'm checking the .NET Framework class library reference source and what it does when specifiying the ControlStyles.ContainerControl style is apply the WS_EX_CONTROLPARENT (0x00010000L) extended window style flag to the current window:

if (GetStyle(ControlStyles.ContainerControl)) {
    cp.ExStyle |= NativeMethods.WS_EX_CONTROLPARENT;
}
Countryen commented 5 years ago

Hey @ElektroStudios did you delete your last comment about "bad testing"? I've got an e-mail but can't find the comment on here.

Anyways, I just wanted to ask something - not directly targeting the issue, maybe just for some clarification / information in general (I am curious):

(Note: I know little about Win32 / native Windows / MFC programming)

ElektroStudios commented 5 years ago

@Countryen

Yes I deleted it because I was erroneous about what I said in that comment then it only could had bring confusion. I wrote that deleted message because I was testing my shell-extension and by mistake I set the value of ControlStyles.ContainerControl to False instead of True, I didn't noticed it, so I got a hang and then I thought the TabControlFix was not working and I published that deleted message, but I was wrong, I can confirm TabControlFix is working fine to solve this issue. And for you, did you tested the TabControlFix class in C#?.

* Why does every Control handle WndProc?

Because it is inherited from the base class: System.Windows.Forms.Control.

In case of what you really are asking is "why every control processes window messages?": because all WinForms controls (everything, including a Form) are in essence a win32 window...

From Wikipedia:

In Win32 application programming, WindowProc (or window procedure) is a user-defined callback function that processes messages sent to a window. This function is specified when an application registers its window class and can be named anything (not necessarily WindowProc).

The window procedure is responsible for handling all messages that are sent to a window. The function prototype of WindowProc is given by:

From microsoft.docs:

Windows-based applications are event-driven. They do not make explicit function calls (such as C run-time library calls) to obtain input. Instead, they wait for the system to pass input to them.

The system passes all input for an application to the various windows in the application. Each window has a function, called a window procedure, that the system calls whenever it has input for the window. The window procedure processes the input and returns control to the system.

Windows Messages

The system passes input to a window procedure in the form of a message. Messages are generated by both the system and applications. The system generates a message at each input event—for example, when the user types, moves the mouse, or clicks a control such as a scroll bar. The system also generates messages in response to changes in the system brought about by an application, such as when an application changes the pool of system font resources or resizes one of its windows. An application can generate messages to direct its own windows to perform tasks or to communicate with windows in other applications.

The system sends a message to a window procedure with a set of four parameters: a window handle, a message identifier, and two values called message parameters. The window handle identifies the window for which the message is intended. The system uses it to determine which window procedure should receive the message.

A message identifier is a named constant that identifies the purpose of a message. When a window procedure receives a message, it uses a message identifier to determine how to process the message. For example, the message identifier WM_PAINT tells the window procedure that the window's client area has changed and must be repainted.

Message parameters specify data or the location of data used by a window procedure when processing a message. The meaning and value of the message parameters depend on the message. 

* Doesn't only the "Window" (in our case the System.Windows.Forms.Form) handle WindowProc?

No. As mentioned, all WinForms controls (everything, including a Form) are in essence a win32 window, they just have different "shapes" (different visual representation and different functionalities), but all windows has a default window procedure, so all controls has a WndProc (and DefWndProc) method, inherited from the already mentioned base class: System.Windows.Forms.Control.

Note that what I explained is not the same case for WPF controls, from what I know they even does not expose a window handle by default, but they can arbitrary do it for interoperability purposes between Win32 / WinFroms and WPF.

* Does the "Window" just call the WndProc of the Controls but it's actually inside the WindowProc?
* Does every Control (and even nested ones) get a WindowProc / WndProc call?
* Or is that only true with TabControls?

The three questions can be answered with:

Every window (every form and control, regardless of the window hierarchy / multi-nesting) receives and processes its own window messages.

I hope this information has been helpful to you!.

Countryen commented 5 years ago

@ElektroStudios Okay thanks for the info about the deleted post. No haven't tried it yet, will in a bit.

Thanks also for the insight on Windows Controls "Windows". A bit weird to call a "Text Box" a "Window" I guess, but it's "Windows" after all :D I wonder how WPF handles it then.

Countryen commented 5 years ago

@ElektroStudios Okay, tried your fix from https://github.com/dwmkerr/sharpshell/issues/233#issuecomment-439649406 now.

Works fine for me. Same logging, constantly Message: 0x087 w/o the fix/subclass.

dwmkerr commented 5 years ago

Just trying to catch up here @ElektroStudios and @Countryen, can you let me know:

  1. Does handling the message fix the issue (i.e. can I fix it with another handler in the SharpShell code)
  2. Or is it a case that we have to make sure we change the style of tab controls (in which case I can mitigate a little with docs/FAQ, and then aim to maybe automatically check for tabs and fix them)

I am worried about changing things dynamically in controls, because it doesn't cover scenarios where clients dynamically create interfaces. But I might have missed some details from the conversation!

ElektroStudios commented 5 years ago 
1. Does handling the message fix the issue (i.e. can I fix it with another handler in the SharpShell code)

2. Or is it a case that we have to make sure we change the style of tab controls (in which case I can mitigate a little with docs/FAQ, and then aim to maybe automatically check for tabs and fix them)

In resume, the thing is that any of both methodologies fixes the issue. I'm not sure in which way the WM_ACTIVATE can be related to WS_EX_CONTROLPARENT extended window style in the meaning that when that window style is not applied to a tab-control, it makes the property sheet hang (explorer crash) when WM_ACTIVATE is received. Maybe the root of the problem could be other different thing than the WM_ACTIVATE message or WS_EX_CONTROLPARENT style that we just didn't noticed, but these are the two facts or solutions that we have in clear:

  1. Handling WM_ACTIVATE message and returning a non-zero value for that message in the WindowProc function of the PropertyPageProxy class, it fixes the issue. But this message handling only would be needed if a tab-control is added in the UI, because that is the control that causes the property sheet to hang. We didn't noticed any other control to cause a hang/crash.

  2. Applying the WS_EX_CONTROLPARENT style to the tab control(s) of the property sheet UI, it fixes the issue.

That's all.

In my personal opinion, and if "automatically check for tabs and fix them" is not viable at all for any reason do you have, then... see:

While I think maybe the first solution could be considered an overkill since handling WM_ACTIVATE message and returning a non-zero value it only has sense to prevent a hang when a tab control is added in the property sheet UI, however, I think it does not have any unwanted effect, I mean, I think it could be safe to handle WM_ACTIVATE message and returning a non-zero value regardless of whether it is an overkill solution or it is not. It should really matter a lot that additional message handling in SharpShell source-code if that is the only (and most direct) found solution by the moment?. At least that way it would avoid end-user interaction to force them to sub-class any TabControl to set it's WS_EX_CONTROLPARENT style (as shown in the VB.NET sample code above).

I think what you've said about "automatically check for tabs and fix them" it would be the proper, safest solution. But I'm just saying that handling WM_ACTIVATE message (and returning a non-zero value, I insist) fixes it too and so that alternative solution would be easier to implement for you. For me it really does not matter what solution you will choose at the end, but I'm a little "afraid" if handling WM_ACTIVATE message to return a non-zero value could have undesired effects that we didn't noticed yet in other scenarios.

Thanks for read.

dwmkerr commented 5 years ago

Thanks @ElektroStudios - I've raised #249, if it looks sane to you let me know and I'll merge and release!

ElektroStudios commented 5 years ago

@dwmkerr yes, it looks "sane". As far @Countryen and I have experienced, that is the only required modification in the SharpShell source-code to fix the issue.

Thanks for your support and, if you would like to close this issue because now it is solved.