Revert CLs landed in HttpStreamFactoryImpl to track down a crasher

Revert CLs landed in HttpStreamFactoryImpl to track down a crasher

There is a top crasher in proxy resolution. This CL is to revert the 
proxy refactoring CL (c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9) so we 
can see if the crash goes away. To have a clean revert, this CL also 
reverted five additional changes.

Revert "Extract Proxy Resolution out of HttpStreamFactoryImpl::Job"
crrev.com/c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9.

Revert "Fix SpdySessionKey for HTTP/2 alternative Jobs."
crrev.com/1fc76912bc055188dc76c09f6032096c079d4d86.

Revert "Convert some DCHECKs to CHECKs to help track down a proxy bug."
crrev.com/89ca16bcbd71651d183a2d8c430a10d3275cbd40.

Revert "Return Job as unique_ptr from factory methods."
crrev.com/6b86d7c8b20f25282abef00a275c40ced632aab4.

Revert "Return Request as unique_ptr from JobController::Start()."
crrev.com/f4bdc5a34983b679ca8b138ad493391b8220f807.

Revert "Fix HttpStreamFactoryImpl::JobController::GetLoadState()"
crrev.com/b6270d12412565789accd0aa7d37f11a86982fb1.

BUG=723589, 475060
Review-Url: https://codereview.chromium.org/2895263003
Cr-Commit-Position: refs/heads/master@{#473636}
Committed: https://chromium.googlesource.com/chromium/src/+/8fb01a7d9c7310372afa618e9b4a6799e68a3365

[xunjieli, 2017-05-22 16:54:07]

Description was changed from

==========
Revert CLs landed in HttpStreamFactoryImpl to track down a crasher

There is a top crasher in proxy resolution. This CL is to revert the proxy
refactoring work in HttpStreamFactoryImpl::JobController so we can see if
the crash goes away. To have a clean revert, this CL also reverted four
additional changes.


Revert "Extract Proxy Resolution out of HttpStreamFactoryImpl::Job"

This reverts commit c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9.

Revert "Convert some DCHECKs to CHECKs to help track down a proxy bug."

This reverts commit 89ca16bcbd71651d183a2d8c430a10d3275cbd40.

Revert "Return Job as unique_ptr from factory methods."

This reverts commit 6b86d7c8b20f25282abef00a275c40ced632aab4.

Revert "Return Request as unique_ptr from JobController::Start()."

This reverts commit f4bdc5a34983b679ca8b138ad493391b8220f807.

Revert "Fix HttpStreamFactoryImpl::JobController::GetLoadState()"

This reverts commit b6270d12412565789accd0aa7d37f11a86982fb1.

BUG=723589
==========

to

==========
Revert CLs landed in HttpStreamFactoryImpl to track down a crasher

There is a top crasher in proxy resolution. This CL is to revert the proxy
refactoring work in HttpStreamFactoryImpl::JobController so we can see if
the crash goes away. To have a clean revert, this CL also reverted four
additional changes.


Revert "Extract Proxy Resolution out of HttpStreamFactoryImpl::Job"
crrev.com/c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9.

Revert "Convert some DCHECKs to CHECKs to help track down a proxy bug."
crrev.com/89ca16bcbd71651d183a2d8c430a10d3275cbd40.

Revert "Return Job as unique_ptr from factory methods."
crrev.com/6b86d7c8b20f25282abef00a275c40ced632aab4.

Revert "Return Request as unique_ptr from JobController::Start()."
crrev.com/f4bdc5a34983b679ca8b138ad493391b8220f807.

Revert "Fix HttpStreamFactoryImpl::JobController::GetLoadState()"
crrev.com/b6270d12412565789accd0aa7d37f11a86982fb1.

BUG=723589
==========

[xunjieli, 2017-05-22 16:55:50]

xunjieli@chromium.org changed reviewers:
+ bnc@chromium.org

[xunjieli, 2017-05-22 16:55:51]

Bence, PTAL. 

(cc-ing my other reviewers for their information).

[xunjieli, 2017-05-22 16:56:21]

Description was changed from

==========
Revert CLs landed in HttpStreamFactoryImpl to track down a crasher

There is a top crasher in proxy resolution. This CL is to revert the proxy
refactoring work in HttpStreamFactoryImpl::JobController so we can see if
the crash goes away. To have a clean revert, this CL also reverted four
additional changes.


Revert "Extract Proxy Resolution out of HttpStreamFactoryImpl::Job"
crrev.com/c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9.

Revert "Convert some DCHECKs to CHECKs to help track down a proxy bug."
crrev.com/89ca16bcbd71651d183a2d8c430a10d3275cbd40.

Revert "Return Job as unique_ptr from factory methods."
crrev.com/6b86d7c8b20f25282abef00a275c40ced632aab4.

Revert "Return Request as unique_ptr from JobController::Start()."
crrev.com/f4bdc5a34983b679ca8b138ad493391b8220f807.

Revert "Fix HttpStreamFactoryImpl::JobController::GetLoadState()"
crrev.com/b6270d12412565789accd0aa7d37f11a86982fb1.

BUG=723589
==========

to

==========
Revert CLs landed in HttpStreamFactoryImpl to track down a crasher

There is a top crasher in proxy resolution. This CL is to revert the proxy
refactoring work in HttpStreamFactoryImpl::JobController so we can see if
the crash goes away. To have a clean revert, this CL also reverted four
additional changes.


Revert "Extract Proxy Resolution out of HttpStreamFactoryImpl::Job"
crrev.com/c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9.

Revert "Convert some DCHECKs to CHECKs to help track down a proxy bug."
crrev.com/89ca16bcbd71651d183a2d8c430a10d3275cbd40.

Revert "Return Job as unique_ptr from factory methods."
crrev.com/6b86d7c8b20f25282abef00a275c40ced632aab4.

Revert "Return Request as unique_ptr from JobController::Start()."
crrev.com/f4bdc5a34983b679ca8b138ad493391b8220f807.

Revert "Fix HttpStreamFactoryImpl::JobController::GetLoadState()"
crrev.com/b6270d12412565789accd0aa7d37f11a86982fb1.

BUG=723589,475060
==========

[xunjieli, 2017-05-22 16:56:27]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[xunjieli, 2017-05-22 16:56:47]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2017-05-22 17:09:26]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 17:09:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xunjieli, 2017-05-22 17:12:33]

