Pepper: Tighten GetLiveModule in PluginRegistry.

Pepper: Tighten GetLiveModule in PluginRegistry.

On PepperPluginInstanceImpl::Delete(), we send a synchronous DidDestroy message
to the plugin.  The plugin can exit() in response to this message.

While the renderer is waiting for a response (and holding a reference to
PepperPluginInstanceImpl on the stack), another plugin for the page could
create an instance of a plugin using the same path as the deleted instance.

This can cause a PluginModule to be reused on the host for a plugin which is no
longer live. In this case, sending DidCreate will fail.

Alternatively, the lifetime of PluginModule could be cleaned up, but there are
many uses of PepperPluginInstanceImpl::module() throughout the codebase. I'm
happy to look at seeing if that can be cleaned up in another change.

BUG=
R=dmichael@chromium.org, teravest@google.com

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234659

[dmichael (off chromium), 2013-11-11 23:41:36]

I'm not excited about this strategy... especially if it has to special-case the
empty set.

I'll be interested to see if clearing the module earlier is practical.

Here's another idea you can try, to see if it's any simpler than that...
PpapiPluginInstanceImpl::Delete could call:
 module_->InstanceDeleted(this);
instead of that being in the destructor.

If PluginModule::InstanceDeleted deletes the last instance, it can inform the
plugin registry.

Or something.

May not work right.

https://codereview.chromium.org/69363002/diff/70001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_plugin_registry.cc (right):

https://codereview.chromium.org/69363002/diff/70001/content/renderer/pepper/p...
content/renderer/pepper/pepper_plugin_registry.cc:48: if (it ==
live_modules_.end())
I think "it" and "jt" are too similar.
Maybe:
it->module_iter
jt->instance_iter
?

[teravest, 2013-11-12 15:19:34]

On 2013/11/11 23:41:36, dmichael wrote:
> I'm not excited about this strategy... especially if it has to special-case
the
> empty set.
> 
> I'll be interested to see if clearing the module earlier is practical.
> 
> Here's another idea you can try, to see if it's any simpler than that...
> PpapiPluginInstanceImpl::Delete could call:
>  module_->InstanceDeleted(this);
> instead of that being in the destructor.
> 
> If PluginModule::InstanceDeleted deletes the last instance, it can inform the
> plugin registry.
> 
> Or something.
> 
> May not work right.

From a cursory look, this seems plausible. I'll try it this morning. Thanks for
the idea.

> 
>
https://codereview.chromium.org/69363002/diff/70001/content/renderer/pepper/p...
> File content/renderer/pepper/pepper_plugin_registry.cc (right):
> 
>
https://codereview.chromium.org/69363002/diff/70001/content/renderer/pepper/p...
> content/renderer/pepper/pepper_plugin_registry.cc:48: if (it ==
> live_modules_.end())
> I think "it" and "jt" are too similar.
> Maybe:
> it->module_iter
> jt->instance_iter
> ?

[teravest1, 2013-11-12 16:21:35]

Sadly, that won't work.

We can't call module_->InstanceDeleted(this); before sending DidDestory.

This is because DidDestroy does this:
void DidDestroy(PP_Instance instance) {
  HostDispatcher::GetForInstance(instance)->Send(
      new PpapiMsg_PPPInstance_DidDestroy(API_ID_PPP_INSTANCE, instance));
}

but looking up the instance in HostDispatcher is no longer possible
once we call PluginModule::InstanceDeleted.

