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.
Created attachment 187821 [details] Patch
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
Committed r142618: <http://trac.webkit.org/changeset/142618>
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.
(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?
Rolled it out in http://trac.webkit.org/changeset/142637
(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 on attachment 187821 [details] Patch Attachment 187821 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16515649
Created attachment 188061 [details] for trybots
Committed r142747: <http://trac.webkit.org/changeset/142747>
(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
Heads up: this *may* have broken the component build, at least for TestWebKitAPI and may get rolled out soonish. Hop on #webkit or #chromium.
Re-opened since this is blocked by bug 109746
Committed r143175: <http://trac.webkit.org/changeset/143175>
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).