Bence, I feel really bad but I need to revert one more of your CLs ([Fix
SpdySessionKey for HTTP/2 alternative Jobs]). Job::GetSpdySessionKey() is not
safe to invoke before proxy resolution completion because GetSpdySessionKey()
can call into |proxy_info_|.

[xunjieli, 2017-05-22 17:13:51]

Description was changed from

==========
Revert CLs landed in HttpStreamFactoryImpl to track down a crasher

There is a top crasher in proxy resolution. This CL is to revert the proxy
refactoring work in HttpStreamFactoryImpl::JobController so we can see if
the crash goes away. To have a clean revert, this CL also reverted four
additional changes.


Revert "Extract Proxy Resolution out of HttpStreamFactoryImpl::Job"
crrev.com/c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9.

Revert "Convert some DCHECKs to CHECKs to help track down a proxy bug."
crrev.com/89ca16bcbd71651d183a2d8c430a10d3275cbd40.

Revert "Return Job as unique_ptr from factory methods."
crrev.com/6b86d7c8b20f25282abef00a275c40ced632aab4.

Revert "Return Request as unique_ptr from JobController::Start()."
crrev.com/f4bdc5a34983b679ca8b138ad493391b8220f807.

Revert "Fix HttpStreamFactoryImpl::JobController::GetLoadState()"
crrev.com/b6270d12412565789accd0aa7d37f11a86982fb1.

BUG=723589,475060
==========

to

==========
Revert CLs landed in HttpStreamFactoryImpl to track down a crasher

There is a top crasher in proxy resolution. This CL is to revert the 
proxy refactoring CL (c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9)  in 
HttpStreamFactoryImpl::JobController so we can see if the crash goes 
away. To have a clean revert, this CL also reverted five
additional changes.


Revert "Fix SpdySessionKey for HTTP/2 alternative Jobs."
crrev.com/1fc76912bc055188dc76c09f6032096c079d4d86.

Revert "Extract Proxy Resolution out of HttpStreamFactoryImpl::Job"
crrev.com/c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9.

Revert "Convert some DCHECKs to CHECKs to help track down a proxy bug."
crrev.com/89ca16bcbd71651d183a2d8c430a10d3275cbd40.

Revert "Return Job as unique_ptr from factory methods."
crrev.com/6b86d7c8b20f25282abef00a275c40ced632aab4.

Revert "Return Request as unique_ptr from JobController::Start()."
crrev.com/f4bdc5a34983b679ca8b138ad493391b8220f807.

