Plugin Power Saver: Throttled Plugins should block TCPSocket reads.

Plugin Power Saver: Throttled Plugins should block TCPSocket reads.

BUG=458687, 403800
Committed: https://crrev.com/c7634dec83f331accdb94d455cf0a53ddd3bd98b
Cr-Commit-Position: refs/heads/master@{#316963}

[tommycli, 2015-02-13 23:17:25]

tommycli@chromium.org changed reviewers:
+ dmichael@chromium.org

[tommycli, 2015-02-13 23:17:25]

dmichael: PTAL

This is the TCP Socket throttling implementation we discussed. It works in my
cursory tests (prevents video plugins from buffering while throttled).

I think it's worthwhile to make part of the PPS throttling process.

Thanks!

[tommycli, 2015-02-13 23:32:29]

Patchset #3 (id:40001) has been deleted

[dmichael (off chromium), 2015-02-17 22:00:52]

https://codereview.chromium.org/929483004/diff/20001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc
(right):

https://codereview.chromium.org/929483004/diff/20001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:333:
return PP_ERROR_INPROGRESS;
Hmm...  I think I'd like this better if it wasn't surfacing a behavior change to
the plugin. For example, if the plugin is throttled at the time it attempts its
first TCP Read, it will get PP_ERROR_INPROGRESS. That error return doesn't
really make sense unless there really is already a pending Read call.

I would prefer if we could actually "pause"...  since this is inherently an
async action, we ought to be able to register to observe changes in throttle
state. We could then stash the request for later (or make it, and stash the
response). If/when we learn the plugin is not throttled, we continue with the
request. So from the plugin process's perspective, the TCP operation is just
taking a long time.

WDYT?

WDYT?

https://codereview.chromium.org/929483004/diff/20001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:947:
int pending_host_id = ppapi_host_->AddPendingResourceHost(host.Pass());
Now that you're keeping the BrowserPpapiHostImpl, you can replace this with:
host_->GetPpapiHost()->AddPending...

and delete ppapi_host_.

[tommycli, 2015-02-17 23:21:13]

dmichael: I addressed your comments.

Doing it the 'right' way actually seems to improve plugin behavior. (The
previous patchset seemed to cause plugins to 'lag' before buffering on
unthrottle).

https://codereview.chromium.org/929483004/diff/20001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc
(right):

https://codereview.chromium.org/929483004/diff/20001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:333:
return PP_ERROR_INPROGRESS;
On 2015/02/17 22:00:52, dmichael wrote:
> Hmm...  I think I'd like this better if it wasn't surfacing a behavior change
to
> the plugin. For example, if the plugin is throttled at the time it attempts
its
> first TCP Read, it will get PP_ERROR_INPROGRESS. That error return doesn't
> really make sense unless there really is already a pending Read call.
> 
> I would prefer if we could actually "pause"...  since this is inherently an
> async action, we ought to be able to register to observe changes in throttle
> state. We could then stash the request for later (or make it, and stash the
> response). If/when we learn the plugin is not throttled, we continue with the
> request. So from the plugin process's perspective, the TCP operation is just
> taking a long time.
> 
> WDYT?
> 
> WDYT? 

Done. I think what you said is sensible.

https://codereview.chromium.org/929483004/diff/20001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:947:
int pending_host_id = ppapi_host_->AddPendingResourceHost(host.Pass());
On 2015/02/17 22:00:52, dmichael wrote:
> Now that you're keeping the BrowserPpapiHostImpl, you can replace this with:
> host_->GetPpapiHost()->AddPending...
> 
> and delete ppapi_host_.

Done.

[tommycli, 2015-02-18 18:28:26]

tommycli@chromium.org changed reviewers:
+ tsepez@chromium.org

[tommycli, 2015-02-18 18:28:27]

tsepez: May I have a security review of the view_messages.h changes? Thanks!

[dmichael (off chromium), 2015-02-18 18:58:09]

https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right):

https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:144: new
ObserverList<InstanceObserver>());
If you use map::insert, I believe your value type doesn't have to be copyable.
You will want a constructor that takes the instance_data and ObserverList. But
in that case, you should be able to hold the ObserverList by scoped_ptr and
Pass() it in.

https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
File content/browser/renderer_host/pepper/browser_ppapi_host_impl.h (right):

https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:38: virtual void
OnThrottleStateChanged(bool is_throttled) = 0;
Maybe add a note about what this function means and the fact that it's invoked
on the IO thread?

https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h
(right):

https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:234: //
If the plugin is throttled, we completing socket reads until unthrottled.
I think you meant something like "...we won't complete socket reads..."

[tommycli, 2015-02-18 19:19:10]

dmichael: Thanks.

Do you think this patch will have adverse side effects?

I don't know much about TCP. If a read request takes a really long time, is that
bad? Will the plugin assume the connection dropped?

