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

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

This reverts commit 8fb01a7d9c7310372afa618e9b4a6799e68a3365.

BUG=475060, 723589
Review-Url: https://codereview.chromium.org/2910463004
Cr-Commit-Position: refs/heads/master@{#477009}
Committed: https://chromium.googlesource.com/chromium/src/+/96f2a40e670b3b03ce657bfc06ddb23d4fb3ca66

[xunjieli, 2017-05-26 17:05:19]

Patchset #2 (id:20001) has been deleted

[xunjieli, 2017-05-26 17:11:01]

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

[xunjieli, 2017-05-26 17:11:02]

zhongyi@ and bnc@ PTAL at diff between PS#1 and PS#2.

(I am planning to land this with your l-g-t-m and only after I confirmed that
M60 has branched. )

[mmenke, 2017-05-26 17:28:04]

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

[mmenke, 2017-05-26 17:28:05]

Dirveby comments.

https://codereview.chromium.org/2910463004/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2910463004/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:112:
CHECK_EQ(STATE_RESOLVE_PROXY_COMPLETE, next_state_);
Think can change this back to a DCHECK, and remove the TODO

https://codereview.chromium.org/2910463004/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:723: CHECK(!pac_request_);
DCHECK, remove TODO.

[xunjieli, 2017-05-26 17:35:16]

https://codereview.chromium.org/2910463004/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2910463004/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:112:
CHECK_EQ(STATE_RESOLVE_PROXY_COMPLETE, next_state_);
On 2017/05/26 17:28:04, mmenke wrote:
> Think can change this back to a DCHECK, and remove the TODO

Why? I thought we haven't figured out the root cause of the use-after-free
(ProxyInfo::Reset() problem). Should we keep the CHECKs for now?

[xunjieli, 2017-05-26 17:43:24]

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

[mmenke, 2017-05-26 17:43:39]

On 2017/05/26 17:35:16, xunjieli wrote:
>
https://codereview.chromium.org/2910463004/diff/40001/net/http/http_stream_fa...
> File net/http/http_stream_factory_impl_job_controller.cc (right):
> 
>
https://codereview.chromium.org/2910463004/diff/40001/net/http/http_stream_fa...
> net/http/http_stream_factory_impl_job_controller.cc:112:
> CHECK_EQ(STATE_RESOLVE_PROXY_COMPLETE, next_state_);
> On 2017/05/26 17:28:04, mmenke wrote:
> > Think can change this back to a DCHECK, and remove the TODO
> 
> Why? I thought we haven't figured out the root cause of the use-after-free
> (ProxyInfo::Reset() problem). Should we keep the CHECKs for now?

Oh, I thought your last comment on https://crubug.com/723589, and the fact you
marked that issue as fixed, meant you understood the issue.

[commit-bot: I haz the power, 2017-05-26 17:43:55]

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-26 17:52:02]

On 2017/05/26 17:43:39, mmenke wrote:
> On 2017/05/26 17:35:16, xunjieli wrote:
> >
>
https://codereview.chromium.org/2910463004/diff/40001/net/http/http_stream_fa...
> > File net/http/http_stream_factory_impl_job_controller.cc (right):
> > 
> >
>
https://codereview.chromium.org/2910463004/diff/40001/net/http/http_stream_fa...
> > net/http/http_stream_factory_impl_job_controller.cc:112:
> > CHECK_EQ(STATE_RESOLVE_PROXY_COMPLETE, next_state_);
> > On 2017/05/26 17:28:04, mmenke wrote:
> > > Think can change this back to a DCHECK, and remove the TODO
> > 
> > Why? I thought we haven't figured out the root cause of the use-after-free
> > (ProxyInfo::Reset() problem). Should we keep the CHECKs for now?
> 
> Oh, I thought your last comment on https://crubug.com/723589, and the fact you
> marked that issue as fixed, meant you understood the issue.

Got it. Sorry for the confusion. 723589 was a Stable Blocker, so I thought I
should close it sooner than later. 
Cherie pointed out that we pass an invalid alternative service to Preconnect
Jobs. This is a real issue and is pre-existing.
My CL introduced a bug where if the alternative service is valid, we'll still
use the origin host/port instead of the alternative host/port as |destination|.
I did see a couple of crashers related to AltSvc, so this would explain that.
I am still not sure what would have caused the ProxyInfo::Reset() bug. I am
hoping we would have more time to get to the bottom of it in M61.

