View Issue Details

IDProjectCategoryView StatusLast Update
0001126OpenMPT[All Projects] Accessibilitypublic2018-11-13 07:50
ReporterherodotasAssigned ToSaga Musix 
PriorityhighSeverityblockReproducibilityalways
Status resolvedResolutionfixed 
Platformx64OSWindowsOS Version7
Product VersionOpenMPT 1.28.00.* (current testing) 
Target VersionOpenMPT 1.28.01.00 (goals)Fixed in VersionOpenMPT 1.28.01.00 (goals) 
Summary0001126: Using DDE to open files from the shell when no OpenMPT instance is running is problematic
Description

When loading archived module, just getting annoying error message. It is not critical, module loading and playing normally. Picture included.

TagsNo tags attached.
Has the bug occurred in previous versions?No, just in 1.28.*
Tested code revision (in case you know it)

Activities

herodotas

herodotas

2018-06-01 17:07

reporter  

mptm.jpg (323,001 bytes)
Saga Musix

Saga Musix

2018-06-01 17:09

administrator   ~0003540

This message comes from Windows, not OpenMPT. I also noticed it starting to happen recently, starting at about the same time as a Windows update happened. Something in Windows' DDE handling might have changed, because I don't think there were any changes in the early OpenMPT setup code that would be run until this message is shown.

herodotas

herodotas

2018-06-01 17:16

reporter   ~0003541

But with 1.27.08 this message dont appear.

Saga Musix

Saga Musix

2018-06-01 17:23

administrator   ~0003542

Last edited: 2018-06-01 18:23

View 2 revisions

Seems to have started somewhere between r10065 (exclusive) and r10159 (inclusive). revisions inbetween are sadly not available on buildbot.

Likely candidate: GDI+ image support was added in r10067 and may cause initialization to take longer than what Windows is willing to wait for.

Saga Musix

Saga Musix

2018-06-01 22:02

administrator   ~0003543

As I suspected, GDI+ initialization might be at least part of the issue. Starting from r10077, we use GDI+ and that's where the issue started happening. The issue is not new (it could happen at random before), it is just more pronounced when having to load GDI+. We might have to create a window that accepts DDE messages way earlier in the intialization code.

manx

manx

2018-06-02 06:14

administrator   ~0003544

Creating anything DDE accepting earlier cannot possibly react to DDE messages earlier unless we actually start a thread doing that (which would open another can of worms, like the requirement to marshal DDE requests back to the main thread (probably SendMessage) or deal with initialization race conditions (blocking on SendMessage during initialization) or dealing with windows going bonkers because the first window we create is not in the main thread). The main thread is busy initializing ourselves and cannot react to any DDE message before it actually starts running the message loop.
The solution may be to completely stop using DDE because it is broken.

https://blogs.msdn.microsoft.com/oldnewthing/20070226-00/?p=27863
https://blogs.msdn.microsoft.com/oldnewthing/20100503-00/?p=14183
https://msdn.microsoft.com/en-us/library/cc144175.aspx

