Pepper: Allow plugins to call PPB_UDP_Socket::SendTo multiple times.

content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc

Index: content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
diff --git a/content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc b/content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
index 448599d23fb749305b612f60d0cd73e7bd093698..482c8af03aa6c7e994acc44a87462ad7cdc822f8 100644
--- a/content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
+++ b/content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
@@ -31,6 +31,7 @@
 
 using ppapi::NetAddressPrivateImpl;
 using ppapi::host::NetErrorToPepperError;
+using ppapi::proxy::UDPSocketResourceBase;
 
 namespace {
 
@@ -40,6 +41,17 @@ size_t g_num_instances = 0;
 
 namespace content {
 
+PepperUDPSocketMessageFilter::PendingSend::PendingSend(
+    const net::IPAddressNumber& address,
+    int port,
+    const scoped_refptr<net::IOBufferWithSize>& buffer,
+    const ppapi::host::ReplyMessageContext& context)
+    : address(address), port(port), buffer(buffer), context(context) {
+}
+
+PepperUDPSocketMessageFilter::PendingSend::~PendingSend() {
+}
+
 PepperUDPSocketMessageFilter::PepperUDPSocketMessageFilter(
     BrowserPpapiHostImpl* host,
     PP_Instance instance,
@@ -47,8 +59,8 @@ PepperUDPSocketMessageFilter::PepperUDPSocketMessageFilter(
     : allow_address_reuse_(false),
       allow_broadcast_(false),
       closed_(false),
-      remaining_recv_slots_(
-          ppapi::proxy::UDPSocketResourceBase::kPluginReceiveBufferSlots),
+      remaining_recv_slots_(UDPSocketResourceBase::kPluginReceiveBufferSlots),
+      socket_busy_(false),
       external_plugin_(host->external_plugin()),
       private_api_(private_api),
       render_process_id_(0),
@@ -143,14 +155,12 @@ int32_t PepperUDPSocketMessageFilter::OnMsgSetOption(
 
       int net_result = net::ERR_UNEXPECTED;
       if (name == PP_UDPSOCKET_OPTION_SEND_BUFFER_SIZE) {
-        if (integer_value >
-            ppapi::proxy::UDPSocketResourceBase::kMaxSendBufferSize) {
+        if (integer_value > UDPSocketResourceBase::kMaxSendBufferSize) {
           return PP_ERROR_BADARGUMENT;
         }
         net_result = socket_->SetSendBufferSize(integer_value);
       } else {
-        if (integer_value >
-            ppapi::proxy::UDPSocketResourceBase::kMaxReceiveBufferSize) {
+        if (integer_value > UDPSocketResourceBase::kMaxReceiveBufferSize) {
           return PP_ERROR_BADARGUMENT;
         }
         net_result = socket_->SetReceiveBufferSize(integer_value);
@@ -231,7 +241,7 @@ int32_t PepperUDPSocketMessageFilter::OnMsgRecvSlotAvailable(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (remaining_recv_slots_ <
-          ppapi::proxy::UDPSocketResourceBase::kPluginReceiveBufferSlots) {
+      UDPSocketResourceBase::kPluginReceiveBufferSlots) {
     remaining_recv_slots_++;
   }
 
@@ -304,8 +314,7 @@ void PepperUDPSocketMessageFilter::DoRecvFrom() {
   DCHECK(!recvfrom_buffer_.get());
   DCHECK_GT(remaining_recv_slots_, 0u);
 
-  recvfrom_buffer_ = new net::IOBuffer(
-      ppapi::proxy::UDPSocketResourceBase::kMaxReadSize);
+  recvfrom_buffer_ = new net::IOBuffer(UDPSocketResourceBase::kMaxReadSize);
 
   // Use base::Unretained(this), so that the lifespan of this object doesn't
   // have to last until the callback is called.
@@ -314,7 +323,7 @@ void PepperUDPSocketMessageFilter::DoRecvFrom() {
   // called.
   int net_result = socket_->RecvFrom(
       recvfrom_buffer_.get(),
-      ppapi::proxy::UDPSocketResourceBase::kMaxReadSize,
+      UDPSocketResourceBase::kMaxReadSize,
       &recvfrom_address_,
       base::Bind(&PepperUDPSocketMessageFilter::OnRecvFromCompleted,
                  base::Unretained(this)));
@@ -334,24 +343,15 @@ void PepperUDPSocketMessageFilter::DoSendTo(
     return;
   }
 
-  if (sendto_buffer_.get()) {
-    SendSendToError(context, PP_ERROR_INPROGRESS);
-    return;
-  }
-
   size_t num_bytes = data.size();
   if (num_bytes == 0 ||
-      num_bytes > static_cast<size_t>(
-                      ppapi::proxy::UDPSocketResourceBase::kMaxWriteSize)) {
+      num_bytes > static_cast<size_t>(UDPSocketResourceBase::kMaxWriteSize)) {
     // Size of |data| is checked on the plugin side.
     NOTREACHED();
     SendSendToError(context, PP_ERROR_BADARGUMENT);
     return;
   }
 
-  sendto_buffer_ = new net::IOBufferWithSize(num_bytes);
-  memcpy(sendto_buffer_->data(), data.data(), num_bytes);
-
   net::IPAddressNumber address;
   int port;
   if (!NetAddressPrivateImpl::NetAddressToIPEndPoint(addr, &address, &port)) {
@@ -359,17 +359,43 @@ void PepperUDPSocketMessageFilter::DoSendTo(
     return;
   }
 
-  // Please see OnMsgRecvFrom() for the reason why we use base::Unretained(this)
+  scoped_refptr<net::IOBufferWithSize> buffer(
+      new net::IOBufferWithSize(num_bytes));
+  memcpy(buffer->data(), data.data(), num_bytes);
+
+  if (!socket_busy_) {
+    CallSendTo(address, port, buffer, context);

[dmichael (off chromium), 2014/10/08 17:20:46]

It looks currently like CallSendTo might call OnSendToCompleted immediately,
while CallSendTo is still on the stack. And...  OnSendToCompleted will again
call CallSendTo, and so on until net returns ERR_IO_PENDING or we run out of
buffers in the queue.

Maybe it would be cleaner to just always push on the stack here, and always call
"CallSendTo".

Then, CallSendTo checks socket_busy_. If it's not busy, it loops through the
queue and sends everything it can.

WDYT?

[bbudge, 2014/10/08 21:44:35]

I like it. Done.

+  } else {
+    // The socket is busy so queue the buffer. Make sure a malicious plugin
+    // can't queue up an unlimited number of buffers.
+    if (pending_sends_.size() ==
+        UDPSocketResourceBase::kPluginSendBufferSlots) {
+      SendSendToError(context, PP_ERROR_FAILED);
+      return;
+    }
+    pending_sends_.push(PendingSend(address, port, buffer, context));
+  }
+}
+
+void PepperUDPSocketMessageFilter::CallSendTo(
+    const net::IPAddressNumber& address,
+    int port,
+    const scoped_refptr<net::IOBufferWithSize>& buffer,
+    const ppapi::host::ReplyMessageContext& context) {
+  // See OnMsgRecvFrom() for the reason why we use base::Unretained(this)
   // when calling |socket_| methods.
   int net_result = socket_->SendTo(
-      sendto_buffer_.get(),
-      sendto_buffer_->size(),
+      buffer.get(),
+      buffer->size(),
       net::IPEndPoint(address, port),
       base::Bind(&PepperUDPSocketMessageFilter::OnSendToCompleted,
                  base::Unretained(this),
+                 buffer,
                  context));
   if (net_result != net::ERR_IO_PENDING)
-    OnSendToCompleted(context, net_result);
+    OnSendToCompleted(buffer, context, net_result);
+  else
+    socket_busy_ = true;
 }
 
 void PepperUDPSocketMessageFilter::Close() {
@@ -411,17 +437,24 @@ void PepperUDPSocketMessageFilter::OnRecvFromCompleted(int net_result) {
 }
 
 void PepperUDPSocketMessageFilter::OnSendToCompleted(
+    scoped_refptr<net::IOBufferWithSize> buffer,

[yzshen1, 2014/10/08 18:05:38]

Why do we need this?
Is it because you are concerned that the underlying net::UDPServerSocket doesn't
retain a ref to |buffer| during Send?

[bbudge, 2014/10/08 21:44:35]

Yes, it's to keep it alive until the send completes. net::UDPServerSocket
requires the caller to hold the buffer until the callback is called if the
socket can't complete the SendTo immediately.

     const ppapi::host::ReplyMessageContext& context,
     int net_result) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  DCHECK(sendto_buffer_.get());
 
   int32_t pp_result = NetErrorToPepperError(net_result);
   if (pp_result < 0)
     SendSendToError(context, pp_result);
   else
     SendSendToReply(context, PP_OK, pp_result);
-  sendto_buffer_ = NULL;
+
+  socket_busy_ = false;
+
+  if (!pending_sends_.empty()) {
+    const PendingSend& send = pending_sends_.front();
+    CallSendTo(send.address, send.port, send.buffer, send.context);
+    pending_sends_.pop();

[yzshen1, 2014/10/08 18:05:38]

I think we should pop() before calling CallSendTo().

[bbudge, 2014/10/08 21:44:35]

I think you are right, especially in light of David's comment in CallSendTo.
Done.

+  }
 }
 
 void PepperUDPSocketMessageFilter::SendBindReply(