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

[Gabriel Ivăncescu]

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.