Oilpan: GC_TRACING: Improve object path dump

Oilpan: GC_TRACING: Improve object path dump

We frequently saw object path dump like:

    Path to 1d6ec5d8 of WebCore::Node
    <- 7abc4660 of Persistent

Then, we needed to investigate what this Persistent is. It took much time.

This CL introduces 'tracing name' property to Persistent if
ENABLE(GC_TRACING). The property is automatically filled with
backtrace of a Persistent constructor. We'll get dump like:

    Path to 1bd56220 of WebCore::Node
    <- 1bd55ff8 of WebCore::Node
    <- 1bd55dc8 of WebCore::Node
    <- 7d046600 of Persistent ... Backtrace:
            Persistent<HTMLSelectElement, ThreadLocalPersistents<(ThreadAffinity)1> >::Persistent(HTMLSelectElement*)
            v8::Handle<v8::Object> V8DOMWrapper::associateObjectWithWrapper<V8HTMLSelectElement, HTMLSelectElement>(HTMLSelectElement*, WrapperTypeInfo const*, v8::Handle<v8::Object>, v8::Isolate*, WrapperConfiguration::Lifetime)
            v8::Handle<v8::Object> V8DOMWrapper::associateObjectWithWrapper<V8HTMLSelectElement, HTMLSelectElement>(WTF::RawPtr<HTMLSelectElement>, WrapperTypeInfo const*, v8::Handle<v8::Object>, v8::Isolate*, WrapperConfiguration::Lifetime)
            V8HTMLSelectElement::createWrapper(WTF::RawPtr<HTMLSelectElement>, v8::Handle<v8::Object>, v8::Isolate*)

Additional improvement of the object path dump:
* Correct host info name for heap collections
* Don't try to dump unknown objects.

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176185

[tkent, 2014-06-13 07:49:40]

Please take a look.

[kouhei (in TOK), 2014-06-13 08:01:23]

On 2014/06/13 07:49:40, tkent wrote:
> Please take a look.

lgtm

[Mads Ager (chromium), 2014-06-13 08:16:33]

Another way of doing this is to print a stack trace on all persistent
allocations. I have been doing that locally. That way we don't have to polute
the code base with this.

Chromium-side patch (linux version):

diff --git a/base/debug/stack_trace_posix.cc b/base/debug/stack_trace_posix.cc
index 261ce95..af842a6 100644
--- a/base/debug/stack_trace_posix.cc
+++ b/base/debug/stack_trace_posix.cc
@@ -16,6 +16,7 @@
 
 #include <map>
 #include <ostream>
+#include <iostream>
 #include <string>
 #include <vector>
 
@@ -45,6 +46,10 @@
 #include "base/third_party/symbolize/symbolize.h"
 #endif
 
+extern "C" void Base_Debug_BackTrace() {
+    base::debug::StackTrace().OutputToStream(&std::cerr);
+}
+
 namespace base {
 namespace debug {


Blink-side patch:

diff --git a/Source/platform/heap/Handle.h b/Source/platform/heap/Handle.h
index 6de5dbc..ecf0819 100644
--- a/Source/platform/heap/Handle.h
+++ b/Source/platform/heap/Handle.h
@@ -201,6 +201,7 @@ protected:
         m_next = m_prev->m_next;
         m_prev->m_next = this;
         m_next->m_prev = this;
+        Heap::printStackTrace(this);
     }
 
     inline explicit PersistentBase(const PersistentBase& otherref)
diff --git a/Source/platform/heap/Heap.cpp b/Source/platform/heap/Heap.cpp
index 5d6ff06..3dc2b39 100644
--- a/Source/platform/heap/Heap.cpp
+++ b/Source/platform/heap/Heap.cpp
@@ -31,6 +31,7 @@
 #include "config.h"
 #include "platform/heap/Heap.h"
 
+#include <execinfo.h>
 #include "platform/TraceEvent.h"
 #include "platform/heap/ThreadState.h"
 #include "public/platform/Platform.h"
@@ -1314,6 +1315,15 @@ bool CallbackStack::popAndInvokeCallback(CallbackStack**
first, Visitor* visitor
     return true;
 }
 
+extern "C" void Base_Debug_BackTrace();
+
+void Heap::printStackTrace(void* pointer)
+{
+    fprintf(stderr, "\nStackTrace (%p): \n", pointer);
+    Base_Debug_BackTrace();
+    printf("\n\n");
+}
+
 class MarkingVisitor : public Visitor {
 public:
 #if ENABLE(GC_TRACING)
@@ -1338,7 +1348,7 @@ public:
         }
         ObjectGraph::AddResult result =
objectGraph().add(reinterpret_cast<uintptr_t>(objectPointer),
std::make_pair(reinterpret_cast<uintptr_t>(m_hostObject), m_hostName));
         ASSERT(result.isNewEntry);
-        // fprintf(stderr, "%s[%p] -> %s[%p]\n", m_hostName.ascii().data(),
m_hostObject, className.ascii().data(), objectPointer);
+        fprintf(stderr, "%s[%p] -> %s[%p]\n", m_hostName.ascii().data(),
m_hostObject, className.ascii().data(), objectPointer);
 #endif
         if (callback)
             Heap::pushTraceCallback(const_cast<void*>(objectPointer),
callback);
diff --git a/Source/platform/heap/Heap.h b/Source/platform/heap/Heap.h
index c33cb75..eccd7cc 100644
--- a/Source/platform/heap/Heap.h
+++ b/Source/platform/heap/Heap.h
@@ -883,6 +883,9 @@ private:
 
 class PLATFORM_EXPORT Heap {
 public:
+
+    static void printStackTrace(void*);
+
     static void init();
     static void shutdown();
     static void doShutdown();

[tkent, 2014-06-13 08:19:40]

Yeah, I used to add WTFReportBacktrace() to all Persistent constructors locally.
But this way is not smart :)

