Skip to content

Commit

Permalink
Revert "Reland "Remove DOMDataStore::WrappedReference and hold TraceW…
Browse files Browse the repository at this point in the history 
…rapperV8Reference directly in ephemeron map.""

This reverts commit 21cf7ca.

Reason for revert: https://bugs.chromium.org/p/chromium/issues/detail?id=1019839#c39

Original change's description:
> Reland "Remove DOMDataStore::WrappedReference and hold TraceWrapperV8Reference directly in ephemeron map."
> 
> This reverts commit bdc5aa8.
> 
> Reason for revert: Crasher has been stealth-fixed, see https://crbug.com/1021171.
> 
> Original change's description:
> > Revert "Remove DOMDataStore::WrappedReference and hold TraceWrapperV8Reference directly in ephemeron map."
> > 
> > This reverts commit 481fb0c.
> > 
> > Reason for revert: speculative revert for crbug.com/1019839
> > 
> > Original change's description:
> > > Remove DOMDataStore::WrappedReference and hold TraceWrapperV8Reference directly in ephemeron map.
> > > 
> > > Change-Id: I6ef8b0098a795dc550037306e7b972ea907b0fc1
> > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1884285
> > > Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
> > > Reviewed-by: Kentaro Hara <haraken@chromium.org>
> > > Commit-Queue: Jeremy Roman <jbroman@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#710616}
> > 
> > TBR=jbroman@chromium.org,haraken@chromium.org,mlippautz@chromium.org
> > 
> > # Not skipping CQ checks because original CL landed > 1 day ago.
> > 
> > Change-Id: Iba339b519f2aa602773ceb79c95948c1cffdcbdb
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1893282
> > Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> > Reviewed-by: Kentaro Hara <haraken@chromium.org>
> > Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#711289}
> 
> TBR=ulan@chromium.org,jbroman@chromium.org,haraken@chromium.org,mlippautz@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Change-Id: Iae455af51181d191f08e7af16cd8a1413aea2b37
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1899783
> Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
> Reviewed-by: Jeremy Roman <jbroman@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#712945}

TBR=ulan@chromium.org,jbroman@chromium.org,haraken@chromium.org,mlippautz@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

(cherry picked from commit 85a5bc5)

