Ensure that all NPN_PluginThreadAsyncCall callbacks are invoked before NPP_De...

Ensure that NPN_PluginThreadAsyncCall callbacks are not invoked after NPP_Destroy.
TEST=none
BUG=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30628

[apatrick, 2009-10-27 20:15:58]

What do you think about doing it this way? Is pumping the message loop in
NPP_Destroy safe?

[jam, 2009-10-27 20:23:15]

I'm not sure what side effects can exist because of this.  Perhaps it will lead
to other instances in the process getting called, which might not be expected by
the plugin.

The least risky way is to have a ScopedRunnableMethodFactory as a member
variable of PluginInstance, and use it to run the method.  That way if
PluginInstance dies (or you revoke all tasks in Destroy manually), the callbacks
won't run.

[apatrick, 2009-10-27 20:32:15]

If I use a ScopedRunnableMethodFactory to schedule the async callback, is it
guaranteed that the PluginInstance object will be destroyed immediately after
NPP_Destroy? The problem I'm seeing is that the PluginInstance object remains
after NPP_Destroy. But that might be only because there are pending method tasks
pointing at it. I would need to do likewise for the NPN_ScheduleTimer tasks as
well right? Are there any other method tasks run on the PluginInstance? There
are none in plugin_instance.cc but I don't know about elsewhere.

[apatrick, 2009-10-27 21:05:55]

Unfortunately ScopedRunnableMethodFactory isn't thread safe and
NPN_PluginThreadAsyncCall can be called from any thread. I'll fall back to the
original plan of checking webplugin_ isn't null but without pumping the message
loop. Unless you have any other ideas?

[apatrick, 2009-10-27 22:20:02]

Uploaded.

[jam, 2009-10-28 06:46:48]

The change lgtm, can a regression test be added?  Thanks

[jam, 2009-10-28 15:34:51]

You can call RevokeAll() on the factor before calling NPP_Destroy, which
will make the runnable methods not run.

On Tue, Oct 27, 2009 at 1:32 PM, <apatrick@google.com> wrote:

> If I use a ScopedRunnableMethodFactory to schedule the async callback, is
> it
> guaranteed that the PluginInstance object will be destroyed immediately
> after
> NPP_Destroy? The problem I'm seeing is that the PluginInstance object
> remains
> after NPP_Destroy. But that might be only because there are pending method
> tasks
> pointing at it. I would need to do likewise for the NPN_ScheduleTimer tasks
> as
> well right? Are there any other method tasks run on the PluginInstance?
> There
> are none in plugin_instance.cc but I don't know about elsewhere.
>
>
> http://codereview.chromium.org/338050
>

[apatrick, 2009-10-28 16:55:24]

I've been thinking about that. Two problems...

How could I arrange things to guarantee that the posted task was not run
before hitting NPP_Destroy? I could call NPN_PluginThreadAsyncCall in the
final NPP_SetWindow with the null window handle and test that the task is
never run. That doesn't seem like much of a test but posting the task before
then seems like there would be a potential race condition.

At least as an npapi ui_test, it seems that NPP_Destroy might be the last
chance for the test to flag an error whereas the task, if it ran at the
wrong time, would happen after that. I have to wait until I get to my desk
to verify that though.

Al

On Tue, Oct 27, 2009 at 11:46 PM, <jam@chromium.org> wrote:

> The change lgtm, can a regression test be added?  Thanks
>
>
> http://codereview.chromium.org/338050
>

[apatrick, 2009-10-28 17:37:31]

Confirmed on the second point. The npapi tests signal success or failure by
invoking javascript functions using NPN_Evaluate. The latest time the test
could do this would be in NPP_Destroy because the instance is invalid after
that. I could make the test crash or timeout in the event that the callback
was invoked after NPP_Destroy. Will do the former.

On Wed, Oct 28, 2009 at 9:55 AM, Alastair Patrick <apatrick@google.com>wrote:

> I've been thinking about that. Two problems...
>
> How could I arrange things to guarantee that the posted task was not run
> before hitting NPP_Destroy? I could call NPN_PluginThreadAsyncCall in the
> final NPP_SetWindow with the null window handle and test that the task is
> never run. That doesn't seem like much of a test but posting the task before
> then seems like there would be a potential race condition.
>
> At least as an npapi ui_test, it seems that NPP_Destroy might be the last
> chance for the test to flag an error whereas the task, if it ran at the
> wrong time, would happen after that. I have to wait until I get to my desk
> to verify that though.
>
> Al
>
>
> On Tue, Oct 27, 2009 at 11:46 PM, <jam@chromium.org> wrote:
>
>> The change lgtm, can a regression test be added?  Thanks
>>
>>
>> http://codereview.chromium.org/338050
>>
>
>

[jam, 2009-10-28 17:51:42]

On 2009/10/28 17:37:31, apatrick wrote:
> Confirmed on the second point. The npapi tests signal success or failure by
> invoking javascript functions using NPN_Evaluate. The latest time the test
> could do this would be in NPP_Destroy because the instance is invalid after
> that. I could make the test crash or timeout in the event that the callback
> was invoked after NPP_Destroy. Will do the former.

Another option is to have two instances of the plugin in the test.  One in an
iframe that you destroy.  They could communicate using global variables and the
long lived instance on the main page could be the one to signal that the test is
complete.  We have used this pattern in some tests.

> 
> On Wed, Oct 28, 2009 at 9:55 AM, Alastair Patrick <apatrick@google.com>wrote:
> 
> > I've been thinking about that. Two problems...
> >
> > How could I arrange things to guarantee that the posted task was not run
> > before hitting NPP_Destroy? I could call NPN_PluginThreadAsyncCall in the
> > final NPP_SetWindow with the null window handle and test that the task is
> > never run. That doesn't seem like much of a test but posting the task before
> > then seems like there would be a potential race condition.
> >
> > At least as an npapi ui_test, it seems that NPP_Destroy might be the last
> > chance for the test to flag an error whereas the task, if it ran at the
> > wrong time, would happen after that. I have to wait until I get to my desk
> > to verify that though.
> >
> > Al
> >
> >
> > On Tue, Oct 27, 2009 at 11:46 PM, <mailto:jam@chromium.org> wrote:
> >
> >> The change lgtm, can a regression test be added?  Thanks
> >>
> >>
> >> http://codereview.chromium.org/338050
> >>
> >
> >
>

[jam, 2009-10-28 18:35:11]

ah yes, I forgot that SRMF isn't thread safe.  Checking webplugin_ without
pumping the message loop sounds fine.

On Tue, Oct 27, 2009 at 2:05 PM, <apatrick@google.com> wrote:

> Unfortunately ScopedRunnableMethodFactory isn't thread safe and
> NPN_PluginThreadAsyncCall can be called from any thread. I'll fall back to
> the
> original plan of checking webplugin_ isn't null but without pumping the
> message
> loop. Unless you have any other ideas?
>
>
> http://codereview.chromium.org/338050
>

[apatrick, 2009-10-28 20:03:00]

Added test.

[jam, 2009-10-29 22:16:30]

lgtm

Sorry for the delay, just saw this.

http://codereview.chromium.org/338050/diff/4006/4011
File webkit/glue/plugins/test/plugin_thread_async_call_test.h (right):

http://codereview.chromium.org/338050/diff/4006/4011#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
nit: should just be 2009

http://codereview.chromium.org/338050/diff/4006/4011#newcode11
Line 11: #include "base/time.h"
nit: the first three includes don't look like they're needed