Re: [PATCH 1/8] riched20: Stub for ITextRange interface and implement ITextDocument::Range. (resend try 4)
On Thu, Sep 11, 2014 at 05:35:38PM +0800, Jactry Zeng wrote:
+static HRESULT CreateITextRange(IRichEditOleImpl *reOle, LONG start, LONG end, + ITextRangeImpl *txtRge, ITextRange** ppRange) +{ + txtRge->ITextRange_iface.lpVtbl = &trvt; + txtRge->ref = 1; + txtRge->reOle = reOle; + txtRge->start = start; + txtRge->end = end; + list_add_head(&reOle->rangelist, &txtRge->entry); + *ppRange = &txtRge->ITextRange_iface; + return S_OK; +}
This looks very suspicious to me. You're storing a reference to 'reOle' so you should AddRef it somewhere. It may be better to pass the interface ptr, AddRef and store that. Also, is the purpose of rangelist just to be able to NULL out reOle when the parent object is destroyed? If so, this can go away if you correctly handle the ref counting. Huw.
Hi Huw, 2014-09-11 19:55 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
This looks very suspicious to me. You're storing a reference to 'reOle' so you should AddRef it somewhere. It may be better to pass the interface ptr, AddRef and store that.
Also, is the purpose of rangelist just to be able to NULL out reOle when the parent object is destroyed? If so, this can go away if you correctly handle the ref counting.
Thanks for your review! I have tested ref count of reOle/txtDoc in Windows, and ref count of reOle/txtDoc didn't increase after ITextDocument::Range. So I didn't call AddRef in ITextDocument::Range. create_interfaces(&w, &reOle, &txtDoc, NULL); refcount = get_refcount((IUnknown *)txtDoc); ok(refcount == 3, "got wrong ref count: %d\n", refcount); hres = ITextDocument_Range(txtDoc, 0, 0, &txtRge); ok(hres == S_OK, "ITextDocument_Range fails 0x%x.\n", hres); refcount = get_refcount((IUnknown *)txtRge); ok(refcount == 1, "get wrong refcount: returned %d expected 1\n", refcount); refcount = get_refcount((IUnknown *)txtDoc); ok(refcount == 3, "got wrong ref count: %d\n", refcount); -- Regards, Jactry Zeng
On Thu, Sep 11, 2014 at 10:01:51PM +0800, Jactry Zeng wrote:
Hi Huw, 2014-09-11 19:55 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
This looks very suspicious to me. You're storing a reference to 'reOle' so you should AddRef it somewhere. It may be better to pass the interface ptr, AddRef and store that.
Also, is the purpose of rangelist just to be able to NULL out reOle when the parent object is destroyed? If so, this can go away if you correctly handle the ref counting.
Thanks for your review!
I have tested ref count of reOle/txtDoc in Windows, and ref count of reOle/txtDoc didn't increase after ITextDocument::Range. So I didn't call AddRef in ITextDocument::Range.
An obvious question is then: what happens to the range object if you release all references to the txtdoc object? Does the range object still remain functional? Huw.
2014-09-11 22:15 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
An obvious question is then: what happens to the range object if you release all references to the txtdoc object? Does the range object still remain functional?
No, the program will crash when calling a function of ITextRange after released all references of txtDoc. -- Regards, Jactry Zeng
On Thu, Sep 11, 2014 at 10:39:05PM +0800, Jactry Zeng wrote:
2014-09-11 22:15 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
An obvious question is then: what happens to the range object if you release all references to the txtdoc object? Does the range object still remain functional?
No, the program will crash when calling a function of ITextRange after released all references of txtDoc.
Nice. Well hopefully nothing depends on that, so we don't have to follow their broken implementation. I think you should implement it as I suggested and don't bother adding tests for the exact refcount number (nothing should care about the exact number anyway). Huw.
2014-09-11 22:53 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
Nice. Well hopefully nothing depends on that, so we don't have to follow their broken implementation. I think you should implement it as I
suggested
and don't bother adding tests for the exact refcount number (nothing should care about the exact number anyway).
So these should been added into the implementation: 1. AddRef(reOle) in CreateITextRange() (both ITextDocument::Range and ITextRange::GetDuplicate will call this function to fill a ITextRange interface); 2. Release(reOle) in ITextRange::Release() 3. Pass IRichEditOle and ITextRange into CreateITextRange() instead of IRichEditOleImpl and ITextRangeImpl Is it the right way? Thanks again. :) btw, the rangelist was implemented for ITextRange::Delete. Range of all ITextRange in one IRichEditOle should be reseted after we used ITextRange::Delete to delete some text in the RichEdit Control. And a rangelist will make this work easier. -- Regards, Jactry Zeng
On 11.09.2014 16:39, Jactry Zeng wrote:
2014-09-11 22:15 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
An obvious question is then: what happens to the range object if you release all references to the txtdoc object? Does the range object still remain functional?
No, the program will crash when calling a function of ITextRange after released all references of txtDoc.
Are you sure? Doesn't this test below show exactly the opposite? It seems to be safe to release the ITextDocument, and afterwards call an ITextRange function - it will just return CO_E_RELEASED since the parent object was already released. Or am I misunderstanding this code?
+ release_interfaces(&w, &reOle, &txtDoc, NULL); + hres = ITextRange_CanEdit(txtRge, NULL); + ok(hres == CO_E_RELEASED, "ITextRange after ITextDocument destroyed\n"); + ITextRange_Release(txtRge);
At my opinion this behaviour is so fundamental in the ITextRange implementation, that I doubt any app will still work when you replace all that with proper refcounting. To reproduce the behaviour above (which obviously passes on the wine testbot) it is necessary to keep track of all ITextRange objects - and the best method for that is using a list, no matter how "ugly" and unusual it is for interface implementations. Please note that MS never designed this interface to be threadsafe, there are even big warnings for developers on MSDN. Regards, Sebastian
2014-09-12 4:43 GMT+08:00 Sebastian Lackner <sebastian(a)fds-team.de>: > > > Are you sure? Doesn't this test below show exactly the opposite? It seems to be safe > to release the ITextDocument, and afterwards call an ITextRange function - it will just > return CO_E_RELEASED since the parent object was already released. Or am I misunderstanding > this code? So sorry, I made a mistake yesterday here. I added more a ITextRange::Release before the ITextRange calling in my new tests code yesterday. Functions of ITextRange will return CO_E_RELEASED after reOle/txtDoc were released and will crash after the ITextRange was released. Thank you! -- Regards, Jactry Zeng
On Fri, Sep 12, 2014 at 10:47:44AM +0800, Jactry Zeng wrote:
2014-09-12 4:43 GMT+08:00 Sebastian Lackner <sebastian(a)fds-team.de>:
Are you sure? Doesn't this test below show exactly the opposite? It seems to
be safe
to release the ITextDocument, and afterwards call an ITextRange function - it will just return CO_E_RELEASED since the parent object was already released. Or am I misunderstanding this code?
So sorry, I made a mistake yesterday here. I added more a ITextRange::Release before the ITextRange calling in my new tests code yesterday. Functions of ITextRange will return CO_E_RELEASED after reOle/txtDoc were released and will crash after the ITextRange was released.
In that case let's go with your proposed implementation. Huw.
Hi Huw, This is a newer version: static HRESULT CreateITextRange(IRichEditOle *reOle, LONG start, LONG end, ITextRangeImpl *txtRge, ITextRange** ppRange) { IRichEditOleImpl *reOleImpl = impl_from_IRichEditOle(reOle); txtRge->ITextRange_iface.lpVtbl = &trvt; txtRge->ref = 1; IRichEditOle_AddRef(reOle); txtRge->reOle = reOleImpl; txtRge->start = start; txtRge->end = end; list_add_head(&reOleImpl->rangelist, &txtRge->entry); *ppRange = &txtRge->ITextRange_iface; return S_OK; } I will prefer passing ITextRangeImpl into CreateITextRange instead of ITextRange interface. So we don't need to impl_from_ITextRange again. Is it also ok? btw, I didn't alloc txtRge (txtRge = heap_alloc(sizeof(ITextRangeImpl));) in CreateITextRange(), because CreateITextRange() was just created for sharing code with ITextRange::GetDuplicate and GetDuplicate will return E_FAIL for all any other error[0] when ITextDocument::Range returning E_OUTOFMEMORY.[1] Thanks. [0] http://msdn.microsoft.com/en-us/library/windows/desktop/bb787840(v=vs.85).as... [1] http://msdn.microsoft.com/en-us/library/windows/desktop/bb774097(v=vs.85).as... 2014-09-12 15:53 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
On Fri, Sep 12, 2014 at 10:47:44AM +0800, Jactry Zeng wrote:
2014-09-12 4:43 GMT+08:00 Sebastian Lackner <sebastian(a)fds-team.de>:
Are you sure? Doesn't this test below show exactly the opposite? It
seems to
be safe
to release the ITextDocument, and afterwards call an ITextRange function - it will just return CO_E_RELEASED since the parent object was already released. Or am I misunderstanding this code?
So sorry, I made a mistake yesterday here. I added more a ITextRange::Release before the ITextRange calling in my new tests code yesterday. Functions of ITextRange will return CO_E_RELEASED after reOle/txtDoc were released and will crash after the ITextRange was released.
In that case let's go with your proposed implementation.
Huw.
-- Regards, Jactry Zeng
On Fri, Sep 12, 2014 at 05:32:19PM +0800, Jactry Zeng wrote:
Hi Huw,
This is a newer version: static HRESULT CreateITextRange(IRichEditOle *reOle, LONG start, LONG end, ITextRangeImpl *txtRge, ITextRange** ppRange) { IRichEditOleImpl *reOleImpl = impl_from_IRichEditOle(reOle);
txtRge->ITextRange_iface.lpVtbl = &trvt; txtRge->ref = 1; IRichEditOle_AddRef(reOle); txtRge->reOle = reOleImpl; txtRge->start = start; txtRge->end = end; list_add_head(&reOleImpl->rangelist, &txtRge->entry); *ppRange = &txtRge->ITextRange_iface; return S_OK; }
I will prefer passing ITextRangeImpl into CreateITextRange instead of ITextRange interface. So we don't need to impl_from_ITextRange again. Is it also ok?
btw, I didn't alloc txtRge (txtRge = heap_alloc(sizeof(ITextRangeImpl));) in CreateITextRange(), because CreateITextRange() was just created for sharing code with ITextRange:: GetDuplicate and GetDuplicate will return E_FAIL for all any other error[0] when ITextDocument::Range returning E_OUTOFMEMORY.[1]
Passing IRichEditOle is nicer than passing IRichEditOleImpl so that's a good improvement. However, you really do want to do the allocation in this function (you then don't need the ITextRangeImpl param at all). This function should return E_OUTOFMEMORY on allocation failure. If GetDuplicate really needs to return E_FAIL, it can check for this function failing and return E_FAIL. Huw.
2014-09-12 17:47 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
txtRge->ref = 1; IRichEditOle_AddRef(reOle);
I feel little perplexed here. We want ITextRange::XXX returns CO_E_RELEASED when reOle and txtDoc were released. So we don't need AddRef(reOle) here? And then we can just pass a RichEditOleImpl into CreateITextRange(). Thanks. -- Regards, Jactry Zeng
On Fri, Sep 12, 2014 at 06:31:01PM +0800, Jactry Zeng wrote:
2014-09-12 17:47 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
txtRge->ref = 1; IRichEditOle_AddRef(reOle);
I feel little perplexed here. We want ITextRange::XXX returns CO_E_RELEASED when reOle and txtDoc were released. So we don't need AddRef(reOle) here? And then we can just pass a RichEditOleImpl into CreateITextRange().
We'd already decided that you don't need the AddRef, so I assumed you would remove it. If you prefer to pass RichEditOleImpl then just pass that. Huw.
2014-09-12 18:36 GMT+08:00 Huw Davies <huw(a)codeweavers.com>:
We'd already decided that you don't need the AddRef, so I assumed you would remove it. If you prefer to pass RichEditOleImpl then just pass that.
I see. Thanks again. :) -- Regards, Jactry Zeng
participants (3)
-
Huw Davies -
Jactry Zeng -
Sebastian Lackner