Re: [PATCH 1/8] riched20: Stub for ITextRange interface and implement ITextDocument::Range. (resend try 4)

[Huw Davies]

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.

[PATCH 1/8] riched20: Stub for ITextRange interface and implement ITextDocument::Range. (resend try 4)

[Jactry Zeng]

Hi Huw,
2014-09-11 19:55 GMT+08:00 Huw Davies :
> 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);

[PATCH 1/8] riched20: Stub for ITextRange interface and implement ITextDocument::Range. (resend try 4)

[Huw Davies]

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 :
> > 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.

[PATCH 1/8] riched20: Stub for ITextRange interface and implement ITextDocument::Range. (resend try 4)

[Jactry Zeng]

2014-09-11 22:15 GMT+08:00 Huw Davies :
> 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.

[PATCH 1/8] riched20: Stub for ITextRange interface and implement ITextDocument::Range. (resend try 4)

[Huw Davies]

On Thu, Sep 11, 2014 at 10:39:05PM +0800, Jactry Zeng wrote:
> 2014-09-11 22:15 GMT+08:00 Huw Davies :
> > 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.

[PATCH 1/8] riched20: Stub for ITextRange interface and implement ITextDocument::Range. (resend try 4)

[Jactry Zeng]

2014-09-11 22:53 GMT+08:00 Huw Davies :
> 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.

[PATCH 1/8] riched20: Stub for ITextRange interface and implement ITextDocument::Range. (resend try 4)

[Sebastian Lackner]

On 11.09.2014 16:39, Jactry Zeng wrote:
> 2014-09-11 22:15 GMT+08:00 Huw Davies :
> > 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

[PATCH 1/8] riched20: Stub for ITextRange interface and implement ITextDocument::Range. (resend try 4)

[Jactry Zeng]

2014-09-12 4:43 GMT+08:00 Sebastian Lackner :
> 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!

[PATCH 1/8] riched20: Stub for ITextRange interface and implement ITextDocument::Range. (resend try 4)

[Huw Davies]

On Fri, Sep 12, 2014 at 10:47:44AM +0800, Jactry Zeng wrote:
> 2014-09-12 4:43 GMT+08:00 Sebastian Lackner :
> > 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.

[PATCH 1/8] riched20: Stub for ITextRange interface and implement ITextDocument::Range. (resend try 4)

[Jactry Zeng]

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 :
> On Fri, Sep 12, 2014 at 10:47:44AM +0800, Jactry Zeng wrote:
> > 2014-09-12 4:43 GMT+08:00 Sebastian Lackner :
> > > 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.

[PATCH 1/8] riched20: Stub for ITextRange interface and implement ITextDocument::Range. (resend try 4)

[Huw Davies]

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.

[PATCH 1/8] riched20: Stub for ITextRange interface and implement ITextDocument::Range. (resend try 4)

[Jactry Zeng]

2014-09-12 17:47 GMT+08:00 Huw Davies :
> > 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.

[PATCH 1/8] riched20: Stub for ITextRange interface and implement ITextDocument::Range. (resend try 4)

[Huw Davies]

On Fri, Sep 12, 2014 at 06:31:01PM +0800, Jactry Zeng wrote:
> 2014-09-12 17:47 GMT+08:00 Huw Davies :
> > > 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.

[PATCH 1/8] riched20: Stub for ITextRange interface and implement ITextDocument::Range. (resend try 4)

[Jactry Zeng]

2014-09-12 18:36 GMT+08:00 Huw Davies :
> 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. :)