Re: [PATCH v2 3/5] ieframe: Add stub implementation of IInternetExplorerManager.
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
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.
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
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?
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
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.
participants (2)
-
Jacek Caban -
Zebediah Figura