Unify and generalize thread static persistent finalization.

Unify and generalize thread static persistent finalization.

Make ThreadState's registerAsStaticReference() generally available,
and not specific to LSan-only registration of static persistents
that must be cleared prior to performing leak detection.

By doing so, it can be used to handle thread-local static persistents
also.

Upon a thread finalizing its ThreadState, these static persistents
will be cleared & released. In order to make that safe and not leave
dangling PersistentNode references around afterwards, generalize
the release mechanism to also clear the Persistent/PersistentCollectionBase
that registered the persistent node.

R=haraken
BUG=

Committed: https://crrev.com/7de35e892724a0307ddd6440c69a2223e0fd4755
Cr-Commit-Position: refs/heads/master@{#389448}

[sof, 2016-04-24 20:32:01]

Description was changed from

==========
Rework finalization of thread-local persistents on shutdown.

R=
BUG=
==========

to

==========
Unify and generalize thread static persistent finalization.

Make ThreadState's registerAsStaticReference() generally available,
and not specific to LSan-only registration of static persistents
that must be cleared prior to performing leak detection.

By doing so, it can be used to handle thread-local static persistents
also.

Upon a thread finalizing its ThreadState, these static persistents
will be cleared & released. In order to make that safe and not leave
dangling PersistentNode references around afterwards, generalize
the release mechanism to also clear the Persistent/PersistentCollectionBase
that registered the persistent node.

R=
BUG=
==========

[sof, 2016-04-24 21:19:10]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2016-04-24 21:19:11]

please take a look.

[haraken, 2016-04-25 08:39:22]

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

[haraken, 2016-04-25 08:39:23]

https://codereview.chromium.org/1919663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/1919663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Handle.h:554:
(reinterpret_cast<PersistentHeapCollectionBase<Collection>*>(self))->uninitialize();

Why does PersistentHeapCollectionBase need to call uninitialize() while
PersistentBase doesn't need to call it?

https://codereview.chromium.org/1919663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Handle.h:571: if (!m_persistentNode)

Just curious, why do we need this check?

https://codereview.chromium.org/1919663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1919663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:1341: // but no known
use for it.

Yeah, I prefer adding this restriction too. registerStaticPersistentNode should
be used for only "static" persistent nodes (i.e., never freed).

[sof, 2016-04-25 08:53:05]

https://codereview.chromium.org/1919663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/1919663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Handle.h:554:
(reinterpret_cast<PersistentHeapCollectionBase<Collection>*>(self))->uninitialize();
On 2016/04/25 08:39:23, haraken wrote:
> 
> Why does PersistentHeapCollectionBase need to call uninitialize() while
> PersistentBase doesn't need to call it?

It also needs to clear the connection; see
PersistentRegion::releasePersistentNode() calling clear() on those.

https://codereview.chromium.org/1919663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Handle.h:571: if (!m_persistentNode)
On 2016/04/25 08:39:23, haraken wrote:
> 
> Just curious, why do we need this check?

If you have a PersistentCollectionBase stuck in TLS (via ThreadSpecific<>),
it'll destruct the object. That musn't touch m_persistentNode.

https://codereview.chromium.org/1919663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1919663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:1341: // but no known
use for it.
On 2016/04/25 08:39:23, haraken wrote:
> 
> Yeah, I prefer adding this restriction too. registerStaticPersistentNode
should
> be used for only "static" persistent nodes (i.e., never freed).
> 

Agreed; a thread needing to free a persistent while running would be it, but I'd
like to see where & when that is needed first.

[haraken, 2016-04-25 09:05:54]

LGTM

[sof, 2016-04-25 09:08:36]

Description was changed from

==========
Unify and generalize thread static persistent finalization.

Make ThreadState's registerAsStaticReference() generally available,
and not specific to LSan-only registration of static persistents
that must be cleared prior to performing leak detection.

By doing so, it can be used to handle thread-local static persistents
also.

Upon a thread finalizing its ThreadState, these static persistents
will be cleared & released. In order to make that safe and not leave
dangling PersistentNode references around afterwards, generalize
the release mechanism to also clear the Persistent/PersistentCollectionBase
that registered the persistent node.

R=
BUG=
==========

