1368080 - Bad handling of multiple HTTP/2 pushes of the same resource

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jake Archibald]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

Steps to reproduce:

Scrappy test case: https://github.com/jakearchibald/http2-push-test
Video of issue: https://www.youtube.com/watch?v=N___ogRnhIA


Actual results:

From the video:

0:06 - Resource '/data' pushed along with navigation.
0:11 - '/data' fetched and correctly comes from push cache.
0:14 - Page reloaded, pushing '/data' again.
0:20 - Page reloaded, pushing '/data' again, while it's already in the push cache. This causes Firefox to reject the stream with an INTERNAL_ERROR.
0:28 - '/data' fetched, and it comes from the server, not the push cache.


Expected results:

Push streams may be rejected if the browser already has the resource, but it should use the CANCEL or REFUSED_STREAM code.

The big problem is that Firefox then drops the item it already had in the push cache.

Firefox should either keep the item it already had (what Chrome does), or allow the new item to overwrite the old one.

Comment 1

[Jake Archibald]

This was spotted in nightly btw.

Comment 2

[:Atoll]

Writeup including this bug by reporter: https://jakearchibald.com/2017/h2-push-tougher-than-i-thought/

Comment 3

[u408661]

OK, finally got around to looking at this. Both issues mentioned here (wrong error code & removed push) exist, quite obviously. The issue with the missing push on duplicate lies in this: when we go to do cleanup on stuff we've allocated for the duplicate push, we follow the regular cleanup stream logic. This is 99.9% correct - the only thing we do in that code path that's wrong (in this case) is to remove the entry in the push cache without regard to its stream id (we only remove based on hash key, and since both pushes are for the same URL, the hash keys are the same). The easy solution here is to make that removal contingent on the stream ID of the stored push and the stream ID of the push we're cleaning up being the same. It's a pretty straightforward patch. As with many of these things, writing the test for our setup will be the tricky bit. Either way, patch should exist this week.

Comment 4

[u408661]

Attachment: Bug 1368080 - Use REFUSED_STREAM instead of INTERNAL_ERROR on duplicate push key.

Review commit: https://reviewboard.mozilla.org/r/153322/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153322/

Comment 5

[u408661]

Attachment: Bug 1368080 - Only remove pushed streams from cache when canceling the stream if the ids match.

Review commit: https://reviewboard.mozilla.org/r/153324/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153324/

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 8882214 [details]
Bug 1368080 - Use REFUSED_STREAM instead of INTERNAL_ERROR on duplicate push key.

https://reviewboard.mozilla.org/r/153322/#review158832

Comment 7

[Patrick McManus [:mcmanus]]

Comment on attachment 8882215 [details]
Bug 1368080 - Only remove pushed streams from cache when canceling the stream if the ids match.

https://reviewboard.mozilla.org/r/153324/#review158834

thanks

Comment 8

[u408661]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4690e384036b76793177e139b6fcc153ca7013a5

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/992ddd920585
Use REFUSED_STREAM instead of INTERNAL_ERROR on duplicate push key. r=mcmanus
https://hg.mozilla.org/integration/autoland/rev/671e8c23db24
Only remove pushed streams from cache when canceling the stream if the ids match. r=mcmanus

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/992ddd920585
https://hg.mozilla.org/mozilla-central/rev/671e8c23db24