[Mads Ager (chromium), 2014-06-13 08:22:29]

On 2014/06/13 08:19:40, tkent wrote:
> Yeah, I used to add WTFReportBacktrace() to all Persistent constructors
locally.
> But this way is not smart :)

The good think with doing the backtrace is that you get all persistents. This
patch only annotate a small subset of the persistents and it would not have
given me any information in connection with the leaks that I fixed yesterday. In
order for this patch to really work out we have to add this annotation to all
persistents and we didn't want to do that (Slava did that on the branch and it
got pretty ugly). Can we keep these patches locally and not commit them so we
don't have to polute the code base with persistent annotations?

[tkent, 2014-06-13 08:28:55]

On 2014/06/13 08:22:29, Mads Ager (chromium) wrote:
> On 2014/06/13 08:19:40, tkent wrote:
> > Yeah, I used to add WTFReportBacktrace() to all Persistent constructors
> locally.
> > But this way is not smart :)
> 
> The good think with doing the backtrace is that you get all persistents. This
> patch only annotate a small subset of the persistents and it would not have
> given me any information in connection with the leaks that I fixed yesterday.
In
> order for this patch to really work out we have to add this annotation to all
> persistents and we didn't want to do that (Slava did that on the branch and it
> got pretty ugly). Can we keep these patches locally and not commit them so we
> don't have to polute the code base with persistent annotations?

I also think annotating all of Persistents is not practical.  How about landing
the platform/heap/ part of Patch Set 1?

[Mads Ager (chromium), 2014-06-13 08:39:44]

On 2014/06/13 08:28:55, tkent wrote:
> On 2014/06/13 08:22:29, Mads Ager (chromium) wrote:
> > On 2014/06/13 08:19:40, tkent wrote:
> > > Yeah, I used to add WTFReportBacktrace() to all Persistent constructors
> > locally.
> > > But this way is not smart :)
> > 
> > The good think with doing the backtrace is that you get all persistents.
This
> > patch only annotate a small subset of the persistents and it would not have
> > given me any information in connection with the leaks that I fixed
yesterday.
> In
> > order for this patch to really work out we have to add this annotation to
all
> > persistents and we didn't want to do that (Slava did that on the branch and
it
> > got pretty ugly). Can we keep these patches locally and not commit them so
we
> > don't have to polute the code base with persistent annotations?
> 
> I also think annotating all of Persistents is not practical.  How about
landing
> the platform/heap/ part of Patch Set 1?

I had this locally as well and decided to do the stack tracing instead because I
couldn't find the right persistent to annotate. With this you still have to do
manual annotation and search to find the persistent to annotate. Therefore, I'm
not entirely convinced that having this code without having all persistents
annotated is that useful. However, if you found this useful I think it would be
OK to land the platform/heap part of this. That limits the ugliness to our code
and it will be easy to pull out again.

[haraken, 2014-06-13 09:00:22]