Revert "Fix HttpStreamFactoryImpl::JobController::GetLoadState()"
crrev.com/b6270d12412565789accd0aa7d37f11a86982fb1.

BUG=723589,475060
==========

[xunjieli, 2017-05-22 17:14:10]

Description was changed from

==========
Revert CLs landed in HttpStreamFactoryImpl to track down a crasher

There is a top crasher in proxy resolution. This CL is to revert the 
proxy refactoring CL (c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9)  in 
HttpStreamFactoryImpl::JobController so we can see if the crash goes 
away. To have a clean revert, this CL also reverted five
additional changes.


Revert "Fix SpdySessionKey for HTTP/2 alternative Jobs."
crrev.com/1fc76912bc055188dc76c09f6032096c079d4d86.

Revert "Extract Proxy Resolution out of HttpStreamFactoryImpl::Job"
crrev.com/c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9.

Revert "Convert some DCHECKs to CHECKs to help track down a proxy bug."
crrev.com/89ca16bcbd71651d183a2d8c430a10d3275cbd40.

Revert "Return Job as unique_ptr from factory methods."
crrev.com/6b86d7c8b20f25282abef00a275c40ced632aab4.

Revert "Return Request as unique_ptr from JobController::Start()."
crrev.com/f4bdc5a34983b679ca8b138ad493391b8220f807.

Revert "Fix HttpStreamFactoryImpl::JobController::GetLoadState()"
crrev.com/b6270d12412565789accd0aa7d37f11a86982fb1.

BUG=723589,475060
==========

to

==========
Revert CLs landed in HttpStreamFactoryImpl to track down a crasher

There is a top crasher in proxy resolution. This CL is to revert the 
proxy refactoring CL (c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9) so we 
can see if the crash goes away. To have a clean revert, this CL also reverted
five additional changes.

Revert "Fix SpdySessionKey for HTTP/2 alternative Jobs."
crrev.com/1fc76912bc055188dc76c09f6032096c079d4d86.

Revert "Extract Proxy Resolution out of HttpStreamFactoryImpl::Job"
crrev.com/c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9.

Revert "Convert some DCHECKs to CHECKs to help track down a proxy bug."
crrev.com/89ca16bcbd71651d183a2d8c430a10d3275cbd40.

Revert "Return Job as unique_ptr from factory methods."
crrev.com/6b86d7c8b20f25282abef00a275c40ced632aab4.

Revert "Return Request as unique_ptr from JobController::Start()."
crrev.com/f4bdc5a34983b679ca8b138ad493391b8220f807.

Revert "Fix HttpStreamFactoryImpl::JobController::GetLoadState()"
crrev.com/b6270d12412565789accd0aa7d37f11a86982fb1.

BUG=723589,475060
==========

[xunjieli, 2017-05-22 17:14:17]

Description was changed from

==========
Revert CLs landed in HttpStreamFactoryImpl to track down a crasher

There is a top crasher in proxy resolution. This CL is to revert the 
proxy refactoring CL (c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9) so we 
can see if the crash goes away. To have a clean revert, this CL also reverted
five additional changes.

Revert "Fix SpdySessionKey for HTTP/2 alternative Jobs."
crrev.com/1fc76912bc055188dc76c09f6032096c079d4d86.

Revert "Extract Proxy Resolution out of HttpStreamFactoryImpl::Job"
crrev.com/c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9.

Revert "Convert some DCHECKs to CHECKs to help track down a proxy bug."
crrev.com/89ca16bcbd71651d183a2d8c430a10d3275cbd40.

Revert "Return Job as unique_ptr from factory methods."
crrev.com/6b86d7c8b20f25282abef00a275c40ced632aab4.

Revert "Return Request as unique_ptr from JobController::Start()."
crrev.com/f4bdc5a34983b679ca8b138ad493391b8220f807.

Revert "Fix HttpStreamFactoryImpl::JobController::GetLoadState()"
crrev.com/b6270d12412565789accd0aa7d37f11a86982fb1.

BUG=723589,475060
==========

to

==========
Revert CLs landed in HttpStreamFactoryImpl to track down a crasher

There is a top crasher in proxy resolution. This CL is to revert the 
proxy refactoring CL (c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9) so we 
can see if the crash goes away. To have a clean revert, this CL also 
reverted five additional changes.

