Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 38 users
Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Feature

Blocked on:
issue 328167
issue 686631
issue 699821



Sign in to add a comment
Cancel unnecessary push streams when it's discovered they're in cache
Project Member Reported by willchan@chromium.org, Apr 16 2013 Back to list
When the server tries to push a resource that's already in Chromium's cache, we should RST_STREAM the stream. I need to doublecheck the appropriate status code for this.

This is a bit tricky due to the separation between the caching and SPDY layers. Probably need to add delegates :P
 
Comment 1 by hkhalil@google.com, Apr 16 2013
The correct RST_STREAM status code should be CANCEL.
Comment 2 by akalin@chromium.org, Jul 31 2013
Issue 265562 has been merged into this issue.
Comment 3 by akalin@chromium.org, Jul 31 2013
Duped bug has nice repro steps.
Comment 4 by piatek@chromium.org, Jul 31 2013
Cc: piatek@chromium.org
Cc: sidv@chromium.org
It would be great to bump a UMA when this occurs so we can know if this is a problem.

Is there any way the client can signal a cache cancel to the origin? I imagine sites that use push will make a best-effort attempt to avoid pushing resources in cache, and a signal that something is in the cache would be tremendously useful to that end.

Labels: Hotlist-GoogleApps
Cc: akalin@chromium.org
Labels: M-34
Owner: jgraettinger@chromium.org
willchan@ et al, is it sufficient to cancel just those requests for which the cached entry is definitively known to be valid (within max-age/Expires)? As opposed to entries which would have required conditional requests to the server (If-Modified-Since, etc)?
Comment 9 by fenix@google.com, Dec 12 2013
Yes, for pushes with a "get" method (as opposed to HEAD, which would be
useless to RST).
We should probably have a quick chat about the if-modified-since part,
since one of the ideas of some of the push stuff is to anticipate that the
client would need to verify such and server-push a response to a HEAD
request...
Yeah, what Roberto said.
jgraettinger, any update here?
The consensus so far has been to implement this as part of a larger integration of server-push and the cache (crbug.com/328167). This specific ticket is blocked on that effort.
Blockedon: chromium:328167
Labels: -M-34 M-36
This is still on the roadmap, but is not an immediate priority. Pushing out into a later release. Let me know if this feature issue is better off closed, and then re-created when picked up again.
Labels: -M-36 M-37 MovedFrom-36
Moving all non essential bugs to the next Milestone.
Labels: -M-37 MovedFrom-37
This issue has already been moved once and is lower than Priority 1,therefore removing mstone.
Owner: b...@chromium.org
Transferring ownership.
Comment 18 by b...@chromium.org, Sep 16 2014
Cc: vivianz@chromium.org anan...@chromium.org
Issue 113426 has been merged into this issue.
Labels: -Cr-Internals-Network-SPDY Cr-Internals-Network-HTTP2
Migrate from Cr-Internals-Network-SPDY to Cr-Internals-Network-HTTP2
Cc: igrigo...@chromium.org
Cc: gavinp@chromium.org
Components: Internals>Network>Cache
Comment 22 by n...@fb.com, Apr 8 2016
Labels: DevRel-Facebook
Cc: tombergan@chromium.org
Cc: spelc...@chromium.org
Comment 25 by b...@chromium.org, Jun 24 2016
Cc: b...@chromium.org
Owner: ----
Status: Available
Cc: zhongyi@chromium.org
zhongyi@, you're working on this right?
Cc: rch@chromium.org ckrasic@chromium.org
Issue 635102 has been merged into this issue.
Owner: zhongyi@chromium.org
I will be actively working on this, the design doc is available: http://goo.gl/OgWqwO
Components: Internals>Network>QUIC
Status: Assigned
Comment 31 by y...@yoav.ws, Sep 8 2016
Cc: y...@yoav.ws
CANCEL is probably the best error code currently, but note that the IETF is considering adding a new error code for this case:
https://lists.w3.org/Archives/Public/ietf-http-wg/2016JulSep/0524.html
What will we do if there's a fresh entry in the cache but the pushed item is fresher? I.e. later Last-Modified, different ETag.
Project Member Comment 34 by bugdroid1@chromium.org, Sep 23 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/96d0202cee58e8165e100e996b7936ec538a11d0

commit 96d0202cee58e8165e100e996b7936ec538a11d0
Author: zhongyi <zhongyi@chromium.org>
Date: Fri Sep 23 04:10:52 2016

Add methods in spdy/quic session to get stream id of pushed stream given the request url

BUG=232040