On Tue, Nov 12, 2013 at 8:19 AM,  <teravest@chromium.org> wrote:
> On 2013/11/11 23:41:36, dmichael wrote:
>>
>> I'm not excited about this strategy... especially if it has to
>> special-case
>
> the
>>
>> empty set.
>
>
>> I'll be interested to see if clearing the module earlier is practical.
>
>
>> Here's another idea you can try, to see if it's any simpler than that...
>> PpapiPluginInstanceImpl::Delete could call:
>>   module_->InstanceDeleted(this);
>> instead of that being in the destructor.
>
>
>> If PluginModule::InstanceDeleted deletes the last instance, it can inform
>> the
>> plugin registry.
>
>
>> Or something.
>
>
>> May not work right.
>
>
> From a cursory look, this seems plausible. I'll try it this morning. Thanks
> for
> the idea.
>
>
>
>
>
https://codereview.chromium.org/69363002/diff/70001/content/renderer/pepper/p...
>>
>> File content/renderer/pepper/pepper_plugin_registry.cc (right):
>
>
>
>
https://codereview.chromium.org/69363002/diff/70001/content/renderer/pepper/p...
>>
>> content/renderer/pepper/pepper_plugin_registry.cc:48: if (it ==
>> live_modules_.end())
>> I think "it" and "jt" are too similar.
>> Maybe:
>> it->module_iter
>> jt->instance_iter
>> ?
>
>
>
>
> https://codereview.chromium.org/69363002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[teravest, 2013-11-12 17:13:50]

I've updated the change to rename "it" and "jt" as you suggested. Also I now use
"empty()" instead of "size() == 0".

I looked at the change to see if I could add a dead_modules_ to
PepperPluginRegistry, but that won't work, since the check we want to make is
whether any instance references the module without having been Delete()d. One
instance can't register its module in dead_modules_ since another instance may
not have had Delete() called on it yet.

Sadly, I think this change might be the simplest way to express the lifetime
issues here.

[dmichael (off chromium), 2013-11-12 17:28:48]

On 2013/11/12 17:13:50, teravest wrote:
> I've updated the change to rename "it" and "jt" as you suggested. Also I now
use
> "empty()" instead of "size() == 0".
> 
> I looked at the change to see if I could add a dead_modules_ to
> PepperPluginRegistry, but that won't work, since the check we want to make is
> whether any instance references the module without having been Delete()d.
But we know the instance is in the process of being deleted, since it calls
module_->InstanceDeleted()...

Can't PluginModule, in there, determine if *all* remaining plugins are dying
somehow? And only in that case tell the PepperPluginRegistry that the
PluginModule is dying?

> One
> instance can't register its module in dead_modules_ since another instance may
> not have had Delete() called on it yet.
> 
> Sadly, I think this change might be the simplest way to express the lifetime
> issues here.
Might be...  lgtm if the above suggestion isn't practical.

[teravest, 2013-11-12 17:44:58]

On Tue, Nov 12, 2013 at 10:28 AM,  <dmichael@chromium.org> wrote:
> On 2013/11/12 17:13:50, teravest wrote:
>>
>> I've updated the change to rename "it" and "jt" as you suggested. Also I
>> now
>
> use
>>
>> "empty()" instead of "size() == 0".
>
>
>> I looked at the change to see if I could add a dead_modules_ to
>> PepperPluginRegistry, but that won't work, since the check we want to make
>> is
>> whether any instance references the module without having been Delete()d.
>
> But we know the instance is in the process of being deleted, since it calls
> module_->InstanceDeleted()...

No, that happens in
PepperPluginInstanceImpl::~PepperPluginInstanceImpl, not in
PepperPluginInstanceImpl::Delete().

We can't call that before DidDestroy, see my earlier comment about
trying that approach.

>
> Can't PluginModule, in there, determine if *all* remaining plugins are dying
> somehow? And only in that case tell the PepperPluginRegistry that the
> PluginModule is dying?

Well, PluginModule could have an internal count and instances could
call something like
  module->MarkInstanceDeletedForModuleLiveness();
...and increment the count, which PluginModule could compare with
instances_.size().

Then PepperPluginRegistry would call
module->AllInstancesMarkedDeleted() or something similar instead of
the logic today.

That would look like:
bool AllInstancesMarkedDeleted() {
  // Require that at least one instance was marked deleted.
  // Otherwise, we could return true if InstanceCreated was never called yet.
  return instances_marked_deleted_count_ > 0 &&
      instances_marked_deleted_count_ == instances.size();
}

I don't like that approach because I've had to think of a new term
(marked deleted), and we're still left with a weird 0-case check
somewhere and a comment.