On 2014/06/13 08:39:44, Mads Ager (chromium) wrote:
> On 2014/06/13 08:28:55, tkent wrote:
> > On 2014/06/13 08:22:29, Mads Ager (chromium) wrote:
> > > On 2014/06/13 08:19:40, tkent wrote:
> > > > Yeah, I used to add WTFReportBacktrace() to all Persistent constructors
> > > locally.
> > > > But this way is not smart :)
> > > 
> > > The good think with doing the backtrace is that you get all persistents.
> This
> > > patch only annotate a small subset of the persistents and it would not
have
> > > given me any information in connection with the leaks that I fixed
> yesterday.
> > In
> > > order for this patch to really work out we have to add this annotation to
> all
> > > persistents and we didn't want to do that (Slava did that on the branch
and
> it
> > > got pretty ugly). Can we keep these patches locally and not commit them so
> we
> > > don't have to polute the code base with persistent annotations?
> > 
> > I also think annotating all of Persistents is not practical.  How about
> landing
> > the platform/heap/ part of Patch Set 1?
> 
> I had this locally as well and decided to do the stack tracing instead because
I
> couldn't find the right persistent to annotate. With this you still have to do
> manual annotation and search to find the persistent to annotate. Therefore,
I'm
> not entirely convinced that having this code without having all persistents
> annotated is that useful. However, if you found this useful I think it would
be
> OK to land the platform/heap part of this. That limits the ugliness to our
code
> and it will be easy to pull out again.

I agree with Mads here.

If we can do the annotation in Persistent<X> (using some macro/template hacks)
and cover all persistent handles without polluting the code base, I'm fine with
this approach.

[zerny-chromium, 2014-06-13 09:07:57]

On 2014/06/13 09:00:22, haraken wrote:
> On 2014/06/13 08:39:44, Mads Ager (chromium) wrote:
> > On 2014/06/13 08:28:55, tkent wrote:
> > > On 2014/06/13 08:22:29, Mads Ager (chromium) wrote:
> > > > On 2014/06/13 08:19:40, tkent wrote:
> > > > > Yeah, I used to add WTFReportBacktrace() to all Persistent
constructors
> > > > locally.
> > > > > But this way is not smart :)
> > > > 
> > > > The good think with doing the backtrace is that you get all persistents.
> > This
> > > > patch only annotate a small subset of the persistents and it would not
> have
> > > > given me any information in connection with the leaks that I fixed
> > yesterday.
> > > In
> > > > order for this patch to really work out we have to add this annotation
to
> > all
> > > > persistents and we didn't want to do that (Slava did that on the branch
> and
> > it
> > > > got pretty ugly). Can we keep these patches locally and not commit them
so
> > we
> > > > don't have to polute the code base with persistent annotations?
> > > 
> > > I also think annotating all of Persistents is not practical.  How about
> > landing
> > > the platform/heap/ part of Patch Set 1?
> > 
> > I had this locally as well and decided to do the stack tracing instead
because
> I
> > couldn't find the right persistent to annotate. With this you still have to
do
> > manual annotation and search to find the persistent to annotate. Therefore,
> I'm
> > not entirely convinced that having this code without having all persistents
> > annotated is that useful. However, if you found this useful I think it would
> be
> > OK to land the platform/heap part of this. That limits the ugliness to our
> code
> > and it will be easy to pull out again.
> 
> I agree with Mads here.
> 
> If we can do the annotation in Persistent<X> (using some macro/template hacks)
> and cover all persistent handles without polluting the code base, I'm fine
with
> this approach.

How about having storing either the stack dump (or a particular frame if it can
be reliably identified) with the Persistent instead of a name. That would give
use the needed information and seems reasonable to have committed behind the
GC_TRACING flag.

[tkent, 2014-06-16 01:03:33]

On 2014/06/13 09:07:57, zerny-chromium wrote:
> How about having storing either the stack dump (or a particular frame if it
can
> be reliably identified) with the Persistent instead of a name. That would give
> use the needed information and seems reasonable to have committed behind the
> GC_TRACING flag.

Storing stack information sounds nice!

The Patch Set 2 implemented it.  It shows:

Path to 1bd56220 of WebCore::Node 
<- 1bd55ff8 of WebCore::Node 
<- 1bd55dc8 of WebCore::Node 
<- 7d046600 of Persistent ... Backtrace: 
        Persistent<HTMLSelectElement, ThreadLocalPersistents<(ThreadAffinity)1>
>::Persistent(HTMLSelectElement*) 
        v8::Handle<v8::Object>
V8DOMWrapper::associateObjectWithWrapper<V8HTMLSelectElement,
HTMLSelectElement>(HTMLSelectElement*, WrapperTypeInfo const*,
v8::Handle<v8::Object>, v8::Isolate*, WrapperConfiguration::Lifetime) 
        v8::Handle<v8::Object>
