Re: [PATCH v2 3/5] ieframe: Add stub implementation of IInternetExplorerManager.

[Jacek Caban]

Hi Zebediah,

On 08.08.2017 20:04, Zebediah Figura wrote:
> HRESULT register_class_object(BOOL do_reg)
>  {
>      HRESULT hres;
> - static DWORD cookie;
> -
>
> static DWORD ie_cookie, iem_cookie;
>
> if(do_reg) {
>     hres = CoRegisterClassObject(&CLSID_InternetExplorer,
>             (IUnknown*)&InternetExplorerFactory, CLSCTX_SERVER,
> -
>            REGCLS_MULTIPLEUSE|REGCLS_SUSPENDED, &cookie);
> -
>    if (FAILED(hres)) {
> -
>            REGCLS_MULTIPLEUSE, &ie_cookie);
> -
>    if (FAILED(hres))
> -
>        ERR("failed to register object %08x\n", hres);
> -
>    hres = CoRegisterClassObject(&CLSID_InternetExplorerManager,
> -
>            (IUnknown*)&InternetExplorerManagerFactory, CLSCTX_SERVER,
> -
>            REGCLS_MULTIPLEUSE, &iem_cookie);
> -
>    if (FAILED(hres))
>        ERR("failed to register object %08x\n", hres);
> -
>        return hres;
> -
>    }
> -
> -
>    hres = CoResumeClassObjects();
> -
>    if(SUCCEEDED(hres))
> -
>        return hres;
> -
> -
>    ERR("failed to resume object %08x\n", hres);
> }
> -
> - return CoRevokeClassObject(cookie);
> - else
> - {
> -
>    hres = CoRevokeClassObject(ie_cookie);
> -
>    if (FAILED(hres))
> -
>        ERR("failed to register object %08x\n", hres);
> -
>    hres = CoRevokeClassObject(iem_cookie);
> -
>    if (FAILED(hres))
> -
>        ERR("failed to register object %08x\n", hres);
> - }
> - return hres;
>
> }

Are you sure that's how it's supposed to work? I didn't test it myself,
but given how it's registered and documented, I'd expect that
CLSID_InternetExplorerManager should live in a different process from
CLSID_InternetExplorer. It should be easy to distinguish those processes
with -startmanager command line argument.
> +static HRESULT WINAPI InternetExplorerManager_CreateObject(IInternetExplorerManager *iface, DWORD config, LPCWSTR url, REFIID riid, void **ppv)
> +{
> - FIXME("(%p)->(0x%x, %s, %s, %p) semi-stub\n", iface, config, debugstr_w(url), debugstr_guid(riid), ppv);
> -
> - return CoCreateInstance(&CLSID_InternetExplorer, NULL, CLSCTX_LOCAL_SERVER, riid, ppv);
>
> +}

Using CoCreateInstance is very questionable here (eg. due to the comment
above). Also, a patch adding a stub should just return E_NOTIMPL.
Implementation deserves a separated patch. I'd expect that it should
just use internal ieframe functions to create a new IE instance.

Also, I'd suggest to reorder patches to something like: 2, 4, 5, 1
(likely replaced by support for -startmanager command line argument), 3
and then a new patch implementing CreateObject.

Thanks,
Jacek

[PATCH v2 3/5] ieframe: Add stub implementation of IInternetExplorerManager.

[Zebediah Figura]

On 08/09/2017 06:07 AM, Jacek Caban wrote:
> Hi Zebediah,
>
> On 08.08.2017 20:04, Zebediah Figura wrote:
> > HRESULT register_class_object(BOOL do_reg)
> >   {
> >       HRESULT hres;
> > - static DWORD cookie;
> > -
> >
> > static DWORD ie_cookie, iem_cookie;
> >
> > if(do_reg) {
> >      hres = CoRegisterClassObject(&CLSID_InternetExplorer,
> >              (IUnknown*)&InternetExplorerFactory, CLSCTX_SERVER,
> > -
> >            REGCLS_MULTIPLEUSE|REGCLS_SUSPENDED, &cookie);
> > -
> >    if (FAILED(hres)) {
> > -
> >            REGCLS_MULTIPLEUSE, &ie_cookie);
> > -
> >    if (FAILED(hres))
> > -
> >        ERR("failed to register object %08x\n", hres);
> > -
> >    hres = CoRegisterClassObject(&CLSID_InternetExplorerManager,
> > -
> >            (IUnknown*)&InternetExplorerManagerFactory, CLSCTX_SERVER,
> > -
> >            REGCLS_MULTIPLEUSE, &iem_cookie);
> > -
> >    if (FAILED(hres))
> >         ERR("failed to register object %08x\n", hres);
> > -
> >        return hres;
> > -
> >    }
> > -
> > -
> >    hres = CoResumeClassObjects();
> > -
> >    if(SUCCEEDED(hres))
> > -
> >        return hres;
> > -
> > -
> >    ERR("failed to resume object %08x\n", hres);
> > }
> > -
> > - return CoRevokeClassObject(cookie);
> > - else
> > - {
> > -
> >    hres = CoRevokeClassObject(ie_cookie);
> > -
> >    if (FAILED(hres))
> > -
> >        ERR("failed to register object %08x\n", hres);
> > -
> >    hres = CoRevokeClassObject(iem_cookie);
> > -
> >    if (FAILED(hres))
> > -
> >        ERR("failed to register object %08x\n", hres);
> > - }
> > - return hres;
> > }
>
> Are you sure that's how it's supposed to work? I didn't test it myself, 
> but given how it's registered and documented, I'd expect that 
> CLSID_InternetExplorerManager should live in a different process from 
> CLSID_InternetExplorer. It should be easy to distinguish those processes 
> with -startmanager command line argument.

