Skip to content

Commit

Permalink
chore: enable v8 oilpan (#30853)
Browse files Browse the repository at this point in the history
* chore: enable v8 oilpan

* chore: update patches
  • Loading branch information
deepak1556 committed Sep 7, 2021
1 parent 2d5b456 commit 9f39865
Show file tree
Hide file tree
Showing 4 changed files with 260 additions and 4 deletions.
4 changes: 0 additions & 4 deletions build/args/all.gn
Expand Up @@ -33,7 +33,3 @@ is_cfi = false
allow_runtime_configurable_key_storage = true

enable_cet_shadow_stack = false

# TODO(deepak1556): remove once https://bugs.chromium.org/p/chromium/issues/detail?id=1241610
# is fixed.
enable_blink_heap_use_v8_oilpan = false
2 changes: 2 additions & 0 deletions patches/v8/.patches
Expand Up @@ -6,3 +6,5 @@ workaround_an_undefined_symbol_error.patch
do_not_export_private_v8_symbols_on_windows.patch
fix_build_deprecated_attirbute_for_older_msvc_versions.patch
fix_disable_implies_dcheck_for_node_stream_array_buffers.patch
cppgc-js_fix_snapshot_node_merging.patch
cppgc-js_support_eager_traced_value_in_ephemeron_pairs.patch
50 changes: 50 additions & 0 deletions patches/v8/cppgc-js_fix_snapshot_node_merging.patch
@@ -0,0 +1,50 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Michael Lippautz <mlippautz@chromium.org>
Date: Tue, 24 Aug 2021 12:34:24 +0200
Subject: cppgc-js: Fix snapshot node merging

In Blink, WindowProxy may be referred from two diffrent JS wrapper
objects during page refresh (same site navigation reusing parts of the
DOM). In this intermediate state, the old frame state is not yet
reclaimed while the new state is already being added.

We would like to only merge nodes when there's a 1:1 relation between
C++ and JS objects. Unfortunately, WindowProxy breaks that assumption
in that the C++ object doesn't directly point to the wrapper. In
addition, merging this case is important as otherwise detachedness
would not be propagated to the Window object (JS wrapper) which is the
main user of detachedness.

The CL allows overriding merged nodes, picking a random merged state
during pageload while still resulting in the regular snapshot behavior
outside of reloading the same page.

The proper fix is addressing chromium:1218404 and only create merged
nodes when the back reference points to the same object.

Bug: chromium:1241610
Change-Id: Ie77b51a56ce90ef377124304bb025342a724c600
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3114139
Reviewed-by: Anton Bikineev <bikineev@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#76453}

diff --git a/src/heap/cppgc-js/cpp-snapshot.cc b/src/heap/cppgc-js/cpp-snapshot.cc
index 28859d84f66aafa6b90325086d2289f2ef43a704..e72c6ad46998bb40ba91d37a740dc0ecfee2838b 100644
--- a/src/heap/cppgc-js/cpp-snapshot.cc
+++ b/src/heap/cppgc-js/cpp-snapshot.cc
@@ -47,7 +47,13 @@ class EmbedderNode : public v8::EmbedderGraph::Node {
void SetWrapperNode(v8::EmbedderGraph::Node* wrapper_node) {
// An embedder node may only be merged with a single wrapper node, as
// consumers of the graph may merge a node and its wrapper node.
- DCHECK_NULL(wrapper_node_);
+ //
+ // TODO(chromium:1218404): Add a DCHECK() to avoid overriding an already
+ // set `wrapper_node_`. This can currently happen with global proxies that
+ // are rewired (and still kept alive) after reloading a page, see
+ // `CreateMergedNode`. We accept overriding the wrapper node in such cases,
+ // leading to a random merged node and separated nodes for all other
+ // proxies.
wrapper_node_ = wrapper_node;
}
Node* WrapperNode() final { return wrapper_node_; }
@@ -0,0 +1,208 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Michael Lippautz <mlippautz@chromium.org>
Date: Fri, 3 Sep 2021 14:17:59 +0200
Subject: cppgc-js: Support eager traced value in ephemeron pairs

Before this patch, both key and value of an ephemeron pair was always
considered to be GarbageCollected objects.

This patch adjusts the snapshotting mechanism to accomodate that
values may not be GarbageCollected objects and must thus be eagerly
traced for visibility and edge creation.

In practice this only shows up in Blink when associating an existing
wrappable with a wrapper in a non-main world, e.g., through an
extension. In this case, DOMWrapperMap keeps the wrapper value through
a TracedReference in the ephemeron map with the existing wrappable as
key. The semantics are intended to be general ephemeron semantics,
i.e., value needs to be kept alive when the key is alive. This is
visualized in DevTools as the main wrapper/wrappable pair (which is
merged into a single node for the snapshot) retaining the non-main
world wrapper.

Bug: chromium:1245894
Change-Id: Ibfa6722f20c76f94c310f9a040f0d3d4b9083bbb
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3140601
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#76658}

diff --git a/src/heap/cppgc-js/cpp-snapshot.cc b/src/heap/cppgc-js/cpp-snapshot.cc
index e72c6ad46998bb40ba91d37a740dc0ecfee2838b..f64a7de116ac69221e3610370c7e2a5f0b7c60e5 100644
--- a/src/heap/cppgc-js/cpp-snapshot.cc
+++ b/src/heap/cppgc-js/cpp-snapshot.cc
@@ -264,6 +264,10 @@ class State final : public StateBase {
ephemeron_edges_.insert(&value);
}

+ void AddEagerEphemeronEdge(const void* value, cppgc::TraceCallback callback) {
+ eager_ephemeron_edges_.insert({value, callback});
+ }
+
template <typename Callback>
void ForAllEphemeronEdges(Callback callback) {
for (const HeapObjectHeader* value : ephemeron_edges_) {
@@ -271,10 +275,20 @@ class State final : public StateBase {
}
}

+ template <typename Callback>
+ void ForAllEagerEphemeronEdges(Callback callback) {
+ for (const auto& pair : eager_ephemeron_edges_) {
+ callback(pair.first, pair.second);
+ }
+ }
+
private:
bool is_weak_container_ = false;
// Values that are held alive through ephemerons by this particular key.
std::unordered_set<const HeapObjectHeader*> ephemeron_edges_;
+ // Values that are eagerly traced and held alive through ephemerons by this
+ // particular key.
+ std::unordered_map<const void*, cppgc::TraceCallback> eager_ephemeron_edges_;
};

// Root states are similar to regular states with the difference that they are
@@ -404,6 +418,9 @@ class CppGraphBuilderImpl final {
void VisitForVisibility(State& parent, const TracedReferenceBase&);
void VisitEphemeronForVisibility(const HeapObjectHeader& key,
const HeapObjectHeader& value);
+ void VisitEphemeronWithNonGarbageCollectedValueForVisibility(
+ const HeapObjectHeader& key, const void* value,
+ cppgc::TraceDescriptor value_desc);
void VisitWeakContainerForVisibility(const HeapObjectHeader&);
void VisitRootForGraphBuilding(RootState&, const HeapObjectHeader&,
const cppgc::SourceLocation&);
@@ -421,7 +438,7 @@ class CppGraphBuilderImpl final {
}

void AddEdge(State& parent, const HeapObjectHeader& header,
- const std::string& edge_name = {}) {
+ const std::string& edge_name) {
DCHECK(parent.IsVisibleNotDependent());
auto& current = states_.GetExistingState(header);
if (!current.IsVisibleNotDependent()) return;
@@ -449,7 +466,8 @@ class CppGraphBuilderImpl final {
}
}

- void AddEdge(State& parent, const TracedReferenceBase& ref) {
+ void AddEdge(State& parent, const TracedReferenceBase& ref,
+ const std::string& edge_name) {
DCHECK(parent.IsVisibleNotDependent());
v8::Local<v8::Value> v8_value = ref.Get(cpp_heap_.isolate());
if (!v8_value.IsEmpty()) {
@@ -457,12 +475,19 @@ class CppGraphBuilderImpl final {
parent.set_node(AddNode(*parent.header()));
}
auto* v8_node = graph_.V8Node(v8_value);
- graph_.AddEdge(parent.get_node(), v8_node);
+ if (!edge_name.empty()) {
+ graph_.AddEdge(parent.get_node(), v8_node,
+ parent.get_node()->InternalizeEdgeName(edge_name));
+ } else {
+ graph_.AddEdge(parent.get_node(), v8_node);
+ }

// References that have a class id set may have their internal fields
// pointing back to the object. Set up a wrapper node for the graph so
// that the snapshot generator can merge the nodes appropriately.
- if (!ref.WrapperClassId()) return;
+ // Even with a set class id, do not set up a wrapper node when the edge
+ // has a specific name.
+ if (!ref.WrapperClassId() || !edge_name.empty()) return;

void* back_reference_object = ExtractEmbedderDataBackref(
reinterpret_cast<v8::internal::Isolate*>(cpp_heap_.isolate()),
@@ -612,8 +637,18 @@ class WeakVisitor : public JSVisitor {
void VisitEphemeron(const void* key, const void* value,
cppgc::TraceDescriptor value_desc) final {
// For ephemerons, the key retains the value.
+ // Key always must be a GarbageCollected object.
+ auto& key_header = HeapObjectHeader::FromObject(key);
+ if (!value_desc.base_object_payload) {
+ // Value does not represent an actual GarbageCollected object but rather
+ // should be traced eagerly.
+ graph_builder_.VisitEphemeronWithNonGarbageCollectedValueForVisibility(
+ key_header, value, value_desc);
+ return;
+ }
+ // Regular path where both key and value are GarbageCollected objects.
graph_builder_.VisitEphemeronForVisibility(
- HeapObjectHeader::FromObject(key), HeapObjectHeader::FromObject(value));
+ key_header, HeapObjectHeader::FromObject(value));
}

protected:
@@ -659,7 +694,7 @@ class GraphBuildingVisitor final : public JSVisitor {
void Visit(const void*, cppgc::TraceDescriptor desc) final {
graph_builder_.AddEdge(
parent_scope_.ParentAsRegularState(),
- HeapObjectHeader::FromObject(desc.base_object_payload));
+ HeapObjectHeader::FromObject(desc.base_object_payload), edge_name_);
}
void VisitWeakContainer(const void* object,
cppgc::TraceDescriptor strong_desc,
@@ -669,7 +704,8 @@ class GraphBuildingVisitor final : public JSVisitor {
// container itself.
graph_builder_.AddEdge(
parent_scope_.ParentAsRegularState(),
- HeapObjectHeader::FromObject(strong_desc.base_object_payload));
+ HeapObjectHeader::FromObject(strong_desc.base_object_payload),
+ edge_name_);
}
void VisitRoot(const void*, cppgc::TraceDescriptor desc,
const cppgc::SourceLocation& loc) final {
@@ -681,12 +717,18 @@ class GraphBuildingVisitor final : public JSVisitor {
const void*, const cppgc::SourceLocation&) final {}
// JS handling.
void Visit(const TracedReferenceBase& ref) final {
- graph_builder_.AddEdge(parent_scope_.ParentAsRegularState(), ref);
+ graph_builder_.AddEdge(parent_scope_.ParentAsRegularState(), ref,
+ edge_name_);
+ }
+
+ void set_edge_name(std::string edge_name) {
+ edge_name_ = std::move(edge_name);
}

private:
CppGraphBuilderImpl& graph_builder_;
const ParentScope& parent_scope_;
+ std::string edge_name_;
};

// Base class for transforming recursion into iteration. Items are processed
@@ -779,6 +821,19 @@ void CppGraphBuilderImpl::VisitForVisibility(State* parent,
}
}

+void CppGraphBuilderImpl::
+ VisitEphemeronWithNonGarbageCollectedValueForVisibility(
+ const HeapObjectHeader& key, const void* value,
+ cppgc::TraceDescriptor value_desc) {
+ auto& key_state = states_.GetOrCreateState(key);
+ // Eagerly trace the value here, effectively marking key as visible and
+ // queuing processing for all reachable values.
+ ParentScope parent_scope(key_state);
+ VisiblityVisitor visitor(*this, parent_scope);
+ value_desc.callback(&visitor, value);
+ key_state.AddEagerEphemeronEdge(value, value_desc.callback);
+}
+
void CppGraphBuilderImpl::VisitEphemeronForVisibility(
const HeapObjectHeader& key, const HeapObjectHeader& value) {
auto& key_state = states_.GetOrCreateState(key);
@@ -834,6 +889,12 @@ void CppGraphBuilderImpl::Run() {
state.ForAllEphemeronEdges([this, &state](const HeapObjectHeader& value) {
AddEdge(state, value, "part of key -> value pair in ephemeron table");
});
+ object_visitor.set_edge_name(
+ "part of key -> value pair in ephemeron table");
+ state.ForAllEagerEphemeronEdges(
+ [&object_visitor](const void* value, cppgc::TraceCallback callback) {
+ callback(&object_visitor, value);
+ });
});
// Add roots.
{

0 comments on commit 9f39865

Please sign in to comment.