From 593da80b8bdaee8564dac079957e5b021dcc970d Mon Sep 17 00:00:00 2001 From: Vyacheslav Egorov Date: Fri, 27 Jun 2025 06:53:41 -0700 Subject: [PATCH] [vm] Switch reloadSources to object parameters Currently it using legacy stringified parameters which makes it hard to pass complex structured data to it. TEST=ci CoreLibraryReviewExempt: vm-service implementation changes no affecting public corelib APIs. Change-Id: I1291e0a2971ad51fef4bc4a2d53e7ec0a76b3131 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/437221 Reviewed-by: Ben Konyi Commit-Queue: Slava Egorov --- runtime/lib/vmservice.cc | 8 ---- runtime/vm/bootstrap_natives.h | 1 - runtime/vm/service.cc | 74 +++++++++++++++++++++------------- runtime/vm/service.h | 8 +--- runtime/vm/service_test.cc | 44 ++++++++++---------- sdk/lib/vmservice/message.dart | 68 ++++++++++++------------------- 6 files changed, 95 insertions(+), 108 deletions(-) diff --git a/runtime/lib/vmservice.cc b/runtime/lib/vmservice.cc index 3ee5a38895a..5193eef1587 100644 --- a/runtime/lib/vmservice.cc +++ b/runtime/lib/vmservice.cc @@ -78,14 +78,6 @@ DEFINE_NATIVE_ENTRY(VMService_SendRootServiceMessage, 0, 1) { return Object::null(); } -DEFINE_NATIVE_ENTRY(VMService_SendObjectRootServiceMessage, 0, 1) { -#ifndef PRODUCT - GET_NON_NULL_NATIVE_ARGUMENT(Array, message, arguments->NativeArgAt(0)); - return Service::HandleObjectRootMessage(message); -#endif - return Object::null(); -} - DEFINE_NATIVE_ENTRY(VMService_OnStart, 0, 0) { #ifndef PRODUCT if (FLAG_trace_service) { diff --git a/runtime/vm/bootstrap_natives.h b/runtime/vm/bootstrap_natives.h index db5cb2f93a9..53dff2dafb8 100644 --- a/runtime/vm/bootstrap_natives.h +++ b/runtime/vm/bootstrap_natives.h @@ -288,7 +288,6 @@ namespace dart { V(Profiler_getCurrentTag, 0) \ V(VMService_SendIsolateServiceMessage, 2) \ V(VMService_SendRootServiceMessage, 1) \ - V(VMService_SendObjectRootServiceMessage, 1) \ V(VMService_OnStart, 0) \ V(VMService_OnExit, 0) \ V(VMService_OnServerAddressChange, 1) \ diff --git a/runtime/vm/service.cc b/runtime/vm/service.cc index fa3407acbad..1276b5daee1 100644 --- a/runtime/vm/service.cc +++ b/runtime/vm/service.cc @@ -750,6 +750,10 @@ class BoolParameter : public MethodParameter { return (strcmp("true", value) == 0) || (strcmp("false", value) == 0); } + virtual bool ValidateObject(const Object& value) const { + return value.IsBool(); + } + static bool Parse(const char* value, bool default_value = false) { if (value == nullptr) { return default_value; @@ -836,9 +840,15 @@ class RunnableIsolateParameter : public MethodParameter { : MethodParameter(name, true) {} virtual bool Validate(const char* value) const { - Isolate* isolate = Isolate::Current(); - return (value != nullptr) && (isolate != nullptr) && - (isolate->is_runnable()); + // We assume the request has been forwarded to the current isolate and + // isolateId matches it. + return (value != nullptr) && ValidateCurrentIsolate(); + } + + virtual bool ValidateObject(const Object& value) const { + // We assume the request has been forwarded to the current isolate and + // isolateId matches it. + return value.IsString() && ValidateCurrentIsolate(); } virtual void PrintError(const char* name, @@ -847,6 +857,12 @@ class RunnableIsolateParameter : public MethodParameter { js->PrintError(kIsolateMustBeRunnable, "Isolate must be runnable before this request is made."); } + + private: + static bool ValidateCurrentIsolate() { + Isolate* isolate = Isolate::Current(); + return (isolate != nullptr) && (isolate->is_runnable()); + } }; class EnumParameter : public MethodParameter { @@ -961,15 +977,13 @@ void Service::PostError(const String& method_name, js.PostReply(); } -ErrorPtr Service::InvokeMethod(Isolate* I, - const Array& msg, - bool parameters_are_dart_objects) { +ErrorPtr Service::InvokeMethod(Isolate* I, const Array& msg) { Thread* T = Thread::Current(); ASSERT(I == T->isolate()); ASSERT(I != nullptr); ASSERT(T->execution_state() == Thread::kThreadInVM); ASSERT(!msg.IsNull()); - ASSERT(msg.Length() == 6); + ASSERT(msg.Length() == 7); { StackZone zone(T); @@ -978,13 +992,15 @@ ErrorPtr Service::InvokeMethod(Isolate* I, Instance& reply_port = Instance::Handle(Z); Instance& seq = String::Handle(Z); String& method_name = String::Handle(Z); + Bool& parameters_are_dart_objects = Bool::Handle(Z); Array& param_keys = Array::Handle(Z); Array& param_values = Array::Handle(Z); reply_port ^= msg.At(1); seq ^= msg.At(2); method_name ^= msg.At(3); - param_keys ^= msg.At(4); - param_values ^= msg.At(5); + parameters_are_dart_objects ^= msg.At(4); + param_keys ^= msg.At(5); + param_values ^= msg.At(6); ASSERT(!method_name.IsNull()); ASSERT(seq.IsNull() || seq.IsString() || seq.IsNumber()); @@ -1003,7 +1019,7 @@ ErrorPtr Service::InvokeMethod(Isolate* I, Dart_Port reply_port_id = (reply_port.IsNull() ? ILLEGAL_PORT : SendPort::Cast(reply_port).Id()); js.Setup(zone.GetZone(), reply_port_id, seq, method_name, param_keys, - param_values, parameters_are_dart_objects); + param_values, parameters_are_dart_objects.value()); // |id_zone| is the zone that will be stored into the |JSONStream| that we // are about to create, meaning that it is where temporary Service IDs may @@ -1071,11 +1087,6 @@ ErrorPtr Service::HandleRootMessage(const Array& msg_instance) { return InvokeMethod(isolate, msg_instance); } -ErrorPtr Service::HandleObjectRootMessage(const Array& msg_instance) { - Isolate* isolate = Isolate::Current(); - return InvokeMethod(isolate, msg_instance, true); -} - ErrorPtr Service::HandleIsolateMessage(Isolate* isolate, const Array& msg) { ASSERT(isolate != nullptr); const Error& error = Error::Handle(InvokeMethod(isolate, msg)); @@ -3945,7 +3956,7 @@ static const MethodParameter* const reload_kernel_params[] = { RUNNABLE_ISOLATE_PARAMETER, new BoolParameter("force", false), new BoolParameter("pause", false), - new StringParameter("kernelFilePath", false), + new DartStringParameter("kernelFilePath", true), nullptr, }; @@ -3956,11 +3967,6 @@ static void ReloadKernel(Thread* thread, JSONStream* js) { #if defined(DART_PRECOMPILED_RUNTIME) js->PrintError(kFeatureDisabled, "Compiler is disabled in AOT mode."); #else - if (!js->HasParam("kernelFilePath")) { - PrintMissingParamError(js, "kernelFilePath"); - return; - } - IsolateGroup* isolate_group = thread->isolate_group(); if (isolate_group->library_tag_handler() == nullptr) { js->PrintError(kFeatureDisabled, @@ -4000,7 +4006,9 @@ static void ReloadKernel(Thread* thread, JSONStream* js) { return; } - void* file = (*file_open)(js->LookupParam("kernelFilePath"), /*write=*/false); + const String& kernel_file_path = String::CheckedHandle( + thread->zone(), js->LookupObjectParam("kernelFilePath")); + void* file = (*file_open)(kernel_file_path.ToCString(), /*write=*/false); if (file == nullptr) { js->PrintError(kIsolateReloadBarred, "The specified kernel file could not be read. Please ensure " @@ -4013,7 +4021,7 @@ static void ReloadKernel(Thread* thread, JSONStream* js) { (*file_read)(&kernel_buffer, &kernel_buffer_size, file); const bool force_reload = - BoolParameter::Parse(js->LookupParam("force"), false); + js->LookupObjectParam("force") == Bool::True().ptr(); isolate_group->ReloadKernel(js, force_reload, kernel_buffer, kernel_buffer_size); @@ -4027,8 +4035,8 @@ static const MethodParameter* const reload_sources_params[] = { RUNNABLE_ISOLATE_PARAMETER, new BoolParameter("force", false), new BoolParameter("pause", false), - new StringParameter("rootLibUri", false), - new StringParameter("packagesUri", false), + new DartStringParameter("rootLibUri", false), + new DartStringParameter("packagesUri", false), nullptr, }; @@ -4062,10 +4070,18 @@ static void ReloadSources(Thread* thread, JSONStream* js) { return; } const bool force_reload = - BoolParameter::Parse(js->LookupParam("force"), false); + js->LookupObjectParam("force") == Bool::True().ptr(); - isolate_group->ReloadSources(js, force_reload, js->LookupParam("rootLibUri"), - js->LookupParam("packagesUri")); + String& root_lib_uri = String::Handle(thread->zone()); + root_lib_uri ^= js->LookupObjectParam("rootLibUri"); + + String& packages_uri = String::Handle(thread->zone()); + packages_uri ^= js->LookupObjectParam("packagesUri"); + + isolate_group->ReloadSources( + js, force_reload, + root_lib_uri.IsNull() ? nullptr : root_lib_uri.ToCString(), + packages_uri.IsNull() ? nullptr : packages_uri.ToCString()); Service::CheckForPause(isolate, js); @@ -4075,7 +4091,7 @@ static void ReloadSources(Thread* thread, JSONStream* js) { void Service::CheckForPause(Isolate* isolate, JSONStream* stream) { // Should we pause? isolate->set_should_pause_post_service_request( - BoolParameter::Parse(stream->LookupParam("pause"), false)); + stream->LookupObjectParam("pause") == Bool::True().ptr()); } ErrorPtr Service::MaybePause(Isolate* isolate, const Error& error) { diff --git a/runtime/vm/service.h b/runtime/vm/service.h index c77c549c423..beffbd026d7 100644 --- a/runtime/vm/service.h +++ b/runtime/vm/service.h @@ -131,10 +131,6 @@ class Service : public AllStatic { // Handles a message which is not directed to an isolate. static ErrorPtr HandleRootMessage(const Array& message); - // Handles a message which is not directed to an isolate and also - // expects the parameter keys and values to be actual dart objects. - static ErrorPtr HandleObjectRootMessage(const Array& message); - // Handles a message which is directed to a particular isolate. static ErrorPtr HandleIsolateMessage(Isolate* isolate, const Array& message); @@ -249,9 +245,7 @@ class Service : public AllStatic { } private: - static ErrorPtr InvokeMethod(Isolate* isolate, - const Array& message, - bool parameters_are_dart_objects = false); + static ErrorPtr InvokeMethod(Isolate* isolate, const Array& message); static void EmbedderHandleMessage(EmbedderServiceHandler* handler, JSONStream* js); diff --git a/runtime/vm/service_test.cc b/runtime/vm/service_test.cc index c80b2c66f81..b4be98b8d5a 100644 --- a/runtime/vm/service_test.cc +++ b/runtime/vm/service_test.cc @@ -91,16 +91,16 @@ static ArrayPtr Eval(Dart_Handle lib, const char* expr) { Api::UnwrapGrowableObjectArrayHandle(zone, expr_val); const Array& result = Array::Handle(Array::MakeFixedLength(value)); GrowableObjectArray& growable = GrowableObjectArray::Handle(); - growable ^= result.At(4); - // Append dummy isolate id to parameter values. - growable.Add(dummy_isolate_id); - Array& array = Array::Handle(Array::MakeFixedLength(growable)); - result.SetAt(4, array); growable ^= result.At(5); // Append dummy isolate id to parameter values. growable.Add(dummy_isolate_id); - array = Array::MakeFixedLength(growable); + Array& array = Array::Handle(Array::MakeFixedLength(growable)); result.SetAt(5, array); + growable ^= result.At(6); + // Append dummy isolate id to parameter values. + growable.Add(dummy_isolate_id); + array = Array::MakeFixedLength(growable); + result.SetAt(6, array); return result.ptr(); } @@ -278,7 +278,7 @@ ISOLATE_UNIT_TEST_CASE(Service_Code) { // Request an invalid code object. service_msg = - Eval(lib, "[0, port, '0', 'getObject', ['objectId'], ['code/0']]"); + Eval(lib, "[0, port, '0', 'getObject', false, ['objectId'], ['code/0']]"); HandleIsolateMessage(isolate, service_msg); EXPECT_EQ(MessageHandler::kOK, handler.HandleNextMessage()); EXPECT_SUBSTRING("\"error\"", handler.msg()); @@ -286,7 +286,7 @@ ISOLATE_UNIT_TEST_CASE(Service_Code) { // The following test checks that a code object can be found only // at compile_timestamp()-code.EntryPoint(). service_msg = EvalF(lib, - "[0, port, '0', 'getObject', " + "[0, port, '0', 'getObject', false, " "['objectId'], ['code/%" Px64 "-%" Px "']]", compile_timestamp, entry); HandleIsolateMessage(isolate, service_msg); @@ -306,7 +306,7 @@ ISOLATE_UNIT_TEST_CASE(Service_Code) { // Expect this to fail because the address is not the entry point. uintptr_t address = entry + 16; service_msg = EvalF(lib, - "[0, port, '0', 'getObject', " + "[0, port, '0', 'getObject', false, " "['objectId'], ['code/%" Px64 "-%" Px "']]", compile_timestamp, address); HandleIsolateMessage(isolate, service_msg); @@ -317,7 +317,7 @@ ISOLATE_UNIT_TEST_CASE(Service_Code) { // Expect this to fail because the timestamp is wrong. address = entry; service_msg = EvalF(lib, - "[0, port, '0', 'getObject', " + "[0, port, '0', 'getObject', false, " "['objectId'], ['code/%" Px64 "-%" Px "']]", compile_timestamp - 1, address); HandleIsolateMessage(isolate, service_msg); @@ -327,7 +327,7 @@ ISOLATE_UNIT_TEST_CASE(Service_Code) { // Request native code at address. Expect the null code object back. address = last; service_msg = EvalF(lib, - "[0, port, '0', 'getObject', " + "[0, port, '0', 'getObject', false, " "['objectId'], ['code/native-%" Px "']]", address); HandleIsolateMessage(isolate, service_msg); @@ -337,7 +337,7 @@ ISOLATE_UNIT_TEST_CASE(Service_Code) { // Request malformed native code. service_msg = EvalF(lib, - "[0, port, '0', 'getObject', ['objectId'], " + "[0, port, '0', 'getObject', false, ['objectId'], " "['code/native%" Px "']]", address); HandleIsolateMessage(isolate, service_msg); @@ -405,7 +405,7 @@ ISOLATE_UNIT_TEST_CASE(Service_PcDescriptors) { // Fetch object. service_msg = EvalF(lib, - "[0, port, '0', 'getObject', " + "[0, port, '0', 'getObject', false, " "['objectId'], ['%s']]", id); HandleIsolateMessage(isolate, service_msg); @@ -477,7 +477,7 @@ ISOLATE_UNIT_TEST_CASE(Service_LocalVarDescriptors) { // Fetch object. service_msg = EvalF(lib, - "[0, port, '0', 'getObject', " + "[0, port, '0', 'getObject', false, " "['objectId'], ['%s']]", id); HandleIsolateMessage(isolate, service_msg); @@ -538,7 +538,8 @@ ISOLATE_UNIT_TEST_CASE(Service_PersistentHandles) { Array& service_msg = Array::Handle(); // Get persistent handles. - service_msg = Eval(lib, "[0, port, '0', '_getPersistentHandles', [], []]"); + service_msg = + Eval(lib, "[0, port, '0', '_getPersistentHandles', false, [], []]"); HandleIsolateMessage(isolate, service_msg); EXPECT_EQ(MessageHandler::kOK, handler.HandleNextMessage()); // Look for a heart beat. @@ -555,7 +556,8 @@ ISOLATE_UNIT_TEST_CASE(Service_PersistentHandles) { } // Get persistent handles (again). - service_msg = Eval(lib, "[0, port, '0', '_getPersistentHandles', [], []]"); + service_msg = + Eval(lib, "[0, port, '0', '_getPersistentHandles', false, [], []]"); HandleIsolateMessage(isolate, service_msg); EXPECT_EQ(MessageHandler::kOK, handler.HandleNextMessage()); EXPECT_SUBSTRING("\"type\":\"_PersistentHandles\"", handler.msg()); @@ -620,12 +622,12 @@ ISOLATE_UNIT_TEST_CASE(Service_EmbedderRootHandler) { } Array& service_msg = Array::Handle(); - service_msg = Eval(lib, "[0, port, '\"', 'alpha', [], []]"); + service_msg = Eval(lib, "[0, port, '\"', 'alpha', false, [], []]"); HandleRootMessage(service_msg); EXPECT_EQ(MessageHandler::kOK, handler.HandleNextMessage()); EXPECT_STREQ("{\"jsonrpc\":\"2.0\", \"result\":alpha,\"id\":\"\\\"\"}", handler.msg()); - service_msg = Eval(lib, "[0, port, 1, 'beta', [], []]"); + service_msg = Eval(lib, "[0, port, 1, 'beta', false, [], []]"); HandleRootMessage(service_msg); EXPECT_EQ(MessageHandler::kOK, handler.HandleNextMessage()); EXPECT_STREQ("{\"jsonrpc\":\"2.0\", \"error\":beta,\"id\":1}", handler.msg()); @@ -668,12 +670,12 @@ ISOLATE_UNIT_TEST_CASE(Service_EmbedderIsolateHandler) { Isolate* isolate = thread->isolate(); Array& service_msg = Array::Handle(); - service_msg = Eval(lib, "[0, port, '0', 'alpha', [], []]"); + service_msg = Eval(lib, "[0, port, '0', 'alpha', false, [], []]"); HandleIsolateMessage(isolate, service_msg); EXPECT_EQ(MessageHandler::kOK, handler.HandleNextMessage()); EXPECT_STREQ("{\"jsonrpc\":\"2.0\", \"result\":alpha,\"id\":\"0\"}", handler.msg()); - service_msg = Eval(lib, "[0, port, '0', 'beta', [], []]"); + service_msg = Eval(lib, "[0, port, '0', 'beta', false, [], []]"); HandleIsolateMessage(isolate, service_msg); EXPECT_EQ(MessageHandler::kOK, handler.HandleNextMessage()); EXPECT_STREQ("{\"jsonrpc\":\"2.0\", \"error\":beta,\"id\":\"0\"}", @@ -725,7 +727,7 @@ ISOLATE_UNIT_TEST_CASE(Service_Profile) { } Array& service_msg = Array::Handle(); - service_msg = Eval(lib, "[0, port, '0', 'getCpuSamples', [], []]"); + service_msg = Eval(lib, "[0, port, '0', 'getCpuSamples', false, [], []]"); HandleIsolateMessage(isolate, service_msg); EXPECT_EQ(MessageHandler::kOK, handler.HandleNextMessage()); // Expect profile diff --git a/sdk/lib/vmservice/message.dart b/sdk/lib/vmservice/message.dart index 07f0cc82156..80ed579d51c 100644 --- a/sdk/lib/vmservice/message.dart +++ b/sdk/lib/vmservice/message.dart @@ -147,12 +147,29 @@ class Message { // elements in the list are strings, making consumption by C++ simpler. // This has a side effect that boolean literal values like true become 'true' // and thus indistinguishable from the string literal 'true'. - List _makeAllString(List list) { - var new_list = List.filled(list.length, ""); + static void _convertAllToStringInPlace(List list) { for (var i = 0; i < list.length; i++) { - new_list[i] = list[i].toString(); + list[i] = list[i].toString(); } - return new_list; + } + + List _toRequest(RawReceivePort responsePort) { + final parametersAreObjects = _methodNeedsObjectParameters(method!); + final keys = params.keys.toList(growable: false); + var values = params.values.cast().toList(growable: false); + if (!parametersAreObjects) { + _convertAllToStringInPlace(values); + } + // Keep in sync with Service::InvokeMethod in service.cc. + return List.filled(7, null) + ..[0] = + 0 // Make room for OOB message type. + ..[1] = responsePort.sendPort + ..[2] = serial + ..[3] = method + ..[4] = parametersAreObjects + ..[5] = keys + ..[6] = values; } Future sendToIsolate( @@ -168,19 +185,7 @@ class Message { ports.remove(receivePort); _setResponseFromPort(value); }; - final keys = _makeAllString(params.keys.toList(growable: false)); - final values = _makeAllString( - params.values.cast().toList(growable: false), - ); - final request = List.filled(6, null) - ..[0] = - 0 // Make room for OOB message type. - ..[1] = receivePort.sendPort - ..[2] = serial - ..[3] = method - ..[4] = keys - ..[5] = values; - if (!sendIsolateServiceMessage(sendPort, request)) { + if (!sendIsolateServiceMessage(sendPort, _toRequest(receivePort))) { receivePort.close(); ports.remove(receivePort); _completer.complete( @@ -205,6 +210,9 @@ class Message { case '_writeDevFSFiles': case '_readDevFSFile': case '_spawnUri': + case '_reloadKernel': + case '_reloadSources': + case 'reloadSources': return true; default: return false; @@ -217,28 +225,7 @@ class Message { receivePort.close(); _setResponseFromPort(value); }; - var keys = params.keys.toList(growable: false); - var values = params.values.cast().toList(growable: false); - if (!_methodNeedsObjectParameters(method!)) { - keys = _makeAllString(keys); - values = _makeAllString(values); - } - final request = List.filled(6, null) - ..[0] = - 0 // Make room for OOB message type. - ..[1] = receivePort.sendPort - ..[2] = serial - ..[3] = method - ..[4] = keys - ..[5] = values; - - if (_methodNeedsObjectParameters(method!)) { - // We use a different method invocation path here. - sendObjectRootServiceMessage(request); - } else { - sendRootServiceMessage(request); - } - + sendRootServiceMessage(_toRequest(receivePort)); return _completer.future; } @@ -263,6 +250,3 @@ external bool sendIsolateServiceMessage(SendPort sp, List m); @pragma("vm:external-name", "VMService_SendRootServiceMessage") external void sendRootServiceMessage(List m); - -@pragma("vm:external-name", "VMService_SendObjectRootServiceMessage") -external void sendObjectRootServiceMessage(List m);