1409452 - Dooming cache entry check for download happens too early

Status: NEW

(Core :: Networking: HTTP, enhancement, P3)

Description

[Honza Bambas (:mayhemer)]

https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/netwerk/protocol/http/nsHttpChannel.cpp#1710

This is too early, the downloader code may not call SetChannelIsForDownload.

We need to duplicate that code in HttpBaseChannel::SetChannelIsForDownload as well.


Log:


2017-10-17 16:52:06.322 │ Main Thread │ nsHttpChannel::OnStartRequest [this=000001EBE83FA000 request=000001EBEE46A120 status=0]
2017-10-17 16:52:06.322 │ Main Thread │ nsHttpChannel::ProcessResponse [this=000001EBE83FA000 httpStatus=200]
2017-10-17 16:52:06.322 │ Main Thread │ nsHttpChannel::ContinueProcessResponse1 [this=000001EBE83FA000, rv=804b0001]
2017-10-17 16:52:06.322 │ Main Thread │ nsHttpChannel::ProcessNormal [this=000001EBE83FA000]
2017-10-17 16:52:06.322 │ Main Thread │ nsHttpChannel::ContinueProcessNormal [this=000001EBE83FA000]
2017-10-17 16:52:06.322 │ Main Thread │ nsHttpChannel::InitCacheEntry [this=000001EBE83FA000 entry=000001EBEBF6BD00]
2017-10-17 16:52:06.322 │ Main Thread │ nsHttpChannel::AddCacheEntryHeaders [this=000001EBE83FA000] begin
2017-10-17 16:52:06.322 │ Main Thread │ nsHttpChannel::CallOnStartRequest [this=000001EBE83FA000]
2017-10-17 16:52:06.322 │ Main Thread │   calling mListener->OnStartRequest [this=000001EBE83FA000, listener=000001EBE8C8DFB8]

** here we do the check

2017-10-17 16:52:06.322 │ Main Thread │ HttpBaseChannel::SetApplyConversion [this=000001EBE83FA000 value=0]
2017-10-17 16:52:06.322 │ Main Thread │ nsHttpChannel::GetCacheKey [this=000001EBE83FA000]
2017-10-17 16:52:06.322 │ Main Thread │ HttpBaseChannel::DoApplyContentConversions [this=000001EBE83FA000]
2017-10-17 16:52:06.322 │ Main Thread │ Trading cache input stream for output stream [channel=000001EBE83FA000]
2017-10-17 16:52:06.323 │ Main Thread │ nsHttpChannel::OnDataAvailable [this=000001EBE83FA000 request=000001EBEE46A120 offset=0 count=998]
2017-10-17 16:52:06.323 │ Main Thread │ sending progress and status notification [this=000001EBE83FA000 status=804b0006 progress=998/-1]
2017-10-17 16:52:06.323 │ Main Thread │   804b0006 = STATUS_RECEIVING_FROM
2017-10-17 16:52:06.324 │ Main Thread │ HttpBaseChannel::SetApplyConversion [this=000001EBE83FA000 value=1]
2017-10-17 16:52:06.324 │ Main Thread │ nsHttpChannel::MessageDiversionStarted [this=000001EBE83FA000]
2017-10-17 16:52:06.324 │ Main Thread │ nsHttpChannel::SuspendInternal [this=000001EBE83FA000]
2017-10-17 16:52:06.336 │ Main Thread │ HttpBaseChannel::SetApplyConversion [this=000001EBE83FA000 value=1]
2017-10-17 16:52:06.337 │ Main Thread │ nsHttpChannel::OnClassOfServiceUpdated this=000001EBE83FA000, cos=96
2017-10-17 16:52:06.337 │ Main Thread │   cos = Throttleable, UrgentStart
2017-10-17 16:52:06.337 │ Main Thread │ HttpBaseChannel::AddAsNonTailRequest this=000001EBE83FA000, rc=000001EBED88E380, already added=1
2017-10-17 16:52:06.337 │ Main Thread │ HttpBaseChannel::SetChannelIsForDownload 000001EBE83FA000 1

** and here we get the info (delayed)

Comment 1

[Valentin Gosu [:valentin]]

When filing this kind of bugs, can you also triage it?
It is unclear whether you intend to work on it yourself, how important it is, or what the next steps are.
Thanks!

Comment 2

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #1)
> When filing this kind of bugs, can you also triage it?
> It is unclear whether you intend to work on it yourself, how important it
> is, or what the next steps are.
> Thanks!

This all should be cleared in some "triage instructions" doc or something.  I didn't feel obligated to triage as well when filing.  For me this is P2: no immediate damage, but subsequent perf impact (we can wipe cache by downloads when <=50MB or unknown at onstart time)

I will not take bugs I will not work on immediately.  That's a very bad habit.  We have a large team, whoever can take it or Jason can find owners.