>
>
>> One
>> instance can't register its module in dead_modules_ since another instance
>> may
>> not have had Delete() called on it yet.
>
>
>> Sadly, I think this change might be the simplest way to express the
>> lifetime
>> issues here.
>
> Might be...  lgtm if the above suggestion isn't practical.
>
>
> https://codereview.chromium.org/69363002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmichael (off chromium), 2013-11-12 17:53:58]

Okay, I give up. The patch you have is probably the best we can do. Thanks
for bearing with me.


On Tue, Nov 12, 2013 at 10:44 AM, Justin TerAvest <teravest@chromium.org>wrote:

> On Tue, Nov 12, 2013 at 10:28 AM,  <dmichael@chromium.org> wrote:
> > On 2013/11/12 17:13:50, teravest wrote:
> >>
> >> I've updated the change to rename "it" and "jt" as you suggested. Also I
> >> now
> >
> > use
> >>
> >> "empty()" instead of "size() == 0".
> >
> >
> >> I looked at the change to see if I could add a dead_modules_ to
> >> PepperPluginRegistry, but that won't work, since the check we want to
> make
> >> is
> >> whether any instance references the module without having been
> Delete()d.
> >
> > But we know the instance is in the process of being deleted, since it
> calls
> > module_->InstanceDeleted()...
>
> No, that happens in
> PepperPluginInstanceImpl::~PepperPluginInstanceImpl, not in
> PepperPluginInstanceImpl::Delete().
>
> We can't call that before DidDestroy, see my earlier comment about
> trying that approach.
>
> >
> > Can't PluginModule, in there, determine if *all* remaining plugins are
> dying
> > somehow? And only in that case tell the PepperPluginRegistry that the
> > PluginModule is dying?
>
> Well, PluginModule could have an internal count and instances could
> call something like
>   module->MarkInstanceDeletedForModuleLiveness();
> ...and increment the count, which PluginModule could compare with
> instances_.size().
>
> Then PepperPluginRegistry would call
> module->AllInstancesMarkedDeleted() or something similar instead of
> the logic today.
>
> That would look like:
> bool AllInstancesMarkedDeleted() {
>   // Require that at least one instance was marked deleted.
>   // Otherwise, we could return true if InstanceCreated was never called
> yet.
>   return instances_marked_deleted_count_ > 0 &&
>       instances_marked_deleted_count_ == instances.size();
> }
>
> I don't like that approach because I've had to think of a new term
> (marked deleted), and we're still left with a weird 0-case check
> somewhere and a comment.
>
>
> >
> >
> >> One
> >> instance can't register its module in dead_modules_ since another
> instance
> >> may
> >> not have had Delete() called on it yet.
> >
> >
> >> Sadly, I think this change might be the simplest way to express the
> >> lifetime
> >> issues here.
> >
> > Might be...  lgtm if the above suggestion isn't practical.
> >
> >
> > https://codereview.chromium.org/69363002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[teravest1, 2013-11-12 18:05:43]

Thanks for walking through this with me.
Mind giving it an lgtm?

