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.