Temporarily CHECK use after free in Value

Temporarily CHECK use after free in Value

One of the hypotheses for https://crbug.com/697817 is that a Value is used
after free (see #5 there). To verify/refute that hypothesis, this CL
temporarily introduces and CHECKS an |alive_| flag. This change will be
reverted after a couple of days in Canary, and only serves investigation
purposes.

BUG=697817
Review-Url: https://codereview.chromium.org/2743363004
Cr-Commit-Position: refs/heads/master@{#456801}
Committed: https://chromium.googlesource.com/chromium/src/+/e2477101857d4c38dd2eb7fb67312b7fd0022f5e

[vabr (Chromium), 2017-03-14 12:31:54]

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

[commit-bot: I haz the power, 2017-03-14 12:32:16]

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-03-14 14:07:36]

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

[commit-bot: I haz the power, 2017-03-14 14:07:36]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2017-03-14 14:45:45]

vabr@chromium.org changed reviewers:
+ brettw@chromium.org, jdoerrie@chromium.org

[vabr (Chromium), 2017-03-14 14:45:46]

Hi Brett and Jan.

What do you think about these temporary checks to investigate the crash?

Adding the dedicated |alive_| flag seems heavy-handed, but I did not find good
alternatives allowing me to check the liveness. In particular, squashing it into
|type_| would be a nightmare through many dependencies on the Type enum
(https://codereview.chromium.org/2750533003/).

While inflating the size of Value might impact memory consumption, I intend to
only leave it in for a couple of days, so nothing but a few Canaries and one Dev
build should be affected.

Cheers,
Vaclav

[jdoerrie, 2017-03-14 15:31:56]

On 2017/03/14 14:45:46, vabr (Chromium) wrote:
> Hi Brett and Jan.
> 
> What do you think about these temporary checks to investigate the crash?
> 
> Adding the dedicated |alive_| flag seems heavy-handed, but I did not find good
> alternatives allowing me to check the liveness. In particular, squashing it
into
> |type_| would be a nightmare through many dependencies on the Type enum
> (https://codereview.chromium.org/2750533003/).
> 
> While inflating the size of Value might impact memory consumption, I intend to
> only leave it in for a couple of days, so nothing but a few Canaries and one
Dev
> build should be affected.
> 
> Cheers,
> Vaclav

LGTM. I discussed a quick idea with Vaclav where I thought we could simply set
the type to NONE after destruction and assert that the type is never NONE when
entering |InternalCleanup|. However, on further thought it turns out this will
lead to many wrong positives, as there are legitimate use cases to have a NONE
value. Seeing how complicated adding a DELETED type would be, I am fine with
temporarily adding this simple flag.

[brettw, 2017-03-14 18:09:45]

lgtm

[vabr (Chromium), 2017-03-14 19:56:45]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2017-03-14 19:57: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-03-14 20:06:15]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1489521405293890, "parent_rev":
"e75f1381dccdeef00b30479ad55432f60ee8db7d", "commit_rev":
"e2477101857d4c38dd2eb7fb67312b7fd0022f5e"}

[commit-bot: I haz the power, 2017-03-14 20:06:56]

Description was changed from

==========
Temporarily CHECK use after free in Value

One of the hypotheses for https://crbug.com/697817 is that a Value is used
after free (see #5 there). To verify/refute that hypothesis, this CL
temporarily introduces and CHECKS an |alive_| flag. This change will be
reverted after a couple of days in Canary, and only serves investigation
purposes.

BUG=697817
==========

to

==========
Temporarily CHECK use after free in Value

One of the hypotheses for https://crbug.com/697817 is that a Value is used
after free (see #5 there). To verify/refute that hypothesis, this CL
temporarily introduces and CHECKS an |alive_| flag. This change will be
reverted after a couple of days in Canary, and only serves investigation
purposes.

BUG=697817

Review-Url: https://codereview.chromium.org/2743363004
Cr-Commit-Position: refs/heads/master@{#456801}
Committed:
https://chromium.googlesource.com/chromium/src/+/e2477101857d4c38dd2eb7fb6731...
==========

[commit-bot: I haz the power, 2017-03-14 20:06:57]

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

[vabr (Chromium), 2017-03-20 20:12:06]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2761773005/ by vabr@chromium.org.

The reason for reverting is: Removing the temporary CHECK. As explained in
https://crbug.com/697817#c13, it ended up inconclusive, and it also does not
protect completely against the use of corrupted memory..