As a first pass fix, maybe track URLRequestJobs instead of URLRequests, and move all NQE calls into URLRequestJob from URLRequestHttpJob?  Or possibly move them all into URLRequestHttpJob from URLRequestJob?  Still thinking tracking them isn't the best way to go, but don't want to delve into it enough to suggest a complete solution.

Having said that, one option is to have some sort of NQEPerRequestData field, and make it a member of the URLRequestJob itself.  Not sure if you need to be able to peer into URLRequests when they're not calling into the NQE, though, so maybe not a viable option.

Could alternatively move the stuff into URLRequest, but I think scoping things to a URLRequestJob makes more sense.

There's another bug here:  Requests are added to the NQE's list in NetworkQualityEstimator::NotifyStartTransaction, unconditionally.  But this method can be called multiple times for the same URLRequestJob, without intervening calls that remove the request from the list.

Thanks for filing this bug. 

Right now, throughput analyzer gets notified from 3 different methods:
(1) From URLRequestHttpJob::StartTransactionInternal(): This results in  request getting added to a map.
(2) URLRequestHttpJob::NotifyURLRequestDestroyed(): Kicks the request out of the map.
(3) URLRequestHttpJob::DoneWithRequest(CompletionCause reason): This also kicks the request out of the map.

Note that all the calls are already from URLRequestHttpJob.

mmenke: I think there is some confusion because of a 4th NQE call from URLRequestJob::RecordBytesRead(). This call is not propagated to throughput analyzer, so this call actually has no effect on tracking of the requests in the map.

My understanding is that in the UrlRequestHttpJob's destructor, DoneWithRequest() is called, and so the entry should always be kicked out of the map. 

Also, Re#3: Since it is a map keyed by URLRequest ptr,  I believe subsequent insertions (e.g., request results in multiple transactions) should not have any effect on the correctness or memory usage. Although, NQE can save on some computations.

Thoughts?

mmenke, Does #4 answer all the questions? If yes, I am inclined to close this as "WontFix".

Sorry, forgot to look at this again.  it looks to me like you're right.  I think I missed the DoneWithRequest path.  With it, do we even need the NotifyURLRequestDestroyed path? 

Probably not, but I will check if I can remove it. Thanks for the suggestion.