Revert "Fix SpdySessionKey for HTTP/2 alternative Jobs."
crrev.com/1fc76912bc055188dc76c09f6032096c079d4d86.

Revert "Extract Proxy Resolution out of HttpStreamFactoryImpl::Job"
crrev.com/c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9.

Revert "Convert some DCHECKs to CHECKs to help track down a proxy bug."
crrev.com/89ca16bcbd71651d183a2d8c430a10d3275cbd40.

Revert "Return Job as unique_ptr from factory methods."
crrev.com/6b86d7c8b20f25282abef00a275c40ced632aab4.

Revert "Return Request as unique_ptr from JobController::Start()."
crrev.com/f4bdc5a34983b679ca8b138ad493391b8220f807.

Revert "Fix HttpStreamFactoryImpl::JobController::GetLoadState()"
crrev.com/b6270d12412565789accd0aa7d37f11a86982fb1.

BUG=723589,475060
==========

[xunjieli, 2017-05-22 17:14:49]

Description was changed from

==========
Revert CLs landed in HttpStreamFactoryImpl to track down a crasher

There is a top crasher in proxy resolution. This CL is to revert the 
proxy refactoring CL (c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9) so we 
can see if the crash goes away. To have a clean revert, this CL also 
reverted five additional changes.

Revert "Fix SpdySessionKey for HTTP/2 alternative Jobs."
crrev.com/1fc76912bc055188dc76c09f6032096c079d4d86.

Revert "Extract Proxy Resolution out of HttpStreamFactoryImpl::Job"
crrev.com/c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9.

Revert "Convert some DCHECKs to CHECKs to help track down a proxy bug."
crrev.com/89ca16bcbd71651d183a2d8c430a10d3275cbd40.

Revert "Return Job as unique_ptr from factory methods."
crrev.com/6b86d7c8b20f25282abef00a275c40ced632aab4.

Revert "Return Request as unique_ptr from JobController::Start()."
crrev.com/f4bdc5a34983b679ca8b138ad493391b8220f807.

Revert "Fix HttpStreamFactoryImpl::JobController::GetLoadState()"
crrev.com/b6270d12412565789accd0aa7d37f11a86982fb1.

BUG=723589,475060
==========

to

==========
Revert CLs landed in HttpStreamFactoryImpl to track down a crasher

There is a top crasher in proxy resolution. This CL is to revert the 
proxy refactoring CL (c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9) so we 
can see if the crash goes away. To have a clean revert, this CL also 
reverted five additional changes.

Revert "Extract Proxy Resolution out of HttpStreamFactoryImpl::Job"
crrev.com/c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9.

Revert "Fix SpdySessionKey for HTTP/2 alternative Jobs."
crrev.com/1fc76912bc055188dc76c09f6032096c079d4d86.

Revert "Convert some DCHECKs to CHECKs to help track down a proxy bug."
crrev.com/89ca16bcbd71651d183a2d8c430a10d3275cbd40.

Revert "Return Job as unique_ptr from factory methods."
crrev.com/6b86d7c8b20f25282abef00a275c40ced632aab4.

Revert "Return Request as unique_ptr from JobController::Start()."
crrev.com/f4bdc5a34983b679ca8b138ad493391b8220f807.

Revert "Fix HttpStreamFactoryImpl::JobController::GetLoadState()"
crrev.com/b6270d12412565789accd0aa7d37f11a86982fb1.

BUG=723589,475060
==========

[mmenke, 2017-05-22 17:17:08]

On 2017/05/22 17:12:33, xunjieli wrote:
> Bence, I feel really bad but I need to revert one more of your CLs ([Fix
> SpdySessionKey for HTTP/2 alternative Jobs]). Job::GetSpdySessionKey() is not
> safe to invoke before proxy resolution completion because GetSpdySessionKey()
> can call into |proxy_info_|.

Egads...Didn't realize quite how much I was suggesting be reverted.  Given this
is a top crasher, I still think this is the next next thing we should try, but I
feel your pain (Or at least better understand the cause of it).

[Bence, 2017-05-22 17:23:12]

LGTM, thank you!

[xunjieli, 2017-05-22 17:54:54]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2017-05-22 17:54:58]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2017-05-22 17:55:10]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 18:38:04]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495475698881400,
"parent_rev": "ede5f6ad4dbb6270e20398cebbca200cc280f8d4", "commit_rev":
"8fb01a7d9c7310372afa618e9b4a6799e68a3365"}

