Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 56

Status

()

Core
Networking: HTTP
RESOLVED FIXED
2 months ago
19 days ago

People

(Reporter: Jake Archibald, Assigned: nwgh)

Tracking

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [necko-active])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 months ago
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.
(Reporter)

Comment 1

2 months ago
This was spotted in nightly btw.
(Reporter)

Updated

2 months ago
Component: Untriaged → Networking: HTTP
Product: Firefox → Core
Assignee: nobody → hurley
Whiteboard: [necko-active]
Writeup including this bug by reporter: https://jakearchibald.com/2017/h2-push-tougher-than-i-thought/
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

21 days ago
mozreview-review
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
Attachment #8882214 - Flags: review?(mcmanus) → review+

Comment 7

21 days ago
mozreview-review
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
Attachment #8882215 - Flags: review?(mcmanus) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4690e384036b76793177e139b6fcc153ca7013a5

Comment 9

21 days ago
Pushed by hurley@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/992ddd920585
https://hg.mozilla.org/mozilla-central/rev/671e8c23db24
Status: NEW → RESOLVED
Last Resolved: 20 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.