Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
chore: [28-x-y] cherry-pick 1 changes from 0-M124
* 0d9350b71fd0 from chromium
- Loading branch information
Showing
2 changed files
with
216 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,215 @@ | ||
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 | ||
From: =?UTF-8?q?Marja=20H=C3=B6ltt=C3=A4?= <marja@google.com> | ||
Date: Tue, 9 Apr 2024 08:32:27 +0000 | ||
Subject: Merge "Fix DOMArrayBuffer::IsDetached()" and "Comment out a CHECK | ||
that a DOMAB has maximally one non-detached JSAB" | ||
MIME-Version: 1.0 | ||
Content-Type: text/plain; charset=UTF-8 | ||
Content-Transfer-Encoding: 8bit | ||
|
||
1) | ||
|
||
A DOMArrayBuffer was maintaining its own "is_detached_" state, and | ||
would consider itself non-detached even if the corresponding | ||
JSArrayBuffer (or, all of them, in case there are several) was | ||
detached. | ||
|
||
Piping in the v8::Isolate would be a too big change for this fix, so this is using v8::Isolate::GetCurrent() for now. | ||
|
||
2) | ||
|
||
Comment out a CHECK that a DOMAB has maximally one non-detached JSAB | ||
|
||
Based on crash reports, this assumption is not true and has to be | ||
investigated. | ||
|
||
Removing this newly introduced CHECK to be able to merge fixes in this | ||
area - we still violate this invariant but the fixes are a step into | ||
the right direction. | ||
|
||
Fix in question: | ||
https://chromium-review.googlesource.com/5387887 | ||
which also introduced this CHECK. | ||
|
||
(cherry picked from commit 04e7550d7aa3bf4ac4e49d7074972d357de139e6) | ||
|
||
Change-Id: I6a46721e24c6f04fe8252bc4a5e94caeec3a8b51 | ||
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5435035 | ||
Commit-Queue: Marja Hölttä <marja@chromium.org> | ||
Reviewed-by: Michael Lippautz <mlippautz@chromium.org> | ||
Cr-Commit-Position: refs/branch-heads/6367@{#667} | ||
Cr-Branched-From: d158c6dc6e3604e6f899041972edf26087a49740-refs/heads/main@{#1274542} | ||
|
||
diff --git a/third_party/blink/renderer/core/typed_arrays/dom_array_buffer.cc b/third_party/blink/renderer/core/typed_arrays/dom_array_buffer.cc | ||
index c85839f1a0bf43bc21611b490bf8b7b17ad385da..de572f8bb9e54a027e4ad5ec994248398ce4046d 100644 | ||
--- a/third_party/blink/renderer/core/typed_arrays/dom_array_buffer.cc | ||
+++ b/third_party/blink/renderer/core/typed_arrays/dom_array_buffer.cc | ||
@@ -46,8 +46,15 @@ const WrapperTypeInfo& DOMArrayBuffer::wrapper_type_info_ = | ||
|
||
static void AccumulateArrayBuffersForAllWorlds( | ||
v8::Isolate* isolate, | ||
- DOMArrayBuffer* object, | ||
+ const DOMArrayBuffer* object, | ||
v8::LocalVector<v8::ArrayBuffer>& buffers) { | ||
+ if (!object->has_non_main_world_wrappers() && IsMainThread()) { | ||
+ const DOMWrapperWorld& world = DOMWrapperWorld::MainWorld(isolate); | ||
+ v8::Local<v8::Object> wrapper = world.DomDataStore().Get(object, isolate); | ||
+ buffers.push_back(v8::Local<v8::ArrayBuffer>::Cast(wrapper)); | ||
+ return; | ||
+ } | ||
+ | ||
Vector<scoped_refptr<DOMWrapperWorld>> worlds; | ||
DOMWrapperWorld::AllWorldsInCurrentThread(worlds); | ||
for (const auto& world : worlds) { | ||
@@ -256,6 +263,52 @@ v8::MaybeLocal<v8::Value> DOMArrayBuffer::Wrap(ScriptState* script_state) { | ||
wrapper); | ||
} | ||
|
||
+bool DOMArrayBuffer::IsDetached() const { | ||
+ if (contents_.BackingStore() == nullptr) { | ||
+ return is_detached_; | ||
+ } | ||
+ if (is_detached_) { | ||
+ return true; | ||
+ } | ||
+ | ||
+ v8::Isolate* isolate = v8::Isolate::GetCurrent(); | ||
+ v8::HandleScope handle_scope(isolate); | ||
+ v8::LocalVector<v8::ArrayBuffer> buffer_handles(isolate); | ||
+ AccumulateArrayBuffersForAllWorlds(isolate, this, buffer_handles); | ||
+ | ||
+ // There may be several v8::ArrayBuffers corresponding to the DOMArrayBuffer, | ||
+ // but at most one of them may be non-detached. | ||
+ int nondetached_count = 0; | ||
+ int detached_count = 0; | ||
+ | ||
+ for (const auto& buffer_handle : buffer_handles) { | ||
+ if (buffer_handle->WasDetached()) { | ||
+ ++detached_count; | ||
+ } else { | ||
+ ++nondetached_count; | ||
+ } | ||
+ } | ||
+ | ||
+ // This CHECK fires even though it should not. TODO(330759272): Investigate | ||
+ // under which conditions we end up with multiple non-detached JSABs for the | ||
+ // same DOMAB and potentially restore this check. | ||
+ | ||
+ // CHECK_LE(nondetached_count, 1); | ||
+ | ||
+ return nondetached_count == 0 && detached_count > 0; | ||
+} | ||
+ | ||
+v8::Local<v8::Object> DOMArrayBuffer::AssociateWithWrapper( | ||
+ v8::Isolate* isolate, | ||
+ const WrapperTypeInfo* wrapper_type_info, | ||
+ v8::Local<v8::Object> wrapper) { | ||
+ if (!DOMWrapperWorld::Current(isolate).IsMainWorld()) { | ||
+ has_non_main_world_wrappers_ = true; | ||
+ } | ||
+ return ScriptWrappable::AssociateWithWrapper(isolate, wrapper_type_info, | ||
+ wrapper); | ||
+} | ||
+ | ||
DOMArrayBuffer* DOMArrayBuffer::Slice(size_t begin, size_t end) const { | ||
begin = std::min(begin, ByteLength()); | ||
end = std::min(end, ByteLength()); | ||
diff --git a/third_party/blink/renderer/core/typed_arrays/dom_array_buffer.h b/third_party/blink/renderer/core/typed_arrays/dom_array_buffer.h | ||
index 17d75fe891a78ffa480207a2b595b9f942ca5e38..881497e93dd6bec0f2f0908068d72dbf7b072362 100644 | ||
--- a/third_party/blink/renderer/core/typed_arrays/dom_array_buffer.h | ||
+++ b/third_party/blink/renderer/core/typed_arrays/dom_array_buffer.h | ||
@@ -87,6 +87,17 @@ class CORE_EXPORT DOMArrayBuffer : public DOMArrayBufferBase { | ||
|
||
void Trace(Visitor*) const override; | ||
|
||
+ bool IsDetached() const override; | ||
+ | ||
+ v8::Local<v8::Object> AssociateWithWrapper( | ||
+ v8::Isolate* isolate, | ||
+ const WrapperTypeInfo* wrapper_type_info, | ||
+ v8::Local<v8::Object> wrapper) override; | ||
+ | ||
+ bool has_non_main_world_wrappers() const { | ||
+ return has_non_main_world_wrappers_; | ||
+ } | ||
+ | ||
private: | ||
v8::Maybe<bool> TransferDetachable(v8::Isolate*, | ||
v8::Local<v8::Value> detach_key, | ||
@@ -97,6 +108,8 @@ class CORE_EXPORT DOMArrayBuffer : public DOMArrayBufferBase { | ||
// support only v8::String as the detach key type. It's also convenient that | ||
// we can write `array_buffer->SetDetachKey(isolate, "my key")`. | ||
TraceWrapperV8Reference<v8::String> detach_key_; | ||
+ | ||
+ bool has_non_main_world_wrappers_ = false; | ||
}; | ||
|
||
} // namespace blink | ||
diff --git a/third_party/blink/renderer/core/typed_arrays/dom_array_buffer_base.h b/third_party/blink/renderer/core/typed_arrays/dom_array_buffer_base.h | ||
index 95511438595715d030e5f18760b210cf7e58311a..985044ccc1de97dca903b22e5f41e55be52f2533 100644 | ||
--- a/third_party/blink/renderer/core/typed_arrays/dom_array_buffer_base.h | ||
+++ b/third_party/blink/renderer/core/typed_arrays/dom_array_buffer_base.h | ||
@@ -27,7 +27,9 @@ class CORE_EXPORT DOMArrayBufferBase : public ScriptWrappable { | ||
|
||
size_t ByteLength() const { return contents_.DataLength(); } | ||
|
||
- bool IsDetached() const { return is_detached_; } | ||
+ // TODO(331348222): It doesn't make sense to detach DomSharedArrayBuffers, | ||
+ // remove that possibility. | ||
+ virtual bool IsDetached() const { return is_detached_; } | ||
|
||
void Detach() { is_detached_ = true; } | ||
|
||
diff --git a/third_party/blink/renderer/modules/gamepad/BUILD.gn b/third_party/blink/renderer/modules/gamepad/BUILD.gn | ||
index 572d8ce27fa808707ae17ed05f14e2ed103f131e..a871cbd002795bf49ad48f0002ac4996135f8b10 100644 | ||
--- a/third_party/blink/renderer/modules/gamepad/BUILD.gn | ||
+++ b/third_party/blink/renderer/modules/gamepad/BUILD.gn | ||
@@ -55,6 +55,7 @@ source_set("unit_tests") { | ||
"//testing/gtest", | ||
"//third_party/blink/renderer/modules", | ||
"//third_party/blink/renderer/platform", | ||
+ "//third_party/blink/renderer/platform:test_support", | ||
"//third_party/blink/renderer/platform/wtf", | ||
] | ||
} | ||
diff --git a/third_party/blink/renderer/modules/gamepad/gamepad_comparisons_test.cc b/third_party/blink/renderer/modules/gamepad/gamepad_comparisons_test.cc | ||
index e0a7f48630ba423b19641232c026d72ba71dfc4b..b9f422e6fff36da62575d803f132573a24d03c05 100644 | ||
--- a/third_party/blink/renderer/modules/gamepad/gamepad_comparisons_test.cc | ||
+++ b/third_party/blink/renderer/modules/gamepad/gamepad_comparisons_test.cc | ||
@@ -4,9 +4,11 @@ | ||
|
||
#include "third_party/blink/renderer/modules/gamepad/gamepad_comparisons.h" | ||
|
||
+#include "base/test/task_environment.h" | ||
#include "device/gamepad/public/cpp/gamepad.h" | ||
#include "testing/gtest/include/gtest/gtest.h" | ||
#include "third_party/blink/renderer/modules/gamepad/gamepad.h" | ||
+#include "third_party/blink/renderer/platform/testing/main_thread_isolate.h" | ||
|
||
namespace blink { | ||
|
||
@@ -241,6 +243,11 @@ class GamepadComparisonsTest : public testing::Test { | ||
list[0] = gamepad; | ||
return list; | ||
} | ||
+ | ||
+ private: | ||
+ // Needed so we can do v8::Isolate::GetCurrent(). | ||
+ base::test::TaskEnvironment task_environment_; | ||
+ blink::test::MainThreadIsolate isolate_; | ||
}; | ||
|
||
TEST_F(GamepadComparisonsTest, EmptyListCausesNoActivation) { | ||
diff --git a/third_party/blink/renderer/platform/bindings/dom_data_store.h b/third_party/blink/renderer/platform/bindings/dom_data_store.h | ||
index 54d809db8b5d7cb7f15276c3cd741edab92afabb..1111dd3697c268e90f8f98b4622aa1eb14efad7e 100644 | ||
--- a/third_party/blink/renderer/platform/bindings/dom_data_store.h | ||
+++ b/third_party/blink/renderer/platform/bindings/dom_data_store.h | ||
@@ -114,7 +114,7 @@ class DOMDataStore final : public GarbageCollected<DOMDataStore> { | ||
// Clears all references. | ||
void Dispose(); | ||
|
||
- v8::Local<v8::Object> Get(ScriptWrappable* object, v8::Isolate* isolate) { | ||
+ v8::Local<v8::Object> Get(const ScriptWrappable* object, v8::Isolate* isolate) { | ||
if (is_main_world_) | ||
return object->MainWorldWrapper(isolate); | ||
auto it = wrapper_map_.find(object); |