[Bence, 2017-05-26 18:31:44]

LGTM.

[mmenke, 2017-05-26 18:33:49]

In that case, when you re-land this, suggest re-opening the bug, and marking it
an M-61 stable blocker.  Hopefully removes the need to re-triage the crashes
when they shop up against as a top crasher.

[xunjieli, 2017-05-26 18:35:12]

On 2017/05/26 18:33:49, mmenke wrote:
> In that case, when you re-land this, suggest re-opening the bug, and marking
it
> an M-61 stable blocker.  Hopefully removes the need to re-triage the crashes
> when they shop up against as a top crasher.

SGTM! doing that now.

[xunjieli, 2017-05-26 18:37:50]

Description was changed from

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

This reverts commit 8fb01a7d9c7310372afa618e9b4a6799e68a3365.

BUG=475060
==========

to

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

This reverts commit 8fb01a7d9c7310372afa618e9b4a6799e68a3365.

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

[commit-bot: I haz the power, 2017-05-26 19:04:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 19:04:51]

Dry run: This issue passed the CQ dry run.

[Zhongyi Shi, 2017-05-30 23:47:03]

https://codereview.chromium.org/2910463004/diff/80001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2910463004/diff/80001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller.cc:802:
enable_ip_based_pooling_, session_->net_log());
Nice!

https://codereview.chromium.org/2910463004/diff/80001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller_unittest.cc (right):

https://codereview.chromium.org/2910463004/diff/80001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller_unittest.cc:1492: 
Does this test fail with PS#1 but pass with PS#2?

Presumably as you are adding MockQuicData, this main job is using Alternative
Service. How about adding expectation explicitly check the main job is an
alternative job? And can we have one more test where without Alt Svc set, the
preconnect main job is using a normal TCP job?

[xunjieli, 2017-06-02 20:33:03]

Thanks, PTAL.

https://codereview.chromium.org/2910463004/diff/80001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job_controller_unittest.cc (right):

https://codereview.chromium.org/2910463004/diff/80001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job_controller_unittest.cc:1492: 
On 2017/05/30 23:47:03, Zhongyi Shi wrote:
> Does this test fail with PS#1 but pass with PS#2?
> 
Nope. It doesn't. I tried using an invalid AltSvc to trigger a bug but it was
caught by the DCHECK in GetAlternativeServiceInfoInternal().

> Presumably as you are adding MockQuicData, this main job is using Alternative
> Service. How about adding expectation explicitly check the main job is an
> alternative job? And can we have one more test where without Alt Svc set, the
> preconnect main job is using a normal TCP job?

Done. Cherie, the preconnect jobs are all of job type PRECONNECT.

[xunjieli, 2017-06-05 12:10:23]

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

[commit-bot: I haz the power, 2017-06-05 12:10:31]

Dry run: 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-06-05 12:12:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-05 12:12:41]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xunjieli, 2017-06-05 15:51:00]

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

[xunjieli, 2017-06-05 15:51:15]

Cherie, a friendly ping! Thanks.

[commit-bot: I haz the power, 2017-06-05 15:51:47]

Dry run: CQ is trying da patch.

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

[Zhongyi Shi, 2017-06-05 16:54:02]

lgtm

[xunjieli, 2017-06-05 16:54:54]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2017-06-05 16:54:57]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2017-06-05 16:54:58]

The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org
Link to the patchset: https://codereview.chromium.org/2910463004/#ps120001
(title: "rebased")

[commit-bot: I haz the power, 2017-06-05 16:55:11]

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-06-05 17:24:31]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496681697669890,
"parent_rev": "9187085c7cdff3a2864af92f566d7afacbb3060c", "commit_rev":
"96f2a40e670b3b03ce657bfc06ddb23d4fb3ca66"}

[commit-bot: I haz the power, 2017-06-05 17:24:43]

Description was changed from

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

This reverts commit 8fb01a7d9c7310372afa618e9b4a6799e68a3365.

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

to

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

This reverts commit 8fb01a7d9c7310372afa618e9b4a6799e68a3365.

BUG=475060, 723589

Review-Url: https://codereview.chromium.org/2910463004
Cr-Commit-Position: refs/heads/master@{#477009}
Committed:
https://chromium.googlesource.com/chromium/src/+/96f2a40e670b3b03ce657bfc06dd...
==========

[commit-bot: I haz the power, 2017-06-05 17:24:48]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/96f2a40e670b3b03ce657bfc06dd...