On Tue, Nov 12, 2013 at 10:53 AM, David Michael <dmichael@chromium.org> wrote:
> Okay, I give up. The patch you have is probably the best we can do. Thanks
> for bearing with me.
>
>
> On Tue, Nov 12, 2013 at 10:44 AM, Justin TerAvest <teravest@chromium.org>
> wrote:
>>
>> On Tue, Nov 12, 2013 at 10:28 AM,  <dmichael@chromium.org> wrote:
>> > On 2013/11/12 17:13:50, teravest wrote:
>> >>
>> >> I've updated the change to rename "it" and "jt" as you suggested. Also
>> >> I
>> >> now
>> >
>> > use
>> >>
>> >> "empty()" instead of "size() == 0".
>> >
>> >
>> >> I looked at the change to see if I could add a dead_modules_ to
>> >> PepperPluginRegistry, but that won't work, since the check we want to
>> >> make
>> >> is
>> >> whether any instance references the module without having been
>> >> Delete()d.
>> >
>> > But we know the instance is in the process of being deleted, since it
>> > calls
>> > module_->InstanceDeleted()...
>>
>> No, that happens in
>> PepperPluginInstanceImpl::~PepperPluginInstanceImpl, not in
>> PepperPluginInstanceImpl::Delete().
>>
>> We can't call that before DidDestroy, see my earlier comment about
>> trying that approach.
>>
>> >
>> > Can't PluginModule, in there, determine if *all* remaining plugins are
>> > dying
>> > somehow? And only in that case tell the PepperPluginRegistry that the
>> > PluginModule is dying?
>>
>> Well, PluginModule could have an internal count and instances could
>> call something like
>>   module->MarkInstanceDeletedForModuleLiveness();
>> ...and increment the count, which PluginModule could compare with
>> instances_.size().
>>
>> Then PepperPluginRegistry would call
>> module->AllInstancesMarkedDeleted() or something similar instead of
>> the logic today.
>>
>> That would look like:
>> bool AllInstancesMarkedDeleted() {
>>   // Require that at least one instance was marked deleted.
>>   // Otherwise, we could return true if InstanceCreated was never called
>> yet.
>>   return instances_marked_deleted_count_ > 0 &&
>>       instances_marked_deleted_count_ == instances.size();
>> }
>>
>> I don't like that approach because I've had to think of a new term
>> (marked deleted), and we're still left with a weird 0-case check
>> somewhere and a comment.
>>
>>
>> >
>> >
>> >> One
>> >> instance can't register its module in dead_modules_ since another
>> >> instance
>> >> may
>> >> not have had Delete() called on it yet.
>> >
>> >
>> >> Sadly, I think this change might be the simplest way to express the
>> >> lifetime
>> >> issues here.
>> >
>> > Might be...  lgtm if the above suggestion isn't practical.
>> >
>> >
>> > https://codereview.chromium.org/69363002/
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmichael (off chromium), 2013-11-12 18:40:05]

I thought I already did a couple comments ago? lgtm


On Tue, Nov 12, 2013 at 11:05 AM, Justin TerAvest <teravest@google.com>wrote:

> Thanks for walking through this with me.
> Mind giving it an lgtm?
>
> On Tue, Nov 12, 2013 at 10:53 AM, David Michael <dmichael@chromium.org>
> wrote:
> > Okay, I give up. The patch you have is probably the best we can do.
> Thanks
> > for bearing with me.
> >
> >
> > On Tue, Nov 12, 2013 at 10:44 AM, Justin TerAvest <teravest@chromium.org
> >
> > wrote:
> >>
> >> On Tue, Nov 12, 2013 at 10:28 AM,  <dmichael@chromium.org> wrote:
> >> > On 2013/11/12 17:13:50, teravest wrote:
> >> >>
> >> >> I've updated the change to rename "it" and "jt" as you suggested.
> Also
> >> >> I
> >> >> now
> >> >
> >> > use
> >> >>
> >> >> "empty()" instead of "size() == 0".
> >> >
> >> >
> >> >> I looked at the change to see if I could add a dead_modules_ to
> >> >> PepperPluginRegistry, but that won't work, since the check we want to
> >> >> make
> >> >> is
> >> >> whether any instance references the module without having been
> >> >> Delete()d.
> >> >
> >> > But we know the instance is in the process of being deleted, since it
> >> > calls
> >> > module_->InstanceDeleted()...
> >>
> >> No, that happens in
> >> PepperPluginInstanceImpl::~PepperPluginInstanceImpl, not in
> >> PepperPluginInstanceImpl::Delete().
> >>
> >> We can't call that before DidDestroy, see my earlier comment about
> >> trying that approach.
> >>
> >> >
> >> > Can't PluginModule, in there, determine if *all* remaining plugins are
> >> > dying
> >> > somehow? And only in that case tell the PepperPluginRegistry that the
> >> > PluginModule is dying?
> >>
> >> Well, PluginModule could have an internal count and instances could
> >> call something like
> >>   module->MarkInstanceDeletedForModuleLiveness();
> >> ...and increment the count, which PluginModule could compare with
> >> instances_.size().
> >>
> >> Then PepperPluginRegistry would call
> >> module->AllInstancesMarkedDeleted() or something similar instead of
> >> the logic today.
> >>
> >> That would look like:
> >> bool AllInstancesMarkedDeleted() {
> >>   // Require that at least one instance was marked deleted.
> >>   // Otherwise, we could return true if InstanceCreated was never called
> >> yet.
> >>   return instances_marked_deleted_count_ > 0 &&
> >>       instances_marked_deleted_count_ == instances.size();
> >> }
> >>
> >> I don't like that approach because I've had to think of a new term
> >> (marked deleted), and we're still left with a weird 0-case check
> >> somewhere and a comment.
> >>
> >>
> >> >
> >> >
> >> >> One
> >> >> instance can't register its module in dead_modules_ since another
> >> >> instance
> >> >> may
> >> >> not have had Delete() called on it yet.
> >> >
> >> >
> >> >> Sadly, I think this change might be the simplest way to express the
> >> >> lifetime
> >> >> issues here.
> >> >
> >> > Might be...  lgtm if the above suggestion isn't practical.
> >> >
> >> >
> >> > https://codereview.chromium.org/69363002/
> >
> >
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-11-12 19:53:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/69363002/100002