V8DOMWrapper::associateObjectWithWrapper<V8HTMLSelectElement,
HTMLSelectElement>(WTF::RawPtr<HTMLSelectElement>, WrapperTypeInfo const*,
v8::Handle<v8::Object>, v8::Isolate*, WrapperConfiguration::Lifetime) 
        V8HTMLSelectElement::createWrapper(WTF::RawPtr<HTMLSelectElement>,
v8::Handle<v8::Object>, v8::Isolate*)

[haraken, 2014-06-16 01:15:41]

https://codereview.chromium.org/337653002/diff/60001/Source/platform/heap/Han...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/337653002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:290: Persistent() : m_raw(0) { recordBacktrace();
}

I'm not sure if you want to record a backtrace for a null persistent.

https://codereview.chromium.org/337653002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:292: Persistent(std::nullptr_t) : m_raw(0) {
recordBacktrace(); }

Ditto.

https://codereview.chromium.org/337653002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:320: m_raw = other;

You'll need recordBacktrace() here as well.

https://codereview.chromium.org/337653002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:347: visitor->setHostInfo(this,
m_tracingName.isEmpty() ? "Persistent" : m_tracingName);

Just help me understand: In what situation can the m_tracingName be empty?

https://codereview.chromium.org/337653002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:370: m_raw = other;

You'll need recordBacktrace() here.

https://codereview.chromium.org/337653002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:377: m_raw = other;

You'll need recordBacktrace() here.

https://codereview.chromium.org/337653002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:384: m_raw = other;

You'll need recordBacktrace() here.

https://codereview.chromium.org/337653002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:391: m_raw = other;

You'll need recordBacktrace() here.

My proposal is:

- Implement a private helper method to set m_raw (e.g. Persistent::setRaw()),
and call recordBacktrace() in the helper method.
- Call recordBacktrace() only when m_raw is not 0.

https://codereview.chromium.org/337653002/diff/60001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/337653002/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:1620: const int framesToSkip = 4;

It would be better if we could determine this number dynamically instead of
hard-coding; i.e., find "Persistent::Persistent" and skip the frames below the
"Persistent::Persistent".

https://codereview.chromium.org/337653002/diff/60001/Source/wtf/Assertions.h
File Source/wtf/Assertions.h (right):

https://codereview.chromium.org/337653002/diff/60001/Source/wtf/Assertions.h#...
Source/wtf/Assertions.h:128: FrameToNameScope(void*);

Add explicit.

[tkent, 2014-06-16 02:34:47]

https://codereview.chromium.org/337653002/diff/60001/Source/platform/heap/Han...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/337653002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:347: visitor->setHostInfo(this,
m_tracingName.isEmpty() ? "Persistent" : m_tracingName);
On 2014/06/16 01:15:40, haraken wrote:
> 
> Just help me understand: In what situation can the m_tracingName be empty?

When we forget to add recordBacktrace() :-)

https://codereview.chromium.org/337653002/diff/60001/Source/platform/heap/Han...
Source/platform/heap/Handle.h:391: m_raw = other;
On 2014/06/16 01:15:40, haraken wrote:
> 
> You'll need recordBacktrace() here.
> 
> My proposal is:
> 
> - Implement a private helper method to set m_raw (e.g. Persistent::setRaw()),
> and call recordBacktrace() in the helper method.
> - Call recordBacktrace() only when m_raw is not 0.

I thought adding recordBacktrace() to constructors was enough. In this policy we
needed to add recordBacktrace()  even if m_raw is 0 and we didn't need to add
recordBacktrace to operator=.
Your policy, adding recordBacktrace to any code to update m_raw, is also
reasonable.  I changed so in Patch Set 3.

I don't apply the seRaw() helper idea because I'd like to initialize m_raw in
initializer lists as possible.

https://codereview.chromium.org/337653002/diff/60001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/337653002/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:1620: const int framesToSkip = 4;
On 2014/06/16 01:15:40, haraken wrote:
> 
> It would be better if we could determine this number dynamically instead of
> hard-coding; i.e., find "Persistent::Persistent" and skip the frames below the
> "Persistent::Persistent".

Done.

https://codereview.chromium.org/337653002/diff/60001/Source/wtf/Assertions.h
File Source/wtf/Assertions.h (right):

https://codereview.chromium.org/337653002/diff/60001/Source/wtf/Assertions.h#...
Source/wtf/Assertions.h:128: FrameToNameScope(void*);
On 2014/06/16 01:15:40, haraken wrote:
> 
> Add explicit.

Done.

[haraken, 2014-06-16 02:41:37]

LGTM.

[Mads Ager (chromium), 2014-06-16 05:41:11]

LGTM!

[tkent, 2014-06-16 07:35:53]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-06-16 07:36:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/337653002/80001

[commit-bot: I haz the power, 2014-06-16 08:02:49]

Change committed as 176185