[PATCH 1/5] jscript: Retrieve the item's dispatch in a helper function.
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/jscript.c | 45 +++++++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/dlls/jscript/jscript.c b/dlls/jscript/jscript.c index 31277c6..d6fa7c9 100644 --- a/dlls/jscript/jscript.c +++ b/dlls/jscript/jscript.c @@ -129,6 +129,30 @@ static void release_named_item_script_obj(named_item_t *item) item->script_obj = NULL; } +static HRESULT retrieve_named_item_disp(IActiveScriptSite *site, named_item_t *item) +{ + IUnknown *unk; + HRESULT hr; + + if(!site) + return E_UNEXPECTED; + + hr = IActiveScriptSite_GetItemInfo(site, item->name, SCRIPTINFO_IUNKNOWN, &unk, NULL); + if(FAILED(hr)) { + WARN("GetItemInfo failed: %08x\n", hr); + return hr; + } + + hr = IUnknown_QueryInterface(unk, &IID_IDispatch, (void**)&item->disp); + IUnknown_Release(unk); + if(FAILED(hr)) { + WARN("object does not implement IDispatch\n"); + return hr; + } + + return S_OK; +} + named_item_t *lookup_named_item(script_ctx_t *ctx, const WCHAR *item_name, unsigned flags) { named_item_t *item; @@ -140,25 +164,10 @@ named_item_t *lookup_named_item(script_ctx_t *ctx, const WCHAR *item_name, unsig hr = create_named_item_script_obj(ctx, item); if(FAILED(hr)) return NULL; } + if(!item->disp && (flags || !(item->flags & SCRIPTITEM_CODEONLY))) { - IUnknown *unk; - - if(!ctx->site) - return NULL; - - hr = IActiveScriptSite_GetItemInfo(ctx->site, item_name, - SCRIPTINFO_IUNKNOWN, &unk, NULL); - if(FAILED(hr)) { - WARN("GetItemInfo failed: %08x\n", hr); - continue; - } - - hr = IUnknown_QueryInterface(unk, &IID_IDispatch, (void**)&item->disp); - IUnknown_Release(unk); - if(FAILED(hr)) { - WARN("object does not implement IDispatch\n"); - continue; - } + hr = retrieve_named_item_disp(ctx->site, item); + if(FAILED(hr)) continue; } return item; -- 2.21.0
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/jscript.c | 44 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/dlls/jscript/jscript.c b/dlls/jscript/jscript.c index d6fa7c9..3fcaab0 100644 --- a/dlls/jscript/jscript.c +++ b/dlls/jscript/jscript.c @@ -414,6 +414,15 @@ static void release_persistent_script_objs(JScript *This) release_named_item_script_obj(iter->named_item); } +static void release_named_item_list(JScript *This) +{ + while(!list_empty(&This->ctx->named_items)) { + named_item_t *iter = LIST_ENTRY(list_head(&This->ctx->named_items), named_item_t, entry); + list_remove(&iter->entry); + release_named_item(iter); + } +} + static void exec_queued_code(JScript *This) { bytecode_t *iter; @@ -433,6 +442,8 @@ static void exec_queued_code(JScript *This) static void decrease_state(JScript *This, SCRIPTSTATE state) { + named_item_t *item, *item_next; + if(This->ctx) { switch(This->ctx->state) { case SCRIPTSTATE_CONNECTED: @@ -453,14 +464,19 @@ static void decrease_state(JScript *This, SCRIPTSTATE state) clear_script_queue(This); release_persistent_script_objs(This); - while(!list_empty(&This->ctx->named_items)) { - named_item_t *iter = LIST_ENTRY(list_head(&This->ctx->named_items), named_item_t, entry); - - list_remove(&iter->entry); - if(iter->disp) - IDispatch_Release(iter->disp); - release_named_item_script_obj(iter); - release_named_item(iter); + LIST_FOR_EACH_ENTRY_SAFE(item, item_next, &This->ctx->named_items, named_item_t, entry) + { + if(item->disp) + { + IDispatch_Release(item->disp); + item->disp = NULL; + } + release_named_item_script_obj(item); + if(!(item->flags & SCRIPTITEM_ISPERSISTENT)) + { + list_remove(&item->entry); + release_named_item(item); + } } if(This->ctx->secmgr) { @@ -688,6 +704,7 @@ static HRESULT WINAPI JScript_SetScriptSite(IActiveScript *iface, IActiveScriptSite *pass) { JScript *This = impl_from_IActiveScript(iface); + named_item_t *item; LCID lcid; HRESULT hres; @@ -732,6 +749,16 @@ static HRESULT WINAPI JScript_SetScriptSite(IActiveScript *iface, } } + /* Retrieve new dispatches for persistent named items */ + LIST_FOR_EACH_ENTRY(item, &This->ctx->named_items, named_item_t, entry) + { + if(!item->disp) + { + hres = retrieve_named_item_disp(pass, item); + if(FAILED(hres)) return hres; + } + } + This->site = pass; IActiveScriptSite_AddRef(This->site); @@ -829,6 +856,7 @@ static HRESULT WINAPI JScript_Close(IActiveScript *iface) decrease_state(This, SCRIPTSTATE_CLOSED); clear_persistent_code_list(This); + release_named_item_list(This); return S_OK; } -- 2.21.0
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/tests/jscript.c | 86 +++++++++++++++++++++++++++++++++--- 1 file changed, 81 insertions(+), 5 deletions(-) diff --git a/dlls/jscript/tests/jscript.c b/dlls/jscript/tests/jscript.c index d79b974..ef0099d 100644 --- a/dlls/jscript/tests/jscript.c +++ b/dlls/jscript/tests/jscript.c @@ -109,6 +109,7 @@ DEFINE_EXPECT(GetItemInfo_global); DEFINE_EXPECT(GetItemInfo_global_code); DEFINE_EXPECT(GetItemInfo_visible); DEFINE_EXPECT(GetItemInfo_visible_code); +DEFINE_EXPECT(GetItemInfo_persistent); DEFINE_EXPECT(testCall); static const CLSID *engine_clsid = &CLSID_JScript; @@ -146,7 +147,7 @@ static ULONG WINAPI Dispatch_Release(IDispatch *iface) return 1; } -static ULONG global_named_item_ref, visible_named_item_ref, visible_code_named_item_ref; +static ULONG global_named_item_ref, visible_named_item_ref, visible_code_named_item_ref, persistent_named_item_ref; static ULONG WINAPI global_AddRef(IDispatch *iface) { @@ -178,6 +179,16 @@ static ULONG WINAPI visible_code_Release(IDispatch *iface) return --visible_code_named_item_ref; } +static ULONG WINAPI persistent_AddRef(IDispatch *iface) +{ + return ++persistent_named_item_ref; +} + +static ULONG WINAPI persistent_Release(IDispatch *iface) +{ + return --persistent_named_item_ref; +} + static HRESULT WINAPI Dispatch_GetTypeInfoCount(IDispatch *iface, UINT *pctinfo) { ok(0, "unexpected call\n"); @@ -261,6 +272,18 @@ static const IDispatchVtbl visible_code_named_item_vtbl = { static IDispatch visible_code_named_item = { &visible_code_named_item_vtbl }; +static const IDispatchVtbl persistent_named_item_vtbl = { + Dispatch_QueryInterface, + persistent_AddRef, + persistent_Release, + Dispatch_GetTypeInfoCount, + Dispatch_GetTypeInfo, + Dispatch_GetIDsOfNames, + Dispatch_Invoke +}; + +static IDispatch persistent_named_item = { &persistent_named_item_vtbl }; + static HRESULT WINAPI ActiveScriptSite_QueryInterface(IActiveScriptSite *iface, REFIID riid, void **ppv) { *ppv = NULL; @@ -320,6 +343,12 @@ static HRESULT WINAPI ActiveScriptSite_GetItemInfo(IActiveScriptSite *iface, LPC *ppiunkItem = (IUnknown*)&visible_code_named_item; return S_OK; } + if(!wcscmp(pstrName, L"persistent")) { + CHECK_EXPECT(GetItemInfo_persistent); + IDispatch_AddRef(&persistent_named_item); + *ppiunkItem = (IUnknown*)&persistent_named_item; + return S_OK; + } ok(0, "unexpected call %s\n", wine_dbgstr_w(pstrName)); return E_NOTIMPL; } @@ -1163,6 +1192,8 @@ static void test_named_items(void) ok(hr == E_UNEXPECTED, "AddNamedItem returned: %08x\n", hr); hr = IActiveScript_AddNamedItem(script, L"codeOnlyItem", SCRIPTITEM_CODEONLY); ok(hr == E_UNEXPECTED, "AddNamedItem returned: %08x\n", hr); + hr = IActiveScript_AddNamedItem(script, L"persistent", SCRIPTITEM_ISPERSISTENT | SCRIPTITEM_CODEONLY); + ok(hr == E_UNEXPECTED, "AddNamedItem returned: %08x\n", hr); SET_EXPECT(GetLCID); hr = IActiveScript_SetScriptSite(script, &ActiveScriptSite); @@ -1180,10 +1211,13 @@ static void test_named_items(void) ok(hr == S_OK, "AddNamedItem failed: %08x\n", hr); hr = IActiveScript_AddNamedItem(script, L"codeOnlyItem", SCRIPTITEM_CODEONLY); ok(hr == S_OK, "AddNamedItem failed: %08x\n", hr); + hr = IActiveScript_AddNamedItem(script, L"persistent", SCRIPTITEM_ISPERSISTENT | SCRIPTITEM_CODEONLY); + ok(hr == S_OK, "AddNamedItem failed: %08x\n", hr); ok(global_named_item_ref > 0, "global_named_item_ref = %u\n", global_named_item_ref); ok(visible_named_item_ref == 0, "visible_named_item_ref = %u\n", visible_named_item_ref); ok(visible_code_named_item_ref == 0, "visible_code_named_item_ref = %u\n", visible_code_named_item_ref); + ok(persistent_named_item_ref == 0, "persistent_named_item_ref = %u\n", persistent_named_item_ref); hr = IActiveScript_GetScriptDispatch(script, L"noContext", &disp); ok(hr == E_INVALIDARG, "GetScriptDispatch returned: %08x\n", hr); @@ -1267,6 +1301,7 @@ static void test_named_items(void) ok(global_named_item_ref > 0, "global_named_item_ref = %u\n", global_named_item_ref); ok(visible_named_item_ref > 0, "visible_named_item_ref = %u\n", visible_named_item_ref); ok(visible_code_named_item_ref > 0, "visible_code_named_item_ref = %u\n", visible_code_named_item_ref); + ok(persistent_named_item_ref == 0, "persistent_named_item_ref = %u\n", persistent_named_item_ref); SET_EXPECT(testCall); parse_script(parse, L"visibleItem.testCall();"); @@ -1433,6 +1468,28 @@ static void test_named_items(void) IDispatchEx_Release(dispex2); IDispatchEx_Release(dispex); + SET_EXPECT(OnEnterScript); + SET_EXPECT(OnLeaveScript); + hr = IActiveScriptParse_ParseScriptText(parse, L"var x = 13;\n", L"persistent", NULL, NULL, 0, 0, SCRIPTTEXT_ISPERSISTENT, NULL, NULL); + ok(hr == S_OK, "ParseScriptText failed: %08x\n", hr); + CHECK_CALLED(OnEnterScript); + CHECK_CALLED(OnLeaveScript); + SET_EXPECT(OnEnterScript); + SET_EXPECT(OnLeaveScript); + hr = IActiveScriptParse_ParseScriptText(parse, L"x = 10;\n", L"persistent", NULL, NULL, 0, 0, 0, NULL, NULL); + ok(hr == S_OK, "ParseScriptText failed: %08x\n", hr); + CHECK_CALLED(OnEnterScript); + CHECK_CALLED(OnLeaveScript); + SET_EXPECT(OnEnterScript); + SET_EXPECT(OnLeaveScript); + hr = IActiveScriptParse_ParseScriptText(parse, L"x", L"persistent", NULL, NULL, 0, 0, SCRIPTTEXT_ISEXPRESSION, &var, NULL); + ok(hr == S_OK, "ParseScriptText failed: %08x\n", hr); + ok(V_VT(&var) == VT_I4 && V_I4(&var) == 10, "Unexpected 'x': V_VT = %d, V_I4 = %d\n", V_VT(&var), V_I4(&var)); + CHECK_CALLED(OnEnterScript); + CHECK_CALLED(OnLeaveScript); + + dispex = get_script_dispatch(script, L"persistent"); + /* reinitialize script engine */ SET_EXPECT(OnStateChange_DISCONNECTED); @@ -1448,30 +1505,48 @@ static void test_named_items(void) ok(global_named_item_ref == 0, "global_named_item_ref = %u\n", global_named_item_ref); ok(visible_named_item_ref == 0, "visible_named_item_ref = %u\n", visible_named_item_ref); ok(visible_code_named_item_ref == 0, "visible_code_named_item_ref = %u\n", visible_code_named_item_ref); + ok(persistent_named_item_ref == 0, "persistent_named_item_ref = %u\n", persistent_named_item_ref); hr = IActiveScript_GetScriptDispatch(script, L"codeOnlyItem", &disp); ok(hr == E_UNEXPECTED, "hr = %08x, expected E_UNEXPECTED\n", hr); SET_EXPECT(GetLCID); SET_EXPECT(OnStateChange_INITIALIZED); + SET_EXPECT(GetItemInfo_persistent); hr = IActiveScript_SetScriptSite(script, &ActiveScriptSite); ok(hr == S_OK, "SetScriptSite failed: %08x\n", hr); CHECK_CALLED(GetLCID); CHECK_CALLED(OnStateChange_INITIALIZED); + CHECK_CALLED(GetItemInfo_persistent); + ok(persistent_named_item_ref > 0, "persistent_named_item_ref = %u\n", persistent_named_item_ref); hr = IActiveScript_AddNamedItem(script, L"codeOnlyItem", SCRIPTITEM_CODEONLY); ok(hr == S_OK, "AddNamedItem failed: %08x\n", hr); SET_EXPECT(OnStateChange_CONNECTED); - SET_EXPECT_MULTI(OnEnterScript, 4); - SET_EXPECT_MULTI(OnLeaveScript, 4); + SET_EXPECT_MULTI(OnEnterScript, 5); + SET_EXPECT_MULTI(OnLeaveScript, 5); + SET_EXPECT(GetIDsOfNames); hr = IActiveScript_SetScriptState(script, SCRIPTSTATE_CONNECTED); ok(hr == S_OK, "SetScriptState(SCRIPTSTATE_CONNECTED) failed: %08x\n", hr); CHECK_CALLED(OnStateChange_CONNECTED); - CHECK_CALLED_MULTI(OnEnterScript, 4); - CHECK_CALLED_MULTI(OnLeaveScript, 4); + CHECK_CALLED_MULTI(OnEnterScript, 5); + CHECK_CALLED_MULTI(OnLeaveScript, 5); + todo_wine CHECK_CALLED(GetIDsOfNames); test_state(script, SCRIPTSTATE_CONNECTED); + dispex2 = get_script_dispatch(script, L"persistent"); + ok(dispex != dispex2, "Same script dispatch returned for \"persistent\" named item\n"); + IDispatchEx_Release(dispex2); + IDispatchEx_Release(dispex); + SET_EXPECT(OnEnterScript); + SET_EXPECT(OnLeaveScript); + hr = IActiveScriptParse_ParseScriptText(parse, L"x", L"persistent", NULL, NULL, 0, 0, SCRIPTTEXT_ISEXPRESSION, &var, NULL); + ok(hr == S_OK, "ParseScriptText failed: %08x\n", hr); + ok(V_VT(&var) == VT_I4 && V_I4(&var) == 13, "Unexpected 'x': V_VT = %d, V_I4 = %d\n", V_VT(&var), V_I4(&var)); + CHECK_CALLED(OnEnterScript); + CHECK_CALLED(OnLeaveScript); + dispex = get_script_dispatch(script, NULL); for (i = 0; i < ARRAY_SIZE(global_idents); i++) { @@ -1592,6 +1667,7 @@ static void test_named_items(void) ok(global_named_item_ref == 0, "global_named_item_ref = %u\n", global_named_item_ref); ok(visible_named_item_ref == 0, "visible_named_item_ref = %u\n", visible_named_item_ref); ok(visible_code_named_item_ref == 0, "visible_code_named_item_ref = %u\n", visible_code_named_item_ref); + ok(persistent_named_item_ref == 0, "persistent_named_item_ref = %u\n", persistent_named_item_ref); test_state(script, SCRIPTSTATE_CLOSED); IActiveScriptParse_Release(parse); -- 2.21.0
Hi Gabriel, On 23.03.2020 14:53, Gabriel Ivăncescu wrote:
SET_EXPECT(OnStateChange_CONNECTED); - SET_EXPECT_MULTI(OnEnterScript, 4); - SET_EXPECT_MULTI(OnLeaveScript, 4); + SET_EXPECT_MULTI(OnEnterScript, 5); + SET_EXPECT_MULTI(OnLeaveScript, 5); + SET_EXPECT(GetIDsOfNames); hr = IActiveScript_SetScriptState(script, SCRIPTSTATE_CONNECTED); ok(hr == S_OK, "SetScriptState(SCRIPTSTATE_CONNECTED) failed: %08x\n", hr); CHECK_CALLED(OnStateChange_CONNECTED); - CHECK_CALLED_MULTI(OnEnterScript, 4); - CHECK_CALLED_MULTI(OnLeaveScript, 4); + CHECK_CALLED_MULTI(OnEnterScript, 5); + CHECK_CALLED_MULTI(OnLeaveScript, 5); + todo_wine CHECK_CALLED(GetIDsOfNames); test_state(script, SCRIPTSTATE_CONNECTED);
Before this patch that code verified that GetIDsOfNames is not called for any code. Now, after your change, it does no longer does that. Please try to move the test into a separated pass instead of mixing all tests together. Thanks, Jacek
Hi Jacek, On 24/03/2020 02:38, Jacek Caban wrote:
Hi Gabriel,
On 23.03.2020 14:53, Gabriel Ivăncescu wrote:
SET_EXPECT(OnStateChange_CONNECTED); - SET_EXPECT_MULTI(OnEnterScript, 4); - SET_EXPECT_MULTI(OnLeaveScript, 4); + SET_EXPECT_MULTI(OnEnterScript, 5); + SET_EXPECT_MULTI(OnLeaveScript, 5); + SET_EXPECT(GetIDsOfNames); hr = IActiveScript_SetScriptState(script, SCRIPTSTATE_CONNECTED); ok(hr == S_OK, "SetScriptState(SCRIPTSTATE_CONNECTED) failed: %08x\n", hr); CHECK_CALLED(OnStateChange_CONNECTED); - CHECK_CALLED_MULTI(OnEnterScript, 4); - CHECK_CALLED_MULTI(OnLeaveScript, 4); + CHECK_CALLED_MULTI(OnEnterScript, 5); + CHECK_CALLED_MULTI(OnLeaveScript, 5); + todo_wine CHECK_CALLED(GetIDsOfNames); test_state(script, SCRIPTSTATE_CONNECTED);
Before this patch that code verified that GetIDsOfNames is not called for any code. Now, after your change, it does no longer does that. Please try to move the test into a separated pass instead of mixing all tests together.
I can't really move it to a separate test, it's because the 'var x' itself calls it, which is needed to test the persistent item + persistent code relationship, so I can't get rid of that variable. However I can use the new 'expected_GetIDsOfNames_iface' variable to test that it's only the specific iface (persistent item) that is called here. Is that a good approach? Thanks, Gabriel
On 24.03.2020 14:42, Gabriel Ivăncescu wrote:
However I can use the new 'expected_GetIDsOfNames_iface' variable to test that it's only the specific iface (persistent item) that is called here. Is that a good approach?
It would be better to have a separated GetIDsOfNames for persistent named item that would use separated CHECK_EXPECT() values. Thanks, Jacek
If the bytecode has a named item context, its disp is looked for the identifier before any globals, unless it has the SCRIPTITEM_CODEONLY flag. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/engine.c | 21 ++++++++++++++++++++- dlls/jscript/tests/jscript.c | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/dlls/jscript/engine.c b/dlls/jscript/engine.c index 896ba60..1d28967 100644 --- a/dlls/jscript/engine.c +++ b/dlls/jscript/engine.c @@ -585,6 +585,21 @@ static HRESULT detach_variable_object(script_ctx_t *ctx, call_frame_t *frame, BO return S_OK; } +static BOOL lookup_ident_in_item_disp(script_ctx_t *ctx, named_item_t *item, BSTR identifier, exprval_t *ret) +{ + DISPID id; + + if(item && !(item->flags & SCRIPTITEM_CODEONLY) && item->disp && + SUCCEEDED(disp_get_id(ctx, item->disp, identifier, identifier, 0, &id))) + { + if (ret) + exprval_set_disp_ref(ret, item->disp, id); + return TRUE; + } + + return FALSE; +} + static BOOL lookup_global_members(script_ctx_t *ctx, BSTR identifier, exprval_t *ret) { named_item_t *item; @@ -677,6 +692,9 @@ static HRESULT identifier_eval(script_ctx_t *ctx, BSTR identifier, exprval_t *re exprval_set_disp_ref(ret, to_disp(script_obj), id); return S_OK; } + + if(lookup_ident_in_item_disp(ctx, ctx->call_ctx->bytecode->named_item, identifier, ret)) + return S_OK; } } @@ -3063,7 +3081,8 @@ HRESULT exec_source(script_ctx_t *ctx, DWORD flags, bytecode_t *bytecode, functi hres = jsdisp_propput_name(variable_obj, function->variables[i].name, jsval_obj(func_obj)); jsdisp_release(func_obj); - }else if(!lookup_globals || !lookup_global_members(ctx, function->variables[i].name, NULL)) { + }else if(!lookup_ident_in_item_disp(ctx, bytecode->named_item, function->variables[i].name, NULL) && + (!lookup_globals || !lookup_global_members(ctx, function->variables[i].name, NULL))) { DISPID id = 0; hres = jsdisp_get_id(variable_obj, function->variables[i].name, fdexNameEnsure, &id); diff --git a/dlls/jscript/tests/jscript.c b/dlls/jscript/tests/jscript.c index ef0099d..9329087 100644 --- a/dlls/jscript/tests/jscript.c +++ b/dlls/jscript/tests/jscript.c @@ -200,9 +200,13 @@ static HRESULT WINAPI Dispatch_GetTypeInfo(IDispatch *iface, UINT iTInfo, LCID l return DISP_E_BADINDEX; } +static IDispatch *expected_GetIDsOfNames_iface; + static HRESULT WINAPI Dispatch_GetIDsOfNames(IDispatch *iface, REFIID riid, LPOLESTR *names, UINT name_cnt, LCID lcid, DISPID *ids) { + if(expected_GetIDsOfNames_iface) + ok(iface == expected_GetIDsOfNames_iface, "Wrong iface, got %p, expected %p\n", iface, expected_GetIDsOfNames_iface); ok(name_cnt == 1, "name_cnt = %u\n", name_cnt); if(!wcscmp(names[0], L"testCall")) { *ids = 1; @@ -1343,6 +1347,38 @@ static void test_named_items(void) CHECK_CALLED(OnEnterScript); CHECK_CALLED(OnLeaveScript); + expected_GetIDsOfNames_iface = &visible_named_item; + SET_EXPECT(OnEnterScript); + SET_EXPECT(GetIDsOfNames); + SET_EXPECT(OnLeaveScript); + hr = IActiveScriptParse_ParseScriptText(parse, L"var abc;\n", L"visibleItem", NULL, NULL, 0, 0, 0, NULL, NULL); + ok(hr == S_OK, "ParseScriptText failed: %08x\n", hr); + CHECK_CALLED(OnEnterScript); + CHECK_CALLED(GetIDsOfNames); + CHECK_CALLED(OnLeaveScript); + SET_EXPECT(OnEnterScript); + SET_EXPECT(OnLeaveScript); + hr = IActiveScriptParse_ParseScriptText(parse, L"abc = 5;\n", L"visibleItem", NULL, NULL, 0, 0, 0, NULL, NULL); + ok(hr == S_OK, "ParseScriptText failed: %08x\n", hr); + CHECK_CALLED(OnEnterScript); + CHECK_CALLED(OnLeaveScript); + SET_EXPECT(OnEnterScript); + SET_EXPECT(GetIDsOfNames); + SET_EXPECT(OnLeaveScript); + hr = IActiveScriptParse_ParseScriptText(parse, L"testVar_global = 5;\n", L"visibleItem", NULL, NULL, 0, 0, 0, NULL, NULL); + ok(hr == S_OK, "ParseScriptText failed: %08x\n", hr); + CHECK_CALLED(OnEnterScript); + CHECK_CALLED(GetIDsOfNames); + CHECK_CALLED(OnLeaveScript); + expected_GetIDsOfNames_iface = NULL; + + SET_EXPECT(OnEnterScript); + SET_EXPECT(OnLeaveScript); + hr = IActiveScriptParse_ParseScriptText(parse, L"var abc; testVar_global = 5;\n", L"visibleCodeItem", NULL, NULL, 0, 0, 0, NULL, NULL); + ok(hr == S_OK, "ParseScriptText failed: %08x\n", hr); + CHECK_CALLED(OnEnterScript); + CHECK_CALLED(OnLeaveScript); + SET_EXPECT(OnEnterScript); SET_EXPECT(OnLeaveScript); hr = IActiveScriptParse_ParseScriptText(parse, L"global_this = this;\n", L"globalItem", NULL, NULL, 0, 0, SCRIPTTEXT_ISPERSISTENT, NULL, NULL); -- 2.21.0
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/jscript.c | 3 +++ dlls/jscript/tests/jscript.c | 25 ++++++++++++++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/dlls/jscript/jscript.c b/dlls/jscript/jscript.c index 3fcaab0..eb328d2 100644 --- a/dlls/jscript/jscript.c +++ b/dlls/jscript/jscript.c @@ -757,6 +757,9 @@ static HRESULT WINAPI JScript_SetScriptSite(IActiveScript *iface, hres = retrieve_named_item_disp(pass, item); if(FAILED(hres)) return hres; } + + /* For some reason, Windows clears the CODEONLY flag */ + item->flags &= ~SCRIPTITEM_CODEONLY; } This->site = pass; diff --git a/dlls/jscript/tests/jscript.c b/dlls/jscript/tests/jscript.c index 9329087..c67173c 100644 --- a/dlls/jscript/tests/jscript.c +++ b/dlls/jscript/tests/jscript.c @@ -1504,6 +1504,18 @@ static void test_named_items(void) IDispatchEx_Release(dispex2); IDispatchEx_Release(dispex); + dispex = get_script_dispatch(script, L"persistent"); + SET_EXPECT(OnEnterScript); + SET_EXPECT(OnLeaveScript); + hr = IActiveScriptParse_ParseScriptText(parse, L"this", L"persistent", NULL, NULL, 0, 0, SCRIPTTEXT_ISEXPRESSION, &var, NULL); + ok(hr == S_OK, "ParseScriptText failed: %08x\n", hr); + ok(V_VT(&var) == VT_DISPATCH && V_DISPATCH(&var) == (IDispatch*)dispex, + "Unexpected 'this': V_VT = %d, V_DISPATCH = %p\n", V_VT(&var), V_DISPATCH(&var)); + VariantClear(&var); + CHECK_CALLED(OnEnterScript); + CHECK_CALLED(OnLeaveScript); + IDispatchEx_Release(dispex); + SET_EXPECT(OnEnterScript); SET_EXPECT(OnLeaveScript); hr = IActiveScriptParse_ParseScriptText(parse, L"var x = 13;\n", L"persistent", NULL, NULL, 0, 0, SCRIPTTEXT_ISPERSISTENT, NULL, NULL); @@ -1568,7 +1580,7 @@ static void test_named_items(void) CHECK_CALLED(OnStateChange_CONNECTED); CHECK_CALLED_MULTI(OnEnterScript, 5); CHECK_CALLED_MULTI(OnLeaveScript, 5); - todo_wine CHECK_CALLED(GetIDsOfNames); + CHECK_CALLED(GetIDsOfNames); test_state(script, SCRIPTSTATE_CONNECTED); dispex2 = get_script_dispatch(script, L"persistent"); @@ -1583,6 +1595,17 @@ static void test_named_items(void) CHECK_CALLED(OnEnterScript); CHECK_CALLED(OnLeaveScript); + /* For some reason, CODEONLY flag is removed when persistent items are re-initialized */ + SET_EXPECT(OnEnterScript); + SET_EXPECT(OnLeaveScript); + hr = IActiveScriptParse_ParseScriptText(parse, L"this", L"persistent", NULL, NULL, 0, 0, SCRIPTTEXT_ISEXPRESSION, &var, NULL); + ok(hr == S_OK, "ParseScriptText failed: %08x\n", hr); + ok(V_VT(&var) == VT_DISPATCH && V_DISPATCH(&var) == &persistent_named_item, + "Unexpected 'this': V_VT = %d, V_DISPATCH = %p\n", V_VT(&var), V_DISPATCH(&var)); + VariantClear(&var); + CHECK_CALLED(OnEnterScript); + CHECK_CALLED(OnLeaveScript); + dispex = get_script_dispatch(script, NULL); for (i = 0; i < ARRAY_SIZE(global_idents); i++) { -- 2.21.0
Hi Gabriel, On 23.03.2020 14:53, Gabriel Ivăncescu wrote:
diff --git a/dlls/jscript/jscript.c b/dlls/jscript/jscript.c index 3fcaab0..eb328d2 100644 --- a/dlls/jscript/jscript.c +++ b/dlls/jscript/jscript.c @@ -757,6 +757,9 @@ static HRESULT WINAPI JScript_SetScriptSite(IActiveScript *iface, hres = retrieve_named_item_disp(pass, item); if(FAILED(hres)) return hres; } + + /* For some reason, Windows clears the CODEONLY flag */ + item->flags &= ~SCRIPTITEM_CODEONLY;
This clearly shows that we're still not interpreting those flags correctly and this patch just tries to hide the problem. I think we simply shouldn't depend on the flag but rather on item->disp being set. See the attached patch (on top of your patches). It solves some of test failures that you observed. The remaining problem is that we still store IDispatch for visible items. The attached test shows that we probably shouldn't. Thanks, Jacek
On 24/03/2020 02:34, Jacek Caban wrote:
Hi Gabriel,
On 23.03.2020 14:53, Gabriel Ivăncescu wrote:
diff --git a/dlls/jscript/jscript.c b/dlls/jscript/jscript.c index 3fcaab0..eb328d2 100644 --- a/dlls/jscript/jscript.c +++ b/dlls/jscript/jscript.c @@ -757,6 +757,9 @@ static HRESULT WINAPI JScript_SetScriptSite(IActiveScript *iface, hres = retrieve_named_item_disp(pass, item); if(FAILED(hres)) return hres; } + + /* For some reason, Windows clears the CODEONLY flag */ + item->flags &= ~SCRIPTITEM_CODEONLY;
This clearly shows that we're still not interpreting those flags correctly and this patch just tries to hide the problem. I think we simply shouldn't depend on the flag but rather on item->disp being set. See the attached patch (on top of your patches). It solves some of test failures that you observed. The remaining problem is that we still store IDispatch for visible items. The attached test shows that we probably shouldn't.
I'm not sure if that's going to work. We have tests covering a lot of stuff already; if we change some of it, then some of the tests will fail. If we don't store disp for visible items, so we can pass the new test, then the test at line 1360 will fail. There's also the test at line 1383. Which is same as line 1360, except that it also has the CODEONLY flag (its "disp" was still retrieved earlier, though). In that case, the disp is not used. Both of them have the VISIBLE flag -- and yet only the former one accesses its disp. I don't know how to solve this situation without checking the flags.
On 24/03/2020 15:59, Gabriel Ivăncescu wrote:
On 24/03/2020 02:34, Jacek Caban wrote:
Hi Gabriel,
On 23.03.2020 14:53, Gabriel Ivăncescu wrote:
diff --git a/dlls/jscript/jscript.c b/dlls/jscript/jscript.c index 3fcaab0..eb328d2 100644 --- a/dlls/jscript/jscript.c +++ b/dlls/jscript/jscript.c @@ -757,6 +757,9 @@ static HRESULT WINAPI JScript_SetScriptSite(IActiveScript *iface, hres = retrieve_named_item_disp(pass, item); if(FAILED(hres)) return hres; } + + /* For some reason, Windows clears the CODEONLY flag */ + item->flags &= ~SCRIPTITEM_CODEONLY;
This clearly shows that we're still not interpreting those flags correctly and this patch just tries to hide the problem. I think we simply shouldn't depend on the flag but rather on item->disp being set. See the attached patch (on top of your patches). It solves some of test failures that you observed. The remaining problem is that we still store IDispatch for visible items. The attached test shows that we probably shouldn't.
I'm not sure if that's going to work. We have tests covering a lot of stuff already; if we change some of it, then some of the tests will fail. If we don't store disp for visible items, so we can pass the new test, then the test at line 1360 will fail.
There's also the test at line 1383. Which is same as line 1360, except that it also has the CODEONLY flag (its "disp" was still retrieved earlier, though). In that case, the disp is not used.
Both of them have the VISIBLE flag -- and yet only the former one accesses its disp. I don't know how to solve this situation without checking the flags.
So, I will end up resending the last patch as-is, with changes to the other tests as suggested. The reason is that I can't figure out a way to solve this without actually using the flag. I tried changes to visible items (release old one in lookup, move it after it's looked up instead of holding another reference, etc) and they each made several other tests fail. I'm assuming that, if Windows does clear the flag, it probably resets it for some reason (could be a bug, but vbscript also does it). If you still think it's a bad idea, then feel free to ignore/reject just the last patch in the series. The other patches should work fine without it, with the only thing being a todo_wine added: todo_wine CHECK_CALLED(GetIDsOfNames_persistent); which is not that big of a deal if it's there... And so, if the last patch is rejected, when we figure out how to solve this without clearing the flag, we can get rid of the todo_wine then. Thanks, Gabriel
On 24.03.2020 17:53, Gabriel Ivăncescu wrote:
So, I will end up resending the last patch as-is, with changes to the other tests as suggested.
A more convincing test always helps... Anyway, I tested it a bit more and it looks like we can, indeed, clear this flags. Thanks, Jacek
participants (2)
-
Gabriel Ivăncescu -
Jacek Caban