Skip to content

Commit

Permalink
Reland "Get WrapperTypeInfo via ScriptWrappable"
Browse files Browse the repository at this point in the history
This reverts commit c07cbfe.

Reason for revert: relanding with tests and fixes

Original change's description:
> Revert "Get WrapperTypeInfo via ScriptWrappable"
>
> This reverts commit 81b6e3d.
>
> Reason for revert: Breaks AutotestPrivateApiTest.AutotestPrivate on linux-chromeos-rel
>
> https://ci.chromium.org/ui/p/chromium/builders/luci.chromium.ci/linux-chromeos-rel
> Sample failure: https://ci.chromium.org/ui/p/chromium/builders/ci/linux-chromeos-rel/75905/overview
>
> I think this is the relevant part of the stack trace:
>
> ../../content/public/test/no_renderer_crashes_assertion.cc:102: Failure
> Failed
> Unexpected termination of a renderer process; status: 3, exit_code: 139
> Stack trace:
> #0 0x563e8b77c0da content::NoRendererCrashesAssertion::RenderProcessExited()
> #1 0x563e8967d8c4 content::RenderProcessHostImpl::ProcessDied()
> #2 0x563e8967d72c content::RenderProcessHostImpl::OnChannelError()
> #3 0x563e8ae3ecb5 base::TaskAnnotator::RunTaskImpl()
> #4 0x563e8ae583dd base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWorkImpl()
> #5 0x563e8ae57e60 base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork()
> #6 0x563e8ae58845 base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork()
> #7 0x563e8aec2fef base::MessagePumpEpoll::Run()
> #8 0x563e8ae58bb2 base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::Run()
> #9 0x563e8ae1e7fd base::RunLoop::Run()
> #10 0x563e91a0c6c9 extensions::ResultCatcher::GetNextResult()
> #11 0x563e8ad7fcf6 extensions::ExtensionApiTest::RunExtensionTest()
> #12 0x563e8ad7f999 extensions::ExtensionApiTest::RunExtensionTest()
> #13 0x563e854402ed extensions::AutotestPrivateApiTest::RunAutotestPrivateExtensionTest()
>
> Original change's description:
> > Get WrapperTypeInfo via ScriptWrappable
> >
> > as opposed to using a dedicated internal field for that.
> >
> > Bug: 328117814
> > Change-Id: I01f9aff3ad8a41fafbd2655d23f076a0f76fdc57
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5455405
> > Reviewed-by: Nate Chapin <japhet@chromium.org>
> > Commit-Queue: Andrey Kosyakov <caseq@chromium.org>
> > Cr-Commit-Position: refs/heads/main@{#1288405}
>
> Bug: 328117814
> Change-Id: Id0ad5b6bcab7a99cf31d551df00928708dd93465
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5459075
> Reviewed-by: Jiacheng Guo <gjc@google.com>
> Auto-Submit: Timothy Loh <timloh@chromium.org>
> Commit-Queue: Jiacheng Guo <gjc@google.com>
> Owners-Override: Timothy Loh <timloh@chromium.org>
> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Cr-Commit-Position: refs/heads/main@{#1288546}

Bug: 328117814
Change-Id: Icc69d23b24b71adc9332e3a01c91f336506c035f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5466905
Commit-Queue: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1290020}
  • Loading branch information
caseq authored and Chromium LUCI CQ committed Apr 19, 2024
1 parent 86ac886 commit b148acd
Show file tree
Hide file tree
Showing 8 changed files with 99 additions and 15 deletions.
@@ -0,0 +1,5 @@
specific_include_rules = {
"v8_script_value_serializer_test.cc": [
"+gin/wrappable.h"
]
}
Expand Up @@ -7,6 +7,7 @@
#include "base/test/scoped_feature_list.h"
#include "base/time/time.h"
#include "build/build_config.h"
#include "gin/wrappable.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/public/common/features.h"
Expand Down Expand Up @@ -2380,4 +2381,40 @@ TEST(V8ScriptValueSerializerTest, RoundTripFencedFrameConfigNullValues) {
EXPECT_FALSE(new_config->content_size_.has_value());
}

namespace {

class GinWrappable : public gin::Wrappable<GinWrappable> {
public:
static v8::Local<v8::Object> Create(v8::Isolate* isolate) {
auto* instance = new GinWrappable();
return instance->GetWrapper(isolate).ToLocalChecked();
}
~GinWrappable() override = default;

static gin::WrapperInfo kWrapperInfo;

private:
GinWrappable() = default;
};

gin::WrapperInfo GinWrappable::kWrapperInfo = {gin::kEmbedderNativeGin};

} // namespace

TEST(V8ScriptValueSerializerTest, CoexistWithGin) {
test::TaskEnvironment task_environment;
V8TestingScope scope;
v8::Isolate* const isolate = scope.GetIsolate();
v8::Local<v8::Object> wrapper = GinWrappable::Create(isolate);
ExceptionState exception_state(
isolate, ExceptionContextType::kOperationInvoke, "Window", "postMessage");
scoped_refptr<SerializedScriptValue> serialized_script_value =
V8ScriptValueSerializer(scope.GetScriptState())
.Serialize(wrapper, exception_state);
// Serializing a gin value will throw an exception, which is fine.
// We just want to make sure it does not crash.
EXPECT_TRUE(exception_state.HadException());
EXPECT_FALSE(serialized_script_value);
}

} // namespace blink
Expand Up @@ -271,6 +271,22 @@ void DeserializeInternalFieldCallback(v8::Local<v8::Object> object,
}
}

