April 2014 - Wine-Devel - list.winehq.org

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

by Zhenbo Li

Hi Jacek, 2014-04-26 21:33 GMT+08:00 Jacek Caban <jacek(a)codeweavers.com>: > On 04/26/14 15:20, Zhenbo Li wrote: >> 2014-04-26 20:44 GMT+08:00 Jacek Caban <jacek(a)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

April 26, 2014

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

by 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 <jacek(a)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. > 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

April 26, 2014

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

by 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 <jacek(a)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? 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 -- Have a nice day! Zhenbo Li

April 26, 2014

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

by 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

April 26, 2014

Re: Microsoft make MS-DOS codes avalable to public

by Bryan Baldwin

On 04/26/14 21:28, Akira Nakagawa wrote: > http://www.computerhistory.org/atchm/microsoft-ms-dos-early-source-code/ > I think it does not something for wine☺ PC DOS v1.0 ftw! Now I can run clock and calculator on all my things!

April 26, 2014

Microsoft make MS-DOS codes avalable to public

by Akira Nakagawa

http://www.computerhistory.org/atchm/microsoft-ms-dos-early-source-code/ I think it does not something for wine☺

April 26, 2014

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

by Dmitry Timoshkov

Zhenbo Li <litimetal(a)gmail.com> wrote: > + case VT_R8: > + VarBstrFromR8(V_R8(p), 0, 0, &str); > + break; > + case VT_R4: > + VarBstrFromR4(V_R4(p), 0, 0, &str); > + break; > + case VT_I4: > + VarBstrFromI4(V_I4(p), 0, 0, &str); > + break; All these APIs can fail and return appropriate error code. -- Dmitry.

April 26, 2014

Re: [PATCH 3/3] ddraw/tests: Test P8 to RGB blits (try 2).

by Henri Verbeet

On 24 April 2014 14:36, Stefan Dösinger <stefan(a)codeweavers.com> wrote: > + hr = IDirectDraw2_CreateSurface(ddraw, &surface_desc, &dst, NULL); > + You should probably check the results of this call, like in the other versions.

April 25, 2014

Re: Wine Test analysis, report

by Nikolay Sivov

> http://test.winehq.org/data/fba08e34c4478fdce5055c5c3ead7d8958875bae/#scrru… This one is a surprise to me. Instead of a normal BOM sequences it writes this: --- filesystem.c:1359 <http://source.winehq.org/git/wine.git/?a=blob;f=dlls/scrrun/tests/filesyste…>: Test failed: got L"\f896\200f" --- 0x200f is RTL mark, but 0xf896 is a private are codepoint, with no standard meaning, so it's probably something else encoded. This only happens on Hebrew locale (and probably others RTL). With Japanese it looks like "\f8f3\f8f2", search shows me some ShiftJIS -> UTF translation results, that mention these codes as exactly 0xff/0xfe substitutes. Probably Aric knows more about that.

April 25, 2014

Wine Test analysis, report

by Jeremy White

Just wanted to report that I do periodically do my analysis. It looks as though the test bot is beginning to be usable again, although the results are currently ugly: http://www.winehq.org/~jwhite/fba08e34c447.html I'm waiting for Francois to report that things are essentially operational again; until I get that word, I'm not planning to beat the drum. Francois, can we get an update? Cheers, Jeremy

April 25, 2014