diff --git a/build/args/all.gn b/build/args/all.gn index bb27de7d11c94..86200887efe7c 100644 --- a/build/args/all.gn +++ b/build/args/all.gn @@ -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 diff --git a/patches/v8/.patches b/patches/v8/.patches index ff0b6eb65d118..5136d382b4769 100644 --- a/patches/v8/.patches +++ b/patches/v8/.patches @@ -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 diff --git a/patches/v8/cppgc-js_fix_snapshot_node_merging.patch b/patches/v8/cppgc-js_fix_snapshot_node_merging.patch new file mode 100644 index 0000000000000..51ff5bb597ca8 --- /dev/null +++ b/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 +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 +Commit-Queue: Michael Lippautz +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_; } diff --git a/patches/v8/cppgc-js_support_eager_traced_value_in_ephemeron_pairs.patch b/patches/v8/cppgc-js_support_eager_traced_value_in_ephemeron_pairs.patch new file mode 100644 index 0000000000000..528aa39dac94a --- /dev/null +++ b/patches/v8/cppgc-js_support_eager_traced_value_in_ephemeron_pairs.patch @@ -0,0 +1,208 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Michael Lippautz +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 +Commit-Queue: Michael Lippautz +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 + void ForAllEphemeronEdges(Callback callback) { + for (const HeapObjectHeader* value : ephemeron_edges_) { +@@ -271,10 +275,20 @@ class State final : public StateBase { + } + } + ++ template ++ 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 ephemeron_edges_; ++ // Values that are eagerly traced and held alive through ephemerons by this ++ // particular key. ++ std::unordered_map 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 = 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(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. + {