Clean up HttpStreamFactoryImpl::JobController when Impl::Jobs complete

Clean up HttpStreamFactoryImpl::JobController when Impl::Jobs complete

Background: When a server advertises alternative services support,
JobController creates two Jobs (|main_job_| and |alternative_job_|).
The controller blocks |main_job_| when connecting the
|alternative_job_|. It resumes the |main_job_| when |alternative_job_|
fails or when the timeout happens. When both jobs are destroyed, the
JobController goes away.

Bug: We currently leak JobController in the situation where
|alternative_job_| succeeds and |main_job_| is blocked on alt job.
OnRequestComplete() will be called which then destroys the alt job but
not the main job. This causes MaybeNotifyFactoryOfCompletion() to not
clean up the controller.

Fix: This CL fixes this leak by resetting |main_job_| if it is blocked
(i.e. hasn't started connecting) when |alternative_job_| succeeds. This
CL adds a regression test for this.

Note that there is no leak when orphaned job completes, because
"job_bound_ && bound_job_ != job" will evaluate to true. This CL adds a
|!request_| check so it's obvious that we always clean up controller in
the orphaned case. This CL adds a test to confirm that we did clean up
controller when orphaned job completes (which passes with and without
the change).

TBR=zhongyi@chromium.org
TBR=mmenke@chromium.org

NOTRY=true
NOPRESUBMIT=true

BUG=678768
Review-Url: https://codereview.chromium.org/2619583002
Cr-Commit-Position: refs/heads/master@{#442556}
(cherry picked from commit e368398a3a5357a8742f3829ef08cc45080fb808)

Review-Url: https://codereview.chromium.org/2622193003
Cr-Commit-Position: refs/branch-heads/2924@{#733}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}
Committed: https://chromium.googlesource.com/chromium/src/+/1c7375269bc6e2ef192b74dd1b17f7e4e8b5f63e

[xunjieli, 2017-01-11 15:23:23]

Description was changed from

==========
Clean up HttpStreamFactoryImpl::JobController when Impl::Jobs complete

Background: When a server advertises alternative services support,
JobController creates two Jobs (|main_job_| and |alternative_job_|).
The controller blocks |main_job_| when connecting the
|alternative_job_|. It resumes the |main_job_| when |alternative_job_|
fails or when the timeout happens. When both jobs are destroyed, the
JobController goes away.

Bug: We currently leak JobController in the situation where
|alternative_job_| succeeds and |main_job_| is blocked on alt job.
OnRequestComplete() will be called which then destroys the alt job but
not the main job. This causes MaybeNotifyFactoryOfCompletion() to not
clean up the controller.

Fix: This CL fixes this leak by resetting |main_job_| if it is blocked
(i.e. hasn't started connecting) when |alternative_job_| succeeds. This
CL adds a regression test for this.

Note that there is no leak when orphaned job completes, because
"job_bound_ && bound_job_ != job" will evaluate to true. This CL adds a
|!request_| check so it's obvious that we always clean up controller in
the orphaned case. This CL adds a test to confirm that we did clean up
controller when orphaned job completes (which passes with and without
the change).

BUG=678768

Review-Url: https://codereview.chromium.org/2619583002
Cr-Commit-Position: refs/heads/master@{#442556}
(cherry picked from commit e368398a3a5357a8742f3829ef08cc45080fb808)
==========

to

==========
Clean up HttpStreamFactoryImpl::JobController when Impl::Jobs complete

Background: When a server advertises alternative services support,
JobController creates two Jobs (|main_job_| and |alternative_job_|).
The controller blocks |main_job_| when connecting the
|alternative_job_|. It resumes the |main_job_| when |alternative_job_|
fails or when the timeout happens. When both jobs are destroyed, the
JobController goes away.

Bug: We currently leak JobController in the situation where
|alternative_job_| succeeds and |main_job_| is blocked on alt job.
OnRequestComplete() will be called which then destroys the alt job but
not the main job. This causes MaybeNotifyFactoryOfCompletion() to not
clean up the controller.

Fix: This CL fixes this leak by resetting |main_job_| if it is blocked
(i.e. hasn't started connecting) when |alternative_job_| succeeds. This
CL adds a regression test for this.

Note that there is no leak when orphaned job completes, because
"job_bound_ && bound_job_ != job" will evaluate to true. This CL adds a
|!request_| check so it's obvious that we always clean up controller in
the orphaned case. This CL adds a test to confirm that we did clean up
controller when orphaned job completes (which passes with and without
the change).

TBR=zhongyi@chromium.org
TBR=mmenke@chromium.org

BUG=678768

Review-Url: https://codereview.chromium.org/2619583002
Cr-Commit-Position: refs/heads/master@{#442556}
(cherry picked from commit e368398a3a5357a8742f3829ef08cc45080fb808)
==========

[xunjieli, 2017-01-11 15:23:23]

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

[xunjieli, 2017-01-11 15:24:33]

Hi Cherie and Matt, This is to merge the fix into M56. I have TBR-ed you. I will
confirm net_unittests are passing before landing.

[xunjieli, 2017-01-11 15:56:09]

net_unittests are passing. I am checking this in. There is one minor merge
conflict due JobController::OnStreamReady(), which used to have a ProxyInfo as
the third argument. The ProxyInfo argument was removed in M57.

[xunjieli, 2017-01-11 15:56:22]

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

[commit-bot: I haz the power, 2017-01-11 15:56:48]

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-01-11 15:56:48]

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

[commit-bot: I haz the power, 2017-01-11 15:56:49]

Dry run: CLs for remote refs other than refs/pending/heads/master must contain
NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them

[xunjieli, 2017-01-11 16:06:05]

Description was changed from

==========
Clean up HttpStreamFactoryImpl::JobController when Impl::Jobs complete

Background: When a server advertises alternative services support,
JobController creates two Jobs (|main_job_| and |alternative_job_|).
The controller blocks |main_job_| when connecting the
|alternative_job_|. It resumes the |main_job_| when |alternative_job_|
fails or when the timeout happens. When both jobs are destroyed, the
JobController goes away.

Bug: We currently leak JobController in the situation where
|alternative_job_| succeeds and |main_job_| is blocked on alt job.
OnRequestComplete() will be called which then destroys the alt job but
not the main job. This causes MaybeNotifyFactoryOfCompletion() to not
clean up the controller.

Fix: This CL fixes this leak by resetting |main_job_| if it is blocked
(i.e. hasn't started connecting) when |alternative_job_| succeeds. This
CL adds a regression test for this.

Note that there is no leak when orphaned job completes, because
"job_bound_ && bound_job_ != job" will evaluate to true. This CL adds a
|!request_| check so it's obvious that we always clean up controller in
the orphaned case. This CL adds a test to confirm that we did clean up
controller when orphaned job completes (which passes with and without
the change).

TBR=zhongyi@chromium.org
TBR=mmenke@chromium.org

BUG=678768

Review-Url: https://codereview.chromium.org/2619583002
Cr-Commit-Position: refs/heads/master@{#442556}
(cherry picked from commit e368398a3a5357a8742f3829ef08cc45080fb808)
==========

to

==========
Clean up HttpStreamFactoryImpl::JobController when Impl::Jobs complete

Background: When a server advertises alternative services support,
JobController creates two Jobs (|main_job_| and |alternative_job_|).
The controller blocks |main_job_| when connecting the
|alternative_job_|. It resumes the |main_job_| when |alternative_job_|
fails or when the timeout happens. When both jobs are destroyed, the
JobController goes away.

Bug: We currently leak JobController in the situation where
|alternative_job_| succeeds and |main_job_| is blocked on alt job.
OnRequestComplete() will be called which then destroys the alt job but
not the main job. This causes MaybeNotifyFactoryOfCompletion() to not
clean up the controller.

Fix: This CL fixes this leak by resetting |main_job_| if it is blocked
(i.e. hasn't started connecting) when |alternative_job_| succeeds. This
CL adds a regression test for this.

Note that there is no leak when orphaned job completes, because
"job_bound_ && bound_job_ != job" will evaluate to true. This CL adds a
|!request_| check so it's obvious that we always clean up controller in
the orphaned case. This CL adds a test to confirm that we did clean up
controller when orphaned job completes (which passes with and without
the change).

TBR=zhongyi@chromium.org
TBR=mmenke@chromium.org

NOTRY=true
NOPRESUBMIT=true

BUG=678768

Review-Url: https://codereview.chromium.org/2619583002
Cr-Commit-Position: refs/heads/master@{#442556}
(cherry picked from commit e368398a3a5357a8742f3829ef08cc45080fb808)
==========

[xunjieli, 2017-01-11 16:06:11]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2017-01-11 16:06:33]

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-01-11 16:13:27]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484150771626250,
"parent_rev": "840b3068a69a1eea4c2e7d1e11f8fded6d3aac3d", "commit_rev":
"1c7375269bc6e2ef192b74dd1b17f7e4e8b5f63e"}

[commit-bot: I haz the power, 2017-01-11 16:13:59]

Description was changed from

==========
Clean up HttpStreamFactoryImpl::JobController when Impl::Jobs complete

Background: When a server advertises alternative services support,
JobController creates two Jobs (|main_job_| and |alternative_job_|).
The controller blocks |main_job_| when connecting the
|alternative_job_|. It resumes the |main_job_| when |alternative_job_|
fails or when the timeout happens. When both jobs are destroyed, the
JobController goes away.

Bug: We currently leak JobController in the situation where
|alternative_job_| succeeds and |main_job_| is blocked on alt job.
OnRequestComplete() will be called which then destroys the alt job but
not the main job. This causes MaybeNotifyFactoryOfCompletion() to not
clean up the controller.

Fix: This CL fixes this leak by resetting |main_job_| if it is blocked
(i.e. hasn't started connecting) when |alternative_job_| succeeds. This
CL adds a regression test for this.

Note that there is no leak when orphaned job completes, because
"job_bound_ && bound_job_ != job" will evaluate to true. This CL adds a
|!request_| check so it's obvious that we always clean up controller in
the orphaned case. This CL adds a test to confirm that we did clean up
controller when orphaned job completes (which passes with and without
the change).

TBR=zhongyi@chromium.org
TBR=mmenke@chromium.org

NOTRY=true
NOPRESUBMIT=true

BUG=678768

Review-Url: https://codereview.chromium.org/2619583002
Cr-Commit-Position: refs/heads/master@{#442556}
(cherry picked from commit e368398a3a5357a8742f3829ef08cc45080fb808)
==========

to

==========
Clean up HttpStreamFactoryImpl::JobController when Impl::Jobs complete

Background: When a server advertises alternative services support,
JobController creates two Jobs (|main_job_| and |alternative_job_|).
The controller blocks |main_job_| when connecting the
|alternative_job_|. It resumes the |main_job_| when |alternative_job_|
fails or when the timeout happens. When both jobs are destroyed, the
JobController goes away.

Bug: We currently leak JobController in the situation where
|alternative_job_| succeeds and |main_job_| is blocked on alt job.
OnRequestComplete() will be called which then destroys the alt job but
not the main job. This causes MaybeNotifyFactoryOfCompletion() to not
clean up the controller.

Fix: This CL fixes this leak by resetting |main_job_| if it is blocked
(i.e. hasn't started connecting) when |alternative_job_| succeeds. This
CL adds a regression test for this.

Note that there is no leak when orphaned job completes, because
"job_bound_ && bound_job_ != job" will evaluate to true. This CL adds a
|!request_| check so it's obvious that we always clean up controller in
the orphaned case. This CL adds a test to confirm that we did clean up
controller when orphaned job completes (which passes with and without
the change).

TBR=zhongyi@chromium.org
TBR=mmenke@chromium.org

NOTRY=true
NOPRESUBMIT=true

BUG=678768

Review-Url: https://codereview.chromium.org/2619583002
Cr-Commit-Position: refs/heads/master@{#442556}
(cherry picked from commit e368398a3a5357a8742f3829ef08cc45080fb808)

Review-Url: https://codereview.chromium.org/2622193003
Cr-Commit-Position: refs/branch-heads/2924@{#733}
Cr-Branched-From:
3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}
Committed:
https://chromium.googlesource.com/chromium/src/+/1c7375269bc6e2ef192b74dd1b17...
==========

[commit-bot: I haz the power, 2017-01-11 16:14:00]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/1c7375269bc6e2ef192b74dd1b17...