HeapSnapshotBuilder::analyzeNode() should filter out duplicate cells.
https://bugs.webkit.org/show_bug.cgi?id=209929
<rdar://problem/60974478>
Reviewed by Keith Miller.
HeapSnapshot::finalize() assumes that its list of cells contain no duplicate cells.
HeapSnapshot::appendNode() expects to only be called once for a cell. It doesn't
check for duplicates.
However, with the concurrent GC marker, there’s a racy chance that the same cell
is visited more than once by SlotVisitor, and therefore, SlotVisitor may call
HeapSnapshotBuilder::analyzeNode() (and HeapSnapshot::appendNode()) more than once
for the same cell.
The easiest and cleanest fix for this is to simply keep a HashSet of appended
cells in HeapSnapshotBuilder while it is building the snapshot. We can then use
the hash set to filter out already appended cells, and avoid adding duplicates to
the HeapSnapshot.
* heap/HeapSnapshotBuilder.cpp:
(JSC::HeapSnapshotBuilder::buildSnapshot):
(JSC::HeapSnapshotBuilder::analyzeNode):
* heap/HeapSnapshotBuilder.h:
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@259418 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index fda1cdf..cfcd30e 100644
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,30 @@
+2020-04-02 Mark Lam <mark.lam@apple.com>
+
+ HeapSnapshotBuilder::analyzeNode() should filter out duplicate cells.
+ https://bugs.webkit.org/show_bug.cgi?id=209929
+ <rdar://problem/60974478>
+
+ Reviewed by Keith Miller.
+
+ HeapSnapshot::finalize() assumes that its list of cells contain no duplicate cells.
+ HeapSnapshot::appendNode() expects to only be called once for a cell. It doesn't
+ check for duplicates.
+
+ However, with the concurrent GC marker, there’s a racy chance that the same cell
+ is visited more than once by SlotVisitor, and therefore, SlotVisitor may call
+ HeapSnapshotBuilder::analyzeNode() (and HeapSnapshot::appendNode()) more than once
+ for the same cell.
+
+ The easiest and cleanest fix for this is to simply keep a HashSet of appended
+ cells in HeapSnapshotBuilder while it is building the snapshot. We can then use
+ the hash set to filter out already appended cells, and avoid adding duplicates to
+ the HeapSnapshot.
+
+ * heap/HeapSnapshotBuilder.cpp:
+ (JSC::HeapSnapshotBuilder::buildSnapshot):
+ (JSC::HeapSnapshotBuilder::analyzeNode):
+ * heap/HeapSnapshotBuilder.h:
+
2020-04-02 Angelos Oikonomopoulos <angelos@igalia.com>
Enable offlineasm debug annotations for GCC
diff --git a/Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp b/Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp
index d5b9ad2..ab3e9c6 100644
--- a/Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp
+++ b/Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2016-2019 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-2020 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -73,8 +73,12 @@
m_profiler.vm().heap.collectNow(Sync, CollectionScope::Full);
m_profiler.setActiveHeapAnalyzer(nullptr);
}
- m_snapshot->finalize();
+ {
+ auto locker = holdLock(m_buildingNodeMutex);
+ m_appendedCells.clear();
+ m_snapshot->finalize();
+ }
m_profiler.appendSnapshot(WTFMove(m_snapshot));
}
@@ -89,6 +93,9 @@
return;
auto locker = holdLock(m_buildingNodeMutex);
+ auto addResult = m_appendedCells.add(cell);
+ if (!addResult.isNewEntry)
+ return;
m_snapshot->appendNode(HeapSnapshotNode(cell, getNextObjectIdentifier()));
}
diff --git a/Source/JavaScriptCore/heap/HeapSnapshotBuilder.h b/Source/JavaScriptCore/heap/HeapSnapshotBuilder.h
index 36c2fad..ffdee17 100644
--- a/Source/JavaScriptCore/heap/HeapSnapshotBuilder.h
+++ b/Source/JavaScriptCore/heap/HeapSnapshotBuilder.h
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-2020 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -154,6 +154,7 @@
HashMap<JSCell*, RootData> m_rootData;
HashMap<JSCell*, void*> m_wrappedObjectPointers;
HashMap<JSCell*, String> m_cellLabels;
+ HashSet<JSCell*> m_appendedCells;
SnapshotType m_snapshotType;
};
