Fix for spdy cancelling ServerBoundCertServiceRequest

Fix for SDPY not cancelling ServerBoundCertServiceRequest.

Makes ServerBoundCertService::RequestHandle a real class which handles cancellation on destruction, instead of being just a void* handle.

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

[mattm, 2012-12-28 04:46:12]

I looked at this but found I'm not familiar enough with SpdyStream to know where
to put the CancelRequest call.  Figured it would be easier just to ask you.

[Ryan Hamilton, 2012-12-28 16:30:53]

https://codereview.chromium.org/11689002/diff/1/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

https://codereview.chromium.org/11689002/diff/1/net/spdy/spdy_stream.cc#newco...
net/spdy/spdy_stream.cc:167: domain_bound_cert_request_handle_ = NULL;
This seems like a reasonable place to put this logic.  However, I think it would
be a good idea to use RAII.  Instead of making the RequestHandle a void*, it
could be an object with a destructor that would cancel the request (using
basically this code).  Then when someone make a request to the cert service,
they would store the handle in a scoped_ptr and would be guaranteed that it was
cleaned up on destruction.  Does this sounds feasible?  (I recently did
something like this in QUIC, and was happy with how it turned out.) 
SingleRequestHostResolver is a good example of wrapping up a request into a RAII
object.

[mattm, 2013-01-09 04:27:51]

https://codereview.chromium.org/11689002/diff/1/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

https://codereview.chromium.org/11689002/diff/1/net/spdy/spdy_stream.cc#newco...
net/spdy/spdy_stream.cc:167: domain_bound_cert_request_handle_ = NULL;
On 2012/12/28 16:30:53, Ryan Hamilton wrote:
> This seems like a reasonable place to put this logic.  However, I think it
would
> be a good idea to use RAII.  Instead of making the RequestHandle a void*, it
> could be an object with a destructor that would cancel the request (using
> basically this code).  Then when someone make a request to the cert service,
> they would store the handle in a scoped_ptr and would be guaranteed that it
was
> cleaned up on destruction.  Does this sounds feasible?  (I recently did
> something like this in QUIC, and was happy with how it turned out.) 
> SingleRequestHostResolver is a good example of wrapping up a request into a
RAII
> object.

Done.

[Ryan Hamilton, 2013-01-09 04:48:03]

lgtm

https://codereview.chromium.org/11689002/diff/1/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

https://codereview.chromium.org/11689002/diff/1/net/spdy/spdy_stream.cc#newco...
net/spdy/spdy_stream.cc:167: domain_bound_cert_request_handle_ = NULL;
On 2013/01/09 04:27:51, mattm wrote:
> On 2012/12/28 16:30:53, Ryan Hamilton wrote:
> > This seems like a reasonable place to put this logic.  However, I think it
> would
> > be a good idea to use RAII.  Instead of making the RequestHandle a void*, it
> > could be an object with a destructor that would cancel the request (using
> > basically this code).  Then when someone make a request to the cert service,
> > they would store the handle in a scoped_ptr and would be guaranteed that it
> was
> > cleaned up on destruction.  Does this sounds feasible?  (I recently did
> > something like this in QUIC, and was happy with how it turned out.) 
> > SingleRequestHostResolver is a good example of wrapping up a request into a
> RAII
> > object.
> 
> Done.

Nice!  A bit more code to implement the RequestHandle, but it sure makes the
calling code trivial.

[commit-bot: I haz the power, 2013-01-09 09:58:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/11689002/7001

[commit-bot: I haz the power, 2013-01-09 10:47:35]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-09 20:00:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/11689002/4004

[commit-bot: I haz the power, 2013-01-09 22:13:32]

Change committed as 175902