Sadly, IDropTarget also comes with some amount of development cost and its own can of worms (maintaining a correctly behaving COM local server). If we do not want that, we could also roll our own solution (either FindWindow+SendMessage to a running instance, or registering named synchronization facilities for communication with the running instance (a named pipe for example).
FindWindow is probably easier to implement, and a named pipe may be easier to get close to race-free.

There are 2 aspects to the whole problem:

  1. The way the shell tries to to open files:

    • DDE
    • IDropTarget in shell process
    • IDropTarget local server
    • custom commandline argument to the main exe
    • custom commandline argument to a launcher exe
  2. The way the list of files to open is transferred to the running instance:

    • DDE in main thread
    • DDE in separate thread (communicate with main thread via SendMessage)
    • IDropTarget local server
    • FindWindow/SendMessage to main window
    • FindWindow/SendMessage to handler window in separate thread (communicate with main thread via SendMessage)
    • named pipe polled in main thread
    • named pipe reading in separate thread (communicate with main thread via SendMessage)

I have no idea whatsoever which solution would be the best. This needs further discussion.

Saga Musix

Saga Musix

2018-06-02 10:54

administrator   ~0003545

Either way, the current behaviour is not acceptable and for me is a blocker for an 1.28 release, not a long-term goal.

manx

manx

2018-06-02 11:00

administrator   ~0003546

Fair enough, I just do not think this would be easily solvable.

manx

manx

2018-06-07 09:11

administrator   ~0003547

Regarding all options involving finding the main window from a separate process via FindWindow and sending file open command via SendMessage: We cannot actually easily use the main window for this, because MFC registers the main window class with some internal MFC window class name that is not really useful. Overriding this class name is possible, but feels very hack-ish and unsupported:
https://www.codeproject.com/Articles/196354/Provide-Your-Custom-Class-Name-to-your-MFC-Applica
https://stackoverflow.com/questions/29746215/mfc-random-name-and-class-name-of-main-window-process

So, this probably demands a hidden message-only window used specifically for IPC.

Saga Musix

Saga Musix

2018-06-25 21:21

administrator   ~0003568

A first attempt at using a hidden window (not cleaned up).



ddeReplace.patch (3,624 bytes)
Index: Mptrack.cpp
===================================================================
--- Mptrack.cpp	(revision 10509)
+++ Mptrack.cpp	(working copy)
@@ -58,7 +58,22 @@
 
 OPENMPT_NAMESPACE_BEGIN
 
+static const TCHAR IPCClassName[] = _T("OpenMPT_IPC_Wnd");
 
+static LRESULT CALLBACK IPCWindowProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
+{
+	if(uMsg == WM_COPYDATA)
+	{
+		const auto &copyData = *reinterpret_cast<const COPYDATASTRUCT *>(lParam);
+		const auto str = static_cast<const WCHAR *>(copyData.lpData);
+		const std::wstring name(str, str + copyData.dwData / sizeof(WCHAR));
+		theApp.GetModDocTemplate()->OpenDocumentFile(mpt::PathString::FromWide(name).AsNative().c_str());
+		return TRUE;
+	}
+	return ::DefWindowProc(hwnd, uMsg, wParam, lParam);
+}
+
+
 /////////////////////////////////////////////////////////////////////////////
 // The one and only CTrackApp object
 
@@ -130,6 +145,7 @@
 class CMPTCommandLineInfo: public CCommandLineInfo
 {
 public:
+	std::vector<mpt::PathString> m_fileNames;
 	bool m_bNoDls = false, m_bNoPlugins = false, m_bNoAssembly = false, m_bNoSysCheck = false, m_bNoWine = false,
 		 m_bPortable = false, m_bNoCrashHandler = false, m_bDebugCrashHandler = false;
 #ifdef ENABLE_TESTS
@@ -139,7 +155,7 @@
 public:
 	void ParseParam(LPCTSTR lpszParam, BOOL bFlag, BOOL bLast) override
 	{
-		if ((lpszParam) && (bFlag))
+		if(lpszParam && bFlag)
 		{
 			if (!lstrcmpi(lpszParam, _T("nologo"))) { m_bShowSplash = FALSE; return; }
 			if (!lstrcmpi(lpszParam, _T("nodls"))) { m_bNoDls = true; return; }
@@ -154,6 +170,10 @@
 #ifdef ENABLE_TESTS
 			if (!lstrcmpi(lpszParam, _T("noTests"))) { m_bNoTests = true; return; }
 #endif
+		} else if(lpszParam && !bFlag)
+		{
+			m_fileNames.push_back(mpt::PathString::FromNative(lpszParam));
+			// return;
 		}
 		CCommandLineInfo::ParseParam(lpszParam, bFlag, bLast);
 	}
@@ -786,6 +806,26 @@
 		_CrtSetDebugFillThreshold(0); // Disable buffer filling in secure enhanced CRT functions.
 	#endif
 
+	auto ipcWnd = FindWindow(IPCClassName, nullptr);
+	// Still have to create our own window in case the other process closes...
+	{
+		WNDCLASS ipcWindowClass =
+		{
+			0,
+			IPCWindowProc,
+			0,
+			0,
+			m_hInstance,
+			nullptr,
+			nullptr,
+			nullptr,
+			nullptr,
+			IPCClassName
+		};
+		auto ipcAtom = RegisterClass(&ipcWindowClass);
+		CreateWindow(MAKEINTATOM(ipcAtom), _T("OpenMPT IPC Window"), 0, 0, 0, 0, 0, nullptr, nullptr, m_hInstance, 0);
+	}
+
 	// Initialize OLE MFC support
 	BOOL oleinit = AfxOleInit();
 	ASSERT(oleinit != FALSE); // no MPT_ASSERT here!
@@ -797,7 +837,24 @@
 
 	// Parse command line for standard shell commands, DDE, file open
 	ParseCommandLine(cmdInfo);
-	
+
+	if(ipcWnd && !cmdInfo.m_fileNames.empty())
+	{
+		for(const auto &filename : cmdInfo.m_fileNames)
+		{
+			auto filenameW = filename.ToWide();
+			const DWORD size = static_cast<DWORD>(filenameW.size() * sizeof(WCHAR));
+			COPYDATASTRUCT copyData{ 0, size, &filenameW[0] };
+
+			DWORD_PTR result = 0;
+			bool success = ::SendMessageTimeout(ipcWnd, WM_COPYDATA, 0, reinterpret_cast<LPARAM>(&copyData), SMTO_ABORTIFHUNG | SMTO_BLOCK, 10000, &result) != 0;
+			if(success && result != 0)
+			{
+				ExitProcess(0);
+			}
+		}
+	}
+
 	if(IsDebuggerPresent() && cmdInfo.m_bDebugCrashHandler)
 	{
 		ExceptionHandler::useAnyCrashHandler = true;
@@ -1030,6 +1087,7 @@
 		// we do not want to open an empty new one on startup.
 		cmdInfo.m_nShellCommand = CCommandLineInfo::FileNothing;
 	}
+	// TODO handle multiple files (MultiSelectModel = Player)
 	if(!ProcessShellCommand(cmdInfo))
 	{
 		EndWaitCursor();
ddeReplace.patch (3,624 bytes)
Saga Musix

Saga Musix

2018-06-26 19:59

administrator   ~0003570

Second variant, accepting more than one file and sending them all in a single WM_COPYDATA call. May not be desirable because it blocks longer, will have to investigate.



ddeReplace-2.patch (3,758 bytes)
Index: mptrack/Mptrack.cpp
===================================================================
--- mptrack/Mptrack.cpp	(revision 10509)
+++ mptrack/Mptrack.cpp	(working copy)
@@ -58,7 +58,32 @@
 
 OPENMPT_NAMESPACE_BEGIN
 
+static const TCHAR IPCClassName[] = _T("OpenMPT_IPC_Wnd");
 
+static LRESULT CALLBACK IPCWindowProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
+{
+	if(uMsg == WM_COPYDATA)
+	{
+		const auto &copyData = *reinterpret_cast<const COPYDATASTRUCT *>(lParam);
+		size_t remain = copyData.cbData / sizeof(WCHAR);
+		const auto *str = static_cast<const WCHAR *>(copyData.lpData);
+		while(remain > 0)
+		{
+			size_t length = wcsnlen(str, remain);
+			const std::wstring name(str, length);
+			theApp.OpenDocumentFile(mpt::PathString::FromWide(name).AsNative().c_str());
+			// Null terminator between strings
+			if(length < remain)
+				length++;
+			str += length;
+			remain -= length;
+		}
+		return TRUE;
+	}
+	return ::DefWindowProc(hwnd, uMsg, wParam, lParam);
+}
+
+
 /////////////////////////////////////////////////////////////////////////////
 // The one and only CTrackApp object
 
@@ -130,6 +155,7 @@
 class CMPTCommandLineInfo: public CCommandLineInfo
 {
 public:
+	std::vector<mpt::PathString> m_fileNames;
 	bool m_bNoDls = false, m_bNoPlugins = false, m_bNoAssembly = false, m_bNoSysCheck = false, m_bNoWine = false,
 		 m_bPortable = false, m_bNoCrashHandler = false, m_bDebugCrashHandler = false;
 #ifdef ENABLE_TESTS
@@ -139,7 +165,7 @@
 public:
 	void ParseParam(LPCTSTR lpszParam, BOOL bFlag, BOOL bLast) override
 	{
-		if ((lpszParam) && (bFlag))
+		if(lpszParam && bFlag)
 		{
 			if (!lstrcmpi(lpszParam, _T("nologo"))) { m_bShowSplash = FALSE; return; }
 			if (!lstrcmpi(lpszParam, _T("nodls"))) { m_bNoDls = true; return; }
@@ -154,6 +180,10 @@
 #ifdef ENABLE_TESTS
 			if (!lstrcmpi(lpszParam, _T("noTests"))) { m_bNoTests = true; return; }
 #endif
+		} else if(lpszParam && !bFlag)
+		{
+			m_fileNames.push_back(mpt::PathString::FromNative(lpszParam));
+			// return;
 		}
 		CCommandLineInfo::ParseParam(lpszParam, bFlag, bLast);
 	}
@@ -797,7 +827,46 @@
 
 	// Parse command line for standard shell commands, DDE, file open
 	ParseCommandLine(cmdInfo);
-	
+
+	auto ipcWnd = ::FindWindow(IPCClassName, nullptr);
+	if(ipcWnd && !cmdInfo.m_fileNames.empty())
+	{
+		// TODO: Would it be better to send one message per file due to "hang" status of thread?
+		std::wstring filenameW;
+		for(const auto &filename : cmdInfo.m_fileNames)
+		{
+			filenameW += filename.ToWide() + L'\0';
+		}
+		const DWORD size = static_cast<DWORD>(filenameW.size() * sizeof(WCHAR));
+		COPYDATASTRUCT copyData{ 0, size, &filenameW[0] };
+
+		DWORD_PTR result = 0;
+		bool success = ::SendMessageTimeout(ipcWnd, WM_COPYDATA, 0, reinterpret_cast<LPARAM>(&copyData), SMTO_ABORTIFHUNG | SMTO_BLOCK | SMTO_NOTIMEOUTIFNOTHUNG, 10000, &result) != 0;
+		if(success && result != 0)
+		{
+			ExitProcess(0);
+		}
+	}
+
+	// Still have to create our own window in case the other process closes...
+	{
+		WNDCLASS ipcWindowClass =
+		{
+			0,
+			IPCWindowProc,
+			0,
+			0,
+			m_hInstance,
+			nullptr,
+			nullptr,
+			nullptr,
+			nullptr,
+			IPCClassName
+		};
+		auto ipcAtom = RegisterClass(&ipcWindowClass);
+		CreateWindow(MAKEINTATOM(ipcAtom), _T("OpenMPT IPC Window"), 0, 0, 0, CW_USEDEFAULT, 0, nullptr, nullptr, m_hInstance, 0);
+	}
+
 	if(IsDebuggerPresent() && cmdInfo.m_bDebugCrashHandler)
 	{
 		ExceptionHandler::useAnyCrashHandler = true;
@@ -1030,6 +1099,7 @@
 		// we do not want to open an empty new one on startup.
 		cmdInfo.m_nShellCommand = CCommandLineInfo::FileNothing;
 	}
+	// TODO handle multiple files (MultiSelectModel = Player)
 	if(!ProcessShellCommand(cmdInfo))
 	{
 		EndWaitCursor();
ddeReplace-2.patch (3,758 bytes)
Saga Musix

Saga Musix

2018-06-27 22:01

administrator   ~0003571

Last edited: 2018-07-24 20:04

View 3 revisions

Code has been cleaned up and committed. The new approach can be tested starting from r10519 / OpenMPT 1.28.00.25, soon available from https://builds.openmpt.org/builds/.

The new code will only take full effect with freshly installed setups, so here is what you can do to test the improved behaviour:

  1. Open regedit and go to HKEY_CLASSES_ROOT\OpenMPTFile\shell\Open.
  2. Add a new string value MultiSelectModel with value Player. This is not strictly necessary but will speed up opening multiple files at once.
  3. In the command key, add /shared to the parameter list of the default value after the path to mptrack.exe.
  4. Delete the ddeexec key.
Saga Musix

Saga Musix

2018-06-27 22:03

administrator   ~0003572

Last edited: 2018-07-24 20:09

View 2 revisions

Note that as a result of this change, you can now also finally open more than one file at once from the command line, and you can also use the command line parameter /shared to send the passed file(s) to a running OpenMPT instance the same way as it works for Explorer.

Saga Musix

Saga Musix

2018-07-24 20:04

administrator   ~0003582

Last edited: 2018-07-24 20:10

View 2 revisions

Starting from OpenMPT 1.28.00.28, the command line switch is now /shared rather than /dde, as it is a bad idea to rely on /dde not doing anything extra (it's a built-in command from MFC). I have updated my previous comments accordingly.

herodotas

herodotas

2018-08-18 07:53

reporter   ~0003601

Nothing changes here, latest 1.28.00.28.

Saga Musix

Saga Musix

2018-08-18 11:16

administrator   ~0003602

You need to follow all steps described above, not just replace the executable. If you don't remove the ddeexec key from the Registry, Windows will of course keep trying to use DDE for opening files and it will still fail.

Issue History

Date Modified Username Field Change
2018-06-01 17:07 herodotas New Issue
2018-06-01 17:07 herodotas File Added: mptm.jpg
2018-06-01 17:09 Saga Musix Note Added: 0003540
2018-06-01 17:16 herodotas Note Added: 0003541
2018-06-01 17:23 Saga Musix Note Added: 0003542
2018-06-01 18:23 Saga Musix Note Edited: 0003542 View Revisions
2018-06-01 22:02 Saga Musix Note Added: 0003543
2018-06-02 06:14 manx Note Added: 0003544
2018-06-02 06:15 manx Status new => confirmed
2018-06-02 06:16 manx Target Version => OpenMPT 1.?? (long term goals)
2018-06-02 06:17 manx Severity minor => major
2018-06-02 06:17 manx Summary Annoying error message => Using DDE to open files from the shell into the same runing OpenMPT instane is problematic
2018-06-02 10:54 Saga Musix Note Added: 0003545
2018-06-02 11:00 manx Note Added: 0003546
2018-06-02 11:01 manx Priority normal => high
2018-06-02 11:01 manx Severity major => block
2018-06-02 11:01 manx Target Version OpenMPT 1.?? (long term goals) => OpenMPT 1.28.01.00 (goals)
2018-06-02 11:19 manx Summary Using DDE to open files from the shell into the same runing OpenMPT instane is problematic => Using DDE to open files from the shell into the same runing OpenMPT instance is problematic
2018-06-03 14:31 Saga Musix Summary Using DDE to open files from the shell into the same runing OpenMPT instance is problematic => Using DDE to open files from the shell when no OpenMPT instance is running is problematic
2018-06-07 09:11 manx Note Added: 0003547
2018-06-25 21:21 Saga Musix File Added: ddeReplace.patch
2018-06-25 21:21 Saga Musix Note Added: 0003568
2018-06-26 19:59 Saga Musix File Added: ddeReplace-2.patch
2018-06-26 19:59 Saga Musix Note Added: 0003570
2018-06-27 22:01 Saga Musix Assigned To => Saga Musix
2018-06-27 22:01 Saga Musix Status confirmed => feedback
2018-06-27 22:01 Saga Musix Note Added: 0003571
2018-06-27 22:03 Saga Musix Note Added: 0003572
2018-06-27 22:05 Saga Musix Note Edited: 0003571 View Revisions
2018-07-24 20:04 Saga Musix Note Added: 0003582
2018-07-24 20:04 Saga Musix Note Edited: 0003571 View Revisions
2018-07-24 20:05 Saga Musix Status feedback => resolved
2018-07-24 20:05 Saga Musix Resolution open => fixed
2018-07-24 20:05 Saga Musix Fixed in Version => OpenMPT 1.28.01.00 (goals)
2018-07-24 20:09 Saga Musix Note Edited: 0003572 View Revisions
2018-07-24 20:10 Saga Musix Note Edited: 0003582 View Revisions
2018-08-18 07:53 herodotas Status resolved => feedback
2018-08-18 07:53 herodotas Resolution fixed => reopened
2018-08-18 07:53 herodotas Note Added: 0003601
2018-08-18 11:16 Saga Musix Note Added: 0003602
2018-08-18 11:17 Saga Musix Status feedback => resolved
2018-08-18 11:17 Saga Musix Resolution reopened => fixed