I'm sorry, I don't understand what you mean. Isn't CoRegisterClassObject 
process-agnostic?
> > +static HRESULT WINAPI InternetExplorerManager_CreateObject(IInternetExplorerManager *iface, DWORD config, LPCWSTR url, REFIID riid, void **ppv)
> > +{
> > - FIXME("(%p)->(0x%x, %s, %s, %p) semi-stub\n", iface, config, debugstr_w(url), debugstr_guid(riid), ppv);
> > -
> > - return CoCreateInstance(&CLSID_InternetExplorer, NULL, CLSCTX_LOCAL_SERVER, riid, ppv);
> >
> > +}
>
> Using CoCreateInstance is very questionable here (eg. due to the comment 
> above). Also, a patch adding a stub should just return E_NOTIMPL. 
> Implementation deserves a separated patch. I'd expect that it should 
> just use internal ieframe functions to create a new IE instance.
>
> Also, I'd suggest to reorder patches to something like: 2, 4, 5, 1 
> (likely replaced by support for -startmanager command line argument), 3 
> and then a new patch implementing CreateObject.
>
> Thanks,
> Jacek

Thanks for the review. I'll reorder the existing patches, but leave out 
an implementation of CreateObject() until I understand what it should be 
doing.

[PATCH v2 3/5] ieframe: Add stub implementation of IInternetExplorerManager.

[Jacek Caban]

On 09.08.2017 20:12, Zebediah Figura wrote:
> On 08/09/2017 06:07 AM, Jacek Caban wrote:
> > Hi Zebediah,
> >
> > On 08.08.2017 20:04, Zebediah Figura wrote:
> > > HRESULT register_class_object(BOOL do_reg)
> > >   {
> > >       HRESULT hres;
> > > - static DWORD cookie;
> > > - static DWORD ie_cookie, iem_cookie;
> > >    if(do_reg) {
> > >      hres = CoRegisterClassObject(&CLSID_InternetExplorer,
> > >              (IUnknown*)&InternetExplorerFactory, CLSCTX_SERVER,
> > > -
> > >            REGCLS_MULTIPLEUSE|REGCLS_SUSPENDED, &cookie);
> > > -
> > >    if (FAILED(hres)) {
> > > -
> > >            REGCLS_MULTIPLEUSE, &ie_cookie);
> > > -
> > >    if (FAILED(hres))
> > > -
> > >        ERR("failed to register object %08x\n", hres);
> > > -
> > >    hres = CoRegisterClassObject(&CLSID_InternetExplorerManager,
> > > -
> > >            (IUnknown*)&InternetExplorerManagerFactory,
> > >
> > > CLSCTX_SERVER,
> > > -
> > >            REGCLS_MULTIPLEUSE, &iem_cookie);
> > > -
> > >    if (FAILED(hres))
> > >         ERR("failed to register object %08x\n", hres);
> > > -
> > >        return hres;
> > > -
> > >    }
> > > -
> > > -
> > >    hres = CoResumeClassObjects();
> > > -
> > >    if(SUCCEEDED(hres))
> > > -
> > >        return hres;
> > > -
> > > -
> > >    ERR("failed to resume object %08x\n", hres);
> > > }
> > > -
> > > - return CoRevokeClassObject(cookie);
> > > - else
> > > - {
> > > -
> > >    hres = CoRevokeClassObject(ie_cookie);
> > > -
> > >    if (FAILED(hres))
> > > -
> > >        ERR("failed to register object %08x\n", hres);
> > > -
> > >    hres = CoRevokeClassObject(iem_cookie);
> > > -
> > >    if (FAILED(hres))
> > > -
> > >        ERR("failed to register object %08x\n", hres);
> > > - }
> > > - return hres;
> > > }
> >
> > Are you sure that's how it's supposed to work? I didn't test it
> > myself, but given how it's registered and documented, I'd expect that
> > CLSID_InternetExplorerManager should live in a different process from
> > CLSID_InternetExplorer. It should be easy to distinguish those
> > processes with -startmanager command line argument.
>
> I'm sorry, I don't understand what you mean. Isn't
> CoRegisterClassObject process-agnostic?

