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

[Gabriel Ivăncescu]

On 11/02/2020 18:27, Jacek Caban wrote:
> On 07.02.2020 14:55, Gabriel Ivăncescu wrote:
> > @@ -248,8 +258,10 @@ static void release_script(script_ctx_t *ctx)
> >           list_remove(&iter->entry);
> >           if(iter->disp)
> >               IDispatch_Release(iter->disp);
> > +        release_named_item_script_obj(iter);
> >           heap_free(iter->name);
> > -        heap_free(iter);
> > +        if(!--iter->ref)
> > +            heap_free(iter);
> >       }
> >       if(ctx->host_global) {
>
> A separated release_vbscode() would be cleaner IMO. It would decrease 
> the counter and free the struct if it's the last reference. You could 
> also move freeing the name there.

Did you mean release_named_item() here?

And sure I can move the name freeing there, but it's not really needed 
to be held until last ref. It's needed only for persistent items (not 
persistent code referring to normal items), which is a different thing 
wine doesn't implement yet, so I didn't think of it.
> There is a bigger problem with the patch: script dispatch objects 
> created for persistent scripts on reinitialization will never be released.

I think they will still be released when the engine is closed 
(release_script loops through persistent code and does 
release_named_item_script_obj(code->named_item)).

However, creating them on-demand in exec_script as you suggested might 
be better and also avoid creating them in SetScriptSite, I'll look into 
that, thanks.
> > @@ -504,6 +516,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 +535,21 @@ static HRESULT WINAPI 
> > VBScript_SetScriptSite(IActiveScript *iface, IActiveScript
> >       if(FAILED(hres))
> >           return hres;
> > +    /* Create new script dispatches for persistent code with named 
> > items */
> > +    LIST_FOR_EACH_ENTRY(code, &This->ctx->code_list, vbscode_t, entry)
> > +    {
> > +        if(code->named_item && !code->named_item->script_obj)
> > +        {
> > +            hres = create_script_disp(This->ctx, 
> > &code->named_item->script_obj);
> > +            if(FAILED(hres))
> > +            {
> > +                This->ctx->script_obj->ctx = NULL;
> > -
> >
> > IDispatchEx_Release(&This->ctx->script_obj->IDispatchEx_iface);
> > +                return hres;
> > +            }
> > +        }
> > +    }
> > -
> >
> > This->ctx->site = pass;
> >       IActiveScriptSite_AddRef(This->ctx->site);
> > @@ -659,8 +687,14 @@ static HRESULT WINAPI 
> > VBScript_AddNamedItem(IActiveScript *iface, LPCOLESTR pstr
> >       }
> >       item->disp = disp;
> > +    item->ref = 1;
> >       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);
>
> How about creating it on demand in exec_script instead?
>
> Thanks,
>
> Jacek