[commit-bot: I haz the power, 2013-11-12 19:53:07]

Failed to apply patch for
content/renderer/pepper/pepper_plugin_instance_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/pepper/pepper_plugin_instance_impl.cc
  Hunk #1 FAILED at 524.
  Hunk #2 succeeded at 601 (offset 1 line).
  Hunk #3 succeeded at 633 (offset 1 line).
  1 out of 3 hunks FAILED -- saving rejects to file
content/renderer/pepper/pepper_plugin_instance_impl.cc.rej

Patch:       content/renderer/pepper/pepper_plugin_instance_impl.cc
Index: content/renderer/pepper/pepper_plugin_instance_impl.cc
diff --git a/content/renderer/pepper/pepper_plugin_instance_impl.cc
b/content/renderer/pepper/pepper_plugin_instance_impl.cc
index
29b40c3d6505482ca53f6011fd6a7e94115e30ed..e8047b9376f60decc93c9036ea43fd83ee8beccb
100644
--- a/content/renderer/pepper/pepper_plugin_instance_impl.cc
+++ b/content/renderer/pepper/pepper_plugin_instance_impl.cc
@@ -524,6 +524,7 @@ PepperPluginInstanceImpl::PepperPluginInstanceImpl(
       external_document_load_(false),
       npp_(new NPP_t),
       isolate_(v8::Isolate::GetCurrent()),
+      is_deleted_(false),
       view_change_weak_ptr_factory_(this) {
   pp_instance_ = HostGlobals::Get()->AddInstance(this);
 
@@ -600,6 +601,8 @@ PepperPluginInstanceImpl::~PepperPluginInstanceImpl() {
 // returned, then it needs to keep its own reference on the stack.
 
 void PepperPluginInstanceImpl::Delete() {
+  is_deleted_ = true;
+
   // Keep a reference on the stack. See NOTE above.
   scoped_refptr<PepperPluginInstanceImpl> ref(this);
   // Force the MessageChannel to release its "passthrough object" which should
@@ -630,6 +633,10 @@ void PepperPluginInstanceImpl::Delete() {
   container_ = NULL;
 }
 
+bool PepperPluginInstanceImpl::is_deleted() const {
+  return is_deleted_;
+}
+
 void PepperPluginInstanceImpl::Paint(WebCanvas* canvas,
                                      const gfx::Rect& plugin_rect,
                                      const gfx::Rect& paint_rect) {

[commit-bot: I haz the power, 2013-11-12 20:58:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/69363002/330001

[teravest, 2013-11-12 22:56:33]

Committed patchset #4 manually as r234659 (presubmit successful).