Re: mshtml: Added IHTMLTable::width property implementation. (try 4)

[Jacek Caban]

Hi Zhenbo,

This is better, but:

On 04/26/14 05:12, Zhenbo Li wrote:
> - hres = var2str(&v, &val);
> -
> - if (hres != S_OK){
> -
>    ERR("Set Width(%s) failed when initializing a nsAString!\n",
> -
>        debugstr_variant(&v));
> -
>    nsAString_Finish(&val);

Again, this is not initialized in the error case.
> -
>    return hres;
> - }
> -
> - nsres = nsIDOMHTMLTableElement_SetWidth(This->nstable, &val);
> - nsAString_Finish(&val);
> -
> - if (NS_FAILED(nsres)){
> -
>    ERR("Set Width(%s) failed!\n", debugstr_variant(&v));
> -
>    return E_FAIL;
> - }
> -
> - return S_OK;
>
> }
>
> static HRESULT WINAPI HTMLTable_get_width(IHTMLTable *iface, VARIANT *p)
>  {
>      HTMLTable *This = impl_from_IHTMLTable(iface);
> - FIXME("(%p)->(%p)\n", This, p);
> - return E_NOTIMPL;
> - nsAString val;
> - BSTR bstr;
> - nsresult nsres;
> - HRESULT hres;
> -
> - TRACE("(%p)->(%p)\n", This, p);
> - nsAString_Init(&val, NULL);
> - nsres = nsIDOMHTMLTableElement_GetWidth(This->nstable, &val);
> - if (NS_FAILED(nsres)){
> -
>    ERR("Get Width(%s) failed!\n", debugstr_variant(p));
> -
>    nsAString_Finish(&val);
> -
>    return E_FAIL;
> - }
> -
> - hres = nsstr_to_truncated_bstr(&val, &bstr);
> - if (FAILED(hres)) {
> -
>    SysFreeString(bstr);

And bstr is not initialized in error case.

Also, as Dmitry pointed, you don't check for some errors in var2str.

Jacek

mshtml: Added IHTMLTable::width property implementation. (try 4)

[Zhenbo Li]

Thank you.
I'm sorry for the mistakes I've made.

As this patch still needs improving, maybe mark 104193, 104110, 103576
as rejected?

2014-04-26 20:44 GMT+08:00 Jacek Caban :
> Hi Zhenbo,
>
> This is better, but:
>
> On 04/26/14 05:12, Zhenbo Li wrote:
> - hres = var2str(&v, &val);
> -
> - if (hres != S_OK){
> -
>    ERR("Set Width(%s) failed when initializing a nsAString!\n",
> -
>        debugstr_variant(&v));
> -
>    nsAString_Finish(&val);
>
>
> Again, this is not initialized in the error case.

As I've added
> - default:
> -
>    nsAString_Init(nsstr, NULL);
> -
>    FIXME("unsupported arg %s\n", debugstr_variant(p));
> -
>    return E_NOTIMPL;
> - }

Isn't it enough?

Or you mean here?
> - ret = nsAString_Init(nsstr, str);
> - SysFreeString(str);
> - return ret ? S_OK : E_OUTOFMEMORY;

Should I free a string which failed when initializing due to out-of-memory?
> -
> - hres = nsstr_to_truncated_bstr(&val, &bstr);
> - if (FAILED(hres)) {
> -
>    SysFreeString(bstr);
>
>
> And bstr is not initialized in error case.

The error case comes from
> - ret = end ? SysAllocStringLen(str, end-str) : SysAllocString(str);
> - if(!ret)
> -
>    return E_OUTOFMEMORY;

I have the same question.
> Also, as Dmitry pointed, you don't check for some errors in var2str.

This is my fault, I'll fix it then.
Thanks Dmitry, too
> Jacek

mshtml: Added IHTMLTable::width property implementation. (try 4)

[Jacek Caban]

On 04/26/14 15:20, Zhenbo Li wrote:
> Thank you.
> I'm sorry for the mistakes I've made.
>
> As this patch still needs improving, maybe mark 104193, 104110, 103576
> as rejected?

Don't worry too much about them being marked one way or another.
> 2014-04-26 20:44 GMT+08:00 Jacek Caban :
> > Hi Zhenbo,
> >
> > This is better, but:
> >
> > On 04/26/14 05:12, Zhenbo Li wrote:
> > - hres = var2str(&v, &val);
> > -
> > - if (hres != S_OK){
> > -
> >    ERR("Set Width(%s) failed when initializing a nsAString!\n",
> > -
> >        debugstr_variant(&v));
> > -
> >    nsAString_Finish(&val);
> >
> >
> > Again, this is not initialized in the error case.
>
> As I've added
> > - default:
> > -
> >    nsAString_Init(nsstr, NULL);
> > -
> >    FIXME("unsupported arg %s\n", debugstr_variant(p));
> > -
> >    return E_NOTIMPL;
> > - }
>
> Isn't it enough?

I missed that change, but I think it's wrong. There are other error
cases as well. nsAString_Init failure also leaves the string
uninitialized (and it's on my TODO list to make sure that we may make
nsAString_Init infailable, but that's a different story). It's also
generally sane to expect that if a function fails, it's output parameter
is not initialized. Please remove this new nsAString_Init call and
nsAString_Finish in error case.
> Or you mean here?
> > - ret = nsAString_Init(nsstr, str);
> > - SysFreeString(str);
> > - return ret ? S_OK : E_OUTOFMEMORY;
>
> Should I free a string which failed when initializing due to out-of-memory?

Which one? The way it's done currently is right.
> > -
> > - hres = nsstr_to_truncated_bstr(&val, &bstr);
> > - if (FAILED(hres)) {
> > -
> >    SysFreeString(bstr);
> >
> >
> > And bstr is not initialized in error case.
>
> The error case comes from
> > - ret = end ? SysAllocStringLen(str, end-str) : SysAllocString(str);
> > - if(!ret)
> > -
> >    return E_OUTOFMEMORY;
>
> I have the same question.

Please read the code again. bstr is not initialized in this case, so you
can't free it.

Jacek

mshtml: Added IHTMLTable::width property implementation. (try 4)

[Zhenbo Li]

Hi Jacek,

2014-04-26 21:33 GMT+08:00 Jacek Caban :
> On 04/26/14 15:20, Zhenbo Li wrote:
> > 2014-04-26 20:44 GMT+08:00 Jacek Caban :
> > > Hi Zhenbo,
> > >
> > > This is better, but:
> > >
> > > On 04/26/14 05:12, Zhenbo Li wrote:
> > > - hres = var2str(&v, &val);
> > > -
> > > - if (hres != S_OK){
> > > -
> > >    ERR("Set Width(%s) failed when initializing a nsAString!\n",
> > > -
> > >        debugstr_variant(&v));
> > > -
> > >    nsAString_Finish(&val);
> > >
> > >
> > > Again, this is not initialized in the error case.
> >
> > As I've added
> > > - default:
> > > -
> > >    nsAString_Init(nsstr, NULL);
> > > -
> > >    FIXME("unsupported arg %s\n", debugstr_variant(p));
> > > -
> > >    return E_NOTIMPL;
> > > - }
> >
> > Isn't it enough?
>
> I missed that change, but I think it's wrong. There are other error
> cases as well. nsAString_Init failure also leaves the string
> uninitialized (and it's on my TODO list to make sure that we may make
> nsAString_Init infailable, but that's a different story). It's also
> generally sane to expect that if a function fails, it's output parameter
> is not initialized. Please remove this new nsAString_Init call and
> nsAString_Finish in error case.

Thank you for your explanation.
How about this solution:

+        return nsAString_Init(nsstr, V_BSTR(p))?
+            S_OK : E_OUTOFMEMORY;
+    case VT_R8:
+        hres = VarBstrFromR8(V_R8(p), 0, 0, &str);
+        break;
+    case VT_R4:
+        hres = VarBstrFromR4(V_R4(p), 0, 0, &str);
+        break;
+    case VT_I4:
+        hres = VarBstrFromI4(V_I4(p), 0, 0, &str);
+        break;
+    default:
+        FIXME("unsupported arg %s\n", debugstr_variant(p));
+        hres = E_NOTIMPL;
+        break;
+    }
+    if (FAILED(hres))
+        return hres;
+
+    ret = nsAString_Init(nsstr, str);
+    SysFreeString(str);
+    return ret ? S_OK : E_OUTOFMEMORY;
+}
> > Or you mean here?
> > > - ret = nsAString_Init(nsstr, str);
> > > - SysFreeString(str);
> > > - return ret ? S_OK : E_OUTOFMEMORY;
> >
> > Should I free a string which failed when initializing due to out-of-memory?
>
> Which one? The way it's done currently is right.
> > > -
> > > - hres = nsstr_to_truncated_bstr(&val, &bstr);
> > > - if (FAILED(hres)) {
> > > -
> > >    SysFreeString(bstr);
> > >
> > >
> > > And bstr is not initialized in error case.
> >
> > The error case comes from
> > > - ret = end ? SysAllocStringLen(str, end-str) : SysAllocString(str);
> > > - if(!ret)
> > > -
> > >    return E_OUTOFMEMORY;
> >
> > I have the same question.
>
> Please read the code again. bstr is not initialized in this case, so you
> can't free it.

Sorry, I made a mistake.
When SysAllocString failes, should we mark bstr as NULL?
Like this:

static HRESULT nsstr_to_truncated_bstr(const nsAString *nsstr, BSTR *ret_ptr)
{
//....
    ret = end ? SysAllocStringLen(str, end-str) : SysAllocString(str);

*ret_ptr = ret;
    return ret ? S_OK : E_OUTOFMEMORY;
}

static HRESULT WINAPI HTMLTable_get_width(IHTMLTable *iface, VARIANT *p)
{
//....
    hres = nsstr_to_truncated_bstr(&val, &bstr);
    if (FAILED(hres))
        return hres;
//...
}
> Jacek

mshtml: Added IHTMLTable::width property implementation. (try 4)

[Jacek Caban]

On 04/26/14 16:38, Zhenbo Li wrote:
> Hi Jacek,
>
> 2014-04-26 21:33 GMT+08:00 Jacek Caban :
> > On 04/26/14 15:20, Zhenbo Li wrote:
> > > 2014-04-26 20:44 GMT+08:00 Jacek Caban :
> > > > Hi Zhenbo,
> > > >
> > > > This is better, but:
> > > >
> > > > On 04/26/14 05:12, Zhenbo Li wrote:
> > > > - hres = var2str(&v, &val);
> > > > -
> > > > - if (hres != S_OK){
> > > > -
> > > >    ERR("Set Width(%s) failed when initializing a nsAString!\n",
> > > > -
> > > >        debugstr_variant(&v));
> > > > -
> > > >    nsAString_Finish(&val);
> > > >
> > > >
> > > > Again, this is not initialized in the error case.
> > >
> > > As I've added
> > > > - default:
> > > > -
> > > >    nsAString_Init(nsstr, NULL);
> > > > -
> > > >    FIXME("unsupported arg %s\n", debugstr_variant(p));
> > > > -
> > > >    return E_NOTIMPL;
> > > > - }
> > >
> > > Isn't it enough?
> >
> > I missed that change, but I think it's wrong. There are other error
> > cases as well. nsAString_Init failure also leaves the string
> > uninitialized (and it's on my TODO list to make sure that we may make
> > nsAString_Init infailable, but that's a different story). It's also
> > generally sane to expect that if a function fails, it's output parameter
> > is not initialized. Please remove this new nsAString_Init call and
> > nsAString_Finish in error case.
>
> Thank you for your explanation.
> How about this solution:
> -
>    return nsAString_Init(nsstr, V_BSTR(p))?
> -
>        S_OK : E_OUTOFMEMORY;
> - case VT_R8:
> -
>    hres = VarBstrFromR8(V_R8(p), 0, 0, &str);
> -
>    break;
> - case VT_R4:
> -
>    hres = VarBstrFromR4(V_R4(p), 0, 0, &str);
> -
>    break;
> - case VT_I4:
> -
>    hres = VarBstrFromI4(V_I4(p), 0, 0, &str);
> -
>    break;
> - default:
> -
>    FIXME("unsupported arg %s\n", debugstr_variant(p));
> -
>    hres = E_NOTIMPL;
> -
>    break;
> - }
> - if (FAILED(hres))
> -
>    return hres;
> -
> - ret = nsAString_Init(nsstr, str);
> - SysFreeString(str);
> - return ret ? S_OK : E_OUTOFMEMORY;
>
> +}

This looks good.
> > > Or you mean here?
> > > > - ret = nsAString_Init(nsstr, str);
> > > > - SysFreeString(str);
> > > > - return ret ? S_OK : E_OUTOFMEMORY;
> > >
> > > Should I free a string which failed when initializing due to out-of-memory?
> >
> > Which one? The way it's done currently is right.
> > > > -
> > > > - hres = nsstr_to_truncated_bstr(&val, &bstr);
> > > > - if (FAILED(hres)) {
> > > > -
> > > >    SysFreeString(bstr);
> > > >
> > > >
> > > > And bstr is not initialized in error case.
> > >
> > > The error case comes from
> > > > - ret = end ? SysAllocStringLen(str, end-str) : SysAllocString(str);
> > > > - if(!ret)
> > > > -
> > > >    return E_OUTOFMEMORY;
> > >
> > > I have the same question.
> >
> > Please read the code again. bstr is not initialized in this case, so you
> > can't free it.
>
> Sorry, I made a mistake.
> When SysAllocString failes, should we mark bstr as NULL?
> Like this:
>
> static HRESULT nsstr_to_truncated_bstr(const nsAString *nsstr, BSTR *ret_ptr)
> {
> //....
>     ret = end ? SysAllocStringLen(str, end-str) : SysAllocString(str);
> *ret_ptr = ret;
> return ret ? S_OK : E_OUTOFMEMORY;
>
> }

You don't need this...
> static HRESULT WINAPI HTMLTable_get_width(IHTMLTable *iface, VARIANT *p)
> {
> //....
>     hres = nsstr_to_truncated_bstr(&val, &bstr);
>     if (FAILED(hres))
>         return hres;
> //...
> }

...once you change this.

Jacek