Avoid dereferencing nullptr during ProfileIOData destruction

Avoid dereferencing nullptr during ProfileIOData destruction

Do not de-reference the cert_transparency_verifier_ field during destruction
if it was not initialized, as that would lead to a crash.

The original assumption was that ProfileIOData::Init() is always called
so the cert_transparency_verifier_ is always initialized. However that
assumption proved to be wrong - ProfileIOData::Init is only called once
a ChromeURLRequestContextFactory is created with the ProfileIOData instance
and a URLRequestContext is created using the Create method (see:
https://cs.chromium.org/chromium/src/chrome/browser/net/chrome_url_request_context_getter.cc?sq=package:chromium&dr=C&rcl=1474946434&l=55)
If that did not happen and ProfileIOData::Init was not called, de-referencing
cert_transparency_verifier_ during destruction would lead to a crash.

BUG=648507
Committed: https://crrev.com/13d5499a28455449c3a108a05d695ecae8d881e0
Cr-Commit-Position: refs/heads/master@{#421304}

[Eran Messeri, 2016-09-27 11:17:47]

Description was changed from

==========
Avoid dereferencing nullptr during ProfileIOData destruction

Do not de-reference the cert_transparency_verifier_ field during destruction
if it was not initialized, as that would lead to a crash.

The original assumption was that ProfileIOData::Init() is always called
so the cert_transparency_verifier_ is always initialized. However that
assumption proved to be wrong - ProfileIOData::Init is only called once
a ChromeURLRequestContextFactory is created with the ProfileIOData instance
and a URLRequestContext is created using the Create method (see:
https://cs.chromium.org/chromium/src/chrome/browser/net/chrome_url_request_co...)
If that did not happen and ProfileIOData::Init was not called, de-referencing
cert_transparency_verifier_ during destruction would lead to a crash.

BUG=648507
==========

to

==========
Avoid dereferencing nullptr during ProfileIOData destruction

Do not de-reference the cert_transparency_verifier_ field during destruction
if it was not initialized, as that would lead to a crash.

The original assumption was that ProfileIOData::Init() is always called
so the cert_transparency_verifier_ is always initialized. However that
assumption proved to be wrong - ProfileIOData::Init is only called once
a ChromeURLRequestContextFactory is created with the ProfileIOData instance
and a URLRequestContext is created using the Create method (see:
https://cs.chromium.org/chromium/src/chrome/browser/net/chrome_url_request_co...)
If that did not happen and ProfileIOData::Init was not called, de-referencing
cert_transparency_verifier_ during destruction would lead to a crash.

BUG=648507
==========

[Eran Messeri, 2016-09-27 11:17:47]

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

[Eran Messeri, 2016-09-27 11:18:33]

Matt, please review this small fix to a release-blocking crash bug.

[Eran Messeri, 2016-09-27 12:31:09]

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

[commit-bot: I haz the power, 2016-09-27 12:31:46]

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, 2016-09-27 13:16:50]

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

[commit-bot: I haz the power, 2016-09-27 13:16:50]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-09-27 15:09:59]

LGTM.  I do wonder if this codepath is really worth keeping, it's caused similar
issues before, and destroying a profile without making a single URLRequest is a
bit of a niche use case.

[Eran Messeri, 2016-09-27 19:42:41]

On 2016/09/27 15:09:59, mmenke wrote:
> LGTM.  I do wonder if this codepath is really worth keeping, it's caused
similar
> issues before, and destroying a profile without making a single URLRequest is
a
> bit of a niche use case.

Are you suggesting we don't need to worry about URLRequests being made during
destruction? What about URLRequests that are in progress?
What I'm aiming to prevent here is a URLRequest that ends up validating a
certificate and notifying the observer set on the cert_transparency_verifier_
during destruction, which may not be valid anymore.
IIRC this was a concern Ryan Sleevi raised - Ryan, do you see a reason to keep
this code?

For now I'll land the fix to un-block the issue, then we discuss it's necessity.
WDYT?

[Eran Messeri, 2016-09-27 19:42:48]

The CQ bit was checked by eranm@chromium.org

[commit-bot: I haz the power, 2016-09-27 19:43:17]

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

[mmenke, 2016-09-27 19:45:54]

On 2016/09/27 19:42:41, Eran Messeri wrote:
> On 2016/09/27 15:09:59, mmenke wrote:
> > LGTM.  I do wonder if this codepath is really worth keeping, it's caused
> similar
> > issues before, and destroying a profile without making a single URLRequest
is
> a
> > bit of a niche use case.
> 
> Are you suggesting we don't need to worry about URLRequests being made during
> destruction? What about URLRequests that are in progress?
> What I'm aiming to prevent here is a URLRequest that ends up validating a
> certificate and notifying the observer set on the cert_transparency_verifier_
> during destruction, which may not be valid anymore.
> IIRC this was a concern Ryan Sleevi raised - Ryan, do you see a reason to keep
> this code?

I'm not sure where you're getting that from?  I mean the code path where we
haven't created or initialized the URLRequestContext...  i.e., the path that
this CL fixes.

> For now I'll land the fix to un-block the issue, then we discuss it's
necessity.
> WDYT?

[commit-bot: I haz the power, 2016-09-27 19:49:30]

Description was changed from

==========
Avoid dereferencing nullptr during ProfileIOData destruction

Do not de-reference the cert_transparency_verifier_ field during destruction
if it was not initialized, as that would lead to a crash.

The original assumption was that ProfileIOData::Init() is always called
so the cert_transparency_verifier_ is always initialized. However that
assumption proved to be wrong - ProfileIOData::Init is only called once
a ChromeURLRequestContextFactory is created with the ProfileIOData instance
and a URLRequestContext is created using the Create method (see:
https://cs.chromium.org/chromium/src/chrome/browser/net/chrome_url_request_co...)
If that did not happen and ProfileIOData::Init was not called, de-referencing
cert_transparency_verifier_ during destruction would lead to a crash.

BUG=648507
==========

to

==========
Avoid dereferencing nullptr during ProfileIOData destruction

Do not de-reference the cert_transparency_verifier_ field during destruction
if it was not initialized, as that would lead to a crash.

The original assumption was that ProfileIOData::Init() is always called
so the cert_transparency_verifier_ is always initialized. However that
assumption proved to be wrong - ProfileIOData::Init is only called once
a ChromeURLRequestContextFactory is created with the ProfileIOData instance
and a URLRequestContext is created using the Create method (see:
https://cs.chromium.org/chromium/src/chrome/browser/net/chrome_url_request_co...)
If that did not happen and ProfileIOData::Init was not called, de-referencing
cert_transparency_verifier_ during destruction would lead to a crash.

BUG=648507
==========

[commit-bot: I haz the power, 2016-09-27 19:49:31]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-27 19:52:10]

Description was changed from

==========
Avoid dereferencing nullptr during ProfileIOData destruction

Do not de-reference the cert_transparency_verifier_ field during destruction
if it was not initialized, as that would lead to a crash.

The original assumption was that ProfileIOData::Init() is always called
so the cert_transparency_verifier_ is always initialized. However that
assumption proved to be wrong - ProfileIOData::Init is only called once
a ChromeURLRequestContextFactory is created with the ProfileIOData instance
and a URLRequestContext is created using the Create method (see:
https://cs.chromium.org/chromium/src/chrome/browser/net/chrome_url_request_co...)
If that did not happen and ProfileIOData::Init was not called, de-referencing
cert_transparency_verifier_ during destruction would lead to a crash.

BUG=648507
==========

to

==========
Avoid dereferencing nullptr during ProfileIOData destruction

Do not de-reference the cert_transparency_verifier_ field during destruction
if it was not initialized, as that would lead to a crash.

The original assumption was that ProfileIOData::Init() is always called
so the cert_transparency_verifier_ is always initialized. However that
assumption proved to be wrong - ProfileIOData::Init is only called once
a ChromeURLRequestContextFactory is created with the ProfileIOData instance
and a URLRequestContext is created using the Create method (see:
https://cs.chromium.org/chromium/src/chrome/browser/net/chrome_url_request_co...)
If that did not happen and ProfileIOData::Init was not called, de-referencing
cert_transparency_verifier_ during destruction would lead to a crash.

BUG=648507

Committed: https://crrev.com/13d5499a28455449c3a108a05d695ecae8d881e0
Cr-Commit-Position: refs/heads/master@{#421304}
==========

[commit-bot: I haz the power, 2016-09-27 19:52:11]

Patchset 1 (id:??) landed as
https://crrev.com/13d5499a28455449c3a108a05d695ecae8d881e0
Cr-Commit-Position: refs/heads/master@{#421304}