Bug 109554 - Web Inspector: Native Memory Instrumentation: reportLeaf method doesn't report the leaf node properly.
Summary: Web Inspector: Native Memory Instrumentation: reportLeaf method doesn't repor...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ilya Tikhonovsky
URL:
Keywords:
Depends on: 109746
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-12 01:57 PST by Ilya Tikhonovsky
Modified: 2013-03-01 02:53 PST (History)
13 users (show)

See Also:


Attachments
Patch (23.24 KB, patch)
2013-02-12 03:17 PST, Ilya Tikhonovsky
yurys: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
for trybots (21.21 KB, patch)
2013-02-13 05:18 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Tikhonovsky 2013-02-12 01:57:39 PST
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.
Comment 1 Ilya Tikhonovsky 2013-02-12 03:17:22 PST
Created attachment 187821 [details]
Patch
Comment 2 Yury Semikhatsky 2013-02-12 05:10:46 PST
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
Comment 3 Ilya Tikhonovsky 2013-02-12 06:55:20 PST
Committed r142618: <http://trac.webkit.org/changeset/142618>
Comment 4 Jessie Berlin 2013-02-12 09:31:13 PST
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.
Comment 5 Jessie Berlin 2013-02-12 09:32:14 PST
(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?
Comment 6 Jessie Berlin 2013-02-12 09:40:40 PST
Rolled it out in http://trac.webkit.org/changeset/142637
Comment 7 Ilya Tikhonovsky 2013-02-12 11:35:58 PST
(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.
Comment 8 Build Bot 2013-02-12 23:13:58 PST
Comment on attachment 187821 [details]
Patch

Attachment 187821 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16515649
Comment 9 Ilya Tikhonovsky 2013-02-13 05:18:05 PST
Created attachment 188061 [details]
for trybots
Comment 10 Ilya Tikhonovsky 2013-02-13 07:32:09 PST
Committed r142747: <http://trac.webkit.org/changeset/142747>
Comment 11 Ilya Tikhonovsky 2013-02-13 07:35:42 PST
(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
Comment 12 Alec Flett 2013-02-13 13:52:45 PST
Heads up: this *may* have broken the component build, at least for TestWebKitAPI and may get rolled out soonish. Hop on #webkit or #chromium.
Comment 13 WebKit Review Bot 2013-02-13 14:01:03 PST
Re-opened since this is blocked by bug 109746
Comment 14 Ilya Tikhonovsky 2013-02-18 01:56:22 PST
Committed r143175: <http://trac.webkit.org/changeset/143175>
Comment 15 Eric Seidel (no email) 2013-03-01 02:53:26 PST
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).