It is in a sense that created proxy/stub abstract where the object
lives, yes. But note that the actual instance of IE object must live in
one process or another. There is a lot of different aspects like
pluggable protocols, script engines, ActiveX objects, document objects
that live in its process and it's quite important for them which process
it is. I did a bit of testing and it seems to be a separated process on
Windows, so we should follow that.

Jacek

[PATCH v2 3/5] ieframe: Add stub implementation of IInternetExplorerManager.

[Zebediah Figura]

On 08/10/2017 09:33 AM, Jacek Caban wrote:
> On 09.08.2017 20:12, Zebediah Figura wrote:
> > On 08/09/2017 06:07 AM, Jacek Caban wrote:
> > > Hi Zebediah,
> > >
> > > On 08.08.2017 20:04, Zebediah Figura wrote:
> > > > HRESULT register_class_object(BOOL do_reg)
> > > >   {
> > > >       HRESULT hres;
> > > > - static DWORD cookie;
> > > > - static DWORD ie_cookie, iem_cookie;
> > > >    if(do_reg) {
> > > >      hres = CoRegisterClassObject(&CLSID_InternetExplorer,
> > > >              (IUnknown*)&InternetExplorerFactory, CLSCTX_SERVER,
> > > > -
> > > >            REGCLS_MULTIPLEUSE|REGCLS_SUSPENDED, &cookie);
> > > > -
> > > >    if (FAILED(hres)) {
> > > > -
> > > >            REGCLS_MULTIPLEUSE, &ie_cookie);
> > > > -
> > > >    if (FAILED(hres))
> > > > -
> > > >        ERR("failed to register object %08x\n", hres);
> > > > -
> > > >    hres = CoRegisterClassObject(&CLSID_InternetExplorerManager,
> > > > - (IUnknown*)&InternetExplorerManagerFactory, CLSCTX_SERVER,
> > > > -
> > > >            REGCLS_MULTIPLEUSE, &iem_cookie);
> > > > -
> > > >    if (FAILED(hres))
> > > >         ERR("failed to register object %08x\n", hres);
> > > > -
> > > >        return hres;
> > > > -
> > > >    }
> > > > -
> > > > -
> > > >    hres = CoResumeClassObjects();
> > > > -
> > > >    if(SUCCEEDED(hres))
> > > > -
> > > >        return hres;
> > > > -
> > > > -
> > > >    ERR("failed to resume object %08x\n", hres);
> > > > }
> > > > -
> > > > - return CoRevokeClassObject(cookie);
> > > > - else
> > > > - {
> > > > -
> > > >    hres = CoRevokeClassObject(ie_cookie);
> > > > -
> > > >    if (FAILED(hres))
> > > > -
> > > >        ERR("failed to register object %08x\n", hres);
> > > > -
> > > >    hres = CoRevokeClassObject(iem_cookie);
> > > > -
> > > >    if (FAILED(hres))
> > > > -
> > > >        ERR("failed to register object %08x\n", hres);
> > > > - }
> > > > - return hres;
> > > > }
> > >
> > > Are you sure that's how it's supposed to work? I didn't test it 
> > > myself, but given how it's registered and documented, I'd expect that 
> > > CLSID_InternetExplorerManager should live in a different process from 
> > > CLSID_InternetExplorer. It should be easy to distinguish those 
> > > processes with -startmanager command line argument.
> >
> > I'm sorry, I don't understand what you mean. Isn't 
> > CoRegisterClassObject process-agnostic?
>
> It is in a sense that created proxy/stub abstract where the object 
> lives, yes. But note that the actual instance of IE object must live in 
> one process or another. There is a lot of different aspects like 
> pluggable protocols, script engines, ActiveX objects, document objects 
> that live in its process and it's quite important for them which process 
> it is. I did a bit of testing and it seems to be a separated process on 
> Windows, so we should follow that.
>
> Jacek

Isn't the IEM object already created in a different process with this 
patch (namely, iexplore.exe -startmanager), since it has to be created 
with CLSCTX_LOCAL_SERVER? And using CoCreateInstance, as with my 
previous patch, should also create an InternetExplorer instance in 
another process?

I don't understand DCOM very well. Perhaps you could expound on said 
testing?

[PATCH v2 3/5] ieframe: Add stub implementation of IInternetExplorerManager.

[Jacek Caban]

