From b38bf5b0b1f149f7af3fd90a2ce12344e7191d03 Mon Sep 17 00:00:00 2001 From: Sathya Gunasekaran Date: Tue, 16 Feb 2021 10:27:07 +0000 Subject: [PATCH] [api] Change CreationContext to return a MaybeHandle The current API returns a Handle which can be optionally null and all the users of this API never actually checked for this null value. Previously, this wasn't a problem as all the possible JSObjects that were user visible would return a valid NativeContext but now there are wasm objects that don't have a valid constructor so don't have a NativeContext. Bug: v8:11451, chromium:1166077 Change-Id: I4fd5edf8f1a750e6f0abb931fd41358e5ae4dfcf Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2692695 Commit-Queue: Sathya Gunasekaran Reviewed-by: Toon Verwaest Reviewed-by: Benedikt Meurer Reviewed-by: Ulan Degenbaev Cr-Commit-Position: refs/heads/master@{#72769} --- include/v8.h | 10 +++- src/api/api.cc | 30 +++++++++++- src/builtins/builtins-object.cc | 5 +- src/d8/d8.cc | 7 ++- src/debug/debug-interface.cc | 6 +-- src/debug/debug-interface.h | 2 +- src/debug/debug.cc | 5 +- src/execution/messages.cc | 4 +- src/heap/factory.cc | 3 +- src/inspector/custom-preview.cc | 6 ++- src/inspector/v8-debugger.cc | 6 ++- src/inspector/v8-heap-profiler-agent-impl.cc | 12 ++++- src/objects/js-objects.cc | 15 +++--- src/objects/js-objects.h | 2 +- src/objects/objects.cc | 6 ++- test/cctest/heap/test-embedder-tracing.cc | 4 +- test/cctest/test-api.cc | 49 ++++++++++---------- test/cctest/test-modules.cc | 2 +- test/unittests/api/remote-object-unittest.cc | 4 +- test/unittests/api/v8-object-unittest.cc | 5 +- 20 files changed, 120 insertions(+), 63 deletions(-) diff --git a/include/v8.h b/include/v8.h index 0a858c85c8..3935dcc31a 100644 --- a/include/v8.h +++ b/include/v8.h @@ -4222,12 +4222,18 @@ class V8_EXPORT Object : public Value { /** * Returns the context in which the object was created. */ + V8_DEPRECATE_SOON("Use MaybeLocal GetCreationContext()") Local CreationContext(); + MaybeLocal GetCreationContext(); /** Same as above, but works for Persistents */ - V8_INLINE static Local CreationContext( + V8_DEPRECATE_SOON( + "Use MaybeLocal GetCreationContext(const " + "PersistentBase& object)") + static Local CreationContext(const PersistentBase& object); + V8_INLINE static MaybeLocal GetCreationContext( const PersistentBase& object) { - return object.val_->CreationContext(); + return object.val_->GetCreationContext(); } /** diff --git a/src/api/api.cc b/src/api/api.cc index 0f6ebd24ab..db75f107d2 100644 --- a/src/api/api.cc +++ b/src/api/api.cc @@ -4710,10 +4710,36 @@ Local v8::Object::Clone() { return Utils::ToLocal(result); } +namespace { +Local CreationContextImpl(i::Handle self) { + i::Handle context; + if (self->GetCreationContext().ToHandle(&context)) { + return Utils::ToLocal(context); + } + + return Local(); +} +} // namespace + Local v8::Object::CreationContext() { auto self = Utils::OpenHandle(this); - i::Handle context = self->GetCreationContext(); - return Utils::ToLocal(context); + return CreationContextImpl(self); +} + +Local v8::Object::CreationContext( + const PersistentBase& object) { + auto self = Utils::OpenHandle(object.val_); + return CreationContextImpl(self); +} + +MaybeLocal v8::Object::GetCreationContext() { + auto self = Utils::OpenHandle(this); + i::Handle context; + if (self->GetCreationContext().ToHandle(&context)) { + return Utils::ToLocal(context); + } + + return MaybeLocal(); } int v8::Object::GetIdentityHash() { diff --git a/src/builtins/builtins-object.cc b/src/builtins/builtins-object.cc index 16f81dc3d0..ee4f03eed8 100644 --- a/src/builtins/builtins-object.cc +++ b/src/builtins/builtins-object.cc @@ -157,8 +157,9 @@ Object ObjectLookupAccessor(Isolate* isolate, Handle object, case LookupIterator::ACCESSOR: { Handle maybe_pair = it.GetAccessors(); if (maybe_pair->IsAccessorPair()) { - Handle native_context = - it.GetHolder()->GetCreationContext(); + Handle native_context = it.GetHolder() + ->GetCreationContext() + .ToHandleChecked(); return *AccessorPair::GetComponent( isolate, native_context, Handle::cast(maybe_pair), component); diff --git a/src/d8/d8.cc b/src/d8/d8.cc index e27d91d1a5..999e8c2b96 100644 --- a/src/d8/d8.cc +++ b/src/d8/d8.cc @@ -1527,7 +1527,12 @@ void Shell::RealmOwner(const v8::FunctionCallbackInfo& args) { i::Handle::cast(i_object)->IsDetached()) { return; } - int index = data->RealmFind(object->CreationContext()); + Local creation_context; + if (!object->GetCreationContext().ToLocal(&creation_context)) { + Throw(args.GetIsolate(), "object doesn't have creation context"); + return; + } + int index = data->RealmFind(creation_context); if (index == -1) return; args.GetReturnValue().Set(index); } diff --git a/src/debug/debug-interface.cc b/src/debug/debug-interface.cc index d6f26bc415..a53f912625 100644 --- a/src/debug/debug-interface.cc +++ b/src/debug/debug-interface.cc @@ -192,12 +192,12 @@ bool GetPrivateMembers(Local context, Local value, return true; } -Local GetCreationContext(Local value) { +MaybeLocal GetCreationContext(Local value) { i::Handle val = Utils::OpenHandle(*value); if (val->IsJSGlobalProxy()) { - return Local(); + return MaybeLocal(); } - return value->CreationContext(); + return value->GetCreationContext(); } void ChangeBreakOnException(Isolate* isolate, ExceptionBreakState type) { diff --git a/src/debug/debug-interface.h b/src/debug/debug-interface.h index 2c821c8c76..f04a91be32 100644 --- a/src/debug/debug-interface.h +++ b/src/debug/debug-interface.h @@ -73,7 +73,7 @@ V8_EXPORT_PRIVATE bool GetPrivateMembers(Local context, * Forwards to v8::Object::CreationContext, but with special handling for * JSGlobalProxy objects. */ -Local GetCreationContext(Local value); +MaybeLocal GetCreationContext(Local value); enum ExceptionBreakState { NoBreakOnException = 0, diff --git a/src/debug/debug.cc b/src/debug/debug.cc index f61080bf93..0b2b9969b6 100644 --- a/src/debug/debug.cc +++ b/src/debug/debug.cc @@ -1423,8 +1423,9 @@ void Debug::InstallDebugBreakTrampoline() { } if (recorded.find(accessor_pair) != recorded.end()) continue; - needs_instantiate.emplace_back(handle(accessor_pair, isolate_), - object.GetCreationContext()); + needs_instantiate.emplace_back( + handle(accessor_pair, isolate_), + object.GetCreationContext().ToHandleChecked()); recorded.insert(accessor_pair); } } diff --git a/src/execution/messages.cc b/src/execution/messages.cc index 611ffacaee..b0ac822298 100644 --- a/src/execution/messages.cc +++ b/src/execution/messages.cc @@ -304,8 +304,8 @@ MaybeHandle ErrorUtils::FormatStackTrace(Isolate* isolate, Handle elems = Handle::cast(raw_stack); const bool in_recursion = isolate->formatting_stack_trace(); - if (!in_recursion) { - Handle error_context = error->GetCreationContext(); + Handle error_context; + if (!in_recursion && error->GetCreationContext().ToHandle(&error_context)) { DCHECK(error_context->IsNativeContext()); if (isolate->HasPrepareStackTraceCallback()) { diff --git a/src/heap/factory.cc b/src/heap/factory.cc index bf83a17e1d..0c7bb7c21e 100644 --- a/src/heap/factory.cc +++ b/src/heap/factory.cc @@ -2574,7 +2574,8 @@ MaybeHandle Factory::NewJSBoundFunction( isolate(), prototype, JSReceiver::GetPrototype(isolate(), target_function), JSBoundFunction); - SaveAndSwitchContext save(isolate(), *target_function->GetCreationContext()); + SaveAndSwitchContext save( + isolate(), *target_function->GetCreationContext().ToHandleChecked()); // Create the [[BoundArguments]] for the result. Handle bound_arguments; diff --git a/src/inspector/custom-preview.cc b/src/inspector/custom-preview.cc index 393e0f15c5..d8e88861cb 100644 --- a/src/inspector/custom-preview.cc +++ b/src/inspector/custom-preview.cc @@ -249,7 +249,11 @@ void generateCustomPreview(int sessionId, const String16& groupName, v8::Local object, v8::MaybeLocal maybeConfig, int maxDepth, std::unique_ptr* preview) { - v8::Local context = object->CreationContext(); + v8::Local context; + if (!object->GetCreationContext().ToLocal(&context)) { + return; + } + v8::Isolate* isolate = context->GetIsolate(); v8::MicrotasksScope microtasksScope(isolate, v8::MicrotasksScope::kDoNotRunMicrotasks); diff --git a/src/inspector/v8-debugger.cc b/src/inspector/v8-debugger.cc index f986938c2e..0a17f9e2f8 100644 --- a/src/inspector/v8-debugger.cc +++ b/src/inspector/v8-debugger.cc @@ -43,8 +43,10 @@ class MatchPrototypePredicate : public v8::debug::QueryObjectPredicate { bool Filter(v8::Local object) override { if (object->IsModuleNamespaceObject()) return false; - v8::Local objectContext = - v8::debug::GetCreationContext(object); + v8::Local objectContext; + if (!v8::debug::GetCreationContext(object).ToLocal(&objectContext)) { + return false; + } if (objectContext != m_context) return false; if (!m_inspector->client()->isInspectableHeapObject(object)) return false; // Get prototype chain for current object until first visited prototype. diff --git a/src/inspector/v8-heap-profiler-agent-impl.cc b/src/inspector/v8-heap-profiler-agent-impl.cc index a845bcbae3..6472c5ca94 100644 --- a/src/inspector/v8-heap-profiler-agent-impl.cc +++ b/src/inspector/v8-heap-profiler-agent-impl.cc @@ -54,9 +54,13 @@ class GlobalObjectNameResolver final : m_offset(0), m_strings(10000), m_session(session) {} const char* GetName(v8::Local object) override { + v8::Local creationContext; + if (!object->GetCreationContext().ToLocal(&creationContext)) { + return ""; + } InspectedContext* context = m_session->inspector()->getContext( m_session->contextGroupId(), - InspectedContext::contextId(object->CreationContext())); + InspectedContext::contextId(creationContext)); if (!context) return ""; String16 name = context->origin(); size_t length = name.length(); @@ -286,7 +290,11 @@ Response V8HeapProfilerAgentImpl::getObjectByHeapObjectId( if (!m_session->inspector()->client()->isInspectableHeapObject(heapObject)) return Response::ServerError("Object is not available"); - *result = m_session->wrapObject(heapObject->CreationContext(), heapObject, + v8::Local creationContext; + if (!heapObject->GetCreationContext().ToLocal(&creationContext)) { + return Response::ServerError("Object is not available"); + } + *result = m_session->wrapObject(creationContext, heapObject, objectGroup.fromMaybe(""), false); if (!*result) return Response::ServerError("Object is not available"); return Response::Success(); diff --git a/src/objects/js-objects.cc b/src/objects/js-objects.cc index dbdce8cef6..91006bcf2d 100644 --- a/src/objects/js-objects.cc +++ b/src/objects/js-objects.cc @@ -534,7 +534,7 @@ Handle JSReceiver::GetConstructorName(Handle receiver) { return GetConstructorHelper(receiver).second; } -Handle JSReceiver::GetCreationContext() { +MaybeHandle JSReceiver::GetCreationContext() { JSReceiver receiver = *this; // Externals are JSObjects with null as a constructor. DCHECK(!receiver.IsExternal(GetIsolate())); @@ -544,20 +544,19 @@ Handle JSReceiver::GetCreationContext() { function = JSFunction::cast(constructor); } else if (constructor.IsFunctionTemplateInfo()) { // Remote objects don't have a creation context. - return Handle::null(); + return MaybeHandle(); } else if (receiver.IsJSGeneratorObject()) { function = JSGeneratorObject::cast(receiver).function(); - } else { - // Functions have null as a constructor, - // but any JSFunction knows its context immediately. - CHECK(receiver.IsJSFunction()); + } else if (receiver.IsJSFunction()) { function = JSFunction::cast(receiver); + } else { + return MaybeHandle(); } return function.has_context() ? Handle(function.context().native_context(), receiver.GetIsolate()) - : Handle::null(); + : MaybeHandle(); } // static @@ -1696,7 +1695,7 @@ Maybe JSReceiver::GetOwnPropertyDescriptor(LookupIterator* it, Handle accessors = Handle::cast(it->GetAccessors()); Handle native_context = - it->GetHolder()->GetCreationContext(); + it->GetHolder()->GetCreationContext().ToHandleChecked(); // 6a. Set D.[[Get]] to the value of X's [[Get]] attribute. desc->set_get(AccessorPair::GetComponent(isolate, native_context, accessors, ACCESSOR_GETTER)); diff --git a/src/objects/js-objects.h b/src/objects/js-objects.h index ed1b705f3a..1e7512642f 100644 --- a/src/objects/js-objects.h +++ b/src/objects/js-objects.h @@ -227,7 +227,7 @@ class JSReceiver : public HeapObject { // returned instead. static Handle GetConstructorName(Handle receiver); - V8_EXPORT_PRIVATE Handle GetCreationContext(); + V8_EXPORT_PRIVATE MaybeHandle GetCreationContext(); V8_WARN_UNUSED_RESULT static inline Maybe GetPropertyAttributes(Handle object, Handle name); diff --git a/src/objects/objects.cc b/src/objects/objects.cc index 58ed1e413e..d9cb7486be 100644 --- a/src/objects/objects.cc +++ b/src/objects/objects.cc @@ -1490,7 +1490,8 @@ MaybeHandle Object::GetPropertyWithAccessor(LookupIterator* it) { // Regular accessor. Handle getter(accessor_pair->getter(), isolate); if (getter->IsFunctionTemplateInfo()) { - SaveAndSwitchContext save(isolate, *holder->GetCreationContext()); + SaveAndSwitchContext save(isolate, + *holder->GetCreationContext().ToHandleChecked()); return Builtins::InvokeApiFunction( isolate, false, Handle::cast(getter), receiver, 0, nullptr, isolate->factory()->undefined_value()); @@ -1595,7 +1596,8 @@ Maybe Object::SetPropertyWithAccessor( // Regular accessor. Handle setter(AccessorPair::cast(*structure).setter(), isolate); if (setter->IsFunctionTemplateInfo()) { - SaveAndSwitchContext save(isolate, *holder->GetCreationContext()); + SaveAndSwitchContext save(isolate, + *holder->GetCreationContext().ToHandleChecked()); Handle argv[] = {value}; RETURN_ON_EXCEPTION_VALUE( isolate, diff --git a/test/cctest/heap/test-embedder-tracing.cc b/test/cctest/heap/test-embedder-tracing.cc index 1613e3e15d..6b5ebb4bc8 100644 --- a/test/cctest/heap/test-embedder-tracing.cc +++ b/test/cctest/heap/test-embedder-tracing.cc @@ -82,7 +82,9 @@ class TestEmbedderHeapTracer final : public v8::EmbedderHeapTracer { void TracePrologue(EmbedderHeapTracer::TraceFlags) final { if (prologue_behavior_ == TracePrologueBehavior::kCallV8WriteBarrier) { auto local = array_.Get(isolate()); - local->Set(local->CreationContext(), 0, v8::Object::New(isolate())) + local + ->Set(local->GetCreationContext().ToLocalChecked(), 0, + v8::Object::New(isolate())) .Check(); } } diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index 44cb5105bf..2b0662657e 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -19079,49 +19079,49 @@ THREADED_TEST(CreationContext) { { Local other_context = Context::New(isolate); Context::Scope scope(other_context); - CHECK(object1->CreationContext() == context1); + CHECK(object1->GetCreationContext().ToLocalChecked() == context1); CheckContextId(object1, 1); - CHECK(func1->CreationContext() == context1); + CHECK(func1->GetCreationContext().ToLocalChecked() == context1); CheckContextId(func1, 1); - CHECK(instance1->CreationContext() == context1); + CHECK(instance1->GetCreationContext().ToLocalChecked() == context1); CheckContextId(instance1, 1); - CHECK(object2->CreationContext() == context2); + CHECK(object2->GetCreationContext().ToLocalChecked() == context2); CheckContextId(object2, 2); - CHECK(func2->CreationContext() == context2); + CHECK(func2->GetCreationContext().ToLocalChecked() == context2); CheckContextId(func2, 2); - CHECK(instance2->CreationContext() == context2); + CHECK(instance2->GetCreationContext().ToLocalChecked() == context2); CheckContextId(instance2, 2); } { Context::Scope scope(context1); - CHECK(object1->CreationContext() == context1); + CHECK(object1->GetCreationContext().ToLocalChecked() == context1); CheckContextId(object1, 1); - CHECK(func1->CreationContext() == context1); + CHECK(func1->GetCreationContext().ToLocalChecked() == context1); CheckContextId(func1, 1); - CHECK(instance1->CreationContext() == context1); + CHECK(instance1->GetCreationContext().ToLocalChecked() == context1); CheckContextId(instance1, 1); - CHECK(object2->CreationContext() == context2); + CHECK(object2->GetCreationContext().ToLocalChecked() == context2); CheckContextId(object2, 2); - CHECK(func2->CreationContext() == context2); + CHECK(func2->GetCreationContext().ToLocalChecked() == context2); CheckContextId(func2, 2); - CHECK(instance2->CreationContext() == context2); + CHECK(instance2->GetCreationContext().ToLocalChecked() == context2); CheckContextId(instance2, 2); } { Context::Scope scope(context2); - CHECK(object1->CreationContext() == context1); + CHECK(object1->GetCreationContext().ToLocalChecked() == context1); CheckContextId(object1, 1); - CHECK(func1->CreationContext() == context1); + CHECK(func1->GetCreationContext().ToLocalChecked() == context1); CheckContextId(func1, 1); - CHECK(instance1->CreationContext() == context1); + CHECK(instance1->GetCreationContext().ToLocalChecked() == context1); CheckContextId(instance1, 1); - CHECK(object2->CreationContext() == context2); + CHECK(object2->GetCreationContext().ToLocalChecked() == context2); CheckContextId(object2, 2); - CHECK(func2->CreationContext() == context2); + CHECK(func2->GetCreationContext().ToLocalChecked() == context2); CheckContextId(func2, 2); - CHECK(instance2->CreationContext() == context2); + CHECK(instance2->GetCreationContext().ToLocalChecked() == context2); CheckContextId(instance2, 2); } } @@ -19140,7 +19140,7 @@ THREADED_TEST(CreationContextOfJsFunction) { Local other_context = Context::New(CcTest::isolate()); Context::Scope scope(other_context); - CHECK(function->CreationContext() == context); + CHECK(function->GetCreationContext().ToLocalChecked() == context); CheckContextId(function, 1); } @@ -19171,13 +19171,12 @@ THREADED_TEST(CreationContextOfJsBoundFunction) { Local other_context = Context::New(CcTest::isolate()); Context::Scope scope(other_context); - CHECK(bound_function1->CreationContext() == context1); + CHECK(bound_function1->GetCreationContext().ToLocalChecked() == context1); CheckContextId(bound_function1, 1); - CHECK(bound_function2->CreationContext() == context1); + CHECK(bound_function2->GetCreationContext().ToLocalChecked() == context1); CheckContextId(bound_function2, 1); } - void HasOwnPropertyIndexedPropertyGetter( uint32_t index, const v8::PropertyCallbackInfo& info) { @@ -24775,7 +24774,7 @@ TEST(ClassPrototypeCreationContext) { Local result = Local::Cast( CompileRun("'use strict'; class Example { }; Example.prototype")); - CHECK(env.local() == result->CreationContext()); + CHECK(env.local() == result->GetCreationContext().ToLocalChecked()); } @@ -25554,7 +25553,7 @@ TEST(ObjectTemplatePerContextIntrinsics) { object->Get(env.local(), v8_str("values")).ToLocalChecked()); auto fn = i::Handle::cast(v8::Utils::OpenHandle(*values)); auto ctx = v8::Utils::OpenHandle(*env.local()); - CHECK_EQ(*fn->GetCreationContext(), *ctx); + CHECK_EQ(*(fn->GetCreationContext().ToHandleChecked()), *ctx); { LocalContext env2; @@ -25570,7 +25569,7 @@ TEST(ObjectTemplatePerContextIntrinsics) { object2->Get(env2.local(), v8_str("values")).ToLocalChecked()); auto fn2 = i::Handle::cast(v8::Utils::OpenHandle(*values2)); auto ctx2 = v8::Utils::OpenHandle(*env2.local()); - CHECK_EQ(*fn2->GetCreationContext(), *ctx2); + CHECK_EQ(*(fn2->GetCreationContext().ToHandleChecked()), *ctx2); } } diff --git a/test/cctest/test-modules.cc b/test/cctest/test-modules.cc index c4fee8ccc9..ab3afab758 100644 --- a/test/cctest/test-modules.cc +++ b/test/cctest/test-modules.cc @@ -743,7 +743,7 @@ TEST(ModuleNamespace) { Local ns = module->GetModuleNamespace(); CHECK_EQ(Module::kInstantiated, module->GetStatus()); Local nsobj = ns->ToObject(env.local()).ToLocalChecked(); - CHECK_EQ(nsobj->CreationContext(), env.local()); + CHECK_EQ(nsobj->GetCreationContext().ToLocalChecked(), env.local()); // a, b CHECK(nsobj->Get(env.local(), v8_str("a")).ToLocalChecked()->IsUndefined()); diff --git a/test/unittests/api/remote-object-unittest.cc b/test/unittests/api/remote-object-unittest.cc index a73db835a4..5b350365c4 100644 --- a/test/unittests/api/remote-object-unittest.cc +++ b/test/unittests/api/remote-object-unittest.cc @@ -39,7 +39,7 @@ TEST_F(RemoteObjectTest, CreationContextOfRemoteContext) { Local remote_context = Context::NewRemoteContext(isolate(), global_template).ToLocalChecked(); - EXPECT_TRUE(remote_context->CreationContext().IsEmpty()); + EXPECT_TRUE(remote_context->GetCreationContext().IsEmpty()); } TEST_F(RemoteObjectTest, CreationContextOfRemoteObject) { @@ -51,7 +51,7 @@ TEST_F(RemoteObjectTest, CreationContextOfRemoteObject) { Local remote_object = constructor_template->NewRemoteInstance().ToLocalChecked(); - EXPECT_TRUE(remote_object->CreationContext().IsEmpty()); + EXPECT_TRUE(remote_object->GetCreationContext().IsEmpty()); } TEST_F(RemoteObjectTest, RemoteContextInstanceChecks) { diff --git a/test/unittests/api/v8-object-unittest.cc b/test/unittests/api/v8-object-unittest.cc index a3c0c2574c..9ebdb12fa7 100644 --- a/test/unittests/api/v8-object-unittest.cc +++ b/test/unittests/api/v8-object-unittest.cc @@ -77,8 +77,9 @@ TEST_F(LapContextTest, CurrentContextInLazyAccessorOnPrototype) { Local object = interface_for_receiver->NewInstance(receiver_context).ToLocalChecked(); object->SetPrototype(caller_context, prototype).ToChecked(); - EXPECT_EQ(receiver_context, object->CreationContext()); - EXPECT_EQ(prototype_context, prototype->CreationContext()); + EXPECT_EQ(receiver_context, object->GetCreationContext().ToLocalChecked()); + EXPECT_EQ(prototype_context, + prototype->GetCreationContext().ToLocalChecked()); EXPECT_EQ(0, call_count); object->Get(caller_context, property_key).ToLocalChecked();