[PATCH v2 0/1] MR899: vbscript: Map known HRESULT values into their FACILITY_VBS counterparts.
Map known HRESULT values into their FACILITY_VBS counterparts
Unless the callee provided its own Description (e.g. via SetErrorInfo) VBscript maps certain well-known HRESULT values into its own error facility and messages, changing Err.Number and also setting Err.Source to itself (unless already provided, in which case it is left alone)
e.g. if the invoked method returns E_NOINTERFACE, The VBScript Err object should show
Err.Number: 0x1AE Err.Source: Microsoft VBScript runtime error Err.Description: Class doesn't support Automation
Rather than the original HRESULT
Err.Number: 0x80004002
-- v2: vbscript: Map known HRESULT values into their FACILITY_VBS counterparts.
From: Kevin Puetz PuetzKevinA@JohnDeere.com
Unless the callee provided its own Description (e.g. via SetErrorInfo) VBscript maps certain well-known HRESULT values into its own error facility and messages, changing Err.Number and also setting Err.Source to itself (unless already provided, in which case it is left alone)
e.g. if the invoked method returns E_NOINTERFACE, The VBScript Err object should show
Err.Number: 0x1AE Err.Source: Microsoft VBScript runtime error Err.Description: Class doesn't support Automation
Rather than the original HRESULT
Err.Number: 0x80004002 --- dlls/vbscript/vbdisp.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/dlls/vbscript/vbdisp.c b/dlls/vbscript/vbdisp.c index 1c6f68255bd..ae4f3aaf783 100644 --- a/dlls/vbscript/vbdisp.c +++ b/dlls/vbscript/vbdisp.c @@ -1654,6 +1654,15 @@ HRESULT disp_call(script_ctx_t *ctx, IDispatch *disp, DISPID id, DISPPARAMS *dp, if(hres == DISP_E_EXCEPTION) { clear_ei(&ctx->ei); ctx->ei = ei; + if(!ctx->ei.bstrDescription) { + ctx->ei.scode = hres = map_hres(ei.scode); + if(HRESULT_FACILITY(hres) == FACILITY_VBS) { + ctx->ei.bstrDescription = get_vbscript_error_string(hres); + if(!ctx->ei.bstrSource) { + ctx->ei.bstrSource = get_vbscript_string(VBS_RUNTIME_ERROR); + } + } + } hres = SCRIPT_E_RECORDED; } return hres;
Fixed bad subject line, sorry about that mishap.
Jacek Caban (@jacek) commented about dlls/vbscript/vbdisp.c:
if(hres == DISP_E_EXCEPTION) { clear_ei(&ctx->ei); ctx->ei = ei;
if(!ctx->ei.bstrDescription) {ctx->ei.scode = hres = map_hres(ei.scode);if(HRESULT_FACILITY(hres) == FACILITY_VBS) {ctx->ei.bstrDescription = get_vbscript_error_string(hres);if(!ctx->ei.bstrSource) {ctx->ei.bstrSource = get_vbscript_string(VBS_RUNTIME_ERROR);}}}
I think that we could fix the logic in exec_script() instead of of duplicating it here. What do you think about something like https://gitlab.winehq.org/jacek/wine/-/commit/f1ce7f0962cbc9dac7cc5df420d31f... ?
On Wed Sep 21 21:27:06 2022 +0000, Jacek Caban wrote:
I think that we could fix the logic in exec_script() instead of of duplicating it here. What do you think about something like https://gitlab.winehq.org/jacek/wine/-/commit/f1ce7f0962cbc9dac7cc5df420d31f... ?
I didn't think of doing it that way, since the mismatch I was seeing came from a non-vbscript source (VBscript calling an oleautomation interface implemented in c++) and that had me thinking about disp_call. But yeah, it shouldn't really matter whether the translation is done before disp_call returns, or after the HRESULT has bubbled up through do_icall -> interp_icall -> exec_script. That would still gets it it translated before anything observes ctx->ei sees it (report_script_error or script code inspecting/logging from the `Err` singleton, so it should work the same in practice).
Your patch lost the `if(HRESULT_FACILITY(hres) == FACILITY_VBS)` check though - I think that's necessary, `VBS_RUNTIME_ERROR` and `get_vbscript_error_string` should only be used if `map_hres` translated something. There's a lot of HRESULT values that map_hres just passes through unchanged, and those shouldn't all just get squashed to `VBS_UNKNOWN_RUNTIME_ERROR`.
I suppose another way would be if the signature of map_hres changed to work on the whole `EXCEPINFO` at once. The get_vbscript_error_string wouldn't need to be a public function, the switch in map_hres could just assign the re-mapped `scode`/`bstrDescription`/`bstrSource` all together (or else do nothing).
Or to put that more specifically, here's an additional test case (using E_UNEXPECTED, which map_hres does not translate) to add to your patch
```c store_script_error = &error1; SET_EXPECT(OnScriptError); hres = parse_script_ar("throwException &h8000FFFF&"); ok(hres == E_UNEXPECTED, "got error: %08lx\n", hres); CHECK_CALLED(OnScriptError);
memset(&ei, 0xcc, sizeof(ei)); hres = IActiveScriptError_GetExceptionInfo(error1, &ei); ok(hres == S_OK, "GetExceptionInfo returned %08lx\n", hres); ok(!ei.wCode, "wCode = %x\n", ei.wCode); ok(!ei.wReserved, "wReserved = %x\n", ei.wReserved); if(is_english) { ok(!ei.bstrSource, "bstrSource = %s\n", wine_dbgstr_w(ei.bstrSource)); ok(!ei.bstrDescription, "bstrDescription = %s\n", wine_dbgstr_w(ei.bstrDescription)); } ok(!ei.bstrHelpFile, "bstrHelpFile = %s\n", wine_dbgstr_w(ei.bstrHelpFile)); ok(!ei.dwHelpContext, "dwHelpContext = %lx\n", ei.dwHelpContext); ok(!ei.pvReserved, "pvReserved = %p\n", ei.pvReserved); ok(!ei.pfnDeferredFillIn, "pfnDeferredFillIn = %p\n", ei.pfnDeferredFillIn); ok(ei.scode == E_UNEXPECTED, "scode = %lx\n", ei.scode); free_ei(&ei);
IActiveScriptError_Release(error1); ```
https://testbot.winehq.org/JobDetails.pl?Key=123976 has the whole patch, and shows this passed on w1064, but failed on wine:
run.c:2493: Test failed: bstrSource = L"Microsoft VBScript runtime error" run.c:2495: Test failed: bstrDescription = L"Unknown runtime error"
Because the strings got set even though map_hres was a no-op https://gitlab.winehq.org/jacek/wine/-/blob/f1ce7f0962cbc9dac7cc5df420d31f20...
Adding back the `if(HRESULT_FACILITY(hres) == FACILITY_VBS)` should fix that.
On Thu Sep 22 01:30:49 2022 +0000, Kevin Puetz wrote:
I didn't think of doing it that way, since the mismatch I was seeing came from a non-vbscript source (VBscript calling an oleautomation interface implemented in c++) and that had me thinking about disp_call. But yeah, it shouldn't really matter whether the translation is done before disp_call returns, or after the HRESULT has bubbled up through do_icall -> interp_icall -> exec_script. That would still gets it it translated before anything observes ctx->ei sees it (report_script_error or script code inspecting/logging from the `Err` singleton, so it should work the same in practice). Your patch lost the `if(HRESULT_FACILITY(hres) == FACILITY_VBS)` check though - I think that's necessary, `VBS_RUNTIME_ERROR` and `get_vbscript_error_string` should only be used if `map_hres` translated something. There's a lot of HRESULT values that map_hres just passes through unchanged, and those shouldn't all just get squashed to `VBS_UNKNOWN_RUNTIME_ERROR`. I suppose another way would be if the signature of map_hres changed to work on the whole `EXCEPINFO` at once. Then get_vbscript_error_string wouldn't need to be a public function, the switch in map_hres could just assign the re-mapped `scode`/`bstrDescription`/`bstrSource` all together (or else do nothing).
Or to put that more specifically, here's an additional test case (using E_UNEXPECTED, which map_hres does not translate) to add to your patch
```c store_script_error = &error1; SET_EXPECT(OnScriptError); hres = parse_script_ar("throwException &h8000FFFF&"); ok(hres == E_UNEXPECTED, "got error: %08lx\n", hres); CHECK_CALLED(OnScriptError);
memset(&ei, 0xcc, sizeof(ei)); hres = IActiveScriptError_GetExceptionInfo(error1, &ei); ok(hres == S_OK, "GetExceptionInfo returned %08lx\n", hres); ok(!ei.wCode, "wCode = %x\n", ei.wCode); ok(!ei.wReserved, "wReserved = %x\n", ei.wReserved); if(is_english) { ok(!ei.bstrSource, "bstrSource = %s\n", wine_dbgstr_w(ei.bstrSource)); ok(!ei.bstrDescription, "bstrDescription = %s\n", wine_dbgstr_w(ei.bstrDescription)); } ok(!ei.bstrHelpFile, "bstrHelpFile = %s\n", wine_dbgstr_w(ei.bstrHelpFile)); ok(!ei.dwHelpContext, "dwHelpContext = %lx\n", ei.dwHelpContext); ok(!ei.pvReserved, "pvReserved = %p\n", ei.pvReserved); ok(!ei.pfnDeferredFillIn, "pfnDeferredFillIn = %p\n", ei.pfnDeferredFillIn); ok(ei.scode == E_UNEXPECTED, "scode = %lx\n", ei.scode); free_ei(&ei);
IActiveScriptError_Release(error1); ```
https://testbot.winehq.org/JobDetails.pl?Key=123976 has the whole patch, and shows this passed on w1064, but failed on wine:
run.c:2493: Test failed: bstrSource = L"Microsoft VBScript runtime error" run.c:2495: Test failed: bstrDescription = L"Unknown runtime error"
Because the strings got set even though map_hres was a no-op https://gitlab.winehq.org/jacek/wine/-/blob/f1ce7f0962cbc9dac7cc5df420d31f20...
Adding back the `if(HRESULT_FACILITY(hres) == FACILITY_VBS)` should fix that.
On Thu Sep 22 01:34:11 2022 +0000, Kevin Puetz wrote:
Or to put that more specifically, here's an additional test case (using E_UNEXPECTED, which map_hres does not translate) to add to your patch
store_script_error = &error1; SET_EXPECT(OnScriptError); hres = parse_script_ar("throwException &h8000FFFF&"); ok(hres == E_UNEXPECTED, "got error: %08lx\n", hres); CHECK_CALLED(OnScriptError); memset(&ei, 0xcc, sizeof(ei)); hres = IActiveScriptError_GetExceptionInfo(error1, &ei); ok(hres == S_OK, "GetExceptionInfo returned %08lx\n", hres); ok(!ei.wCode, "wCode = %x\n", ei.wCode); ok(!ei.wReserved, "wReserved = %x\n", ei.wReserved); if(is_english) { ok(!ei.bstrSource, "bstrSource = %s\n", wine_dbgstr_w(ei.bstrSource)); ok(!ei.bstrDescription, "bstrDescription = %s\n", wine_dbgstr_w(ei.bstrDescription)); } ok(!ei.bstrHelpFile, "bstrHelpFile = %s\n", wine_dbgstr_w(ei.bstrHelpFile)); ok(!ei.dwHelpContext, "dwHelpContext = %lx\n", ei.dwHelpContext); ok(!ei.pvReserved, "pvReserved = %p\n", ei.pvReserved); ok(!ei.pfnDeferredFillIn, "pfnDeferredFillIn = %p\n", ei.pfnDeferredFillIn); ok(ei.scode == E_UNEXPECTED, "scode = %lx\n", ei.scode); free_ei(&ei); IActiveScriptError_Release(error1);https://testbot.winehq.org/JobDetails.pl?Key=123976 has the whole patch, and shows this passed on w1064, but failed on wine:
run.c:2493: Test failed: bstrSource = L"Microsoft VBScript runtime error" run.c:2495: Test failed: bstrDescription = L"Unknown runtime error"
Because the strings got set even though map_hres was a no-op https://gitlab.winehq.org/jacek/wine/-/blob/f1ce7f0962cbc9dac7cc5df420d31f20... Adding back the `if(HRESULT_FACILITY(hres) == FACILITY_VBS)` should fix that.
Agreed. And yes, changing map_hres sounds good to me.
On Fri Sep 23 13:35:52 2022 +0000, Jacek Caban wrote:
Agreed. And yes, changing map_hres sounds good to me.
Ok, I'll try and work something up along those lines.
-
Jacek Caban (＠jacek) -
Kevin Puetz
-
Kevin Puetz (＠puetzk)