to

==========
Unify and generalize thread static persistent finalization.

Make ThreadState's registerAsStaticReference() generally available,
and not specific to LSan-only registration of static persistents
that must be cleared prior to performing leak detection.

By doing so, it can be used to handle thread-local static persistents
also.

Upon a thread finalizing its ThreadState, these static persistents
will be cleared & released. In order to make that safe and not leave
dangling PersistentNode references around afterwards, generalize
the release mechanism to also clear the Persistent/PersistentCollectionBase
that registered the persistent node.

R=haraken
BUG=
==========

[sof, 2016-04-25 09:10:16]

linux_chromium_asan_rel_ng provides LSan coverage wrt static persistents for
unit tests, but also tested in a local build.

[sof, 2016-04-25 09:10:30]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2016-04-25 09:10:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1919663002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1919663002/20001

[commit-bot: I haz the power, 2016-04-25 09:16:16]

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

[commit-bot: I haz the power, 2016-04-25 09:16:17]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sof, 2016-04-25 09:53:00]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2016-04-25 09:53:00]

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

[commit-bot: I haz the power, 2016-04-25 09:53:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1919663002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1919663002/40001

[sof, 2016-04-25 09:58:39]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2016-04-25 09:58:39]

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

[commit-bot: I haz the power, 2016-04-25 09:58:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1919663002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1919663002/60001

[commit-bot: I haz the power, 2016-04-25 11:13:22]

Description was changed from

==========
Unify and generalize thread static persistent finalization.

Make ThreadState's registerAsStaticReference() generally available,
and not specific to LSan-only registration of static persistents
that must be cleared prior to performing leak detection.

By doing so, it can be used to handle thread-local static persistents
also.

Upon a thread finalizing its ThreadState, these static persistents
will be cleared & released. In order to make that safe and not leave
dangling PersistentNode references around afterwards, generalize
the release mechanism to also clear the Persistent/PersistentCollectionBase
that registered the persistent node.

R=haraken
BUG=
==========

to

==========
Unify and generalize thread static persistent finalization.

Make ThreadState's registerAsStaticReference() generally available,
and not specific to LSan-only registration of static persistents
that must be cleared prior to performing leak detection.

By doing so, it can be used to handle thread-local static persistents
also.

Upon a thread finalizing its ThreadState, these static persistents
will be cleared & released. In order to make that safe and not leave
dangling PersistentNode references around afterwards, generalize
the release mechanism to also clear the Persistent/PersistentCollectionBase
that registered the persistent node.

R=haraken
BUG=
==========

[commit-bot: I haz the power, 2016-04-25 11:13:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-25 11:14:24]

Description was changed from

==========
Unify and generalize thread static persistent finalization.

Make ThreadState's registerAsStaticReference() generally available,
and not specific to LSan-only registration of static persistents
that must be cleared prior to performing leak detection.

By doing so, it can be used to handle thread-local static persistents
also.

Upon a thread finalizing its ThreadState, these static persistents
will be cleared & released. In order to make that safe and not leave
dangling PersistentNode references around afterwards, generalize
the release mechanism to also clear the Persistent/PersistentCollectionBase
that registered the persistent node.

R=haraken
BUG=
==========

to

==========
Unify and generalize thread static persistent finalization.

Make ThreadState's registerAsStaticReference() generally available,
and not specific to LSan-only registration of static persistents
that must be cleared prior to performing leak detection.

By doing so, it can be used to handle thread-local static persistents
also.

Upon a thread finalizing its ThreadState, these static persistents
will be cleared & released. In order to make that safe and not leave
dangling PersistentNode references around afterwards, generalize
the release mechanism to also clear the Persistent/PersistentCollectionBase
that registered the persistent node.

R=haraken
BUG=

Committed: https://crrev.com/7de35e892724a0307ddd6440c69a2223e0fd4755
Cr-Commit-Position: refs/heads/master@{#389448}
==========

[commit-bot: I haz the power, 2016-04-25 11:14:25]

Patchset 4 (id:??) landed as
https://crrev.com/7de35e892724a0307ddd6440c69a2223e0fd4755
Cr-Commit-Position: refs/heads/master@{#389448}