[PATCH v3 1/5] vbscript: Fail if we have no context in InvokeEx.
The current code can't cope with it, so do the same thing as in GetDispID, to prevent a possible crash. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- v3: Create fresh script dispatches for persistent code contexts, to avoid corner cases with externally held dispatches. dlls/vbscript/vbdisp.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dlls/vbscript/vbdisp.c b/dlls/vbscript/vbdisp.c index c14cd7b..36eba21 100644 --- a/dlls/vbscript/vbdisp.c +++ b/dlls/vbscript/vbdisp.c @@ -1388,6 +1388,9 @@ static HRESULT WINAPI ScriptDisp_InvokeEx(IDispatchEx *iface, DISPID id, LCID lc TRACE("(%p)->(%x %x %x %p %p %p %p)\n", This, id, lcid, wFlags, pdp, pvarRes, pei, pspCaller); + if (!This->ctx) + return E_UNEXPECTED; + if (id & DISPID_FUNCTION_MASK) { id &= ~DISPID_FUNCTION_MASK; -- 2.21.0
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Simplifies the 4th patch. dlls/vbscript/interp.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/dlls/vbscript/interp.c b/dlls/vbscript/interp.c index b328674..f0820be 100644 --- a/dlls/vbscript/interp.c +++ b/dlls/vbscript/interp.c @@ -110,6 +110,22 @@ static BOOL lookup_global_vars(ScriptDisp *script, const WCHAR *name, ref_t *ref return FALSE; } +static BOOL lookup_global_funcs(ScriptDisp *script, const WCHAR *name, ref_t *ref) +{ + function_t **funcs = script->global_funcs; + size_t i, cnt = script->global_funcs_cnt; + + for(i = 0; i < cnt; i++) { + if(!wcsicmp(funcs[i]->name, name)) { + ref->type = REF_FUNC; + ref->u.f = funcs[i]; + return TRUE; + } + } + + return FALSE; +} + static HRESULT lookup_identifier(exec_ctx_t *ctx, BSTR name, vbdisp_invoke_type_t invoke_type, ref_t *ref) { ScriptDisp *script_obj = ctx->script->script_obj; @@ -177,15 +193,8 @@ static HRESULT lookup_identifier(exec_ctx_t *ctx, BSTR name, vbdisp_invoke_type_ if(lookup_global_vars(script_obj, name, ref)) return S_OK; - - for(i = 0; i < script_obj->global_funcs_cnt; i++) { - function_t *func = script_obj->global_funcs[i]; - if(!wcsicmp(func->name, name)) { - ref->type = REF_FUNC; - ref->u.f = func; - return S_OK; - } - } + if(lookup_global_funcs(script_obj, name, ref)) + return S_OK; hres = get_builtin_id(ctx->script->global_obj, name, &id); if(SUCCEEDED(hres)) { -- 2.21.0
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- This is needed for next patch to avoid forward declaring ScriptDisp. dlls/vbscript/vbscript.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/dlls/vbscript/vbscript.h b/dlls/vbscript/vbscript.h index f37b0ce..589abc3 100644 --- a/dlls/vbscript/vbscript.h +++ b/dlls/vbscript/vbscript.h @@ -55,14 +55,6 @@ typedef struct _vbscode_t vbscode_t; typedef struct _script_ctx_t script_ctx_t; typedef struct _vbdisp_t vbdisp_t; -typedef struct named_item_t { - IDispatch *disp; - DWORD flags; - LPWSTR name; - - struct list entry; -} named_item_t; - typedef enum { VBDISP_CALLGET, VBDISP_LET, @@ -156,6 +148,14 @@ typedef struct { script_ctx_t *ctx; } BuiltinDisp; +typedef struct named_item_t { + IDispatch *disp; + DWORD flags; + LPWSTR name; + + struct list entry; +} named_item_t; + HRESULT create_vbdisp(const class_desc_t*,vbdisp_t**) DECLSPEC_HIDDEN; HRESULT disp_get_id(IDispatch*,BSTR,vbdisp_invoke_type_t,BOOL,DISPID*) DECLSPEC_HIDDEN; HRESULT vbdisp_get_id(vbdisp_t*,BSTR,vbdisp_invoke_type_t,BOOL,DISPID*) DECLSPEC_HIDDEN; -- 2.21.0
Each named item should have its own associated script dispatch object. Identifiers are added to this object, but code does look into the global script object as well. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/vbscript/compile.c | 44 ++++++++++++++++++++++++--------------- dlls/vbscript/interp.c | 29 +++++++++++++++++++------- dlls/vbscript/vbscript.c | 45 +++++++++++++++++++++++++++++++++++++--- dlls/vbscript/vbscript.h | 2 ++ 4 files changed, 92 insertions(+), 28 deletions(-) diff --git a/dlls/vbscript/compile.c b/dlls/vbscript/compile.c index 9888109..faaa6be 100644 --- a/dlls/vbscript/compile.c +++ b/dlls/vbscript/compile.c @@ -1778,26 +1778,30 @@ static HRESULT compile_class(compile_ctx_t *ctx, class_decl_t *class_decl) return S_OK; } -static BOOL lookup_script_identifier(script_ctx_t *script, const WCHAR *identifier) +static BOOL lookup_script_identifier(compile_ctx_t *ctx, script_ctx_t *script, const WCHAR *identifier) { - ScriptDisp *obj = script->script_obj; + ScriptDisp *contexts[] = { ctx->code->context_obj, script->script_obj }; class_desc_t *class; vbscode_t *code; - unsigned i; + unsigned c, i; - for(i = 0; i < obj->global_vars_cnt; i++) { - if(!wcsicmp(obj->global_vars[i]->name, identifier)) - return TRUE; - } + for(c = 0; c < ARRAY_SIZE(contexts); c++) { + if(!contexts[c]) continue; - for(i = 0; i < obj->global_funcs_cnt; i++) { - if(!wcsicmp(obj->global_funcs[i]->name, identifier)) - return TRUE; - } + for(i = 0; i < contexts[c]->global_vars_cnt; i++) { + if(!wcsicmp(contexts[c]->global_vars[i]->name, identifier)) + return TRUE; + } - for(class = obj->classes; class; class = class->next) { - if(!wcsicmp(class->name, identifier)) - return TRUE; + for(i = 0; i < contexts[c]->global_funcs_cnt; i++) { + if(!wcsicmp(contexts[c]->global_funcs[i]->name, identifier)) + return TRUE; + } + + for(class = contexts[c]->classes; class; class = class->next) { + if(!wcsicmp(class->name, identifier)) + return TRUE; + } } LIST_FOR_EACH_ENTRY(code, &script->code_list, vbscode_t, entry) { @@ -1805,7 +1809,7 @@ static BOOL lookup_script_identifier(script_ctx_t *script, const WCHAR *identifi var_desc_t *vars = code->main_code.vars; function_t *func; - if(!code->pending_exec) + if(!code->pending_exec || (code->context_obj && code->context_obj != ctx->code->context_obj)) continue; for(i = 0; i < var_cnt; i++) { @@ -1834,14 +1838,14 @@ static HRESULT check_script_collisions(compile_ctx_t *ctx, script_ctx_t *script) class_desc_t *class; for(i = 0; i < var_cnt; i++) { - if(lookup_script_identifier(script, vars[i].name)) { + if(lookup_script_identifier(ctx, script, vars[i].name)) { FIXME("%s: redefined\n", debugstr_w(vars[i].name)); return E_FAIL; } } for(class = ctx->code->classes; class; class = class->next) { - if(lookup_script_identifier(script, class->name)) { + if(lookup_script_identifier(ctx, script, class->name)) { FIXME("%s: redefined\n", debugstr_w(class->name)); return E_FAIL; } @@ -1862,6 +1866,8 @@ void release_vbscode(vbscode_t *code) if(code->context) IDispatch_Release(code->context); + if(code->context_obj) + IDispatchEx_Release(&code->context_obj->IDispatchEx_iface); heap_pool_free(&code->heap); heap_free(code->bstr_pool); @@ -1944,6 +1950,10 @@ HRESULT compile_script(script_ctx_t *script, const WCHAR *src, const WCHAR *item code = ctx.code = alloc_vbscode(&ctx, src, cookie, start_line); if(!ctx.code) return E_OUTOFMEMORY; + if(item) { + code->context_obj = item->script_obj; + IDispatchEx_AddRef(&code->context_obj->IDispatchEx_iface); + } hres = parse_script(&ctx.parser, code->source, delimiter, flags); if(FAILED(hres)) { diff --git a/dlls/vbscript/interp.c b/dlls/vbscript/interp.c index f0820be..7ff3111 100644 --- a/dlls/vbscript/interp.c +++ b/dlls/vbscript/interp.c @@ -191,6 +191,13 @@ static HRESULT lookup_identifier(exec_ctx_t *ctx, BSTR name, vbdisp_invoke_type_ } } + if(ctx->code->context_obj) { + if(lookup_global_vars(ctx->code->context_obj, name, ref)) + return S_OK; + if(lookup_global_funcs(ctx->code->context_obj, name, ref)) + return S_OK; + } + if(lookup_global_vars(script_obj, name, ref)) return S_OK; if(lookup_global_funcs(script_obj, name, ref)) @@ -230,7 +237,7 @@ static HRESULT lookup_identifier(exec_ctx_t *ctx, BSTR name, vbdisp_invoke_type_ static HRESULT add_dynamic_var(exec_ctx_t *ctx, const WCHAR *name, BOOL is_const, VARIANT **out_var) { - ScriptDisp *script_obj = ctx->script->script_obj; + ScriptDisp *script_obj = ctx->code->context_obj ? ctx->code->context_obj : ctx->script->script_obj; dynamic_var_t *new_var; heap_pool_t *heap; WCHAR *str; @@ -1113,7 +1120,7 @@ static HRESULT interp_deref(exec_ctx_t *ctx) static HRESULT interp_new(exec_ctx_t *ctx) { const WCHAR *arg = ctx->instr->arg1.bstr; - class_desc_t *class_desc; + class_desc_t *class_desc = NULL; vbdisp_t *obj; VARIANT v; HRESULT hres; @@ -1131,10 +1138,14 @@ static HRESULT interp_new(exec_ctx_t *ctx) return stack_push(ctx, &v); } - for(class_desc = ctx->script->script_obj->classes; class_desc; class_desc = class_desc->next) { - if(!wcsicmp(class_desc->name, arg)) - break; - } + if(ctx->code->context_obj) + for(class_desc = ctx->code->context_obj->classes; class_desc; class_desc = class_desc->next) + if(!wcsicmp(class_desc->name, arg)) + break; + if(!class_desc) + for(class_desc = ctx->script->script_obj->classes; class_desc; class_desc = class_desc->next) + if(!wcsicmp(class_desc->name, arg)) + break; if(!class_desc) { FIXME("Class %s not found\n", debugstr_w(arg)); return E_FAIL; @@ -1151,7 +1162,7 @@ static HRESULT interp_new(exec_ctx_t *ctx) static HRESULT interp_dim(exec_ctx_t *ctx) { - ScriptDisp *script_obj = ctx->script->script_obj; + ScriptDisp *script_obj = ctx->code->context_obj ? ctx->code->context_obj : ctx->script->script_obj; const BSTR ident = ctx->instr->arg1.bstr; const unsigned array_id = ctx->instr->arg2.uint; const array_desc_t *array_desc; @@ -1520,7 +1531,9 @@ static HRESULT interp_me(exec_ctx_t *ctx) else if(ctx->script->host_global) disp = ctx->script->host_global; else - disp = (IDispatch*)&ctx->script->script_obj->IDispatchEx_iface; + disp = ctx->code->context_obj + ? (IDispatch*)&ctx->code->context_obj->IDispatchEx_iface + : (IDispatch*)&ctx->script->script_obj->IDispatchEx_iface; IDispatch_AddRef(disp); V_VT(&v) = VT_DISPATCH; diff --git a/dlls/vbscript/vbscript.c b/dlls/vbscript/vbscript.c index 88574fe..15dde7c 100644 --- a/dlls/vbscript/vbscript.c +++ b/dlls/vbscript/vbscript.c @@ -93,7 +93,7 @@ static inline BOOL is_started(VBScript *This) static HRESULT exec_global_code(script_ctx_t *ctx, vbscode_t *code, VARIANT *res) { - ScriptDisp *obj = ctx->script_obj; + ScriptDisp *obj = code->context_obj ? code->context_obj : ctx->script_obj; function_t *func_iter, **new_funcs; dynamic_var_t *var, **new_vars; size_t cnt, i; @@ -196,7 +196,7 @@ named_item_t *lookup_named_item(script_ctx_t *ctx, const WCHAR *name, unsigned f LIST_FOR_EACH_ENTRY(item, &ctx->named_items, named_item_t, entry) { if((item->flags & flags) == flags && !wcsicmp(item->name, name)) { - if(!item->disp) { + if(!item->disp && !(item->flags & SCRIPTITEM_CODEONLY)) { IUnknown *unk; hres = IActiveScriptSite_GetItemInfo(ctx->site, item->name, @@ -234,6 +234,7 @@ static void release_script(script_ctx_t *ctx) { code->pending_exec = TRUE; if(code->last_class) code->last_class->next = NULL; + if(code->context_obj) code->context_obj->ctx = NULL; } else { @@ -248,6 +249,8 @@ static void release_script(script_ctx_t *ctx) list_remove(&iter->entry); if(iter->disp) IDispatch_Release(iter->disp); + iter->script_obj->ctx = NULL; + IDispatchEx_Release(&iter->script_obj->IDispatchEx_iface); heap_free(iter->name); heap_free(iter); } @@ -504,6 +507,7 @@ static ULONG WINAPI VBScript_Release(IActiveScript *iface) static HRESULT WINAPI VBScript_SetScriptSite(IActiveScript *iface, IActiveScriptSite *pass) { VBScript *This = impl_from_IActiveScript(iface); + vbscode_t *code; LCID lcid; HRESULT hres; @@ -522,6 +526,27 @@ static HRESULT WINAPI VBScript_SetScriptSite(IActiveScript *iface, IActiveScript if(FAILED(hres)) return hres; + /* Create fresh script dispatches for persistent code contexts */ + LIST_FOR_EACH_ENTRY(code, &This->ctx->code_list, vbscode_t, entry) + { + if(code->context_obj) + { + ScriptDisp *disp; + + hres = create_script_disp(This->ctx, &disp); + if(FAILED(hres)) + { + This->ctx->script_obj->ctx = NULL; + IDispatchEx_Release(&This->ctx->script_obj->IDispatchEx_iface); + return hres; + } + + code->context_obj->ctx = NULL; + IDispatchEx_Release(&code->context_obj->IDispatchEx_iface); + code->context_obj = disp; + } + } + This->ctx->site = pass; IActiveScriptSite_AddRef(This->ctx->site); @@ -661,6 +686,11 @@ static HRESULT WINAPI VBScript_AddNamedItem(IActiveScript *iface, LPCOLESTR pstr item->disp = disp; item->flags = dwFlags; item->name = heap_strdupW(pstrName); + hres = create_script_disp(This->ctx, &item->script_obj); + if(FAILED(hres)) { + heap_free(item->name); + item->name = NULL; + } if(!item->name) { if(disp) IDispatch_Release(disp); @@ -683,6 +713,7 @@ static HRESULT WINAPI VBScript_AddTypeLib(IActiveScript *iface, REFGUID rguidTyp static HRESULT WINAPI VBScript_GetScriptDispatch(IActiveScript *iface, LPCOLESTR pstrItemName, IDispatch **ppdisp) { VBScript *This = impl_from_IActiveScript(iface); + ScriptDisp *script_obj; TRACE("(%p)->(%s %p)\n", This, debugstr_w(pstrItemName), ppdisp); @@ -694,7 +725,15 @@ static HRESULT WINAPI VBScript_GetScriptDispatch(IActiveScript *iface, LPCOLESTR return E_UNEXPECTED; } - *ppdisp = (IDispatch*)&This->ctx->script_obj->IDispatchEx_iface; + if(pstrItemName) { + named_item_t *item = lookup_named_item(This->ctx, pstrItemName, 0); + if(!item) return E_INVALIDARG; + script_obj = item->script_obj; + } + else + script_obj = This->ctx->script_obj; + + *ppdisp = (IDispatch*)&script_obj->IDispatchEx_iface; IDispatch_AddRef(*ppdisp); return S_OK; } diff --git a/dlls/vbscript/vbscript.h b/dlls/vbscript/vbscript.h index 589abc3..885e065 100644 --- a/dlls/vbscript/vbscript.h +++ b/dlls/vbscript/vbscript.h @@ -152,6 +152,7 @@ typedef struct named_item_t { IDispatch *disp; DWORD flags; LPWSTR name; + ScriptDisp *script_obj; struct list entry; } named_item_t; @@ -349,6 +350,7 @@ struct _vbscode_t { BOOL is_persistent; function_t main_code; IDispatch *context; + ScriptDisp *context_obj; BSTR *bstr_pool; unsigned bstr_pool_size; -- 2.21.0
Sorry for the noise, but another idea just dawned on me. What if we ref count the named_item_t itself, and store it from vbscode_t instead. Then, we set the item->script_obj to NULL when it has to be recreated (for persistent code). This shouldn't affect external users that hold ScriptDisp, since it's only referenced from vbscode_t. Of course we still unlink the named item from the list when script is uninitialized, since that's what tests show. This approach should, I think, fix all corner cases, even with multiple script dispatches referring to same named item. Thoughts?
Hi Gabriel, On 05.02.2020 18:38, Gabriel Ivăncescu wrote:
Sorry for the noise, but another idea just dawned on me.
What if we ref count the named_item_t itself, and store it from vbscode_t instead. Then, we set the item->script_obj to NULL when it has to be recreated (for persistent code). This shouldn't affect external users that hold ScriptDisp, since it's only referenced from vbscode_t.
Of course we still unlink the named item from the list when script is uninitialized, since that's what tests show.
This approach should, I think, fix all corner cases, even with multiple script dispatches referring to same named item.
Thoughts?
Shouldn't we just ignore SCRIPTTEXT_ISPERSISTENT flag when it's executed in context of named item? Your tests seem to indicate that such code does not survive releasing script. Is there any way they could affect a reinitialized script engine? Thanks, Jacek
Hi Jacek, On 05/02/2020 22:54, Jacek Caban wrote:
Hi Gabriel,
On 05.02.2020 18:38, Gabriel Ivăncescu wrote:
Sorry for the noise, but another idea just dawned on me.
What if we ref count the named_item_t itself, and store it from vbscode_t instead. Then, we set the item->script_obj to NULL when it has to be recreated (for persistent code). This shouldn't affect external users that hold ScriptDisp, since it's only referenced from vbscode_t.
Of course we still unlink the named item from the list when script is uninitialized, since that's what tests show.
This approach should, I think, fix all corner cases, even with multiple script dispatches referring to same named item.
Thoughts?
Shouldn't we just ignore SCRIPTTEXT_ISPERSISTENT flag when it's executed in context of named item? Your tests seem to indicate that such code does not survive releasing script. Is there any way they could affect a reinitialized script engine?
Thanks,
Jacek
So, I've added a lot more tests as suggested (using arrays/loops to make it easier to extend), which revealed some interesting quirks that I'll fix. Now, referring to this part: it seems the code is indeed persistent, so we can't clear the flag, even though it's not "accessible". Hence why it probably sent the OnEnterScript/OnLeaveScript in the first place. With my refcounting named_item_t idea, this should be done without special cases for those callbacks and actually re-execute it properly. I used a hacky test to verify that the script with persistent "code context" does get re-executed even with a dangling context -- I used a busy loop like the following: w = DateAdd("s", 5, Now()) Do Until (Now() > w) Loop And printed a trace() when script was uninitialized. It then waited another 5 seconds when it was restarted, proving that the dangling context code was executed again. Now, I know that sort of hack is unacceptable for a wine test. Do you have some idea of how to test for side effects, or should I just let it be? (I can't use visibleItem because it dies when script is uninitialized, and even if you re-add it Windows will fail when restarting the script). After all, the named_item_t refcounting code will still fix the OnEnterScript/OnLeaveScript tests, so technically we do have a minor test for it. Will that be enough? Thanks, Gabriel
On 06.02.2020 16:55, Gabriel Ivăncescu wrote:
Hi Jacek,
On 05/02/2020 22:54, Jacek Caban wrote:
Hi Gabriel,
On 05.02.2020 18:38, Gabriel Ivăncescu wrote:
Sorry for the noise, but another idea just dawned on me.
What if we ref count the named_item_t itself, and store it from vbscode_t instead. Then, we set the item->script_obj to NULL when it has to be recreated (for persistent code). This shouldn't affect external users that hold ScriptDisp, since it's only referenced from vbscode_t.
Of course we still unlink the named item from the list when script is uninitialized, since that's what tests show.
This approach should, I think, fix all corner cases, even with multiple script dispatches referring to same named item.
Thoughts?
Shouldn't we just ignore SCRIPTTEXT_ISPERSISTENT flag when it's executed in context of named item? Your tests seem to indicate that such code does not survive releasing script. Is there any way they could affect a reinitialized script engine?
Thanks,
Jacek
So, I've added a lot more tests as suggested (using arrays/loops to make it easier to extend), which revealed some interesting quirks that I'll fix.
Now, referring to this part: it seems the code is indeed persistent, so we can't clear the flag, even though it's not "accessible". Hence why it probably sent the OnEnterScript/OnLeaveScript in the first place. With my refcounting named_item_t idea, this should be done without special cases for those callbacks and actually re-execute it properly.
I used a hacky test to verify that the script with persistent "code context" does get re-executed even with a dangling context -- I used a busy loop like the following:
w = DateAdd("s", 5, Now()) Do Until (Now() > w) Loop
And printed a trace() when script was uninitialized. It then waited another 5 seconds when it was restarted, proving that the dangling context code was executed again.
Now, I know that sort of hack is unacceptable for a wine test. Do you have some idea of how to test for side effects, or should I just let it be? (I can't use visibleItem because it dies when script is uninitialized, and even if you re-add it Windows will fail when restarting the script).
After all, the named_item_t refcounting code will still fix the OnEnterScript/OnLeaveScript tests, so technically we do have a minor test for it. Will that be enough?
You could probably just use "testCall" there as a test script. Before reinitializing script, you can make sure that it's exposed by a new named item in in global scope and use SET_EXPECT(testCall)/CHECK_CALLED(testCall). And for ref-counting named items, it could also use a bit more testing. My guess would be that we should store named item string (not reference) inside vbscode_t and lookup named items when the code is actually executed. It means that if host adds a new named item with the same name before reinitializing engine, the new named item will be used. Experimenting could show if that guess is right. Thanks, Jacek
Hi Jacek, I sent a (hopefully final) version of the patch series. It should handle all corner cases now. But I'll try to explain my approach below. On 06/02/2020 19:42, Jacek Caban wrote:
On 06.02.2020 16:55, Gabriel Ivăncescu wrote:
Hi Jacek,
On 05/02/2020 22:54, Jacek Caban wrote:
Hi Gabriel,
On 05.02.2020 18:38, Gabriel Ivăncescu wrote:
Sorry for the noise, but another idea just dawned on me.
What if we ref count the named_item_t itself, and store it from vbscode_t instead. Then, we set the item->script_obj to NULL when it has to be recreated (for persistent code). This shouldn't affect external users that hold ScriptDisp, since it's only referenced from vbscode_t.
Of course we still unlink the named item from the list when script is uninitialized, since that's what tests show.
This approach should, I think, fix all corner cases, even with multiple script dispatches referring to same named item.
Thoughts?
Shouldn't we just ignore SCRIPTTEXT_ISPERSISTENT flag when it's executed in context of named item? Your tests seem to indicate that such code does not survive releasing script. Is there any way they could affect a reinitialized script engine?
Thanks,
Jacek
So, I've added a lot more tests as suggested (using arrays/loops to make it easier to extend), which revealed some interesting quirks that I'll fix.
Now, referring to this part: it seems the code is indeed persistent, so we can't clear the flag, even though it's not "accessible". Hence why it probably sent the OnEnterScript/OnLeaveScript in the first place. With my refcounting named_item_t idea, this should be done without special cases for those callbacks and actually re-execute it properly.
I used a hacky test to verify that the script with persistent "code context" does get re-executed even with a dangling context -- I used a busy loop like the following:
w = DateAdd("s", 5, Now()) Do Until (Now() > w) Loop
And printed a trace() when script was uninitialized. It then waited another 5 seconds when it was restarted, proving that the dangling context code was executed again.
Now, I know that sort of hack is unacceptable for a wine test. Do you have some idea of how to test for side effects, or should I just let it be? (I can't use visibleItem because it dies when script is uninitialized, and even if you re-add it Windows will fail when restarting the script).
After all, the named_item_t refcounting code will still fix the OnEnterScript/OnLeaveScript tests, so technically we do have a minor test for it. Will that be enough?
You could probably just use "testCall" there as a test script. Before reinitializing script, you can make sure that it's exposed by a new named item in in global scope and use SET_EXPECT(testCall)/CHECK_CALLED(testCall).
I found out a simpler way. First I set a variable in global persistent code to a value, then from within the persistent named item context, I change that variable's value. When script is uninitialized, the named item is technically gone, but the context the code runs in isn't, and must be recreated. To prove this, when script is restarted, the variable is initialized with the original value (from global persistent code), but because the "code context" code was also persistent it will change the variable again to the new value. Even if its context is inaccessible now, it can still access the global scope. This new value is then tested (runtime error thrown otherwise).
And for ref-counting named items, it could also use a bit more testing. My guess would be that we should store named item string (not reference) inside vbscode_t and lookup named items when the code is actually executed. It means that if host adds a new named item with the same name before reinitializing engine, the new named item will be used. Experimenting could show if that guess is right.
I'll try to summarize the changes here, because it's not technically correct that the entire named item is refcounted, only its memory. Most of the named item, such as its Dispatch object supplied by the script site, or its name, must be released as normal, when the item is detached from the list. In fact, even the "code context" script dispatch must be released when that happens! This is already covered by existing tests, and not doing so would break them. The reason I refcount it is due to persistent code that must still have a sub-context, but with an inaccessible item. We create new script dispatches for all persistent code, including those with sub-contexts, just as we do for the global script dispatch. However, because two vbscode_t can refer to the *same* sub-context, we have to use ref-counted named items (even if they're otherwise detached) so that when we create them inside of the (detached) named item, the first one will create the script dispatch, and subsequent ones referring to the same named item will use the *same* script dispatch, as they should. (i.e. one script dispatch created for each detached named item, no matter how many vbscode_t refer to it) This is where my previous attempt was wrong: it created new script dispatches for each vbscode_t with a NULL script dispatch, even if two of them were supposed to share the same context. An extra indirection here (ref counted named item) is necessary. Other than that, the ref count itself is not test-able, it's just an implementation detail. That's on purpose of course. The tests should be pretty exhaustive this time around. Also, it should be fairly straightforward to implement the SCRIPTITEM_ISPERSISTENT flag for named items in the future.
Hi Gabriel, It looks much better now. I can't do full review until we resolve the issue from the other mail, so below is just one thing I spotted already. On 05.02.2020 17:38, Gabriel Ivăncescu wrote:
@@ -196,7 +196,7 @@ named_item_t *lookup_named_item(script_ctx_t *ctx, const WCHAR *name, unsigned f
LIST_FOR_EACH_ENTRY(item, &ctx->named_items, named_item_t, entry) { if((item->flags & flags) == flags && !wcsicmp(item->name, name)) { - if(!item->disp) { + if(!item->disp && !(item->flags & SCRIPTITEM_CODEONLY)) {
This deserves a separated patch, preferably with a test (and ideally with a test in the same patch as the fix; I generally don't mind sending large set of tests in a single patch for large features like the whole patch series, but for isolated issues with a small fix it makes reviewing much nicer if I can see a test without going searching it in a large test patch). Thanks, Jacek
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/vbscript/tests/vbscript.c | 158 ++++++++++++++++++++++++++++++--- 1 file changed, 144 insertions(+), 14 deletions(-) diff --git a/dlls/vbscript/tests/vbscript.c b/dlls/vbscript/tests/vbscript.c index 10af993..d2e7e81 100644 --- a/dlls/vbscript/tests/vbscript.c +++ b/dlls/vbscript/tests/vbscript.c @@ -93,6 +93,7 @@ DEFINE_EXPECT(OnStateChange_CONNECTED); DEFINE_EXPECT(OnStateChange_DISCONNECTED); DEFINE_EXPECT(OnStateChange_CLOSED); DEFINE_EXPECT(OnStateChange_INITIALIZED); +DEFINE_EXPECT(OnScriptError); DEFINE_EXPECT(OnEnterScript); DEFINE_EXPECT(OnLeaveScript); DEFINE_EXPECT(GetItemInfo_global); @@ -174,9 +175,12 @@ static HRESULT WINAPI Dispatch_GetIDsOfNames(IDispatch *iface, REFIID riid, LPOL LCID lcid, DISPID *ids) { ok(name_cnt == 1, "name_cnt = %u\n", name_cnt); - ok(!wcscmp(names[0], L"testCall"), "names[0] = %s\n", wine_dbgstr_w(names[0])); *ids = 1; - return S_OK; + if (!wcscmp(names[0], L"testCall")) + return S_OK; + + ok(!wcscmp(names[0], L"testSub_global"), "names[0] = %s\n", wine_dbgstr_w(names[0])); + return DISP_E_UNKNOWNNAME; } static HRESULT WINAPI Dispatch_Invoke(IDispatch *iface, DISPID id, REFIID riid, LCID lcid, WORD flags, @@ -308,7 +312,7 @@ static HRESULT WINAPI ActiveScriptSite_OnStateChange(IActiveScriptSite *iface, S static HRESULT WINAPI ActiveScriptSite_OnScriptError(IActiveScriptSite *iface, IActiveScriptError *pscripterror) { - ok(0, "unexpected call\n"); + CHECK_EXPECT(OnScriptError); return E_NOTIMPL; } @@ -431,14 +435,14 @@ static void test_safety(IActiveScript *script) IObjectSafety_Release(safety); } -static IDispatchEx *get_script_dispatch(IActiveScript *script) +static IDispatchEx *get_script_dispatch(IActiveScript *script, const WCHAR *item_name) { IDispatchEx *dispex; IDispatch *disp; HRESULT hres; disp = (void*)0xdeadbeef; - hres = IActiveScript_GetScriptDispatch(script, NULL, &disp); + hres = IActiveScript_GetScriptDispatch(script, item_name, &disp); ok(hres == S_OK, "GetScriptDispatch failed: %08x\n", hres); if(FAILED(hres)) return NULL; @@ -531,7 +535,7 @@ static void test_scriptdisp(void) ok(hres == S_OK, "SetScriptSite failed: %08x\n", hres); CHECK_CALLED(GetLCID); - script_disp2 = get_script_dispatch(vbscript); + script_disp2 = get_script_dispatch(vbscript, NULL); test_state(vbscript, SCRIPTSTATE_UNINITIALIZED); @@ -549,7 +553,7 @@ static void test_scriptdisp(void) test_state(vbscript, SCRIPTSTATE_CONNECTED); - script_disp = get_script_dispatch(vbscript); + script_disp = get_script_dispatch(vbscript, NULL); ok(script_disp == script_disp2, "script_disp != script_disp2\n"); IDispatchEx_Release(script_disp2); @@ -676,7 +680,7 @@ static void test_code_persistence(void) ok(hr == S_OK, "ParseScriptText failed: %08x\n", hr); /* Pending code does not add identifiers to the global scope */ - script_disp = get_script_dispatch(vbscript); + script_disp = get_script_dispatch(vbscript, NULL); id = 0; get_disp_id(script_disp, "x", DISP_E_UNKNOWNNAME, &id); ok(id == -1, "id = %d, expected -1\n", id); @@ -715,7 +719,7 @@ static void test_code_persistence(void) CHECK_CALLED_MULTI(OnLeaveScript, 2); test_state(vbscript, SCRIPTSTATE_CONNECTED); - script_disp = get_script_dispatch(vbscript); + script_disp = get_script_dispatch(vbscript, NULL); id = 0; get_disp_id(script_disp, "x", DISP_E_UNKNOWNNAME, &id); ok(id == -1, "id = %d, expected -1\n", id); @@ -761,7 +765,7 @@ static void test_code_persistence(void) CHECK_CALLED(GetLCID); CHECK_CALLED(OnStateChange_INITIALIZED); - script_disp = get_script_dispatch(vbscript); + script_disp = get_script_dispatch(vbscript, NULL); id = 0; get_disp_id(script_disp, "z", DISP_E_UNKNOWNNAME, &id); ok(id == -1, "id = %d, expected -1\n", id); @@ -777,7 +781,7 @@ static void test_code_persistence(void) CHECK_CALLED(OnLeaveScript); test_state(vbscript, SCRIPTSTATE_CONNECTED); - script_disp = get_script_dispatch(vbscript); + script_disp = get_script_dispatch(vbscript, NULL); id = 0; get_disp_id(script_disp, "z", S_OK, &id); ok(id != -1, "id = -1\n"); @@ -840,7 +844,7 @@ static void test_code_persistence(void) CHECK_CALLED(OnStateChange_CONNECTED); test_state(vbscript, SCRIPTSTATE_CONNECTED); - script_disp = get_script_dispatch(vbscript); + script_disp = get_script_dispatch(vbscript, NULL); id = 0; get_disp_id(script_disp, "y", DISP_E_UNKNOWNNAME, &id); ok(id == -1, "id = %d, expected -1\n", id); @@ -953,7 +957,7 @@ static void test_script_typeinfo(void) "implicit = 10\n" "dim obj\nset obj = new C\n"); - script_disp = get_script_dispatch(vbscript); + script_disp = get_script_dispatch(vbscript, NULL); hr = IDispatchEx_QueryInterface(script_disp, &IID_ITypeInfo, (void**)&typeinfo); ok(hr == E_NOINTERFACE, "QueryInterface(IID_ITypeInfo) returned: %08x\n", hr); hr = IDispatchEx_GetTypeInfo(script_disp, 1, LOCALE_USER_DEFAULT, &typeinfo); @@ -1446,7 +1450,7 @@ static void test_vbscript_uninitializing(void) test_state(script, SCRIPTSTATE_CONNECTED); - dispex = get_script_dispatch(script); + dispex = get_script_dispatch(script, NULL); ok(dispex != NULL, "dispex == NULL\n"); if(dispex) IDispatchEx_Release(dispex); @@ -1702,8 +1706,11 @@ static void test_vbscript_initializing(void) static void test_named_items(void) { + IDispatchEx *script_disp, *script_disp2; IActiveScriptParse *parse; IActiveScript *script; + IDispatch *disp; + DISPID id; ULONG ref; HRESULT hres; @@ -1718,6 +1725,8 @@ static void test_named_items(void) ok(hres == E_UNEXPECTED, "AddNamedItem returned: %08x\n", hres); hres = IActiveScript_AddNamedItem(script, L"globalItem", SCRIPTITEM_GLOBALMEMBERS); ok(hres == E_UNEXPECTED, "AddNamedItem returned: %08x\n", hres); + hres = IActiveScript_AddNamedItem(script, L"code context", SCRIPTITEM_CODEONLY); + ok(hres == E_UNEXPECTED, "AddNamedItem returned: %08x\n", hres); SET_EXPECT(GetLCID); hres = IActiveScript_SetScriptSite(script, &ActiveScriptSite); @@ -1731,10 +1740,19 @@ static void test_named_items(void) hres = IActiveScript_AddNamedItem(script, L"visibleItem", SCRIPTITEM_ISVISIBLE); ok(hres == S_OK, "AddNamedItem failed: %08x\n", hres); + hres = IActiveScript_AddNamedItem(script, L"code context", SCRIPTITEM_CODEONLY); + ok(hres == S_OK, "AddNamedItem failed: %08x\n", hres); 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); + hres = IActiveScript_GetScriptDispatch(script, L"no context", &disp); + ok(hres == E_INVALIDARG, "GetScriptDispatch returned: %08x\n", hres); + + script_disp = get_script_dispatch(script, NULL); + script_disp2 = get_script_dispatch(script, L"code CONTEXT"); + ok(script_disp != script_disp2, "get_script_dispatch returned same dispatch objects.\n"); + SET_EXPECT(OnStateChange_INITIALIZED); hres = IActiveScriptParse_InitNew(parse); ok(hres == S_OK, "InitNew failed: %08x\n", hres); @@ -1762,6 +1780,118 @@ static void test_named_items(void) parse_script(parse, "visibleItem.testCall\n"); CHECK_CALLED(testCall); + hres = IActiveScriptParse_ParseScriptText(parse, L"sub testSub\nend sub\n", L"no context", NULL, NULL, 0, 0, 0, NULL, NULL); + ok(hres == E_INVALIDARG, "ParseScriptText returned: %08x\n", hres); + SET_EXPECT(OnEnterScript); + SET_EXPECT(OnLeaveScript); + hres = IActiveScriptParse_ParseScriptText(parse, L"sub testSub_global\nend sub\n", NULL, NULL, NULL, 0, 0, SCRIPTTEXT_ISPERSISTENT, NULL, NULL); + ok(hres == S_OK, "ParseScriptText failed: %08x\n", hres); + CHECK_CALLED(OnEnterScript); + CHECK_CALLED(OnLeaveScript); + SET_EXPECT(OnEnterScript); + SET_EXPECT(OnLeaveScript); + hres = IActiveScriptParse_ParseScriptText(parse, L"sub testSub\nend sub\n", L"code context", NULL, NULL, 0, 0, 0, NULL, NULL); + ok(hres == S_OK, "ParseScriptText failed: %08x\n", hres); + CHECK_CALLED(OnEnterScript); + CHECK_CALLED(OnLeaveScript); + SET_EXPECT(OnEnterScript); + SET_EXPECT(OnLeaveScript); + hres = IActiveScriptParse_ParseScriptText(parse, L"sub testSub2\nend sub\n", L"Code Context", NULL, NULL, 0, 0, SCRIPTTEXT_ISPERSISTENT, NULL, NULL); + ok(hres == S_OK, "ParseScriptText failed: %08x\n", hres); + CHECK_CALLED(OnEnterScript); + CHECK_CALLED(OnLeaveScript); + + id = 0; + get_disp_id(script_disp, "testSub", DISP_E_UNKNOWNNAME, &id); + ok(id == -1, "id = %d, expected -1\n", id); + id = 0; + get_disp_id(script_disp, "testSub_global", S_OK, &id); + ok(id != -1, "id = -1\n"); + id = 0; + get_disp_id(script_disp2, "testSub_global", DISP_E_UNKNOWNNAME, &id); + ok(id == -1, "id = %d, expected -1\n", id); + id = 0; + get_disp_id(script_disp2, "testSub", S_OK, &id); + ok(id != -1, "id = -1\n"); + id = 0; + get_disp_id(script_disp2, "testSub2", S_OK, &id); + ok(id != -1, "id = -1\n"); + + SET_EXPECT(OnEnterScript); + SET_EXPECT(OnLeaveScript); + hres = IActiveScriptParse_ParseScriptText(parse, L"testSub_global\n", L"code context", NULL, NULL, 0, 0, 0, NULL, NULL); + ok(hres == S_OK, "ParseScriptText failed: %08x\n", hres); + CHECK_CALLED(OnEnterScript); + CHECK_CALLED(OnLeaveScript); + SET_EXPECT(OnScriptError); + SET_EXPECT(OnEnterScript); + SET_EXPECT(OnLeaveScript); + hres = IActiveScriptParse_ParseScriptText(parse, L"testSub_global = 10\n", L"code context", NULL, NULL, 0, 0, 0, NULL, NULL); + ok(FAILED(hres), "ParseScriptText returned: %08x\n", hres); + CHECK_CALLED(OnScriptError); + CHECK_CALLED(OnEnterScript); + CHECK_CALLED(OnLeaveScript); + + IDispatchEx_Release(script_disp2); + IDispatchEx_Release(script_disp); + + SET_EXPECT(OnStateChange_DISCONNECTED); + SET_EXPECT(OnStateChange_INITIALIZED); + SET_EXPECT(OnStateChange_UNINITIALIZED); + hres = IActiveScript_SetScriptState(script, SCRIPTSTATE_UNINITIALIZED); + ok(hres == S_OK, "SetScriptState(SCRIPTSTATE_UNINITIALIZED) failed: %08x\n", hres); + CHECK_CALLED(OnStateChange_DISCONNECTED); + CHECK_CALLED(OnStateChange_INITIALIZED); + CHECK_CALLED(OnStateChange_UNINITIALIZED); + test_no_script_dispatch(script); + + hres = IActiveScript_GetScriptDispatch(script, L"code context", &disp); + ok(hres == E_UNEXPECTED, "hres = %08x, expected E_UNEXPECTED\n", hres); + + SET_EXPECT(GetLCID); + SET_EXPECT(OnStateChange_INITIALIZED); + hres = IActiveScript_SetScriptSite(script, &ActiveScriptSite); + ok(hres == S_OK, "SetScriptSite failed: %08x\n", hres); + CHECK_CALLED(GetLCID); + CHECK_CALLED(OnStateChange_INITIALIZED); + + hres = IActiveScript_AddNamedItem(script, L"code context", SCRIPTITEM_CODEONLY); + ok(hres == S_OK, "AddNamedItem failed: %08x\n", hres); + + SET_EXPECT(OnStateChange_CONNECTED); + SET_EXPECT_MULTI(OnEnterScript, 2); + SET_EXPECT_MULTI(OnLeaveScript, 2); + hres = IActiveScript_SetScriptState(script, SCRIPTSTATE_CONNECTED); + ok(hres == S_OK, "SetScriptState(SCRIPTSTATE_CONNECTED) failed: %08x\n", hres); + CHECK_CALLED(OnStateChange_CONNECTED); + CHECK_CALLED_MULTI(OnEnterScript, 2); + CHECK_CALLED_MULTI(OnLeaveScript, 2); + test_state(script, SCRIPTSTATE_CONNECTED); + + script_disp = get_script_dispatch(script, NULL); + id = 0; + get_disp_id(script_disp, "testSub_global", S_OK, &id); + ok(id != -1, "id = -1\n"); + id = 0; + get_disp_id(script_disp, "testSub", DISP_E_UNKNOWNNAME, &id); + ok(id == -1, "id = %d, expected -1\n", id); + id = 0; + get_disp_id(script_disp, "testSub2", DISP_E_UNKNOWNNAME, &id); + ok(id == -1, "id = %d, expected -1\n", id); + IDispatchEx_Release(script_disp); + + script_disp = get_script_dispatch(script, L"code context"); + id = 0; + get_disp_id(script_disp, "testSub_global", DISP_E_UNKNOWNNAME, &id); + ok(id == -1, "id = %d, expected -1\n", id); + id = 0; + get_disp_id(script_disp, "testSub", DISP_E_UNKNOWNNAME, &id); + ok(id == -1, "id = %d, expected -1\n", id); + id = 0; + get_disp_id(script_disp, "testSub2", DISP_E_UNKNOWNNAME, &id); + ok(id == -1, "id = %d, expected -1\n", id); + IDispatchEx_Release(script_disp); + SET_EXPECT(OnStateChange_DISCONNECTED); SET_EXPECT(OnStateChange_INITIALIZED); SET_EXPECT(OnStateChange_CLOSED); -- 2.21.0
Hi Gabriel, On 05.02.2020 17:38, Gabriel Ivăncescu wrote:
+ SET_EXPECT(OnEnterScript); + SET_EXPECT(OnLeaveScript); + hres = IActiveScriptParse_ParseScriptText(parse, L"testSub_global\n", L"code context", NULL, NULL, 0, 0, 0, NULL, NULL); + ok(hres == S_OK, "ParseScriptText failed: %08x\n", hres); + CHECK_CALLED(OnEnterScript); + CHECK_CALLED(OnLeaveScript); + SET_EXPECT(OnScriptError); + SET_EXPECT(OnEnterScript); + SET_EXPECT(OnLeaveScript); + hres = IActiveScriptParse_ParseScriptText(parse, L"testSub_global = 10\n", L"code context", NULL, NULL, 0, 0, 0, NULL, NULL); + ok(FAILED(hres), "ParseScriptText returned: %08x\n", hres); + CHECK_CALLED(OnScriptError); + CHECK_CALLED(OnEnterScript); + CHECK_CALLED(OnLeaveScript);
A few more similar tests would be nice. For example, a test confirming that a script ran with "code context" can call testSub, a test that code ran in global can't call testSub. Maybe a similar test with variables instead of functions (both implicit and explicit), maybe even classes. Testing 'me' expression would also be nice: something along lines of ok(me is visibleItem, ....). You touch a lot of places in interpreter and most of those changes could have at least a very simple test. Thanks, Jacek
participants (2)
-
Gabriel Ivăncescu -
Jacek Caban