Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
chore: [29-x-y] cherry-pick 1 changes from 0-M124
* 0d9350b71fd0 from chromium
- Loading branch information
Showing
2 changed files
with
218 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,217 @@ | ||
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 69060a1e5f6937896d385b5ceb70f2d49ef8669c..44898805c8c43f1f09a193e3533a6efb71a97462 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,17 @@ 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::AllWorldsInIsolate(isolate, worlds); | ||
for (const auto& world : worlds) { | ||
@@ -256,6 +265,52 @@ v8::Local<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 a7d4bac99e608bcfaa02dc9bfcef20b5a29d0ddc..9d4827f548ca7db3be85011c68d8346fc8dfa909 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 | ||
@@ -91,6 +91,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, | ||
@@ -101,6 +112,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 90c4c70755babdc8c88a7c6bf02803c5858c2194..43618b8ef4b831678b45c72ca47f5729c4f2aaef 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 db1c7e1592b01ed6b29a607a598b810b81aadde4..f95ccb80b08ca72d160c1169e5d10f09ac0fed50 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); |