Oilpan: catch some self-referential leaks (main thread.)

Oilpan: catch some self-referential leaks (main thread.)

Track the number of SelfKeepAlive<>s that are currently active on
the main thread, so as to be able to detect if any remains on
shutdown. A clear indication of a leak, if so.

Also add some debugging support for detecting thread shutdown
leaks due to Persistent<>s not being released; see
PersistentRegion::dumpLivePersistents() and ThreadState::cleanup().

R=haraken
BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200174

[sof, 2015-08-06 09:40:49]

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

[sof, 2015-08-06 09:40:50]

please take a look.

[haraken, 2015-08-06 10:30:58]

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

[haraken, 2015-08-06 10:30:59]

https://codereview.chromium.org/1275863002/diff/1/Source/platform/heap/Thread...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1275863002/diff/1/Source/platform/heap/Thread...
Source/platform/heap/ThreadState.cpp:1551: if
(!ThreadState::current()->isMainThread())

Is there any reason we want to limit the check only to the main thread? (I agree
that other threads will hit ASSERT(persistentCount == 0) when they get detached
but it would be better to have more specific checks to help identify a problem.)

[sof, 2015-08-06 10:40:29]

https://codereview.chromium.org/1275863002/diff/1/Source/platform/heap/Thread...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1275863002/diff/1/Source/platform/heap/Thread...
Source/platform/heap/ThreadState.cpp:1551: if
(!ThreadState::current()->isMainThread())
On 2015/08/06 10:30:59, haraken wrote:
> 
> Is there any reason we want to limit the check only to the main thread? (I
agree
> that other threads will hit ASSERT(persistentCount == 0) when they get
detached
> but it would be better to have more specific checks to help identify a
problem.)

The reason is that there's protection for thread shutdown wrt thread-local
Persistent<>s.

