[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 <bkonyi@google.com> Commit-Queue: Slava Egorov <vegorov@google.com>
diff --git a/runtime/lib/vmservice.cc b/runtime/lib/vmservice.cc index 3ee5a38..5193eef 100644 --- a/runtime/lib/vmservice.cc +++ b/runtime/lib/vmservice.cc
@@ -78,14 +78,6 @@ 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 db5cb2f..53dff2d 100644 --- a/runtime/vm/bootstrap_natives.h +++ b/runtime/vm/bootstrap_natives.h
@@ -288,7 +288,6 @@ 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 fa3407a..1276b5d 100644 --- a/runtime/vm/service.cc +++ b/runtime/vm/service.cc
@@ -750,6 +750,10 @@ 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 @@ : 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ RUNNABLE_ISOLATE_PARAMETER, new BoolParameter("force", false), new BoolParameter("pause", false), - new StringParameter("kernelFilePath", false), + new DartStringParameter("kernelFilePath", true), nullptr, }; @@ -3956,11 +3967,6 @@ #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 @@ 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 @@ (*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 @@ 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 @@ 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 @@ 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 c77c549..beffbd0 100644 --- a/runtime/vm/service.h +++ b/runtime/vm/service.h
@@ -131,10 +131,6 @@ // 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 @@ } 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 c80b2c6..b4be98b 100644 --- a/runtime/vm/service_test.cc +++ b/runtime/vm/service_test.cc
@@ -91,16 +91,16 @@ 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 @@ // 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 @@ // 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 @@ // 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 @@ // 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 @@ // 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 @@ // 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 @@ // 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 @@ // 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 @@ 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 @@ } // 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 @@ } 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* 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 @@ } 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 07f0cc8..80ed579 100644 --- a/sdk/lib/vmservice/message.dart +++ b/sdk/lib/vmservice/message.dart
@@ -147,12 +147,29 @@ // 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<String> _makeAllString(List<Object?> list) { - var new_list = List<String>.filled(list.length, ""); + static void _convertAllToStringInPlace(List<Object?> list) { for (var i = 0; i < list.length; i++) { - new_list[i] = list[i].toString(); + list[i] = list[i].toString(); } - return new_list; + } + + List<Object?> _toRequest(RawReceivePort responsePort) { + final parametersAreObjects = _methodNeedsObjectParameters(method!); + final keys = params.keys.toList(growable: false); + var values = params.values.cast<Object?>().toList(growable: false); + if (!parametersAreObjects) { + _convertAllToStringInPlace(values); + } + // Keep in sync with Service::InvokeMethod in service.cc. + return List<Object?>.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<Response> sendToIsolate( @@ -168,19 +185,7 @@ ports.remove(receivePort); _setResponseFromPort(value); }; - final keys = _makeAllString(params.keys.toList(growable: false)); - final values = _makeAllString( - params.values.cast<Object?>().toList(growable: false), - ); - final request = List<Object?>.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 @@ case '_writeDevFSFiles': case '_readDevFSFile': case '_spawnUri': + case '_reloadKernel': + case '_reloadSources': + case 'reloadSources': return true; default: return false; @@ -217,28 +225,7 @@ receivePort.close(); _setResponseFromPort(value); }; - var keys = params.keys.toList(growable: false); - var values = params.values.cast<Object>().toList(growable: false); - if (!_methodNeedsObjectParameters(method!)) { - keys = _makeAllString(keys); - values = _makeAllString(values); - } - final request = List<dynamic>.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 @@ @pragma("vm:external-name", "VMService_SendRootServiceMessage") external void sendRootServiceMessage(List<Object?> m); - -@pragma("vm:external-name", "VMService_SendObjectRootServiceMessage") -external void sendObjectRootServiceMessage(List<Object?> m);