[icedtea-web] RFC: Handle null NPP instances in plugin calls

[Omair Majid]

On 03/03/2011 11:02 AM, Deepak Bhole wrote:
> * Omair Majid<omajid at redhat.com>  [2011-03-03 10:54]:
>> On 03/02/2011 04:45 PM, Deepak Bhole wrote:
>>> This patch fixes problems that may arise if a call is requested by an
>>> already destroyed applet (race condition).
>>>
>>> Additionally, it also does some very minor cleanup by removing an unused
>>> thread count variable.
>>>
>>> ChangeLog:
>>>
>>> 2011-03-02  Deepak Bhole<dbhole at redhat.com>
>>>
>>>      * plugin/icedteanp/IcedTeaPluginRequestProcessor.cc
>>>      (PluginRequestProcessor): Remove initialization of tc_mutex
>>>      (~PluginRequestProcessor): Remove destruction of tc_mutex
>>>      (eval): Proceed with _eval only if instance is valid.
>>>      (call): Proceed with _call only if instance is valid. Moved initialization
>>>      of result_variant_jniid and result_variant to the top.
>>>      (sendString): Proceed with _getString only if instance is valid. Remove
>>>      thread count incrementer.
>>>      (setMember): Proceed with _setMember only if instance is valid. Remove
>>>      thread count incrementer.
>>>      (sendMember): Proceed with _getMember only if instance is valid. Remove
>>>      thread count incrementer.
>>>      * plugin/icedteanp/IcedTeaPluginRequestProcessor.h: Removed tc_mutex and
>>>      thread_count variables.
>>>
>>> 1.0 is affected too, and I think the patch should be applied there too.
>>>
>>
>> Overall, the patch looks fine. Additional comments inline below.
>>
>>>
>>> plugin-instance-check-cleanup.patch
>>>
>>>
>>> diff -r 5dbf0f9de599 plugin/icedteanp/IcedTeaPluginRequestProcessor.cc
>>> --- a/plugin/icedteanp/IcedTeaPluginRequestProcessor.cc	Wed Mar 02 11:50:30 2011 -0500
>>> +++ b/plugin/icedteanp/IcedTeaPluginRequestProcessor.cc	Wed Mar 02 16:12:14 2011 -0500
>>> @@ -236,6 +233,10 @@
>>>       thread_data.parameters.push_back(NPVARIANT_TO_OBJECT(*window_ptr));
>>>       thread_data.parameters.push_back(&script);
>>>
>>> +    // If instance no longer exists, bail
>>> +    if (!instance)
>>> +        return;
>>> +
>>
>> Any reason this isn't being done as soon as instance is computed?
>>
>>>   #ifdef CHROMIUM_WORKAROUND
>>>       // Workaround for chromium
>>>       _eval(&thread_data);
>>> @@ -282,6 +283,8 @@
>>>       std::string response = std::string();
>>>       JavaRequestProcessor java_request = JavaRequestProcessor();
>>>       JavaResultData* java_result;
>>> +    std::string result_variant_jniid = std::string();
>>> +    NPVariant* result_variant;
>>>
>>
>> My c++ is quite rusty these days, but "std::string
>> result_variant_jniid" is sufficient; you dont need to do "=
>> std::string()"
>>
>
> It is. I just added it to make it clear what was being initialized in
> the function vs what was being initialized elsewhere.
>

Ok.

>>>       reference = atoi(message_parts->at(3)->c_str());
>>>
>>> @@ -331,6 +334,10 @@
>>>       thread_data.parameters.push_back(&arg_count);
>>>       thread_data.parameters.push_back(args_array);
>>>
>>> +    // If instance no longer exists, bail
>>> +    if (!instance)
>>> +        goto cleanup;
>>> +
>>>   #ifdef CHROMIUM_WORKAROUND
>>>       // Workaround for chromium
>>>       _call(&thread_data);
>>> @@ -346,8 +353,7 @@
>>>       }
>>>   #endif
>>>
>>> -    NPVariant* result_variant = (NPVariant*) IcedTeaPluginUtilities::stringToJSID(thread_data.result);
>>> -    std::string result_variant_jniid = std::string();
>>> +    result_variant = (NPVariant*) IcedTeaPluginUtilities::stringToJSID(thread_data.result);
>>>
>>
>> This seems completely unrelated to the rest of this patch.
>>
>
> Nope, it is not. goto cannot cross initialization, which is why it had
> to be moved up.
>

Oh, I see.

>>>       if (result_variant)
>>>       {
>>> @@ -395,6 +401,10 @@
>>>       thread_data.parameters.push_back(instance);
>>>       thread_data.parameters.push_back(variant);
>>>
>>> +    // If instance no longer exists, bail
>>> +    if (!instance)
>>> +        return;
>>> +
>>>   #ifdef CHROMIUM_WORKAROUND
>>>       // Workaround for chromium
>>>       _getString(&thread_data);
>>> @@ -490,6 +494,10 @@
>>>       thread_data.parameters.push_back(&property_identifier);
>>>       thread_data.parameters.push_back(&value);
>>>
>>> +    // If instance no longer exists, bail
>>> +    if (!instance)
>>> +        return;
>>> +
>>
>> Any reason not to add this after the line initializing instance?
>>
>
> Yep. Instance may become invalid at any point in time. It is best to
> wait till last minute.
>

instance is not a volatile, nor is it a reference variable and it's 
never written (after being initialized). I cant figure out how it will 
change. Perhaps a you can add a comment explaining the change?

> Thanks for reviewing! I will repost it later today (same content, just
> broken down as Andrew suggested).
>

That would be great.

Cheers,
Omair