Lost "range" part of initial http request after HttpCache::Transaction::OnCacheReadError
Reported by
pgorszko...@gmail.com,
Oct 2
|
||||||||
Issue description
UserAgent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:62.0) Gecko/20100101 Firefox/62.0
Example URL:
not available
Steps to reproduce the problem:
Cannot find a way to reproduce it on standard chromium/chrome. It happens only with intensive requests for content with "range" option in http header.
Maybe only static analysis of code(http_cache_transaction.cc) can prove the problem.
What is the expected behavior?
After HttpCache::Transaction::OnCacheReadError the network transaction is done with all data from initial http request.
What went wrong?
As a consequence the response from server is 200(instead of 206) and does not contains "range" part. It causes also downloading the whole data instead only part of file.
Did this work before? No
Chrome version: 70.0.3538.9, master Channel: n/a
OS Version: Ubuntu 16.04
Flash Version: Shockwave Flash 31.0 r0
Proposal solution:
Assign an initial_request_ request_ in case of restart option in HttpCache::Transaction::OnCacheReadError
{code:diff}
--- net/http/http_cache_transaction.cc
+++ net/http/http_cache_transaction.cc
@@ -3184,6 +3184,7 @@ int HttpCache::Transaction::OnCacheReadError(int result, bool restart) {
entry_ = NULL;
is_sparse_ = false;
partial_.reset();
+ request_ = initial_request_;
TransitionToState(STATE_GET_BACKEND);
return OK;
}
{code}
,
Oct 2
,
Oct 2
,
Oct 11
Thanks for filing the issue. @reporter: Could you please provide proper repro steps that reproduce the issue and if possible request you to attach actual and expected results screencast/screenshot so that it would be really helpful for triaging the issue. Thanks.!
,
Oct 11
Dear Swarnasree Mukkala, As I wrote in my description: "Steps to reproduce the problem: Cannot find a way to reproduce it on standard chromium/chrome. It happens only with intensive requests for content with "range" option in http header. Maybe only static analysis of code(http_cache_transaction.cc) can prove the problem." I found this issue in our integration of chromium engine (we do many ranged http requests, and from time to time I get this problem), I haven't reproduced it on any site (tried with multiple ranged XHRs but without success with reproduction).
,
Oct 11
Thank you for providing more feedback. Adding the requester to the cc list. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 25
From comment#5 this doesn't seem to be easily reproducible, but only happens with intensive requests. Hence adding label TE-NeedsTriageHelp and requesting someone from Internals>Network>Cache team to have a look into this and help in further triaging it. Thanks!
,
Oct 25
Let me just take it. Probably easiest to to start with a unit test, since we can inject failures in those. It's somewhat disconcerting that you see cache read errors often enough to notice them, though, it's not something that's supposed to be frequent. Also our range code is pretty brittle, you probably want to be ultra-super-duper-triple-careful :(
,
Oct 26
It is not theoretical thing. As I said I can reproduce it in our application. We implement the client for adaptive streaming media player. We do a lot of ranged requests for part of file. Maybe one of the problem is also some bug that this OnCacheReadError happens but we also observed the consequence of it (loosing initial ranged request). Don't you think that my proposal/solution could solve at least consequence of OnCacheReadError?
,
Oct 29
Your suggestion gets to the root of the issue, but it sets the world up differently than normal for range requests: usually the code wants request_ to point to custom_request_ so it can mess around with Range: headers in case it only needs to re-fetch some pieces and not what was requested initially (and also if it needs to add some conditional validation headers). Now, since it just nuked the cache entry, there is no actual need to mess with that, but I am unable to predict all the circumstances; so using PartialData::RestoreHeaders seems safer.
,
Oct 29
,
Nov 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6a1ba1817f033e651481ca3b6e02131ea0f30eac commit 6a1ba1817f033e651481ca3b6e02131ea0f30eac Author: Maks Orlovich <morlovich@chromium.org> Date: Fri Nov 02 00:41:31 2018 HttpCache: restore range: headers when handling cache stream 0 read failure Bug: 891212 Change-Id: Ib4026c1e896ec188cab54c4e30c8ac2f8eb6b98f Reviewed-on: https://chromium-review.googlesource.com/c/1305496 Reviewed-by: Asanka Herath <asanka@chromium.org> Commit-Queue: Maks Orlovich <morlovich@chromium.org> Cr-Commit-Position: refs/heads/master@{#604774} [modify] https://crrev.com/6a1ba1817f033e651481ca3b6e02131ea0f30eac/net/http/http_cache_transaction.cc [modify] https://crrev.com/6a1ba1817f033e651481ca3b6e02131ea0f30eac/net/http/http_cache_unittest.cc [modify] https://crrev.com/6a1ba1817f033e651481ca3b6e02131ea0f30eac/net/http/mock_http_cache.cc [modify] https://crrev.com/6a1ba1817f033e651481ca3b6e02131ea0f30eac/net/http/mock_http_cache.h
,
Nov 2
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by phanindra.mandapaka@chromium.org
, Oct 2