[rfc][icedtea-web] C++-side small fixes (RH 1121549)

[Andrew Azores]

On 07/24/2014 10:14 AM, Jiri Vanek wrote:
> ...
>>> 1. Defect type: RESOURCE_LEAK
>>> 10.
>>> icedtea-web-1.5/plugin/icedteanp/IcedTeaPluginUtils.cc:1154:leaked_handle
>>> – Handle variable
>>> "plugin_file_log_fd" going out of scope leaks the handle.
>>
>> Does this one matter? I added a close() anyway but like Omair says it
>> seems like it will be cleaned
>> up anyway. But it's better to explicitly clean it I suppose.
>
> Yes. As Omair pointed. Remove this close.

Removed.

>>
>>>
> ...
>>>>           }
>>>>           return r2;
>>>
>>>
>>> For those 4 mkdirs - if this directory already exists then the mkdir
>>> should not be called at all
>>> (maybe some check for its write ability?? moreover 100% for different
>>> changeset)
>>> Anyway. THis check is included. So I woud liek to add
>>> (!IcedTeaPluginUtilities::file_exists(r1){} if
>>> (!isWriteableDirectory(){"warning to user}
>>>
>>> As thsoe are called only when  and it do not exists, then more
>>> serious warring with possible
>>> workarounds like check permissions on parents, set XDG_CONFIG* for
>>> first hunk and.. not sure what
>>> for second :) or similar and warning like ITW will not work., SHould
>>> be printed.
>>>
>>
>> So all of this in a separate changeset right? Can't most of this work
>> be done just by checking errno
>> after mkdir fails?
>
> But please, Don't forget it. Othewrwise this check will lost a bit of
> sense.

Updated the patch a bit.

>>
>>>
>>>>       }
>>>> diff --git a/plugin/icedteanp/IcedTeaScriptablePluginObject.cc
>>>> b/plugin/icedteanp/IcedTeaScriptablePluginObject.cc
>>>> --- a/plugin/icedteanp/IcedTeaScriptablePluginObject.cc
>>>> +++ b/plugin/icedteanp/IcedTeaScriptablePluginObject.cc
>>>> @@ -704,7 +704,7 @@ IcedTeaScriptableJavaObject::setProperty
>>>> browser_functions.intfromidentifier(name_id) >= 0) // else if array
>>>> and requesting index
>>>>           {
>>>>
>>>> -            JavaResultData* java_result =
>>>> java_request.getArrayLength(instance_id);
>>>> +            java_result = java_request.getArrayLength(instance_id);
>>>>               if (java_result->error_occurred)
>>>>               {
>>>>                   PLUGIN_ERROR("ERROR: Couldn't fetch array length\n");
>>>>
>>>
>>>
>>> I have looked into code and have not found a reason for
>>> in-the-middle-decklaration. So this is
>>> really probably fix for hidden minor bug.
>>>
>>>
>>> J.
>>
>> Yea, I couldn't find a reason for it to. Seems like it was probably
>> just an accident.
>>
>
>
> After removal of close, ok for head and 1.5 from my side. Please verify
> with omair. He had his issues.
>
>
> J.
>

Pinging Omair.

Thanks,
-- 
Andrew Azores
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rh1121549-fixes-5.patch
Type: text/x-patch
Size: 4518 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140725/16a6f804/rh1121549-fixes-5.patch>