mshtml: Added IHTMLTable::width property implementation. (try 4)
Zhenbo Li
litimetal at gmail.com
Sat Apr 26 09:38:46 CDT 2014
Hi Jacek,
2014-04-26 21:33 GMT+08:00 Jacek Caban <jacek at codeweavers.com>:
> On 04/26/14 15:20, Zhenbo Li wrote:
>> 2014-04-26 20:44 GMT+08:00 Jacek Caban <jacek at codeweavers.com>:
>>> 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
--
Have a nice day!
Zhenbo Li
More information about the wine-devel
mailing list