Review-Url: https://codereview.chromium.org/2351373003
Cr-Commit-Position: refs/heads/master@{#420574}

[modify] https://crrev.com/96d0202cee58e8165e100e996b7936ec538a11d0/net/quic/chromium/quic_chromium_client_session.cc
[modify] https://crrev.com/96d0202cee58e8165e100e996b7936ec538a11d0/net/quic/chromium/quic_chromium_client_session.h
[modify] https://crrev.com/96d0202cee58e8165e100e996b7936ec538a11d0/net/spdy/spdy_session.cc
[modify] https://crrev.com/96d0202cee58e8165e100e996b7936ec538a11d0/net/spdy/spdy_session.h

Project Member Comment 35 by bugdroid1@chromium.org, Oct 12 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e3e4bdd8b41ecb5a6ed5564cafe40bebbcbe1d47

commit e3e4bdd8b41ecb5a6ed5564cafe40bebbcbe1d47
Author: zhongyi <zhongyi@chromium.org>
Date: Wed Oct 12 22:35:07 2016

relnote: Add a new RstStreamErrorCode for resetting streams due to push stream is unclaimed and times out. Flag protected by quic_send_push_stream_timed_out_error, default on.

Merge internal change: 135855149

BUG=232040

Review-Url: https://codereview.chromium.org/2412483004
Cr-Commit-Position: refs/heads/master@{#424886}

[modify] https://crrev.com/e3e4bdd8b41ecb5a6ed5564cafe40bebbcbe1d47/net/quic/core/quic_client_promised_info.cc
[modify] https://crrev.com/e3e4bdd8b41ecb5a6ed5564cafe40bebbcbe1d47/net/quic/core/quic_client_promised_info_test.cc
[modify] https://crrev.com/e3e4bdd8b41ecb5a6ed5564cafe40bebbcbe1d47/net/quic/core/quic_flags_list.h
[modify] https://crrev.com/e3e4bdd8b41ecb5a6ed5564cafe40bebbcbe1d47/net/quic/core/quic_protocol.h
[modify] https://crrev.com/e3e4bdd8b41ecb5a6ed5564cafe40bebbcbe1d47/net/quic/core/quic_protocol_test.cc
[modify] https://crrev.com/e3e4bdd8b41ecb5a6ed5564cafe40bebbcbe1d47/net/quic/core/quic_utils.cc

Project Member Comment 36 by bugdroid1@chromium.org, Oct 14 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/92bd5b4ddf200bd34f55e77b1d835203627c8dc6

commit 92bd5b4ddf200bd34f55e77b1d835203627c8dc6
Author: zhongyi <zhongyi@chromium.org>
Date: Fri Oct 14 21:18:41 2016

Server push cancellation: add metrics to collect
* how many bytes has been pushed,
* how many bytes has been pushed and unclaimed

BUG=232040

Review-Url: https://codereview.chromium.org/2392043002
Cr-Commit-Position: refs/heads/master@{#425465}

[modify] https://crrev.com/92bd5b4ddf200bd34f55e77b1d835203627c8dc6/net/quic/chromium/quic_chromium_client_session.cc
[modify] https://crrev.com/92bd5b4ddf200bd34f55e77b1d835203627c8dc6/net/quic/chromium/quic_chromium_client_session.h
[modify] https://crrev.com/92bd5b4ddf200bd34f55e77b1d835203627c8dc6/net/quic/core/quic_client_promised_info.cc
[modify] https://crrev.com/92bd5b4ddf200bd34f55e77b1d835203627c8dc6/net/quic/core/quic_client_session_base.cc
[modify] https://crrev.com/92bd5b4ddf200bd34f55e77b1d835203627c8dc6/net/quic/core/quic_client_session_base.h
[modify] https://crrev.com/92bd5b4ddf200bd34f55e77b1d835203627c8dc6/tools/metrics/histograms/histograms.xml

Project Member Comment 37 by bugdroid1@chromium.org, Oct 15 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d62d3e8f2f2c3a896e36655f2f2d51936f03deb4

commit d62d3e8f2f2c3a896e36655f2f2d51936f03deb4
Author: zhongyi <zhongyi@chromium.org>
Date: Sat Oct 15 01:12:11 2016

Fix metrics collection when push streams are cancelled by SendRstStreams.
When push stream is cancelled with sending reset streams,
QuicSession::CloseStreamInner is invoked before QuicChromiumClientSession::CloseStream()
which ignores the metrics collection for bytes that has been pushed.

BUG=232040

Review-Url: https://codereview.chromium.org/2423633002
Cr-Commit-Position: refs/heads/master@{#425526}

[modify] https://crrev.com/d62d3e8f2f2c3a896e36655f2f2d51936f03deb4/net/quic/chromium/quic_chromium_client_session.cc

Project Member Comment 38 by bugdroid1@chromium.org, Oct 20 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b10f8ebcdc8deb79d736d6994bd92a6d5a6590ca

commit b10f8ebcdc8deb79d736d6994bd92a6d5a6590ca
Author: zhongyi <zhongyi@chromium.org>
Date: Thu Oct 20 18:54:11 2016

Server push cancellation: add metrics collection for
1. collecting number of stream bytes that has been pushed by the server
2. collecting number of stream bytes that has been pushed by the server but never claimed until expired.

BUG=232040

Review-Url: https://chromiumcodereview.appspot.com/2429313003
Cr-Commit-Position: refs/heads/master@{#426547}

[modify] https://crrev.com/b10f8ebcdc8deb79d736d6994bd92a6d5a6590ca/net/spdy/spdy_session.cc
[modify] https://crrev.com/b10f8ebcdc8deb79d736d6994bd92a6d5a6590ca/net/spdy/spdy_session.h
[modify] https://crrev.com/b10f8ebcdc8deb79d736d6994bd92a6d5a6590ca/net/spdy/spdy_session_unittest.cc
[modify] https://crrev.com/b10f8ebcdc8deb79d736d6994bd92a6d5a6590ca/net/spdy/spdy_stream.h
[modify] https://crrev.com/b10f8ebcdc8deb79d736d6994bd92a6d5a6590ca/tools/metrics/histograms/histograms.xml

Project Member Comment 39 by bugdroid1@chromium.org, Oct 27 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/18bb2d951716466214da9a99a85ad48c7aa4cfa8

commit 18bb2d951716466214da9a99a85ad48c7aa4cfa8
Author: zhongyi <zhongyi@chromium.org>
Date: Thu Oct 27 19:38:50 2016

Server push cancellation: add methods in session layer to cancel push given the pushed url. Not in use.

BUG=232040

Review-Url: https://codereview.chromium.org/2456713002
Cr-Commit-Position: refs/heads/master@{#428114}

[modify] https://crrev.com/18bb2d951716466214da9a99a85ad48c7aa4cfa8/net/quic/chromium/quic_chromium_client_session.cc
[modify] https://crrev.com/18bb2d951716466214da9a99a85ad48c7aa4cfa8/net/quic/chromium/quic_chromium_client_session.h
[modify] https://crrev.com/18bb2d951716466214da9a99a85ad48c7aa4cfa8/net/quic/chromium/quic_chromium_client_session_test.cc
[modify] https://crrev.com/18bb2d951716466214da9a99a85ad48c7aa4cfa8/net/spdy/spdy_session.cc
[modify] https://crrev.com/18bb2d951716466214da9a99a85ad48c7aa4cfa8/net/spdy/spdy_session.h
[modify] https://crrev.com/18bb2d951716466214da9a99a85ad48c7aa4cfa8/net/spdy/spdy_session_unittest.cc

Project Member Comment 40 by bugdroid1@chromium.org, Nov 11 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7465c1b7e3a6d055488065d78c2348c21250e1a5

commit 7465c1b7e3a6d055488065d78c2348c21250e1a5
Author: zhongyi <zhongyi@chromium.org>
Date: Fri Nov 11 01:38:05 2016

Server push cancellation: add PushPromiseHelper which reflects information on the push promise. Implement in both SPDY and QUIC. Also add PushDelegate which consumes the PushPromiseHelper.

BUG=232040

Review-Url: https://codereview.chromium.org/2458793002
Cr-Commit-Position: refs/heads/master@{#431446}

[modify] https://crrev.com/7465c1b7e3a6d055488065d78c2348c21250e1a5/net/net.gypi
[modify] https://crrev.com/7465c1b7e3a6d055488065d78c2348c21250e1a5/net/quic/chromium/quic_chromium_client_session.cc
[modify] https://crrev.com/7465c1b7e3a6d055488065d78c2348c21250e1a5/net/quic/chromium/quic_chromium_client_session.h
[modify] https://crrev.com/7465c1b7e3a6d055488065d78c2348c21250e1a5/net/quic/chromium/quic_chromium_client_session_test.cc
[modify] https://crrev.com/7465c1b7e3a6d055488065d78c2348c21250e1a5/net/quic/core/quic_client_promised_info.cc
[modify] https://crrev.com/7465c1b7e3a6d055488065d78c2348c21250e1a5/net/quic/core/quic_client_promised_info.h
[modify] https://crrev.com/7465c1b7e3a6d055488065d78c2348c21250e1a5/net/quic/core/quic_client_promised_info_test.cc
[modify] https://crrev.com/7465c1b7e3a6d055488065d78c2348c21250e1a5/net/quic/core/quic_client_session_base.cc
[modify] https://crrev.com/7465c1b7e3a6d055488065d78c2348c21250e1a5/net/quic/core/quic_client_session_base.h
[add] https://crrev.com/7465c1b7e3a6d055488065d78c2348c21250e1a5/net/spdy/server_push_delegate.h
[modify] https://crrev.com/7465c1b7e3a6d055488065d78c2348c21250e1a5/net/spdy/spdy_session.cc
[modify] https://crrev.com/7465c1b7e3a6d055488065d78c2348c21250e1a5/net/spdy/spdy_session.h
[modify] https://crrev.com/7465c1b7e3a6d055488065d78c2348c21250e1a5/net/spdy/spdy_session_unittest.cc
[add] https://crrev.com/7465c1b7e3a6d055488065d78c2348c21250e1a5/net/tools/quic/test_tools/push_promise_delegate.cc
[add] https://crrev.com/7465c1b7e3a6d055488065d78c2348c21250e1a5/net/tools/quic/test_tools/push_promise_delegate.h

Project Member Comment 41 by bugdroid1@chromium.org, Nov 11 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0009f3e541663d5b28251983df3f7ebc1506fbdd

commit 0009f3e541663d5b28251983df3f7ebc1506fbdd
Author: zhongyi <zhongyi@chromium.org>
Date: Fri Nov 11 19:47:50 2016

Server push cancellation: Use TestPushDelegate in QuicChromiumClientSessionTest to cancel push.

BUG=232040

Review-Url: https://codereview.chromium.org/2492993002
Cr-Commit-Position: refs/heads/master@{#431621}

[modify] https://crrev.com/0009f3e541663d5b28251983df3f7ebc1506fbdd/net/quic/chromium/quic_chromium_client_session.h
[modify] https://crrev.com/0009f3e541663d5b28251983df3f7ebc1506fbdd/net/quic/chromium/quic_chromium_client_session_test.cc
[modify] https://crrev.com/0009f3e541663d5b28251983df3f7ebc1506fbdd/net/spdy/spdy_session_unittest.cc
[modify] https://crrev.com/0009f3e541663d5b28251983df3f7ebc1506fbdd/net/spdy/spdy_test_utils.cc
[modify] https://crrev.com/0009f3e541663d5b28251983df3f7ebc1506fbdd/net/spdy/spdy_test_utils.h

Project Member Comment 42 by bugdroid1@chromium.org, Nov 12 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1cbc95beeba6502738dbe017f48c31316a2aa4ce

commit 1cbc95beeba6502738dbe017f48c31316a2aa4ce
Author: zhongyi <zhongyi@chromium.org>
Date: Sat Nov 12 01:42:49 2016

Server push cancellation: clean up the code to make sure shared code is in sync with internal code.

Merge internal change: 138927579.

BUG=232040

Review-Url: https://codereview.chromium.org/2497083002
Cr-Commit-Position: refs/heads/master@{#431731}

[modify] https://crrev.com/1cbc95beeba6502738dbe017f48c31316a2aa4ce/net/net.gypi
[modify] https://crrev.com/1cbc95beeba6502738dbe017f48c31316a2aa4ce/net/quic/chromium/quic_chromium_client_session_test.cc
[modify] https://crrev.com/1cbc95beeba6502738dbe017f48c31316a2aa4ce/net/quic/core/quic_client_promised_info.cc
[modify] https://crrev.com/1cbc95beeba6502738dbe017f48c31316a2aa4ce/net/quic/core/quic_client_promised_info_test.cc
[modify] https://crrev.com/1cbc95beeba6502738dbe017f48c31316a2aa4ce/net/quic/core/quic_client_session_base.h
[modify] https://crrev.com/1cbc95beeba6502738dbe017f48c31316a2aa4ce/net/quic/test_tools/quic_test_utils.cc
[modify] https://crrev.com/1cbc95beeba6502738dbe017f48c31316a2aa4ce/net/quic/test_tools/quic_test_utils.h
[delete] https://crrev.com/962594ce00cece346e48f76f45027d596ea8462c/net/tools/quic/test_tools/push_promise_delegate.cc
[delete] https://crrev.com/962594ce00cece346e48f76f45027d596ea8462c/net/tools/quic/test_tools/push_promise_delegate.h

Project Member Comment 43 by bugdroid1@chromium.org, Nov 22 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8f72aeb11c5f8c8824a335fb72aadfa5b03df674

commit 8f72aeb11c5f8c8824a335fb72aadfa5b03df674
Author: zhongyi <zhongyi@chromium.org>
Date: Tue Nov 22 21:21:40 2016

Server push cancellation: add a new class HttpCacheLookupManager which implements ServerPushDelegate
and can create cache transaction to lookup whether the server push has a cached response.

BUG=232040

Review-Url: https://codereview.chromium.org/2503473004
Cr-Commit-Position: refs/heads/master@{#433983}

[add] https://crrev.com/8f72aeb11c5f8c8824a335fb72aadfa5b03df674/net/http/http_cache_lookup_manager.cc
[add] https://crrev.com/8f72aeb11c5f8c8824a335fb72aadfa5b03df674/net/http/http_cache_lookup_manager.h
[add] https://crrev.com/8f72aeb11c5f8c8824a335fb72aadfa5b03df674/net/http/http_cache_lookup_manager_unittest.cc
[modify] https://crrev.com/8f72aeb11c5f8c8824a335fb72aadfa5b03df674/net/net.gypi

Project Member Comment 44 by bugdroid1@chromium.org, Dec 19 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/af25754e6d51d71bd346ac56e36e40ca1ec3803d

commit af25754e6d51d71bd346ac56e36e40ca1ec3803d
Author: zhongyi <zhongyi@chromium.org>
Date: Mon Dec 19 03:36:01 2016

Server push cancellation: add a setter method for server push delegate in HttpNetworkSession. Plumb the server push delegate to QuicStreamFactory/SpdySessionPool so that QuicChromiumClientSession/SpdySession takes server push delegate in contructor.

BUG=232040

Review-Url: https://codereview.chromium.org/2521573006
Cr-Commit-Position: refs/heads/master@{#439397}

[modify] https://crrev.com/af25754e6d51d71bd346ac56e36e40ca1ec3803d/net/http/http_network_session.cc
[modify] https://crrev.com/af25754e6d51d71bd346ac56e36e40ca1ec3803d/net/http/http_network_session.h
[modify] https://crrev.com/af25754e6d51d71bd346ac56e36e40ca1ec3803d/net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc
[modify] https://crrev.com/af25754e6d51d71bd346ac56e36e40ca1ec3803d/net/quic/chromium/quic_chromium_client_session.cc
[modify] https://crrev.com/af25754e6d51d71bd346ac56e36e40ca1ec3803d/net/quic/chromium/quic_chromium_client_session.h
[modify] https://crrev.com/af25754e6d51d71bd346ac56e36e40ca1ec3803d/net/quic/chromium/quic_chromium_client_session_test.cc
[modify] https://crrev.com/af25754e6d51d71bd346ac56e36e40ca1ec3803d/net/quic/chromium/quic_http_stream_test.cc
[modify] https://crrev.com/af25754e6d51d71bd346ac56e36e40ca1ec3803d/net/quic/chromium/quic_stream_factory.cc
[modify] https://crrev.com/af25754e6d51d71bd346ac56e36e40ca1ec3803d/net/quic/chromium/quic_stream_factory.h
[modify] https://crrev.com/af25754e6d51d71bd346ac56e36e40ca1ec3803d/net/spdy/spdy_session.cc
[modify] https://crrev.com/af25754e6d51d71bd346ac56e36e40ca1ec3803d/net/spdy/spdy_session.h
[modify] https://crrev.com/af25754e6d51d71bd346ac56e36e40ca1ec3803d/net/spdy/spdy_session_pool.cc
[modify] https://crrev.com/af25754e6d51d71bd346ac56e36e40ca1ec3803d/net/spdy/spdy_session_pool.h
[modify] https://crrev.com/af25754e6d51d71bd346ac56e36e40ca1ec3803d/net/spdy/spdy_session_unittest.cc

Blocking: 686631
Blockedon: 686631
Blocking: -686631
Project Member Comment 47 by bugdroid1@chromium.org, Feb 11 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d2a4caef5b812732dce7baa2d56716f8ae1f8cd2

commit d2a4caef5b812732dce7baa2d56716f8ae1f8cd2
Author: zhongyi <zhongyi@chromium.org>
Date: Sat Feb 11 10:54:38 2017

Server push cancellation: add NetLogs to track cache lookup transaction
for server push.

BUG=232040

Review-Url: https://codereview.chromium.org/2675343002
Cr-Commit-Position: refs/heads/master@{#449854}

[modify] https://crrev.com/d2a4caef5b812732dce7baa2d56716f8ae1f8cd2/net/http/http_cache_lookup_manager.cc
[modify] https://crrev.com/d2a4caef5b812732dce7baa2d56716f8ae1f8cd2/net/http/http_cache_lookup_manager.h
[modify] https://crrev.com/d2a4caef5b812732dce7baa2d56716f8ae1f8cd2/net/http/http_cache_lookup_manager_unittest.cc
[modify] https://crrev.com/d2a4caef5b812732dce7baa2d56716f8ae1f8cd2/net/log/net_log_event_type_list.h
[modify] https://crrev.com/d2a4caef5b812732dce7baa2d56716f8ae1f8cd2/net/log/net_log_source_type_list.h
[modify] https://crrev.com/d2a4caef5b812732dce7baa2d56716f8ae1f8cd2/net/quic/chromium/quic_chromium_client_session.cc
[modify] https://crrev.com/d2a4caef5b812732dce7baa2d56716f8ae1f8cd2/net/spdy/server_push_delegate.h
[modify] https://crrev.com/d2a4caef5b812732dce7baa2d56716f8ae1f8cd2/net/spdy/spdy_session.cc
[modify] https://crrev.com/d2a4caef5b812732dce7baa2d56716f8ae1f8cd2/net/spdy/spdy_test_utils.cc
[modify] https://crrev.com/d2a4caef5b812732dce7baa2d56716f8ae1f8cd2/net/spdy/spdy_test_utils.h

Project Member Comment 48 by bugdroid1@chromium.org, Feb 14 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c767e3a0419746e45d73fa891ee9291a3a216a8e

commit c767e3a0419746e45d73fa891ee9291a3a216a8e
Author: zhongyi <zhongyi@chromium.org>
Date: Tue Feb 14 01:13:47 2017

Rename www.example.com -> test.example.com for push resource
so that it could be used for integration test. We have test certs valid
for test.example.com only. This clean up will help set up for server
push cancellation integration tests which need both setting up for certs
nd pushed resource.

Merge internal change: 147400096

BUG=232040

Review-Url: https://codereview.chromium.org/2692943003
Cr-Commit-Position: refs/heads/master@{#450182}

[rename] https://crrev.com/c767e3a0419746e45d73fa891ee9291a3a216a8e/net/data/quic_http_response_cache_data/test.example.com/index.html
[rename] https://crrev.com/c767e3a0419746e45d73fa891ee9291a3a216a8e/net/data/quic_http_response_cache_data/test.example.com/map.html
[rename] https://crrev.com/c767e3a0419746e45d73fa891ee9291a3a216a8e/net/data/quic_http_response_cache_data_with_push/test.example.com/favicon.ico
[rename] https://crrev.com/c767e3a0419746e45d73fa891ee9291a3a216a8e/net/data/quic_http_response_cache_data_with_push/test.example.com/index.html
[rename] https://crrev.com/c767e3a0419746e45d73fa891ee9291a3a216a8e/net/data/quic_http_response_cache_data_with_push/test.example.com/index2.html
[rename] https://crrev.com/c767e3a0419746e45d73fa891ee9291a3a216a8e/net/data/quic_http_response_cache_data_with_push/test.example.com/kitten-1.jpg
[modify] https://crrev.com/c767e3a0419746e45d73fa891ee9291a3a216a8e/net/tools/quic/quic_http_response_cache_test.cc

Project Member Comment 49 by bugdroid1@chromium.org, Feb 14 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6f20e71dec40fa752fb16ade304cda16d537bbdd

commit 6f20e71dec40fa752fb16ade304cda16d537bbdd
Author: zhongyi <zhongyi@chromium.org>
Date: Tue Feb 14 23:20:39 2017

Server push cancellation: fix host resolver set up in URLRequestQuicTest
so that customized host resolver rules can be apply when QuicStreamFactory
resolves host. Also change to force quic on test.example.com:443 so that
https requests will always use QUIC, no TCP racing.

BUG=232040

Review-Url: https://codereview.chromium.org/2693083002
Cr-Commit-Position: refs/heads/master@{#450508}

[modify] https://crrev.com/6f20e71dec40fa752fb16ade304cda16d537bbdd/net/url_request/url_request_quic_unittest.cc

Comment 50 by b...@chromium.org, Feb 21 2017
Cc: vitaliyl@chromium.org
Project Member Comment 51 by bugdroid1@chromium.org, Mar 3
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8

commit c83d602cac3c9a7a7ef554d172b7a8cd805f13e8
Author: zhongyi <zhongyi@chromium.org>
Date: Fri Mar 03 01:38:35 2017

Server push cancellation: add a finch trial parameter
"enable_server_push_cancellation" to control the feature of server push
cancellation. Create a HttpCacheLookupManager in HttpCache's constructor
and set it to HttpNetworkSession which will set it thru reaching
QuicChromiumClientSession and SpdySession.

BUG=232040

Review-Url: https://codereview.chromium.org/2692813002
Cr-Commit-Position: refs/heads/master@{#454457}

[modify] https://crrev.com/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8/components/network_session_configurator/network_session_configurator.cc
[modify] https://crrev.com/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8/components/network_session_configurator/network_session_configurator_unittest.cc
[modify] https://crrev.com/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8/net/BUILD.gn
[modify] https://crrev.com/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8/net/http/http_cache.cc
[modify] https://crrev.com/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8/net/http/http_network_session.cc
[modify] https://crrev.com/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8/net/http/http_network_session.h
[modify] https://crrev.com/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8/net/quic/core/quic_server_session_base_test.cc
[modify] https://crrev.com/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8/net/quic/core/quic_session.cc
[modify] https://crrev.com/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8/net/quic/core/quic_session.h
[modify] https://crrev.com/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8/net/spdy/spdy_session_unittest.cc
[modify] https://crrev.com/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8/net/spdy/spdy_test_util_common.cc
[modify] https://crrev.com/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8/net/spdy/spdy_test_util_common.h
[modify] https://crrev.com/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8/net/tools/quic/quic_dispatcher.cc
[modify] https://crrev.com/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8/net/tools/quic/quic_dispatcher.h
[modify] https://crrev.com/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8/net/tools/quic/quic_simple_dispatcher.cc
[modify] https://crrev.com/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8/net/tools/quic/quic_simple_dispatcher.h
[modify] https://crrev.com/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8/net/tools/quic/quic_simple_server_session_test.cc
[modify] https://crrev.com/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8/net/tools/quic/test_tools/mock_quic_session_visitor.h
[modify] https://crrev.com/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8/net/url_request/url_request_quic_unittest.cc
[modify] https://crrev.com/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8/net/url_request/url_request_test_util.cc

Project Member Comment 52 by bugdroid1@chromium.org, Mar 3
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cfb060c669c937702a4ede2d59ae71ae48aafdc3

commit cfb060c669c937702a4ede2d59ae71ae48aafdc3
Author: alph <alph@chromium.org>
Date: Fri Mar 03 10:04:55 2017

Revert of Server push cancellation: add a finch trial parameter (patchset #14 id:280001 of https://codereview.chromium.org/2692813002/ )

Reason for revert:
Broke MSAN https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty%20MSAN/builds/698

Is enable_server_push_cancellation initialized?

Original issue's description:
> Server push cancellation: add a finch trial parameter
> "enable_server_push_cancellation" to control the feature of server push
> cancellation. Create a HttpCacheLookupManager in HttpCache's constructor
> and set it to HttpNetworkSession which will set it thru reaching
> QuicChromiumClientSession and SpdySession.
>
> BUG=232040
>
> Review-Url: https://codereview.chromium.org/2692813002
> Cr-Commit-Position: refs/heads/master@{#454457}
> Committed: https://chromium.googlesource.com/chromium/src/+/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8

TBR=rch@chromium.org,ckrasic@chromium.org,zhongyi@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=232040

Review-Url: https://codereview.chromium.org/2730053002
Cr-Commit-Position: refs/heads/master@{#454554}

[modify] https://crrev.com/cfb060c669c937702a4ede2d59ae71ae48aafdc3/components/network_session_configurator/network_session_configurator.cc
[modify] https://crrev.com/cfb060c669c937702a4ede2d59ae71ae48aafdc3/components/network_session_configurator/network_session_configurator_unittest.cc
[modify] https://crrev.com/cfb060c669c937702a4ede2d59ae71ae48aafdc3/net/BUILD.gn
[modify] https://crrev.com/cfb060c669c937702a4ede2d59ae71ae48aafdc3/net/http/http_cache.cc
[modify] https://crrev.com/cfb060c669c937702a4ede2d59ae71ae48aafdc3/net/http/http_network_session.cc
[modify] https://crrev.com/cfb060c669c937702a4ede2d59ae71ae48aafdc3/net/http/http_network_session.h
[modify] https://crrev.com/cfb060c669c937702a4ede2d59ae71ae48aafdc3/net/quic/core/quic_server_session_base_test.cc
[modify] https://crrev.com/cfb060c669c937702a4ede2d59ae71ae48aafdc3/net/quic/core/quic_session.cc
[modify] https://crrev.com/cfb060c669c937702a4ede2d59ae71ae48aafdc3/net/quic/core/quic_session.h
[modify] https://crrev.com/cfb060c669c937702a4ede2d59ae71ae48aafdc3/net/spdy/spdy_session_unittest.cc
[modify] https://crrev.com/cfb060c669c937702a4ede2d59ae71ae48aafdc3/net/spdy/spdy_test_util_common.cc
[modify] https://crrev.com/cfb060c669c937702a4ede2d59ae71ae48aafdc3/net/spdy/spdy_test_util_common.h
[modify] https://crrev.com/cfb060c669c937702a4ede2d59ae71ae48aafdc3/net/tools/quic/quic_dispatcher.cc
[modify] https://crrev.com/cfb060c669c937702a4ede2d59ae71ae48aafdc3/net/tools/quic/quic_dispatcher.h
[modify] https://crrev.com/cfb060c669c937702a4ede2d59ae71ae48aafdc3/net/tools/quic/quic_simple_dispatcher.cc
[modify] https://crrev.com/cfb060c669c937702a4ede2d59ae71ae48aafdc3/net/tools/quic/quic_simple_dispatcher.h
[modify] https://crrev.com/cfb060c669c937702a4ede2d59ae71ae48aafdc3/net/tools/quic/quic_simple_server_session_test.cc
[modify] https://crrev.com/cfb060c669c937702a4ede2d59ae71ae48aafdc3/net/tools/quic/test_tools/mock_quic_session_visitor.h
[modify] https://crrev.com/cfb060c669c937702a4ede2d59ae71ae48aafdc3/net/url_request/url_request_quic_unittest.cc
[modify] https://crrev.com/cfb060c669c937702a4ede2d59ae71ae48aafdc3/net/url_request/url_request_test_util.cc

Project Member Comment 53 by bugdroid1@chromium.org, Mar 3
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f04bd2c84154b59a3d870993051df4be0b13964f

commit f04bd2c84154b59a3d870993051df4be0b13964f
Author: zhongyi <zhongyi@chromium.org>
Date: Fri Mar 03 21:23:43 2017

Server push cancellation: add a finch trial parameter
"enable_server_push_cancellation" to control the feature of server push
cancellation. Create a HttpCacheLookupManager in HttpCache's constructor
and set it to HttpNetworkSession which will set it thru reaching
QuicChromiumClientSession and SpdySession.

BUG=232040

Review-Url: https://codereview.chromium.org/2692813002
Cr-Original-Commit-Position: refs/heads/master@{#454457}
Committed: https://chromium.googlesource.com/chromium/src/+/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8
Review-Url: https://codereview.chromium.org/2692813002
Cr-Commit-Position: refs/heads/master@{#454679}

[modify] https://crrev.com/f04bd2c84154b59a3d870993051df4be0b13964f/components/network_session_configurator/network_session_configurator.cc
[modify] https://crrev.com/f04bd2c84154b59a3d870993051df4be0b13964f/components/network_session_configurator/network_session_configurator_unittest.cc
[modify] https://crrev.com/f04bd2c84154b59a3d870993051df4be0b13964f/net/BUILD.gn
[modify] https://crrev.com/f04bd2c84154b59a3d870993051df4be0b13964f/net/http/http_cache.cc
[modify] https://crrev.com/f04bd2c84154b59a3d870993051df4be0b13964f/net/http/http_network_session.cc
[modify] https://crrev.com/f04bd2c84154b59a3d870993051df4be0b13964f/net/http/http_network_session.h
[modify] https://crrev.com/f04bd2c84154b59a3d870993051df4be0b13964f/net/quic/core/quic_server_session_base_test.cc
[modify] https://crrev.com/f04bd2c84154b59a3d870993051df4be0b13964f/net/quic/core/quic_session.cc
[modify] https://crrev.com/f04bd2c84154b59a3d870993051df4be0b13964f/net/quic/core/quic_session.h
[modify] https://crrev.com/f04bd2c84154b59a3d870993051df4be0b13964f/net/spdy/spdy_session_unittest.cc
[modify] https://crrev.com/f04bd2c84154b59a3d870993051df4be0b13964f/net/spdy/spdy_test_util_common.cc
[modify] https://crrev.com/f04bd2c84154b59a3d870993051df4be0b13964f/net/spdy/spdy_test_util_common.h
[modify] https://crrev.com/f04bd2c84154b59a3d870993051df4be0b13964f/net/tools/quic/quic_dispatcher.cc
[modify] https://crrev.com/f04bd2c84154b59a3d870993051df4be0b13964f/net/tools/quic/quic_dispatcher.h
[modify] https://crrev.com/f04bd2c84154b59a3d870993051df4be0b13964f/net/tools/quic/quic_simple_dispatcher.cc
[modify] https://crrev.com/f04bd2c84154b59a3d870993051df4be0b13964f/net/tools/quic/quic_simple_dispatcher.h
[modify] https://crrev.com/f04bd2c84154b59a3d870993051df4be0b13964f/net/tools/quic/quic_simple_server_session_test.cc
[modify] https://crrev.com/f04bd2c84154b59a3d870993051df4be0b13964f/net/tools/quic/test_tools/mock_quic_session_visitor.h
[modify] https://crrev.com/f04bd2c84154b59a3d870993051df4be0b13964f/net/url_request/url_request_quic_unittest.cc
[modify] https://crrev.com/f04bd2c84154b59a3d870993051df4be0b13964f/net/url_request/url_request_test_util.cc

Project Member Comment 54 by bugdroid1@chromium.org, Mar 3
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/46a595cd9b30990f40400a81e9b99483f533cafa

commit 46a595cd9b30990f40400a81e9b99483f533cafa
Author: sclittle <sclittle@chromium.org>
Date: Fri Mar 03 23:24:01 2017

Revert of Server push cancellation: add a finch trial parameter (patchset #15 id:300001 of https://codereview.chromium.org/2692813002/ )

Reason for revert:
Broke net_unittests on Win7 (dbg) bot:

https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29

https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/57830

Original issue's description:
> Server push cancellation: add a finch trial parameter
> "enable_server_push_cancellation" to control the feature of server push
> cancellation. Create a HttpCacheLookupManager in HttpCache's constructor
> and set it to HttpNetworkSession which will set it thru reaching
> QuicChromiumClientSession and SpdySession.
>
> BUG=232040
>
> Review-Url: https://codereview.chromium.org/2692813002
> Cr-Original-Commit-Position: refs/heads/master@{#454457}
> Committed: https://chromium.googlesource.com/chromium/src/+/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8
> Review-Url: https://codereview.chromium.org/2692813002
> Cr-Commit-Position: refs/heads/master@{#454679}
> Committed: https://chromium.googlesource.com/chromium/src/+/f04bd2c84154b59a3d870993051df4be0b13964f

TBR=rch@chromium.org,ckrasic@chromium.org,zhongyi@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=232040

Review-Url: https://codereview.chromium.org/2728183003
Cr-Commit-Position: refs/heads/master@{#454706}

[modify] https://crrev.com/46a595cd9b30990f40400a81e9b99483f533cafa/components/network_session_configurator/network_session_configurator.cc
[modify] https://crrev.com/46a595cd9b30990f40400a81e9b99483f533cafa/components/network_session_configurator/network_session_configurator_unittest.cc
[modify] https://crrev.com/46a595cd9b30990f40400a81e9b99483f533cafa/net/BUILD.gn
[modify] https://crrev.com/46a595cd9b30990f40400a81e9b99483f533cafa/net/http/http_cache.cc
[modify] https://crrev.com/46a595cd9b30990f40400a81e9b99483f533cafa/net/http/http_network_session.cc
[modify] https://crrev.com/46a595cd9b30990f40400a81e9b99483f533cafa/net/http/http_network_session.h
[modify] https://crrev.com/46a595cd9b30990f40400a81e9b99483f533cafa/net/quic/core/quic_server_session_base_test.cc
[modify] https://crrev.com/46a595cd9b30990f40400a81e9b99483f533cafa/net/quic/core/quic_session.cc
[modify] https://crrev.com/46a595cd9b30990f40400a81e9b99483f533cafa/net/quic/core/quic_session.h
[modify] https://crrev.com/46a595cd9b30990f40400a81e9b99483f533cafa/net/spdy/spdy_session_unittest.cc
[modify] https://crrev.com/46a595cd9b30990f40400a81e9b99483f533cafa/net/spdy/spdy_test_util_common.cc
[modify] https://crrev.com/46a595cd9b30990f40400a81e9b99483f533cafa/net/spdy/spdy_test_util_common.h
[modify] https://crrev.com/46a595cd9b30990f40400a81e9b99483f533cafa/net/tools/quic/quic_dispatcher.cc
[modify] https://crrev.com/46a595cd9b30990f40400a81e9b99483f533cafa/net/tools/quic/quic_dispatcher.h
[modify] https://crrev.com/46a595cd9b30990f40400a81e9b99483f533cafa/net/tools/quic/quic_simple_dispatcher.cc
[modify] https://crrev.com/46a595cd9b30990f40400a81e9b99483f533cafa/net/tools/quic/quic_simple_dispatcher.h
[modify] https://crrev.com/46a595cd9b30990f40400a81e9b99483f533cafa/net/tools/quic/quic_simple_server_session_test.cc
[modify] https://crrev.com/46a595cd9b30990f40400a81e9b99483f533cafa/net/tools/quic/test_tools/mock_quic_session_visitor.h
[modify] https://crrev.com/46a595cd9b30990f40400a81e9b99483f533cafa/net/url_request/url_request_quic_unittest.cc
[modify] https://crrev.com/46a595cd9b30990f40400a81e9b99483f533cafa/net/url_request/url_request_test_util.cc

This CL is having some issues on some build bots(Win7 in particular in this case), which wasn't included in CQ bots, and run until the code has been landed. *I haven't figure out the way to pre-run before landing*.

Thus it's been landed and reverted. I will try to get the failure on build bots fixed and re-land. Sorry about the annoying land-revert in the last 4 comments.  
Blockedon: 699821
Project Member Comment 57 by bugdroid1@chromium.org, Mar 14
Labels: merge-merged-3029
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f260d6b6ba529ac09689e0755e4674632f9bb753

commit f260d6b6ba529ac09689e0755e4674632f9bb753
Author: Zhongyi Shi <zhongyi@chromium.org>
Date: Tue Mar 14 16:34:19 2017

Revert of Server push cancellation: add a finch trial parameter (patchset #14 id:280001 of https://codereview.chromium.org/2692813002/ )

Reason for revert:
Broke MSAN https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty%20MSAN/builds/698

Is enable_server_push_cancellation initialized?

Original issue's description:
> Server push cancellation: add a finch trial parameter
> "enable_server_push_cancellation" to control the feature of server push
> cancellation. Create a HttpCacheLookupManager in HttpCache's constructor
> and set it to HttpNetworkSession which will set it thru reaching
> QuicChromiumClientSession and SpdySession.
>
> BUG=232040
>
> Review-Url: https://codereview.chromium.org/2692813002
> Cr-Commit-Position: refs/heads/master@{#454457}
> Committed: https://chromium.googlesource.com/chromium/src/+/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8

R=rch@chromium.org
TBR=ckrasic@chromium.org, rch@chromium.org, zhongyi@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=232040

Review-Url: https://codereview.chromium.org/2730053002
Cr-Commit-Position: refs/heads/master@{#454554}
(cherry picked from commit cfb060c669c937702a4ede2d59ae71ae48aafdc3)

Review-Url: https://codereview.chromium.org/2751623002 .
Cr-Commit-Position: refs/branch-heads/3029@{#187}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/f260d6b6ba529ac09689e0755e4674632f9bb753/components/network_session_configurator/network_session_configurator.cc
[modify] https://crrev.com/f260d6b6ba529ac09689e0755e4674632f9bb753/components/network_session_configurator/network_session_configurator_unittest.cc
[modify] https://crrev.com/f260d6b6ba529ac09689e0755e4674632f9bb753/net/BUILD.gn
[modify] https://crrev.com/f260d6b6ba529ac09689e0755e4674632f9bb753/net/http/http_cache.cc
[modify] https://crrev.com/f260d6b6ba529ac09689e0755e4674632f9bb753/net/http/http_network_session.cc
[modify] https://crrev.com/f260d6b6ba529ac09689e0755e4674632f9bb753/net/http/http_network_session.h
[modify] https://crrev.com/f260d6b6ba529ac09689e0755e4674632f9bb753/net/quic/core/quic_server_session_base_test.cc
[modify] https://crrev.com/f260d6b6ba529ac09689e0755e4674632f9bb753/net/quic/core/quic_session.cc
[modify] https://crrev.com/f260d6b6ba529ac09689e0755e4674632f9bb753/net/quic/core/quic_session.h
[modify] https://crrev.com/f260d6b6ba529ac09689e0755e4674632f9bb753/net/spdy/spdy_session_unittest.cc
[modify] https://crrev.com/f260d6b6ba529ac09689e0755e4674632f9bb753/net/spdy/spdy_test_util_common.cc
[modify] https://crrev.com/f260d6b6ba529ac09689e0755e4674632f9bb753/net/spdy/spdy_test_util_common.h
[modify] https://crrev.com/f260d6b6ba529ac09689e0755e4674632f9bb753/net/tools/quic/quic_dispatcher.cc
[modify] https://crrev.com/f260d6b6ba529ac09689e0755e4674632f9bb753/net/tools/quic/quic_dispatcher.h
[modify] https://crrev.com/f260d6b6ba529ac09689e0755e4674632f9bb753/net/tools/quic/quic_simple_dispatcher.cc
[modify] https://crrev.com/f260d6b6ba529ac09689e0755e4674632f9bb753/net/tools/quic/quic_simple_dispatcher.h
[modify] https://crrev.com/f260d6b6ba529ac09689e0755e4674632f9bb753/net/tools/quic/quic_simple_server_session_test.cc
[modify] https://crrev.com/f260d6b6ba529ac09689e0755e4674632f9bb753/net/tools/quic/test_tools/mock_quic_session_visitor.h
[modify] https://crrev.com/f260d6b6ba529ac09689e0755e4674632f9bb753/net/url_request/url_request_quic_unittest.cc
[modify] https://crrev.com/f260d6b6ba529ac09689e0755e4674632f9bb753/net/url_request/url_request_test_util.cc

Cc: nbeloglazov@google.com
If a request has a `no-cache` directive (https://tools.ietf.org/html/rfc7234#section-5.2.1.4)

  "a cache MUST NOT use
   a stored response to satisfy the request without successful
   validation on the origin server"

Thus, I think it would be in error to cancel a Push Request with a `no-cache` request cache directive solely because there is already a cached result.
Agreed.

HTTP/2 request Cache-Control directives should be handled consistently by the cache whether the request is initiated from the client, or pushed by the server.
dwitherspoon2011@ - it's not trivial to predict the `no-cache` directive of a future request when taking the decision to RST a PUSH promise.

I believe in such cases it's better to avoid over-downloading in the common scenario (user hasn't forced-reload their page), rather than optimizing for the less-common-one (force-reload).
I agree with Yoav's comment. If the server pushes URL X and the push is canceled because it's already cached, but the browser later makes a request for X with "no-cache", that request will bypass the cache and be forwarded to the server. This is less efficient than it could be, but there is no violation of the HTTP spec.

I think a better way to handle this case is to convert the pushed response to a 304 when the pushed response matches the cached resource. There are two possible implementations:

1. Don't cancel the push until after receiving the pushed response headers (since the response headers contain ETag and/or Last-Modified). If the pushed resource is already cached, the browser would convert the pushed response to a 304 and cancel the push. However, IMO, this waits too long to cancel the push.

2. Add conditional headers like If-Match to the PUSH_PROMISE so the browser knows exactly which resource is being pushed. This is a good idea but it would need to be spec'd out before we could implement it.
https://lists.w3.org/Archives/Public/ietf-http-wg/2016JulSep/0522.html

But actually, I may have misunderstood dwitherspoon2011@'s comment. What did you mean by "a Push Request with a `no-cache` request cache directive"? Did you mean a future request made by the browser (as Yoav and I assumed) or did you mean a PUSH_PROMISE with a `no-cache` directive?
Project Member Comment 63 by bugdroid1@chromium.org, Apr 14
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec

commit d7dd2db1ad3596d682aa7fd56d3fef6f218250ec
Author: zhongyi <zhongyi@chromium.org>
Date: Fri Apr 14 17:01:25 2017

Server push cancellation: add a finch trial parameter
"enable_server_push_cancellation" to control the feature of server push
cancellation. Create a HttpCacheLookupManager in HttpCache's constructor
and set it to HttpNetworkSession which will set it thru reaching
QuicChromiumClientSession and SpdySession.

BUG=232040

Review-Url: https://codereview.chromium.org/2692813002
Cr-Original-Original-Commit-Position: refs/heads/master@{#454457}
Committed: https://chromium.googlesource.com/chromium/src/+/c83d602cac3c9a7a7ef554d172b7a8cd805f13e8
Review-Url: https://codereview.chromium.org/2692813002
Cr-Original-Commit-Position: refs/heads/master@{#454679}
Committed: https://chromium.googlesource.com/chromium/src/+/f04bd2c84154b59a3d870993051df4be0b13964f
Review-Url: https://codereview.chromium.org/2692813002
Cr-Commit-Position: refs/heads/master@{#464737}

[modify] https://crrev.com/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec/components/network_session_configurator/network_session_configurator.cc
[modify] https://crrev.com/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec/components/network_session_configurator/network_session_configurator_unittest.cc
[modify] https://crrev.com/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec/net/BUILD.gn
[modify] https://crrev.com/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec/net/http/http_cache.cc
[modify] https://crrev.com/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec/net/http/http_network_session.cc
[modify] https://crrev.com/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec/net/http/http_network_session.h
[modify] https://crrev.com/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec/net/quic/core/quic_server_session_base_test.cc
[modify] https://crrev.com/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec/net/quic/core/quic_session.cc
[modify] https://crrev.com/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec/net/quic/core/quic_session.h
[modify] https://crrev.com/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec/net/spdy/spdy_session_unittest.cc
[modify] https://crrev.com/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec/net/spdy/spdy_test_util_common.cc
[modify] https://crrev.com/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec/net/spdy/spdy_test_util_common.h
[modify] https://crrev.com/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec/net/tools/quic/quic_dispatcher.cc
[modify] https://crrev.com/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec/net/tools/quic/quic_dispatcher.h
[modify] https://crrev.com/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec/net/tools/quic/quic_simple_dispatcher.cc
[modify] https://crrev.com/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec/net/tools/quic/quic_simple_dispatcher.h
[modify] https://crrev.com/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec/net/tools/quic/quic_simple_server_session_test.cc
[modify] https://crrev.com/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec/net/tools/quic/test_tools/mock_quic_session_visitor.h
[modify] https://crrev.com/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec/net/url_request/url_request_quic_unittest.cc
[modify] https://crrev.com/d7dd2db1ad3596d682aa7fd56d3fef6f218250ec/net/url_request/url_request_test_util.cc

Labels: Merge-Request-59
The CL landed in Comment 63 was the last CL for finch plumbing, unfortunately it missed the M59 branch cut for one day. This feature is targeted in M59, sending merge request to bring the finch plumbing to M59.

We'll start with Canary experimenting soon. 
Project Member Comment 65 by sheriffbot@chromium.org, Apr 18
Labels: -Merge-Request-59 Merge-Review-59 Hotlist-Merge-Review
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
This seems to be a fairly large network layer change post branch. Can you please confirm if this well tested in canary? Is this critical to target this for M59, or can it wait until M60?
Sorry re-reading #64, is change in #63 only affecting ability to turn finch experiment for this feature?
Yeah, the change in #63 is for finch experiment only, and some integration test. We will need the finch to test this feature. And we will start with Canary. The reason I want to merge to M59 is that, if the canary is well tested, we can then move forward to Beta experimenting.
Since this hasn't been tested in Canary yet, applicable to all OS's, and it's a very large change, my recommendation is to wait until M60 so it goes through the proper canary/dev/beta channels. 
Labels: -Merge-Review-59 Merge-Rejected-59
Chatted offline with zhongyi@, since this feature has not been tested at all, marking it as merge-rejected for M59. 
Thanks abdulsyed@ for the updates.

We will be starting experimenting in Canary in M60 the week of 4/23. If everything looks good, experiment will be pushed to Dev the following week. If we believe there's a strong reason to merge this to M59, we could request merge the finch change to M59 and experiment with the feature. By default, this should be in M60.
Sorry for the late reply, didn't get updates via email: :-(

tombergan@
>  PUSH_PROMISE with a `no-cache` directive?

Yes, I meant that a PUSH_PROMISE with a no-cache directive should bypass any cache check.

If the browser has a cached response, then ANY request with a no-cache directive should bypass the cache, by spec.  A PUSH_PROMISE (which is a server-initiated request) with a no-cache directive should bypass the client side cache.  An implementation where a client (browser) cancels a push promise that has a no-cache directive because it already has a cached response isn't spec compliant.

Following the spec consistently (for server-initiated PUSH_PROMISE and client-initiated request processing) is very useful as it allows the server to proactively update the client's cache (cache busting). 

1. Client gets a response for far-future-expires cached resource.
2. At sometime in future, the server sends PUSH_PROMISE with Cache-Control: no-cache header, ie. no-cache directive.
3. Browser DOES NOT cancel push promise because it MUST not use a cached response to cancel a request (PUSH_PROMISE) with no-cache directive.
4. Browser receives the response from the PUSH PROMISE
5. Client initiates request for a resource, gets it from cache, and sees an updated result (profit!!)

> dwitherspoon2011@:
> An implementation where a client (browser) cancels a push promise
> that has a no-cache directive because it already has a cached response
> isn't spec compliant.

Sorry for the delayed response.

That's not right. The client is allowed to cancel any push promise -- clients are never required to use pushed responses. Cancelling a push promise is roughly equivalent to disabling server push, which again, the client is allowed to do. It would be illegal for Chrome to let a *client request* with no-cache use Chrome's local HTTP cache, but that's not what is happening here -- we're just canceling the push promise, which is always allowed. Suboptimal, maybe, but allowed.

If you want a push promise with no-cache to always invalidate the client's local cache, I think you should raise that proposal with the IETF. I don't see any verbiage in the HTTP/2 spec that requires that behavior, and without an RFC, I doubt you will see all browsers implementing what you want. You may also be interested in the following thread, particularly the section "Pushing and Invalidation":
https://lists.w3.org/Archives/Public/ietf-http-wg/2016JulSep/0526.html

(Note this comment thread is getting off topic, unless you still think zhongyi@'s changes are not spec compliant.)
Thank you for the reply,

I agree the client can cancel a push promise for any reason.  I tried to imply as much in my first comment.

But, a push promise is semantically equivalent to a client initiated request (rfc7540#section-8.2.1).  Any request with a no-cache request directive can not be served directly from the cache (rfc7234#section-5.2.1.4).

This PR is titled “Cancel unnecessary push streams when it’s discovered they’re in cache”.  When a Push Promise has a request with no-cache directive then it is not an unnecessary push promise AND by definition, a validated response is not already in the cache.  To implement it in such a manner is breaking semantic equivalence with a client initiated request.  The semantics and meaning of a request no-cache directive are well defined.  I believe the h2 spec is quite clear that the semantics for a client initiated and server-initiated request are the same.

Breaking semantic equivalence between client initiated request and a server-initiated request seems to be a violation of the RFC.  I would hope that no further RFCs would be needed for this case, nor for any other case where it is convenient to ignore RFC specified request semantics on a server-initiated request (Push Promises).

(Off topic to PR)
Thank you (+tombergan@chromium.org) for the pointer to the mailing list, I will direct the list to the relevant sections of the related RFCs.  Agreed, getting implementors to follow RFCs can be hard. Any other pointers to related discussions/mailing lists would be appreciated (feel free to email directly).  In fact, I believe no-cache directives was only recently implemented in all the browsers (Safari might still require onunload??)
IMO we should not implement the no-cache proposal until there is agreement from the IETF that it is a good idea.

I still disagree on the technical point about spec compliance. Clients are allowed to cancel pushes for any reason, therefore it's spec-compliant to cancel a push promise that has no-cache.

I'm also not convinced that no-cache is the right way to signal invalidation. If the client receives a push promise with no-cache, I would interpret that to mean that all servers between the client and the origin (such as a CDN or reverse proxy between the client and the origin) have ensured that the pushed response is fresh from the origin server. This is what no-cache means: the response has been validated at the origin server. It does *not* mean that the client necessarily has an invalid version of the resource. What if the push promise has no-cache, and we take your idea to not cancel the push, but it turns out the client already has the most up-to-date version of the resource? We wasted time -- we could have canceled the push sooner. Better, IMO, to label the push promise with a conditional header describing the resource so the client can determine if the pushed response is more up-to-date than the resource it has cached. Also see this thread, which I forgot to link earlier:
https://lists.w3.org/Archives/Public/ietf-http-wg/2016JulSep/0522.html

I agree with your high-level point that push promises should have more control over the process of invalidating and revalidating the client cache. Where I disagree is that I'm not convinced that no-cache is the right solution and I think we should wait for wider consensus on the right solution before we implement anything.
Potentially silly question: How do I launch Canary with this enabled? Is there a cmd flag?
Cc: -zhongyi@chromium.org
Re #76: the feature is enabled via finch only, unfortunately we currently do not support command line flag enabled yet. 
Oh, isn't there a way to enable finch stuff via the cmd line? I thought --enable-features might be able to do it, but I couldn't figure out the full name.
--force-fieldtrials=trial/group
Thanks tombergan@chromium.org,

Hopefully, my response to the mailing list will be posted in a few days (it's in review).  I've studied both chains and have not seen the no-cache request directive discussed, so I believe it is new, nor do I see it conflicting with any existing statements.  I believe, the conditional request discussion is tangential but could be used for similar use cases.  

Perhaps this conversation is better suited on the mailing list? I'll continue to reply here for now.

Happy to have this reviewed by IETF mailing list, but I would hope we would not merge any code that might potentially enable non-compliant SPEC behavior by default without giving them time to comment.

Again, I agree that the browser can cancel a push promise for any reason, that is not my concern here.  My concern is that this PR does not treat a server-initiated request semantics the same as a client-initiated request.  If the IETF reaches a consensus that the request directive should have same semantic equivalence for client initiated request and server-initiated request, then I believe this PR as implemented would violate the SPEC. As stated, IMO the SPEC already makes this assertion clear.  The premise "Cancel unnecessary push streams when it's discovered they're in cache" is fine, but a server-initiated (or client-initiated) request with no-cache request directive could not be served directly by cache by definition, and thus would not fall under that statement.

I believe you raised 2 points, one on intermediaries and one on efficiencies:

I assume you are referring to intermediaries (CDN/reverse-proxies) sending push promises independently from the origin server?  From a technical point of view, I completely agree with your assertion proxies MUST NOT independently send a push promise with a no-cache request directive unless they have validated the response with the origin server.  However, this is the exact same model/flow as a response no-cache directive, which is already implemented!  Namely, intermediaries should not replay push promises with a request no-cache directive without validating the cached response, just like it should not serve a cached response with a response no-cache directive without validating it.

> What if the push promise has no-cache, and we take your idea to not cancel the push, but it turns out the client already has the most up-to-date version of the resource? We wasted time -- we could have canceled the push sooner.  Better, IMO, to label the push promise with a conditional header describing the resource so the client can determine if the pushed response is more up-to-date than the resource it has cache.

This problem seems to be generic to the request/response model and cache directives, whether the request is initiated by the browser or by the server.  For example, if a client initiates a request with the no-cache cache directive, then it can be an equal waste of time if the cached response is already in fact valid.  I would hope that any solution to this problem would be generic and solve both cases.  Moreover, I believe existing work in progress (https://tools.ietf.org/html/draft-kazuho-h2-cache-digest-00) would address your concern in a generic way AND be more efficient than canceling the push promise because with that proposal the push promise would not be sent in the first place.  
What's the specific flag to enable this feature then? Helping developers play with this seems useful.
I haven't actually tried this, but based on the code, I think something like this should work:

--force-fieldtrials=QUIC/Enabled
--force-fieldtrial-params=QUIC.Enabled:enable_server_push_cancellation/true

zhongyi@, can you verify the specific flags to use? Can you also verify that the finch experiment works on HTTP/2, not just QUIC, despite the finch experiment name?
--force-fieldtrials=QUIC/Enabled
--force-fieldtrial-params=QUIC.Enabled:enable_server_push_cancellation/true
is the correct command line flags to turn on this feature for both QUIC and SPDY. 

To verify, you could turn on net log internals, navigate to a website where server push is used and search for SERVER_PUSH_LOOKUP_TRANSACTION.

We didn't create a SPDY specific flag for this feature as QUIC has been enabled for most of the users.
Hmm, I'm not seeing SERVER_PUSH_LOOKUP_TRANSACTION in the HTTP/2 logs, and it isn't rejecting pushes for cached assets.

I might be holding it wrong. I'll wait for a proper about:flags flag & give it a spin.
Chrome seems to reject items it already has in the push cache with PROTOCOL_ERROR, whereas it should be CANCEL or REFUSED_STREAM (https://bugs.chromium.org/p/chromium/issues/detail?id=726725). Mentioning it in case the work here already fixes the issue.
Project Member Comment 86 by bugdroid1@chromium.org, Jun 9
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f054cd0787297aa08098edb9dcc2cdde7cc03fc4

commit f054cd0787297aa08098edb9dcc2cdde7cc03fc4
Author: zhongyi <zhongyi@chromium.org>
Date: Fri Jun 09 07:09:10 2017

Add view for server push cancellation field in net internal.

BUG=232040
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2924113003
Cr-Commit-Position: refs/heads/master@{#478219}

[modify] https://crrev.com/f054cd0787297aa08098edb9dcc2cdde7cc03fc4/chrome/browser/resources/net_internals/quic_view.html
[modify] https://crrev.com/f054cd0787297aa08098edb9dcc2cdde7cc03fc4/net/http/http_network_session.cc

Sign in to add a comment