View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0001126 | OpenMPT | Accessibility | public | 2018-06-01 17:07 | 2018-11-13 07:50 |
Reporter | herodotas | Assigned To | Saga Musix | ||
Priority | high | Severity | block | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Platform | x64 | OS | Windows | OS Version | 7 |
Product Version | OpenMPT 1.28.00.* (old testing) | ||||
Target Version | OpenMPT 1.28.01.00 / libopenmpt 0.4.0 (upgrade first) | Fixed in Version | OpenMPT 1.28.01.00 / libopenmpt 0.4.0 (upgrade first) | ||
Summary | 0001126: 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. | ||||
Tags | No tags attached. | ||||
Attached Files | |||||
Has the bug occurred in previous versions? | No, just in 1.28.* | ||||
Tested code revision (in case you know it) | |||||
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. |
|
But with 1.27.08 this message dont appear. |
|
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. |
|
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. |
|
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. https://blogs.msdn.microsoft.com/oldnewthing/20070226-00/?p=27863 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). There are 2 aspects to the whole problem:
I have no idea whatsoever which solution would be the best. This needs further discussion. |
|
Either way, the current behaviour is not acceptable and for me is a blocker for an 1.28 release, not a long-term goal. |
|
Fair enough, I just do not think this would be easily solvable. |
|
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: So, this probably demands a hidden message-only window used specifically for IPC. |
|
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 ©Data = *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>(©Data), 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(); |
|
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 ©Data = *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>(©Data), 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(); |
|
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:
|
|
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 |
|
Starting from OpenMPT 1.28.00.28, the command line switch is now |
|
Nothing changes here, latest 1.28.00.28. |
|
You need to follow all steps described above, not just replace the executable. If you don't remove the |
|
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 | |
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 / libopenmpt 0.4.0 (upgrade first) |
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 | |
2018-07-24 20:04 | Saga Musix | Note Added: 0003582 | |
2018-07-24 20:04 | Saga Musix | Note Edited: 0003571 | |
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 / libopenmpt 0.4.0 (upgrade first) |
2018-07-24 20:09 | Saga Musix | Note Edited: 0003572 | |
2018-07-24 20:10 | Saga Musix | Note Edited: 0003582 | |
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 |