(I don't think we have non-main thread SelfKeepAlive<>s uses atm.)

[haraken, 2015-08-06 10:42:08]

ok, LGTM.

https://codereview.chromium.org/1275863002/diff/1/Source/platform/heap/Thread...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1275863002/diff/1/Source/platform/heap/Thread...
Source/platform/heap/ThreadState.cpp:1551: if
(!ThreadState::current()->isMainThread())
On 2015/08/06 10:40:29, sof wrote:
> On 2015/08/06 10:30:59, haraken wrote:
> > 
> > Is there any reason we want to limit the check only to the main thread? (I
> agree
> > that other threads will hit ASSERT(persistentCount == 0) when they get
> detached
> > but it would be better to have more specific checks to help identify a
> problem.)
> 
> The reason is that there's protection for thread shutdown wrt thread-local
> Persistent<>s.
> 
> (I don't think we have non-main thread SelfKeepAlive<>s uses atm.)

You could just add ThreadState::m_selfKeepAliveCount and assert the zero-ness in
cleanup() without having any isMainThread() checks.

[sof, 2015-08-06 10:59:01]

https://codereview.chromium.org/1275863002/diff/1/Source/platform/heap/Thread...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1275863002/diff/1/Source/platform/heap/Thread...
Source/platform/heap/ThreadState.cpp:1551: if
(!ThreadState::current()->isMainThread())
On 2015/08/06 10:42:08, haraken wrote:
> On 2015/08/06 10:40:29, sof wrote:
> > On 2015/08/06 10:30:59, haraken wrote:
> > > 
> > > Is there any reason we want to limit the check only to the main thread? (I
> > agree
> > > that other threads will hit ASSERT(persistentCount == 0) when they get
> > detached
> > > but it would be better to have more specific checks to help identify a
> > problem.)
> > 
> > The reason is that there's protection for thread shutdown wrt thread-local
> > Persistent<>s.
> > 
> > (I don't think we have non-main thread SelfKeepAlive<>s uses atm.)
> 
> You could just add ThreadState::m_selfKeepAliveCount and assert the zero-ness
in
> cleanup() without having any isMainThread() checks.

You certainly could, but I wonder if we should try to improve the debuggability
of the current assert (ASSERT(!currentCount)) instead.

[haraken, 2015-08-06 11:01:19]

https://codereview.chromium.org/1275863002/diff/1/Source/platform/heap/Thread...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1275863002/diff/1/Source/platform/heap/Thread...
Source/platform/heap/ThreadState.cpp:1551: if
(!ThreadState::current()->isMainThread())
On 2015/08/06 10:59:01, sof wrote:
> On 2015/08/06 10:42:08, haraken wrote:
> > On 2015/08/06 10:40:29, sof wrote:
> > > On 2015/08/06 10:30:59, haraken wrote:
> > > > 
> > > > Is there any reason we want to limit the check only to the main thread?
(I
> > > agree
> > > > that other threads will hit ASSERT(persistentCount == 0) when they get
> > > detached
> > > > but it would be better to have more specific checks to help identify a
> > > problem.)
> > > 
> > > The reason is that there's protection for thread shutdown wrt thread-local
> > > Persistent<>s.
> > > 
> > > (I don't think we have non-main thread SelfKeepAlive<>s uses atm.)
> > 
> > You could just add ThreadState::m_selfKeepAliveCount and assert the
zero-ness
> in
> > cleanup() without having any isMainThread() checks.
> 
> You certainly could, but I wonder if we should try to improve the
debuggability
> of the current assert (ASSERT(!currentCount)) instead.

I don't have a strong opinion. I don't think it's a worth but there would be no
reason to limit the check to the main thread :)

[sof, 2015-08-06 12:21:15]

https://codereview.chromium.org/1275863002/diff/1/Source/platform/heap/Thread...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1275863002/diff/1/Source/platform/heap/Thread...
Source/platform/heap/ThreadState.cpp:1551: if
(!ThreadState::current()->isMainThread())
On 2015/08/06 11:01:19, haraken wrote:
> On 2015/08/06 10:59:01, sof wrote:
> > On 2015/08/06 10:42:08, haraken wrote:
> > > On 2015/08/06 10:40:29, sof wrote:
> > > > On 2015/08/06 10:30:59, haraken wrote:
> > > > > 
> > > > > Is there any reason we want to limit the check only to the main
thread?
> (I
> > > > agree
> > > > > that other threads will hit ASSERT(persistentCount == 0) when they get
> > > > detached
> > > > > but it would be better to have more specific checks to help identify a
> > > > problem.)
> > > > 
> > > > The reason is that there's protection for thread shutdown wrt
thread-local
> > > > Persistent<>s.
> > > > 
> > > > (I don't think we have non-main thread SelfKeepAlive<>s uses atm.)
> > > 
> > > You could just add ThreadState::m_selfKeepAliveCount and assert the
> zero-ness
> > in
> > > cleanup() without having any isMainThread() checks.
> > 
> > You certainly could, but I wonder if we should try to improve the
> debuggability
> > of the current assert (ASSERT(!currentCount)) instead.
> 
> I don't have a strong opinion. I don't think it's a worth but there would be
no
> reason to limit the check to the main thread :)

I've added a debugger-friendly entry point to PersistentRegion to dump the live
persistents - worth having?

[haraken, 2015-08-06 13:49:32]

On 2015/08/06 12:21:15, sof wrote:
>
https://codereview.chromium.org/1275863002/diff/1/Source/platform/heap/Thread...
> File Source/platform/heap/ThreadState.cpp (right):
> 
>
https://codereview.chromium.org/1275863002/diff/1/Source/platform/heap/Thread...
> Source/platform/heap/ThreadState.cpp:1551: if
> (!ThreadState::current()->isMainThread())
> On 2015/08/06 11:01:19, haraken wrote:
> > On 2015/08/06 10:59:01, sof wrote:
> > > On 2015/08/06 10:42:08, haraken wrote:
> > > > On 2015/08/06 10:40:29, sof wrote:
> > > > > On 2015/08/06 10:30:59, haraken wrote:
> > > > > > 
> > > > > > Is there any reason we want to limit the check only to the main
> thread?
> > (I
> > > > > agree
> > > > > > that other threads will hit ASSERT(persistentCount == 0) when they
get
> > > > > detached
> > > > > > but it would be better to have more specific checks to help identify
a
> > > > > problem.)
> > > > > 
> > > > > The reason is that there's protection for thread shutdown wrt
> thread-local
> > > > > Persistent<>s.
> > > > > 
> > > > > (I don't think we have non-main thread SelfKeepAlive<>s uses atm.)
> > > > 
> > > > You could just add ThreadState::m_selfKeepAliveCount and assert the
> > zero-ness
> > > in
> > > > cleanup() without having any isMainThread() checks.
> > > 
> > > You certainly could, but I wonder if we should try to improve the
> > debuggability
> > > of the current assert (ASSERT(!currentCount)) instead.
> > 
> > I don't have a strong opinion. I don't think it's a worth but there would be
> no
> > reason to limit the check to the main thread :)
> 
> I've added a debugger-friendly entry point to PersistentRegion to dump the
live
> persistents - worth having?

Looks nicer!

[sof, 2015-08-06 21:05:48]

On 2015/08/06 13:49:32, haraken wrote:
> On 2015/08/06 12:21:15, sof wrote:
> >
>
https://codereview.chromium.org/1275863002/diff/1/Source/platform/heap/Thread...
> > File Source/platform/heap/ThreadState.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1275863002/diff/1/Source/platform/heap/Thread...
> > Source/platform/heap/ThreadState.cpp:1551: if
> > (!ThreadState::current()->isMainThread())
> > On 2015/08/06 11:01:19, haraken wrote:
> > > On 2015/08/06 10:59:01, sof wrote:
> > > > On 2015/08/06 10:42:08, haraken wrote:
> > > > > On 2015/08/06 10:40:29, sof wrote:
> > > > > > On 2015/08/06 10:30:59, haraken wrote:
> > > > > > > 
> > > > > > > Is there any reason we want to limit the check only to the main
> > thread?
> > > (I
> > > > > > agree
> > > > > > > that other threads will hit ASSERT(persistentCount == 0) when they
> get
> > > > > > detached
> > > > > > > but it would be better to have more specific checks to help
identify
> a
> > > > > > problem.)
> > > > > > 
> > > > > > The reason is that there's protection for thread shutdown wrt
> > thread-local
> > > > > > Persistent<>s.
> > > > > > 
> > > > > > (I don't think we have non-main thread SelfKeepAlive<>s uses atm.)
> > > > > 
> > > > > You could just add ThreadState::m_selfKeepAliveCount and assert the
> > > zero-ness
> > > > in
> > > > > cleanup() without having any isMainThread() checks.
> > > > 
> > > > You certainly could, but I wonder if we should try to improve the
> > > debuggability
> > > > of the current assert (ASSERT(!currentCount)) instead.
> > > 
> > > I don't have a strong opinion. I don't think it's a worth but there would
be
> > no
> > > reason to limit the check to the main thread :)
> > 
> > I've added a debugger-friendly entry point to PersistentRegion to dump the
> live
> > persistents - worth having?
> 
> Looks nicer!

Expanded the dumped output to include the trace callback also; that should make
it possible to derive the type of the Persistent<> leaking (from within a
debugger.)

With that in place, it provides something a bit more general over special-casing
SelfKeepAlive<>s.

[haraken, 2015-08-06 23:15:46]

Still LGTM

[sof, 2015-08-07 12:27:43]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-08-07 12:27:45]

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/1275863002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2015-08-07 12:27:53]

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

[commit-bot: I haz the power, 2015-08-07 13:14:33]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200163

[hiroshige, 2015-08-07 14:23:00]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1280543004/ by hiroshige@chromium.org.

The reason for reverting is: Breaking compilation on chromium trybots:
  error: use of undeclared identifier 'stderr'
  https://codereview.chromium.org/1277473003/
.

[sof, 2015-08-07 15:07:22]

added missing #include + confirmed that this fixes compilation on the failing
linux_chromium_asan_rel_ng bot. Relanding.

[sof, 2015-08-07 15:07:29]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-08-07 15:07:29]

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/1275863002/#ps140001
(title: "add stdio.h include")

[commit-bot: I haz the power, 2015-08-07 15:07:35]

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

[commit-bot: I haz the power, 2015-08-07 15:31:31]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200174

[Justin Novosad, 2015-08-07 21:00:59]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1272573008/ by junov@chromium.org.

The reason for reverting is: Multiple failures on Win Oilpan Debug bot:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Oilpan%20....