On 10.08.2017 18:47, Zebediah Figura wrote:
> On 08/10/2017 09:33 AM, Jacek Caban wrote:
> > On 09.08.2017 20:12, Zebediah Figura wrote:
> > > On 08/09/2017 06:07 AM, Jacek Caban wrote:
> > > > Are you sure that's how it's supposed to work? I didn't test it
> > > > myself, but given how it's registered and documented, I'd expect
> > > > that CLSID_InternetExplorerManager should live in a different
> > > > process from CLSID_InternetExplorer. It should be easy to
> > > > distinguish those processes with -startmanager command line argument.
> > >
> > > I'm sorry, I don't understand what you mean. Isn't
> > > CoRegisterClassObject process-agnostic?
> >
> > It is in a sense that created proxy/stub abstract where the object
> > lives, yes. But note that the actual instance of IE object must live
> > in one process or another. There is a lot of different aspects like
> > pluggable protocols, script engines, ActiveX objects, document
> > objects that live in its process and it's quite important for them
> > which process it is. I did a bit of testing and it seems to be a
> > separated process on Windows, so we should follow that.
> >
> > Jacek
>
> Isn't the IEM object already created in a different process with this
> patch (namely, iexplore.exe -startmanager), since it has to be created
> with CLSCTX_LOCAL_SERVER?

Not really, see how CoRegisterClassObject is used. With your patches a
single process (both with -embeding and -startmanager arguments)
registers both CLSID_InternetExplorer and CLSID_InternetExplorerManager
with REGCLS_MULTIPLEUSE. It means that all instances of both objects
will live in this single process. I think that -embeding should register
CLSID_InternetExplorer and -startmanager should register
CLSID_InternetExplorerManager (probably with REGCLS_SINGLEUSE).
> And using CoCreateInstance, as with my previous patch, should also
> create an InternetExplorer instance in another process?

Not with your patch series, but when you get CoRegisterClassObject right
then yes, it may be actually the right thing to do.
> I don't understand DCOM very well. Perhaps you could expound on said
> testing?

I created an instance of CLSID_InternetExplorerManager and watched task
manager for new iexplore.exe processes. I admit that it's not the most
sophisticated testing ;)

Jacek

[PATCH v2 3/5] ieframe: Add stub implementation of IInternetExplorerManager.

[Zebediah Figura]

On 08/10/2017 01:01 PM, Jacek Caban wrote:
> On 10.08.2017 18:47, Zebediah Figura wrote:
> > On 08/10/2017 09:33 AM, Jacek Caban wrote:
> > > On 09.08.2017 20:12, Zebediah Figura wrote:
> > > > On 08/09/2017 06:07 AM, Jacek Caban wrote:
> > > > > Are you sure that's how it's supposed to work? I didn't test it
> > > > > myself, but given how it's registered and documented, I'd expect
> > > > > that CLSID_InternetExplorerManager should live in a different
> > > > > process from CLSID_InternetExplorer. It should be easy to
> > > > > distinguish those processes with -startmanager command line argument.
> > > >
> > > > I'm sorry, I don't understand what you mean. Isn't
> > > > CoRegisterClassObject process-agnostic?
> > >
> > > It is in a sense that created proxy/stub abstract where the object
> > > lives, yes. But note that the actual instance of IE object must live
> > > in one process or another. There is a lot of different aspects like
> > > pluggable protocols, script engines, ActiveX objects, document
> > > objects that live in its process and it's quite important for them
> > > which process it is. I did a bit of testing and it seems to be a
> > > separated process on Windows, so we should follow that.
> > >
> > > Jacek
> >
> > Isn't the IEM object already created in a different process with this
> > patch (namely, iexplore.exe -startmanager), since it has to be created
> > with CLSCTX_LOCAL_SERVER?
>
> Not really, see how CoRegisterClassObject is used. With your patches a
> single process (both with -embeding and -startmanager arguments)
> registers both CLSID_InternetExplorer and CLSID_InternetExplorerManager
> with REGCLS_MULTIPLEUSE. It means that all instances of both objects
> will live in this single process. I think that -embeding should register
> CLSID_InternetExplorer and -startmanager should register
> CLSID_InternetExplorerManager (probably with REGCLS_SINGLEUSE).

Ah, I see. I'm not sure that's quite right, though, since creating a IEM 
instance will pass both options. It seems like it would be correct to 
register IEM when -startmanager is passed and IE otherwise.

[PATCH v2 3/5] ieframe: Add stub implementation of IInternetExplorerManager.

[Jacek Caban]

On 10.08.2017 20:28, Zebediah Figura wrote:
> It seems like it would be correct to register IEM when -startmanager
> is passed and IE otherwise.

Yes, that sounds right.

Jacek