Change-Id: Ibac7d4ca203761eb2d472cd40364529976e688bf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1903608
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#713472}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1904155
Reviewed-by: Srinivas Sista <srinivassista@chromium.org>
Cr-Commit-Position: refs/branch-heads/3962@{#3}
Cr-Branched-From: 5bb3422-refs/heads/master@{#713166}
  • Loading branch information
@jeremyroman
jeremyroman authored and Srinivas Sista committed Nov 7, 2019
1 parent e17ccd0 commit 8eb0dfd
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 14 deletions.
8 changes: 6 additions & 2 deletions third_party/blink/renderer/platform/bindings/dom_data_store.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,20 @@ DOMDataStore::DOMDataStore(v8::Isolate* isolate, bool is_main_world)
: is_main_world_(is_main_world) {}

void DOMDataStore::Dispose() {
for (auto& it : wrapper_map_) {
for (const auto& it : wrapper_map_) {
// Explicitly reset references so that a following V8 GC will not find them
// and treat them as roots. There's optimizations (see
// EmbedderHeapTracer::IsRootForNonTracingGC) that would not treat them as
// roots and then Blink would not be able to find and remove them from a DOM
// world. Explicitly resetting on disposal avoids that problem
it.value.Clear();
it.value->ref.Clear();
}
}

void DOMDataStore::WrappedReference::Trace(Visitor* visitor) {
visitor->Trace(ref);
}

void DOMDataStore::Trace(Visitor* visitor) {
visitor->Trace(wrapper_map_);
}
Expand Down
38 changes: 26 additions & 12 deletions third_party/blink/renderer/platform/bindings/dom_data_store.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
#include "base/optional.h"
#include "third_party/blink/renderer/platform/bindings/dom_wrapper_world.h"
#include "third_party/blink/renderer/platform/bindings/script_wrappable.h"
#include "third_party/blink/renderer/platform/bindings/trace_wrapper_v8_reference.h"
#include "third_party/blink/renderer/platform/bindings/wrapper_type_info.h"
#include "third_party/blink/renderer/platform/heap/unified_heap_marking_visitor.h"
#include "third_party/blink/renderer/platform/wtf/allocator/allocator.h"
Expand Down Expand Up @@ -119,7 +118,7 @@ class DOMDataStore final : public GarbageCollected<DOMDataStore> {
return object->MainWorldWrapper(isolate);
auto it = wrapper_map_.find(object);
if (it != wrapper_map_.end())
return it->value.NewLocal(isolate);
return it->value->ref.NewLocal(isolate);
return v8::Local<v8::Object>();
}

Expand All @@ -133,12 +132,13 @@ class DOMDataStore final : public GarbageCollected<DOMDataStore> {
return object->SetWrapper(isolate, wrapper_type_info, wrapper);

auto result = wrapper_map_.insert(
object, TraceWrapperV8Reference<v8::Object>(isolate, wrapper));
object, MakeGarbageCollected<WrappedReference>(isolate, wrapper));
if (LIKELY(result.is_new_entry)) {
wrapper_type_info->ConfigureWrapper(&result.stored_value->value.Get());
wrapper_type_info->ConfigureWrapper(
&result.stored_value->value->ref.Get());
} else {
DCHECK(!result.stored_value->value.IsEmpty());
wrapper = result.stored_value->value.NewLocal(isolate);
DCHECK(!result.stored_value->value->ref.IsEmpty());
wrapper = result.stored_value->value->ref.NewLocal(isolate);
}
return result.is_new_entry;
}
Expand All @@ -149,8 +149,8 @@ class DOMDataStore final : public GarbageCollected<DOMDataStore> {
DCHECK(!is_main_world_);
const auto& it = wrapper_map_.find(object);
if (it != wrapper_map_.end()) {
if (it->value.Get() == handle) {
it->value.Clear();
if (it->value->ref.Get() == handle) {
it->value->ref.Clear();
wrapper_map_.erase(it);
return true;
}
Expand All @@ -164,7 +164,7 @@ class DOMDataStore final : public GarbageCollected<DOMDataStore> {
return object->SetReturnValue(return_value);
auto it = wrapper_map_.find(object);
if (it != wrapper_map_.end()) {
return_value.Set(it->value.Get());
return_value.Set(it->value->ref.Get());
return true;
}
return false;
Expand Down Expand Up @@ -198,10 +198,24 @@ class DOMDataStore final : public GarbageCollected<DOMDataStore> {
return wrappable->IsEqualTo(holder);
}

// Wrapper around TraceWrapperV8Reference to allow use in ephemeron hash map
// below.
class PLATFORM_EXPORT WrappedReference final
: public GarbageCollected<WrappedReference> {
public:
WrappedReference() = default;
WrappedReference(v8::Isolate* isolate, v8::Local<v8::Object> handle)
: ref(isolate, handle) {}

virtual void Trace(Visitor*);

TraceWrapperV8Reference<v8::Object> ref;
};

bool is_main_world_;
// Ephemeron map: V8 wrapper will be kept alive as long as ScriptWrappable is.
HeapHashMap<WeakMember<const ScriptWrappable>,
TraceWrapperV8Reference<v8::Object>>
// Ephemeron map: WrappedReference will be kept alive as long as
// ScriptWrappable is alive.
HeapHashMap<WeakMember<const ScriptWrappable>, Member<WrappedReference>>
wrapper_map_;

DISALLOW_COPY_AND_ASSIGN(DOMDataStore);
Expand Down

0 comments on commit 8eb0dfd

Please sign in to comment.