[PATCH 4/5] vbscript: Implement separate script dispatch objects for each named item.

[Jacek Caban]

Hi Gabriel,

On 03.02.2020 17:59, Gabriel Ivăncescu wrote:
> Hi Jacek,
>
> On 03/02/2020 19:17, Jacek Caban wrote:
>> Hi Gabriel,
>>
>> On 31.01.2020 14:29, Gabriel Ivăncescu wrote:
>>> diff --git a/dlls/vbscript/interp.c b/dlls/vbscript/interp.c
>>> index b328674..8ded5cd 100644
>>> --- a/dlls/vbscript/interp.c
>>> +++ b/dlls/vbscript/interp.c
>>> @@ -112,7 +112,7 @@ static BOOL lookup_global_vars(ScriptDisp 
>>> *script, const WCHAR *name, ref_t *ref
>>>     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;
>>> +    ScriptDisp *script_obj = ctx->code->script_obj;
>>
>>
>> Does it really use this script instead of global script global 
>> context? My guess would be that both should be looked up in this 
>> case. A test to clarify that would be nice.
>>
>
> Yes, sorry about that, I forgot to add this test to the actual patch 
> (I did fiddle manually with it though).
>
> I double checked and added such a test, it does appear that it was 
> correct: it cannot access the global context or dispatch at all.
>
> See: https://testbot.winehq.org/JobDetails.pl?Key=64413 


I don't see the test I was interested in  there. I wrote a simple test 
on top of that (see attachment) and it confirms that my guess was right. 
Scripts ran in context of named item can access global properties. The 
test succeeded on win10, but failed on patched Wine.


>>
>>> +    if (!obj)
>>> +    {
>>> +        /* A persistent script with a dangling context calls these,
>>> +           for some reason, even though it won't be executed. */
>>> +        IActiveScriptSite_OnEnterScript(ctx->site);
>>> +        IActiveScriptSite_OnLeaveScript(ctx->site);
>>> +        return S_OK;
>>> +    }
>>
>>
>> Is it just to satisfy a test? I'd suggest to leave it in todo_wine 
>> for OnEnterScript/OnLeaveScript. Otherwise, it would deserve a closer 
>> look to see if we can solve it more generally and avoid such special 
>> case.
>>
>
> It's mostly to satisfy the tests, yes. I didn't realize it would be an 
> issue though. Note that we'd still have to check for if (!obj) and 
> return S_OK, otherwise the code will crash.


It's not really a big deal, I would probably not bother you with that 
the patch would be ready for Wine otherwise. I recently fixed and 
unified those script site notifications. Your tests indicates that there 
is more work needed. I would rather fix it properly and introducing more 
ad-hoc calls like this. But it's probably not worth bothering at the 
moment. if(!obj) return S_OK; combined with todo_wine is probably fine 
(assuming that it still makes sense one we figure out host to handle 
named objects given the problem above).


>>
>>>           return hres;
>>>   +    /* Fix the dangling pointers to the old script dispatch */
>>> +    LIST_FOR_EACH_ENTRY(code, &This->ctx->code_list, vbscode_t, entry)
>>> +        if(code->script_obj)
>>> +            code->script_obj = This->ctx->script_obj;
>>
>>
>> That's not pretty. Depending on result of test I mentioned earlier, 
>> we may want to simply not store global script context in vbscode_t 
>> (and only store named item-based script object).
>>
>
> Yeah it's not pretty, but unfortunately I couldn't find a simpler way 
> without adding extra fields just for this, which to me would be even 
> worse.
>
> Note that the "fix" has to be done for the global context, otherwise 
> persistent code would be useless. For other contexts it seems they are 
> completely "lost" when script is restarted (even adding a named item 
> of same name will not work to gain access to them -- it's in the 
> tests), and they will also do nothing *except* that they'll trigger 
> the OnEnterScript/OnLeaveScript for some reason.
>
> Obviously, this is only for persistent code. 


Taking into account the problem shown by the test, I think we want to 
handle global script object differently. There will be no problems like 
this if we don't use vbscode_t to store global script object.


Thanks,

Jacek

-------------- next part --------------
A non-text attachment was scrubbed...
Name: test.diff
Type: text/x-patch
Size: 732 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20200203/7d0d4207/attachment-0001.bin>