SPDY: Defer SpdyBufferProducer deletions in SpdyWriteQueue

These can post callbacks which re-enter into SpdyWriteQueue.

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

[willchan no longer on Chromium, 2014-05-06 00:12:13]

I have a counter-proposal. How about we delay destroying the SpdyBufferProducer
within the loop iteration? Specifically, change
https://code.google.com/p/chromium/codesearch#chromium/usr/include/c%2B%2B/4....
to move the SpdyBufferProducer into a local vector. After iterating through the
deque, delete all the SpdyBufferProducers in the vector.

I think it's a safer solution as it reduces the number of re-entry points back
into the SPDY state machines. It's also better for latency since we have fewer
yields back to the MessageLoop.

[Johnny, 2014-05-06 15:17:59]

Hmm. I have no strong objections to using a vector, it's a perfectly correct
solution, but I've been developing an opposite opinion regarding re-entrance
into SPDY.

A lot of the complexity in SPDY that I perceive is due to the extreme stack
depth that callback chains can result in. A recent specific example is
SpdyProxyClientSocket::OnWrite, which indirectly violated assumptions about the
delegate made by SpdyStream. The prevalence of WeakPtr guards mid-method also
reflects this: the method is basically throwing up it's hands and saying to the
delegate "I don't know what you do, or under what conditions you can destroy me,
so I have to assume the worst". I'm not fond of these.

I tend to think the solution is instead _more_ post-ing, where each post
transitions SPDY machinery from one consistent state to another consistent state
via a shallow & understandable operation, and WeakPtr is principally used only
to guard callbacks sitting in a MessageLoop. Use of MessageLoop can add latency
to any individual operation, but my current belief is is doesn't add net latency
to the system. I'm interested to hear arguments otherwise.

We can move this conversation elsewhere, though :) If you'd still prefer use of
vectors in this CL, I'm happy to make that change.

[Johnny, 2014-05-06 16:30:28]

Switched to vector, PTAL

[willchan no longer on Chromium, 2014-05-06 16:38:13]

lgtm

https://codereview.chromium.org/265933007/diff/20001/net/spdy/spdy_write_queu...
File net/spdy/spdy_write_queue.cc (right):

https://codereview.chromium.org/265933007/diff/20001/net/spdy/spdy_write_queu...
net/spdy/spdy_write_queue.cc:100: std::vector<SpdyBufferProducer*>
erased_buffer_producers;
You can also use ScopedVector
(https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...)
if you want.

[Johnny, 2014-05-06 16:54:27]

https://codereview.chromium.org/265933007/diff/20001/net/spdy/spdy_write_queu...
File net/spdy/spdy_write_queue.cc (right):

https://codereview.chromium.org/265933007/diff/20001/net/spdy/spdy_write_queu...
net/spdy/spdy_write_queue.cc:100: std::vector<SpdyBufferProducer*>
erased_buffer_producers;
On 2014/05/06 16:38:14, willchan wrote:
> You can also use ScopedVector
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...)
> if you want.

Leaving as-is I think, to preserve explicit control & documentation on when
callbacks are invoked.

[Johnny, 2014-05-06 16:54:32]

The CQ bit was checked by jgraettinger@chromium.org

[commit-bot: I haz the power, 2014-05-06 16:54:47]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/jgraettinger@chromium.org/265933007/20001

[commit-bot: I haz the power, 2014-05-07 07:32:00]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 13:42:36]

Change committed as 268730