namespace {
// We only care for WrapperTypeInfo and do not supply an actual instance of
// the document. Since we need a script wrappable to get type info now, this
// class is a minimal implementation of ScriptWrappable that returns correct
// type info for HTMLDocument.
class DummyHTMLDocumentForSnapshot : public ScriptWrappable {
public:
DummyHTMLDocumentForSnapshot() = default;

private:
const WrapperTypeInfo* GetWrapperTypeInfo() const override {
return V8HTMLDocument::GetWrapperTypeInfo();
}
};
} // namespace

void TakeSnapshotForWorld(v8::SnapshotCreator* snapshot_creator,
const DOMWrapperWorld& world) {
v8::Isolate* isolate = snapshot_creator->GetIsolate();
Expand Down Expand Up @@ -300,11 +316,9 @@ void TakeSnapshotForWorld(v8::SnapshotCreator* snapshot_creator,
v8::Local<v8::Object> document_wrapper = CreatePlatformObject(
isolate, context, world, document_wrapper_type_info);

int indices[] = {kV8DOMWrapperObjectIndex, kV8DOMWrapperTypeIndex};
void* values[] = {nullptr,
const_cast<WrapperTypeInfo*>(document_wrapper_type_info)};
document_wrapper->SetAlignedPointerInInternalFields(std::size(indices),
indices, values);
V8DOMWrapper::SetNativeInfo(
isolate, document_wrapper, document_wrapper_type_info,
MakeGarbageCollected<DummyHTMLDocumentForSnapshot>());

V8PrivateProperty::GetWindowDocumentCachedAccessor(isolate).Set(
context->Global(), document_wrapper);
Expand Down
Expand Up @@ -76,8 +76,12 @@ bool V8DOMWrapper::IsWrapper(v8::Isolate* isolate, v8::Local<v8::Value> value) {
if (object->InternalFieldCount() < kV8DefaultWrapperInternalFieldCount)
return false;

// TODO(b/328117814): this is provisional until migration to new wrappers.
// ToWrapperTypeInfo() can't be used unless we're certainly dealing with
// a DOM wrapper object. Attempting to use it on a wrapper object from
// a parallel universe would result in a crash.
const WrapperTypeInfo* untrusted_wrapper_type_info =
ToWrapperTypeInfo(object);
GetInternalField<WrapperTypeInfo, kV8DOMWrapperTypeIndex>(object);
V8PerIsolateData* per_isolate_data = V8PerIsolateData::From(isolate);
if (!(untrusted_wrapper_type_info && per_isolate_data))
return false;
Expand Down
Expand Up @@ -57,13 +57,14 @@ v8::Local<v8::Template> WrapperTypeInfo::GetV8ClassTemplate(
return v8_template;
}

const WrapperTypeInfo* ToWrapperTypeInfo(
const v8::TracedReference<v8::Object>& wrapper) {
return GetInternalField<WrapperTypeInfo, kV8DOMWrapperTypeIndex>(wrapper);
}

const WrapperTypeInfo* ToWrapperTypeInfo(v8::Local<v8::Object> wrapper) {
return GetInternalField<WrapperTypeInfo, kV8DOMWrapperTypeIndex>(wrapper);
const auto* wrappable = ToScriptWrappable(wrapper->GetIsolate(), wrapper);
const WrapperTypeInfo* type_info =
wrappable ? wrappable->GetWrapperTypeInfo() : nullptr;
DCHECK_EQ(
type_info,
(GetInternalField<WrapperTypeInfo, kV8DOMWrapperTypeIndex>(wrapper)));
return type_info;
}

} // namespace blink
Expand Up @@ -206,9 +206,6 @@ inline ScriptWrappable* ToScriptWrappable(v8::Isolate* isolate,
wrapper);
}

PLATFORM_EXPORT const WrapperTypeInfo* ToWrapperTypeInfo(
const v8::TracedReference<v8::Object>& wrapper);

PLATFORM_EXPORT const WrapperTypeInfo* ToWrapperTypeInfo(
v8::Local<v8::Object> wrapper);

Expand Down
@@ -0,0 +1,9 @@
CONSOLE ERROR: Uncaught (in promise) [object Object]
Tests that we do not crash when rejecting a promise with a gin wrapper.

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".

PASS successfullyParsed is true

TEST COMPLETE

@@ -0,0 +1,17 @@
<!DOCTYPE html>
<script src="../../resources/js-test.js"></script>
<body>
<script>
description('Tests that we do not crash when rejecting a promise with a gin wrapper.');

window.jsTestIsAsync = true;
(async () => {
const promise = new Promise((_, reject) => { reject(testRunner); })
// Finish async, so a an exception thrown by reject() above
// wont' throw us off.
Promise.resolve().then(() => finishJSTest());
await promise;
})();

</script>

0 comments on commit b148acd

Please sign in to comment.