Change priority of enqueued writes when SpdyStream priority changes. |
||
Issue descriptionhttps://crrev.com/c/1045988 implemented changing of SpdyStream priorities. This introduced the following subtle crash: assume that a request is made, then its priority is changed, then it is cancelled, with some very specific timing. When a request is made, HttpNetworkTransaction::Start() called, which calls SpdyStreamRequest::StartRequest(), and then a lot of things are done asynchronously, until finally SpdySession::DoLoop() picks up the queued HEADERS frame and activates the stream. (Activating means moving it from created_streams_ to active_streams_, and changing its stream ID from 0 to the next available odd integer.) If the priority is changed and then the stream is destroyed, in this order, after the stream is created but before it is activated, then CHECK(stream.get()) in SpdySession::DoWrite() on line 2169 fails. This crash is recreated in https://crrev.com/c/1067604. The problem is that when the priority of a stream changes, the already enqueued writes are not moved to a different priority queue. So when a stream is destroyed, SpdySession::DeleteStream() is called, which calls SpdyWriteQueue::RemovePendingWritesForStream(), but that only removes enqueued writes in the new priority bucket, whereas the HEADERS frame is enqueued in the queue corresponding to the old priority. Issue 841511 deals with the crashes themselves, and as usual for crasher bugs, it is restricted. https://crrev.com/c/1045988 is (partially) reverted at https://crrev.com/c/1070655, causing the crash to go away, and that issue can be closed. This issue is dedicated to the underlying cause: in order to fix it, SpdyWriteQueue must support changing of priority of queued frames, and that new method should be called when SpdyStream's priority changes.
,
May 25 2018
Re comment #1: that was the case before changing priorities got reverted at https://crrev.com/c/1070655/2/net/spdy/spdy_http_stream.cc, and it was bad. Bad enough to cause crashes. For any given stream, frames of that stream have to be written in the order they were enqueued, which is exactly what you were asking about. If a stream is allowed to change priority when it already has enqueued frames, then it is necessary to change the priority of those already enqueued frames. I'll need to introduce a new method on SpdyWriteQueue for this.
,
May 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9a2698cba08cf5a471a29d30c8b3e12becabb0e9 commit 9a2698cba08cf5a471a29d30c8b3e12becabb0e9 Author: Bence Béky <bnc@chromium.org> Date: Wed May 30 03:20:01 2018 Implement SpdyWriteQueue::ChangePriorityOfWritesForStream(). Bug: 846541 Change-Id: Ied45ea2c0b9a380488f0c47d81231e1e248a5d8c Reviewed-on: https://chromium-review.googlesource.com/1072348 Reviewed-by: Ryan Hamilton <rch@chromium.org> Commit-Queue: Bence Béky <bnc@chromium.org> Cr-Commit-Position: refs/heads/master@{#562718} [modify] https://crrev.com/9a2698cba08cf5a471a29d30c8b3e12becabb0e9/net/spdy/spdy_write_queue.cc [modify] https://crrev.com/9a2698cba08cf5a471a29d30c8b3e12becabb0e9/net/spdy/spdy_write_queue.h [modify] https://crrev.com/9a2698cba08cf5a471a29d30c8b3e12becabb0e9/net/spdy/spdy_write_queue_unittest.cc
,
May 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/79f37043bd983f0a695c8a03efba55a019a3bd0c commit 79f37043bd983f0a695c8a03efba55a019a3bd0c Author: Bence Béky <bnc@chromium.org> Date: Wed May 30 20:23:04 2018 Simplify removal logic in SpdyWriteQueue. Bug: 846541 Change-Id: I42b790888dfc0aabec1402242a5c5b9f624a5d05 Reviewed-on: https://chromium-review.googlesource.com/1078910 Reviewed-by: Ryan Hamilton <rch@chromium.org> Commit-Queue: Bence Béky <bnc@chromium.org> Cr-Commit-Position: refs/heads/master@{#562974} [modify] https://crrev.com/79f37043bd983f0a695c8a03efba55a019a3bd0c/net/spdy/spdy_write_queue.cc
,
Jun 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f30fbd08de27d8bb7ffcf935cfed44f188e1b901 commit f30fbd08de27d8bb7ffcf935cfed44f188e1b901 Author: Bence Béky <bnc@chromium.org> Date: Tue Jun 12 17:33:11 2018 Take raw instead of weak pointer in SpdyWriteQueue. Two SpdyWriteQueue methods, RemovePendingWritesForStream() and ChangePriorityOfWritesForStream(), currently take WeakPtr<SpdyStream>, but only use the pointer value synchronously (they do not store it). Moreover, call sites only have raw or unique pointers at hand, so it takes extra effort to unnecessarily grab a weak ptr. Bug: 846541 Change-Id: Ib06914aa367442ff69a814f0186c8c0a8b2eb9c4 Reviewed-on: https://chromium-review.googlesource.com/1097202 Commit-Queue: Bence Béky <bnc@chromium.org> Reviewed-by: Ryan Hamilton <rch@chromium.org> Cr-Commit-Position: refs/heads/master@{#566484} [modify] https://crrev.com/f30fbd08de27d8bb7ffcf935cfed44f188e1b901/net/spdy/spdy_session.cc [modify] https://crrev.com/f30fbd08de27d8bb7ffcf935cfed44f188e1b901/net/spdy/spdy_write_queue.cc [modify] https://crrev.com/f30fbd08de27d8bb7ffcf935cfed44f188e1b901/net/spdy/spdy_write_queue.h [modify] https://crrev.com/f30fbd08de27d8bb7ffcf935cfed44f188e1b901/net/spdy/spdy_write_queue_unittest.cc
,
Jun 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/73b85293d8d32de39bca2352634406a856a3aa43 commit 73b85293d8d32de39bca2352634406a856a3aa43 Author: Bence Béky <bnc@chromium.org> Date: Thu Jun 14 04:56:43 2018 Implement changing priority of HTTP/2 streams. Call SpdyWriteQueue::ChangePriorityOfWritesForStream() when the priority of an HTTP or WebSocket request backed by an HTTP/2 stream changes to ensure that already enqueued writes for the stream are moved to the correct write queue. This applies to both active and not-yet-activated streams. Also send HTTP/2 PRIORITY frames if the stream is already active. Add unittest for the case of two requests, the second one with higher priority (but no priority change), to test that the higher priority one will be sent to the wire first. Add unittest for priority change of inactive streams (ones with no assigned stream ID yet) to test that they are activated in priority order. Add unittest for priority change of active streams to test that PRIORITY frames are sent. Bug: 166689, 500673, 846541 Change-Id: I1f598f8d9179facf81a5cd4ee8bffda4ec98b3b3 Reviewed-on: https://chromium-review.googlesource.com/1098628 Commit-Queue: Bence Béky <bnc@chromium.org> Reviewed-by: Ryan Hamilton <rch@chromium.org> Cr-Commit-Position: refs/heads/master@{#567142} [modify] https://crrev.com/73b85293d8d32de39bca2352634406a856a3aa43/net/spdy/spdy_http_stream.cc [modify] https://crrev.com/73b85293d8d32de39bca2352634406a856a3aa43/net/spdy/spdy_network_transaction_unittest.cc [modify] https://crrev.com/73b85293d8d32de39bca2352634406a856a3aa43/net/spdy/spdy_session.cc [modify] https://crrev.com/73b85293d8d32de39bca2352634406a856a3aa43/net/spdy/spdy_session.h [modify] https://crrev.com/73b85293d8d32de39bca2352634406a856a3aa43/net/spdy/spdy_stream.cc [modify] https://crrev.com/73b85293d8d32de39bca2352634406a856a3aa43/net/spdy/spdy_stream.h
,
Jun 14 2018
|
||
►
Sign in to add a comment |
||
Comment 1 by mmenke@chromium.org
, May 25 2018