[commit-bot: I haz the power, 2017-05-22 18:38:15]

Description was changed from

==========
Revert CLs landed in HttpStreamFactoryImpl to track down a crasher

There is a top crasher in proxy resolution. This CL is to revert the 
proxy refactoring CL (c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9) so we 
can see if the crash goes away. To have a clean revert, this CL also 
reverted five additional changes.

Revert "Extract Proxy Resolution out of HttpStreamFactoryImpl::Job"
crrev.com/c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9.

Revert "Fix SpdySessionKey for HTTP/2 alternative Jobs."
crrev.com/1fc76912bc055188dc76c09f6032096c079d4d86.

Revert "Convert some DCHECKs to CHECKs to help track down a proxy bug."
crrev.com/89ca16bcbd71651d183a2d8c430a10d3275cbd40.

Revert "Return Job as unique_ptr from factory methods."
crrev.com/6b86d7c8b20f25282abef00a275c40ced632aab4.

Revert "Return Request as unique_ptr from JobController::Start()."
crrev.com/f4bdc5a34983b679ca8b138ad493391b8220f807.

Revert "Fix HttpStreamFactoryImpl::JobController::GetLoadState()"
crrev.com/b6270d12412565789accd0aa7d37f11a86982fb1.

BUG=723589,475060
==========

to

==========
Revert CLs landed in HttpStreamFactoryImpl to track down a crasher

There is a top crasher in proxy resolution. This CL is to revert the 
proxy refactoring CL (c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9) so we 
can see if the crash goes away. To have a clean revert, this CL also 
reverted five additional changes.

Revert "Extract Proxy Resolution out of HttpStreamFactoryImpl::Job"
crrev.com/c9a993789f4732a105d5ede4f9f36e0f7e0ce9b9.

Revert "Fix SpdySessionKey for HTTP/2 alternative Jobs."
crrev.com/1fc76912bc055188dc76c09f6032096c079d4d86.

Revert "Convert some DCHECKs to CHECKs to help track down a proxy bug."
crrev.com/89ca16bcbd71651d183a2d8c430a10d3275cbd40.

Revert "Return Job as unique_ptr from factory methods."
crrev.com/6b86d7c8b20f25282abef00a275c40ced632aab4.

Revert "Return Request as unique_ptr from JobController::Start()."
crrev.com/f4bdc5a34983b679ca8b138ad493391b8220f807.

Revert "Fix HttpStreamFactoryImpl::JobController::GetLoadState()"
crrev.com/b6270d12412565789accd0aa7d37f11a86982fb1.

BUG=723589,475060

Review-Url: https://codereview.chromium.org/2895263003
Cr-Commit-Position: refs/heads/master@{#473636}
Committed:
https://chromium.googlesource.com/chromium/src/+/8fb01a7d9c7310372afa618e9b4a...
==========

[commit-bot: I haz the power, 2017-05-22 18:38:17]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/8fb01a7d9c7310372afa618e9b4a...

[Zhongyi Shi, 2017-05-22 19:05:38]

zhongyi@chromium.org changed reviewers:
+ zhongyi@chromium.org

[Zhongyi Shi, 2017-05-22 19:05:39]

https://codereview.chromium.org/2895263003/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.cc (left):

https://codereview.chromium.org/2895263003/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:790: main_job_ =
job_factory_->CreateAltSvcJob(
FYI, Ryan and I were just looking at the code today, and we found the original
proxy resolution CL might cause a regression bug here to create a AltSvc Job for
main_job directly in the preconnect case.

In fact, we should check whether the alternative_service is valid, if not,
create a TCP job. Else, set destination from the alternative_service and
ApplyHostMappingRules, then create a alt svc job.

[xunjieli, 2017-05-22 19:15:00]

https://codereview.chromium.org/2895263003/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.cc (left):

https://codereview.chromium.org/2895263003/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:790: main_job_ =
job_factory_->CreateAltSvcJob(
On 2017/05/22 19:05:39, Zhongyi Shi wrote:
> FYI, Ryan and I were just looking at the code today, and we found the original
> proxy resolution CL might cause a regression bug here to create a AltSvc Job
for
> main_job directly in the preconnect case.
> 
> In fact, we should check whether the alternative_service is valid, if not,
> create a TCP job. Else, set destination from the alternative_service and
> ApplyHostMappingRules, then create a alt svc job. 

Acknowledged. Good catch! Will do this when relanding the patch. Thanks, Cherie.