Bug 109554 – Web Inspector: Native Memory Instrumentation: reportLeaf method doesn't report the leaf node properly.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Ilya Tikhonovsky]

I found that HeapGraphSerializer doesn't report the node for the leaf if the leaf has no pointer.
It happens when addPrivateBuffer is calling.
It was hard to detect the root of problem because there were no unit tests.

[Ilya Tikhonovsky]

Created attachment 187821 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 187821 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187821&action=review

> Source/WebCore/ChangeLog:6
> +        In some cases leafs have no pointer so with the old schema we can't generate nodeId for them because we

leafs -> leaves

> Source/WebCore/inspector/HeapGraphSerializer.h:46
> +class HeapGraphSerializerClient {

Turn it into inner class?

> Source/WebCore/inspector/InspectorMemoryAgent.cpp:346
> +    } frontendWrapper(m_frontend);

This way frontendWrapper will be destroyed before graphSerializer.

> Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:42
> +namespace {

This should be namespace TestWebKitAPI for consistency.

> Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:52
> +        m_currentPointer = 0;

Move this into the initializers list.

> Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:57
> +        m_heapSnapshotChunk = heapSnapshotChunk;

Do you expect more than one chunk? If not there should be an assert for this.

> Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:85
> +    void* addNode(const char* className, const char* name, bool isRoot)

Looks like this and next method should go into a separate class, can you extract them?

> Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:113
> +        RefPtr<InspectorObject> chunk = *reinterpret_cast<RefPtr<InspectorObject>*>(&m_heapSnapshotChunk);

Wouldn't RefPtr<InspectorObject> chunk = m_heapSnapshotChunk; work? Or probably we can use m_heapSnapshotChunk directly?

> Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:122
> +        return chunkPart(partName)->toJSONString().replace("\"", "'");

Why do you need to change the " to ' ?

> Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:141
> +    String node(unsigned nodeIndex)

nodeToString?

> Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:154
> +    String edge(unsigned edgeOrdinal)

edgeToString?

> Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:211
> +    EXPECT_EQ(String("['','weak','ownRef','countRef','unknown','Root']"), receiver.dumpLastChunkStrings());

ownRef -> ownPtr, countRef -> refPtr?

> Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:257
> +TEST(HeapGraphSerializerTest, hashSetWithTwoItems)

hashSetWithTwoItems -> hashSetWithTwoStrings

[Ilya Tikhonovsky]

Committed r142618: <http://trac.webkit.org/changeset/142618>

[Jessie Berlin]

This broke the mac build in a bunch of ways, some of which are:

1. You added the wrong file to the Xcode project (HeapGraphGeneratorTest.cpp instead of HeapGraphSerializerTest.cpp)
2. You used the same unique ID for every place a UID was used.
3. You attempted to use headers from WebCore and WTF without marking them as Private headers to be copied

I am going to roll out this patch.

[Jessie Berlin]

(In reply to comment #4)
> This broke the mac build in a bunch of ways, some of which are:
> 
> 1. You added the wrong file to the Xcode project (HeapGraphGeneratorTest.cpp instead of HeapGraphSerializerTest.cpp)
> 2. You used the same unique ID for every place a UID was used.
> 3. You attempted to use headers from WebCore and WTF without marking them as Private headers to be copied
> 
> I am going to roll out this patch.

Note: all of these should have been caught if you had waiting for the Mac EWS to finish. Is there a reason you didn’t?

[Jessie Berlin]

Rolled it out in http://trac.webkit.org/changeset/142637

[Ilya Tikhonovsky]

(In reply to comment #5)
> (In reply to comment #4)
> > This broke the mac build in a bunch of ways, some of which are:
> > 
> > 1. You added the wrong file to the Xcode project (HeapGraphGeneratorTest.cpp instead of HeapGraphSerializerTest.cpp)
> > 2. You used the same unique ID for every place a UID was used.
> > 3. You attempted to use headers from WebCore and WTF without marking them as Private headers to be copied
> > 
> > I am going to roll out this patch.
> 
> Note: all of these should have been caught if you had waiting for the Mac EWS to finish. Is there a reason you didn’t?

The compilation on Mac was broken since yesterday.

[Build Bot]

Comment on attachment 187821 [details]
Patch

Attachment 187821 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16515649

[Ilya Tikhonovsky]

Created attachment 188061 [details]
for trybots

[Ilya Tikhonovsky]

Committed r142747: <http://trac.webkit.org/changeset/142747>

[Ilya Tikhonovsky]

(In reply to comment #2)
> (From update of attachment 187821 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187821&action=review
> 
> > Source/WebCore/ChangeLog:6
> > +        In some cases leafs have no pointer so with the old schema we can't generate nodeId for them because we
> 
> leafs -> leaves

done

> 
> > Source/WebCore/inspector/HeapGraphSerializer.h:46
> > +class HeapGraphSerializerClient {
> 
> Turn it into inner class?

done

> 
> > Source/WebCore/inspector/InspectorMemoryAgent.cpp:346
> > +    } frontendWrapper(m_frontend);
> 
> This way frontendWrapper will be destroyed before graphSerializer.

done

> 
> > Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:42
> > +namespace {
> 
> This should be namespace TestWebKitAPI for consistency.

done

> 
> > Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:52
> > +        m_currentPointer = 0;
> 
> Move this into the initializers list.

done 

> 
> > Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:57
> > +        m_heapSnapshotChunk = heapSnapshotChunk;
> 
> Do you expect more than one chunk? If not there should be an assert for this.

done 

> 
> > Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:85
> > +    void* addNode(const char* className, const char* name, bool isRoot)
> 
> Looks like this and next method should go into a separate class, can you extract them?

done

> 
> > Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:113
> > +        RefPtr<InspectorObject> chunk = *reinterpret_cast<RefPtr<InspectorObject>*>(&m_heapSnapshotChunk);
> 
> Wouldn't RefPtr<InspectorObject> chunk = m_heapSnapshotChunk; work? Or probably we can use m_heapSnapshotChunk directly?

No we wouldn't. HeapSnapshotChunk inherits InspectorObjectBase so we can't simply assign it and
the methods of InspectorObjectBase are not public.


> 
> > Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:122
> > +        return chunkPart(partName)->toJSONString().replace("\"", "'");
> 
> Why do you need to change the " to ' ?

it is a simply hack for better readability of the source code. Without it I need to escape " symbol in the expectations.

> 
> > Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:141
> > +    String node(unsigned nodeIndex)
> 
> nodeToString?

done

> 
> > Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:154
> > +    String edge(unsigned edgeOrdinal)
> 
> edgeToString?

done

> 
> > Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:211
> > +    EXPECT_EQ(String("['','weak','ownRef','countRef','unknown','Root']"), receiver.dumpLastChunkStrings());
> 
> ownRef -> ownPtr, countRef -> refPtr?

Actually the current names and the suggested names are not good.
But in any case it should be a part of a separate patch.

> 
> > Tools/TestWebKitAPI/Tests/WebCore/HeapGraphSerializerTest.cpp:257
> > +TEST(HeapGraphSerializerTest, hashSetWithTwoItems)
> 
> hashSetWithTwoItems -> hashSetWithTwoStrings

done

[Alec Flett]

Heads up: this *may* have broken the component build, at least for TestWebKitAPI and may get rolled out soonish. Hop on #webkit or #chromium.

[WebKit Review Bot]

Re-opened since this is blocked by bug 109746

[Ilya Tikhonovsky]

Committed r143175: <http://trac.webkit.org/changeset/143175>

[Eric Seidel (no email)]

Comment on attachment 188061 [details]
for trybots

Cleared review? from attachment 188061 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).