What if the user actually drops the connection while throttled?

https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right):

https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:144: new
ObserverList<InstanceObserver>());
On 2015/02/18 18:58:09, dmichael wrote:
> If you use map::insert, I believe your value type doesn't have to be copyable.
> You will want a constructor that takes the instance_data and ObserverList. But
> in that case, you should be able to hold the ObserverList by scoped_ptr and
> Pass() it in.

Hey, I tried what you recommended in PS6, but I couldn't get it to work.

I think map::insert still requires a copy. Once map::emplace is allowed, I think
we can no longer require the copy.

I know linked_ptr usage is janky. Sorry.

https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
File content/browser/renderer_host/pepper/browser_ppapi_host_impl.h (right):

https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:38: virtual void
OnThrottleStateChanged(bool is_throttled) = 0;
On 2015/02/18 18:58:09, dmichael wrote:
> Maybe add a note about what this function means and the fact that it's invoked
> on the IO thread?

Done. I assume it's okay that it's invoked on IO thread because the message
filter operates on IO thread also right?

https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h
(right):

https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:234: //
If the plugin is throttled, we completing socket reads until unthrottled.
On 2015/02/18 18:58:09, dmichael wrote:
> I think you meant something like "...we won't complete socket reads..."

Done. I accidentally the whole word.

[dmichael (off chromium), 2015-02-18 20:31:30]

On 2015/02/18 19:19:10, tommycli wrote:
> dmichael: Thanks.
> 
> Do you think this patch will have adverse side effects?
I'm not a TCP expert, but I think in the context of how throttling is designed
right now, this is the most sensible thing we could do.

I guess I have broader concerns, like...  What other APIs will we need to
throttle? Graphics2D? Graphics3D? FileIO? WebSockets? UDP? Is it worth it if we
have to add "throttling" to nearly all our Pepper APIs?

I think this throttling in TCP is reasonable for now if it has the right impact
in your experiments.

> 
> I don't know much about TCP. If a read request takes a really long time, is
that
> bad? Will the plugin assume the connection dropped?
I doubt that most plugins set a timeout for this, but I guess it's possible.

> 
> What if the user actually drops the connection while throttled?
When unthrottled, the plugin will get 1 packet and then the error. It's probably
OK.

> 
>
https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
> File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right):
> 
>
https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
> content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:144: new
> ObserverList<InstanceObserver>());
> On 2015/02/18 18:58:09, dmichael wrote:
> > If you use map::insert, I believe your value type doesn't have to be
copyable.
> > You will want a constructor that takes the instance_data and ObserverList.
But
> > in that case, you should be able to hold the ObserverList by scoped_ptr and
> > Pass() it in.
> 
> Hey, I tried what you recommended in PS6, but I couldn't get it to work.
> 
> I think map::insert still requires a copy. Once map::emplace is allowed, I
think
> we can no longer require the copy.
> 
> I know linked_ptr usage is janky. Sorry.
> 
>
https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
> File content/browser/renderer_host/pepper/browser_ppapi_host_impl.h (right):
> 
>
https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
> content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:38: virtual
void
> OnThrottleStateChanged(bool is_throttled) = 0;
> On 2015/02/18 18:58:09, dmichael wrote:
> > Maybe add a note about what this function means and the fact that it's
invoked
> > on the IO thread?
> 
> Done. I assume it's okay that it's invoked on IO thread because the message
> filter operates on IO thread also right?
> 
>
https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
> File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h
> (right):
> 
>
https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
> content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:234:
//
> If the plugin is throttled, we completing socket reads until unthrottled.
> On 2015/02/18 18:58:09, dmichael wrote:
> > I think you meant something like "...we won't complete socket reads..."
> 
> Done. I accidentally the whole word.

[dmichael (off chromium), 2015-02-18 20:31:40]

https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right):

https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:144: new
ObserverList<InstanceObserver>());
On 2015/02/18 19:19:09, tommycli wrote:
> On 2015/02/18 18:58:09, dmichael wrote:
> > If you use map::insert, I believe your value type doesn't have to be
copyable.
> > You will want a constructor that takes the instance_data and ObserverList.
But
> > in that case, you should be able to hold the ObserverList by scoped_ptr and
> > Pass() it in.
> 
> Hey, I tried what you recommended in PS6, but I couldn't get it to work.
> 
> I think map::insert still requires a copy. Once map::emplace is allowed, I
think
> we can no longer require the copy.
> 
> I know linked_ptr usage is janky. Sorry.
Bummer.

How about ScoperPtrHashMap instead, then?

https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
File content/browser/renderer_host/pepper/browser_ppapi_host_impl.h (right):

https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:38: virtual void
OnThrottleStateChanged(bool is_throttled) = 0;
On 2015/02/18 19:19:09, tommycli wrote:
> On 2015/02/18 18:58:09, dmichael wrote:
> > Maybe add a note about what this function means and the fact that it's
invoked
> > on the IO thread?
> 
> Done. I assume it's okay that it's invoked on IO thread because the message
> filter operates on IO thread also right?
Right, I just want to make sure it's documented right so the next person that
sees it knows what's going on.

[Tom Sepez, 2015-02-18 20:40:12]

Messages LGTM

[tommycli, 2015-02-18 21:29:44]

New patchsets have been uploaded after l-g-t-m from tsepez@chromium.org

[tommycli, 2015-02-18 21:31:17]

dmichael: See if this is better. Thanks!

https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right):

https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:144: new
ObserverList<InstanceObserver>());
On 2015/02/18 20:31:39, dmichael wrote:
> On 2015/02/18 19:19:09, tommycli wrote:
> > On 2015/02/18 18:58:09, dmichael wrote:
> > > If you use map::insert, I believe your value type doesn't have to be
> copyable.
> > > You will want a constructor that takes the instance_data and ObserverList.
> But
> > > in that case, you should be able to hold the ObserverList by scoped_ptr
and
> > > Pass() it in.
> > 
> > Hey, I tried what you recommended in PS6, but I couldn't get it to work.
> > 
> > I think map::insert still requires a copy. Once map::emplace is allowed, I
> think
> > we can no longer require the copy.
> > 
> > I know linked_ptr usage is janky. Sorry.
> Bummer.
> 
> How about ScoperPtrHashMap instead, then?

Done.

https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
File content/browser/renderer_host/pepper/browser_ppapi_host_impl.h (right):

https://codereview.chromium.org/929483004/diff/100001/content/browser/rendere...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:38: virtual void
OnThrottleStateChanged(bool is_throttled) = 0;
On 2015/02/18 20:31:40, dmichael wrote:
> On 2015/02/18 19:19:09, tommycli wrote:
> > On 2015/02/18 18:58:09, dmichael wrote:
> > > Maybe add a note about what this function means and the fact that it's
> invoked
> > > on the IO thread?
> > 
> > Done. I assume it's okay that it's invoked on IO thread because the message
> > filter operates on IO thread also right?
> Right, I just want to make sure it's documented right so the next person that
> sees it knows what's going on.

Done.

[dmichael (off chromium), 2015-02-18 21:44:39]

https://codereview.chromium.org/929483004/diff/160001/content/browser/rendere...
File content/browser/renderer_host/pepper/browser_ppapi_host_impl.h (right):

https://codereview.chromium.org/929483004/diff/160001/content/browser/rendere...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:157:
instance_observers_map_;
Couldn't you still put it in InstanceData, and make InstanceMap a
ScopedPtrHashMap?

[tommycli, 2015-02-18 22:17:27]

dmichael: thanks

https://codereview.chromium.org/929483004/diff/160001/content/browser/rendere...
File content/browser/renderer_host/pepper/browser_ppapi_host_impl.h (right):

https://codereview.chromium.org/929483004/diff/160001/content/browser/rendere...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.h:157:
instance_observers_map_;
On 2015/02/18 21:44:39, dmichael wrote:
> Couldn't you still put it in InstanceData, and make InstanceMap a
> ScopedPtrHashMap?

Done. Ah. I didn't see that.

[dmichael (off chromium), 2015-02-18 22:32:37]

lgtm

https://codereview.chromium.org/929483004/diff/200001/content/browser/rendere...
File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right):

https://codereview.chromium.org/929483004/diff/200001/content/browser/rendere...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:175: if (data !=
nullptr)
nit: I'd just write:
if (data)

[tommycli, 2015-02-18 22:56:26]

New patchsets have been uploaded after l-g-t-m from dmichael@chromium.org

[tommycli, 2015-02-18 22:56:45]

dmichael: thanks!

https://codereview.chromium.org/929483004/diff/200001/content/browser/rendere...
File content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc (right):

https://codereview.chromium.org/929483004/diff/200001/content/browser/rendere...
content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc:175: if (data !=
nullptr)
On 2015/02/18 22:32:37, dmichael wrote:
> nit: I'd just write:
> if (data)

Done.

[tommycli, 2015-02-18 22:56:55]

tommycli@chromium.org changed reviewers:
+ piman@chromium.org

[tommycli, 2015-02-18 22:57:11]

piman: May I have an OWNERs review?

Thanks, Tommy

[piman, 2015-02-18 23:16:30]

lgtm

[tommycli, 2015-02-18 23:17:21]

piman: thanks!

[tommycli, 2015-02-18 23:17:42]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2015-02-18 23:18:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/929483004/220001

[commit-bot: I haz the power, 2015-02-19 01:46:45]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2015-02-19 01:47:18]

Patchset 11 (id:??) landed as
https://crrev.com/c7634dec83f331accdb94d455cf0a53ddd3bd98b
Cr-Commit-Position: refs/heads/master@{#316963}