Skip to content

Commit 593da80

Browse files
mralephCommit Queue
mraleph
authored and
Commit Queue
committed
[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 <[email protected]> Commit-Queue: Slava Egorov <[email protected]>
1 parent 81cb17e commit 593da80

File tree

6 files changed

+93
-106
lines changed

6 files changed

+93
-106
lines changed

‎runtime/lib/vmservice.cc‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -78,14 +78,6 @@ DEFINE_NATIVE_ENTRY(VMService_SendRootServiceMessage, 0, 1) {
7878
return Object::null();
7979
}
8080

81-
DEFINE_NATIVE_ENTRY(VMService_SendObjectRootServiceMessage, 0, 1) {
82-
#ifndef PRODUCT
83-
GET_NON_NULL_NATIVE_ARGUMENT(Array, message, arguments->NativeArgAt(0));
84-
return Service::HandleObjectRootMessage(message);
85-
#endif
86-
return Object::null();
87-
}
88-
8981
DEFINE_NATIVE_ENTRY(VMService_OnStart, 0, 0) {
9082
#ifndef PRODUCT
9183
if (FLAG_trace_service) {

‎runtime/vm/bootstrap_natives.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -288,7 +288,6 @@ namespace dart {
288288
V(Profiler_getCurrentTag, 0) \
289289
V(VMService_SendIsolateServiceMessage, 2) \
290290
V(VMService_SendRootServiceMessage, 1) \
291-
V(VMService_SendObjectRootServiceMessage, 1) \
292291
V(VMService_OnStart, 0) \
293292
V(VMService_OnExit, 0) \
294293
V(VMService_OnServerAddressChange, 1) \

‎runtime/vm/service.cc‎

Lines changed: 45 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -750,6 +750,10 @@ class BoolParameter : public MethodParameter {
750750
return (strcmp("true", value) == 0) || (strcmp("false", value) == 0);
751751
}
752752

753+
virtual bool ValidateObject(const Object& value) const {
754+
return value.IsBool();
755+
}
756+
753757
static bool Parse(const char* value, bool default_value = false) {
754758
if (value == nullptr) {
755759
return default_value;
@@ -836,9 +840,15 @@ class RunnableIsolateParameter : public MethodParameter {
836840
: MethodParameter(name, true) {}
837841

838842
virtual bool Validate(const char* value) const {
839-
Isolate* isolate = Isolate::Current();
840-
return (value != nullptr) && (isolate != nullptr) &&
841-
(isolate->is_runnable());
843+
// We assume the request has been forwarded to the current isolate and
844+
// isolateId matches it.
845+
return (value != nullptr) && ValidateCurrentIsolate();
846+
}
847+
848+
virtual bool ValidateObject(const Object& value) const {
849+
// We assume the request has been forwarded to the current isolate and
850+
// isolateId matches it.
851+
return value.IsString() && ValidateCurrentIsolate();
842852
}
843853

844854
virtual void PrintError(const char* name,
@@ -847,6 +857,12 @@ class RunnableIsolateParameter : public MethodParameter {
847857
js->PrintError(kIsolateMustBeRunnable,
848858
"Isolate must be runnable before this request is made.");
849859
}
860+
861+
private:
862+
static bool ValidateCurrentIsolate() {
863+
Isolate* isolate = Isolate::Current();
864+
return (isolate != nullptr) && (isolate->is_runnable());
865+
}
850866
};
851867

852868
class EnumParameter : public MethodParameter {
@@ -961,15 +977,13 @@ void Service::PostError(const String& method_name,
961977
js.PostReply();
962978
}
963979

964-
ErrorPtr Service::InvokeMethod(Isolate* I,
965-
const Array& msg,
966-
bool parameters_are_dart_objects) {
980+
ErrorPtr Service::InvokeMethod(Isolate* I, const Array& msg) {
967981
Thread* T = Thread::Current();
968982
ASSERT(I == T->isolate());
969983
ASSERT(I != nullptr);
970984
ASSERT(T->execution_state() == Thread::kThreadInVM);
971985
ASSERT(!msg.IsNull());
972-
ASSERT(msg.Length() == 6);
986+
ASSERT(msg.Length() == 7);
973987

974988
{
975989
StackZone zone(T);
@@ -978,13 +992,15 @@ ErrorPtr Service::InvokeMethod(Isolate* I,
978992
Instance& reply_port = Instance::Handle(Z);
979993
Instance& seq = String::Handle(Z);
980994
String& method_name = String::Handle(Z);
995+
Bool& parameters_are_dart_objects = Bool::Handle(Z);
981996
Array& param_keys = Array::Handle(Z);
982997
Array& param_values = Array::Handle(Z);
983998
reply_port ^= msg.At(1);
984999
seq ^= msg.At(2);
9851000
method_name ^= msg.At(3);
986-
param_keys ^= msg.At(4);
987-
param_values ^= msg.At(5);
1001+
parameters_are_dart_objects ^= msg.At(4);
1002+
param_keys ^= msg.At(5);
1003+
param_values ^= msg.At(6);
9881004

9891005
ASSERT(!method_name.IsNull());
9901006
ASSERT(seq.IsNull() || seq.IsString() || seq.IsNumber());
@@ -1003,7 +1019,7 @@ ErrorPtr Service::InvokeMethod(Isolate* I,
10031019
Dart_Port reply_port_id =
10041020
(reply_port.IsNull() ? ILLEGAL_PORT : SendPort::Cast(reply_port).Id());
10051021
js.Setup(zone.GetZone(), reply_port_id, seq, method_name, param_keys,
1006-
param_values, parameters_are_dart_objects);
1022+
param_values, parameters_are_dart_objects.value());
10071023

10081024
// |id_zone| is the zone that will be stored into the |JSONStream| that we
10091025
// are about to create, meaning that it is where temporary Service IDs may
@@ -1071,11 +1087,6 @@ ErrorPtr Service::HandleRootMessage(const Array& msg_instance) {
10711087
return InvokeMethod(isolate, msg_instance);
10721088
}
10731089

1074-
ErrorPtr Service::HandleObjectRootMessage(const Array& msg_instance) {
1075-
Isolate* isolate = Isolate::Current();
1076-
return InvokeMethod(isolate, msg_instance, true);
1077-
}
1078-
10791090
ErrorPtr Service::HandleIsolateMessage(Isolate* isolate, const Array& msg) {
10801091
ASSERT(isolate != nullptr);
10811092
const Error& error = Error::Handle(InvokeMethod(isolate, msg));
@@ -3945,7 +3956,7 @@ static const MethodParameter* const reload_kernel_params[] = {
39453956
RUNNABLE_ISOLATE_PARAMETER,
39463957
new BoolParameter("force", false),
39473958
new BoolParameter("pause", false),
3948-
new StringParameter("kernelFilePath", false),
3959+
new DartStringParameter("kernelFilePath", true),
39493960
nullptr,
39503961
};
39513962

@@ -3956,11 +3967,6 @@ static void ReloadKernel(Thread* thread, JSONStream* js) {
39563967
#if defined(DART_PRECOMPILED_RUNTIME)
39573968
js->PrintError(kFeatureDisabled, "Compiler is disabled in AOT mode.");
39583969
#else
3959-
if (!js->HasParam("kernelFilePath")) {
3960-
PrintMissingParamError(js, "kernelFilePath");
3961-
return;
3962-
}
3963-
39643970
IsolateGroup* isolate_group = thread->isolate_group();
39653971
if (isolate_group->library_tag_handler() == nullptr) {
39663972
js->PrintError(kFeatureDisabled,
@@ -4000,7 +4006,9 @@ static void ReloadKernel(Thread* thread, JSONStream* js) {
40004006
return;
40014007
}
40024008

4003-
void* file = (*file_open)(js->LookupParam("kernelFilePath"), /*write=*/false);
4009+
const String& kernel_file_path = String::CheckedHandle(
4010+
thread->zone(), js->LookupObjectParam("kernelFilePath"));
4011+
void* file = (*file_open)(kernel_file_path.ToCString(), /*write=*/false);
40044012
if (file == nullptr) {
40054013
js->PrintError(kIsolateReloadBarred,
40064014
"The specified kernel file could not be read. Please ensure "
@@ -4013,7 +4021,7 @@ static void ReloadKernel(Thread* thread, JSONStream* js) {
40134021
(*file_read)(&kernel_buffer, &kernel_buffer_size, file);
40144022

40154023
const bool force_reload =
4016-
BoolParameter::Parse(js->LookupParam("force"), false);
4024+
js->LookupObjectParam("force") == Bool::True().ptr();
40174025
isolate_group->ReloadKernel(js, force_reload, kernel_buffer,
40184026
kernel_buffer_size);
40194027

@@ -4027,8 +4035,8 @@ static const MethodParameter* const reload_sources_params[] = {
40274035
RUNNABLE_ISOLATE_PARAMETER,
40284036
new BoolParameter("force", false),
40294037
new BoolParameter("pause", false),
4030-
new StringParameter("rootLibUri", false),
4031-
new StringParameter("packagesUri", false),
4038+
new DartStringParameter("rootLibUri", false),
4039+
new DartStringParameter("packagesUri", false),
40324040
nullptr,
40334041
};
40344042

@@ -4062,10 +4070,18 @@ static void ReloadSources(Thread* thread, JSONStream* js) {
40624070
return;
40634071
}
40644072
const bool force_reload =
4065-
BoolParameter::Parse(js->LookupParam("force"), false);
4073+
js->LookupObjectParam("force") == Bool::True().ptr();
4074+
4075+
String& root_lib_uri = String::Handle(thread->zone());
4076+
root_lib_uri ^= js->LookupObjectParam("rootLibUri");
4077+
4078+
String& packages_uri = String::Handle(thread->zone());
4079+
packages_uri ^= js->LookupObjectParam("packagesUri");
40664080

4067-
isolate_group->ReloadSources(js, force_reload, js->LookupParam("rootLibUri"),
4068-
js->LookupParam("packagesUri"));
4081+
isolate_group->ReloadSources(
4082+
js, force_reload,
4083+
root_lib_uri.IsNull() ? nullptr : root_lib_uri.ToCString(),
4084+
packages_uri.IsNull() ? nullptr : packages_uri.ToCString());
40694085

40704086
Service::CheckForPause(isolate, js);
40714087

@@ -4075,7 +4091,7 @@ static void ReloadSources(Thread* thread, JSONStream* js) {
40754091
void Service::CheckForPause(Isolate* isolate, JSONStream* stream) {
40764092
// Should we pause?
40774093
isolate->set_should_pause_post_service_request(
4078-
BoolParameter::Parse(stream->LookupParam("pause"), false));
4094+
stream->LookupObjectParam("pause") == Bool::True().ptr());
40794095
}
40804096

40814097
ErrorPtr Service::MaybePause(Isolate* isolate, const Error& error) {

‎runtime/vm/service.h‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -131,10 +131,6 @@ class Service : public AllStatic {
131131
// Handles a message which is not directed to an isolate.
132132
static ErrorPtr HandleRootMessage(const Array& message);
133133

134-
// Handles a message which is not directed to an isolate and also
135-
// expects the parameter keys and values to be actual dart objects.
136-
static ErrorPtr HandleObjectRootMessage(const Array& message);
137-
138134
// Handles a message which is directed to a particular isolate.
139135
static ErrorPtr HandleIsolateMessage(Isolate* isolate, const Array& message);
140136

@@ -249,9 +245,7 @@ class Service : public AllStatic {
249245
}
250246

251247
private:
252-
static ErrorPtr InvokeMethod(Isolate* isolate,
253-
const Array& message,
254-
bool parameters_are_dart_objects = false);
248+
static ErrorPtr InvokeMethod(Isolate* isolate, const Array& message);
255249

256250
static void EmbedderHandleMessage(EmbedderServiceHandler* handler,
257251
JSONStream* js);

‎runtime/vm/service_test.cc‎

Lines changed: 21 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -91,16 +91,16 @@ static ArrayPtr Eval(Dart_Handle lib, const char* expr) {
9191
Api::UnwrapGrowableObjectArrayHandle(zone, expr_val);
9292
const Array& result = Array::Handle(Array::MakeFixedLength(value));
9393
GrowableObjectArray& growable = GrowableObjectArray::Handle();
94-
growable ^= result.At(4);
94+
growable ^= result.At(5);
9595
// Append dummy isolate id to parameter values.
9696
growable.Add(dummy_isolate_id);
9797
Array& array = Array::Handle(Array::MakeFixedLength(growable));
98-
result.SetAt(4, array);
99-
growable ^= result.At(5);
98+
result.SetAt(5, array);
99+
growable ^= result.At(6);
100100
// Append dummy isolate id to parameter values.
101101
growable.Add(dummy_isolate_id);
102102
array = Array::MakeFixedLength(growable);
103-
result.SetAt(5, array);
103+
result.SetAt(6, array);
104104
return result.ptr();
105105
}
106106

@@ -278,15 +278,15 @@ ISOLATE_UNIT_TEST_CASE(Service_Code) {
278278

279279
// Request an invalid code object.
280280
service_msg =
281-
Eval(lib, "[0, port, '0', 'getObject', ['objectId'], ['code/0']]");
281+
Eval(lib, "[0, port, '0', 'getObject', false, ['objectId'], ['code/0']]");
282282
HandleIsolateMessage(isolate, service_msg);
283283
EXPECT_EQ(MessageHandler::kOK, handler.HandleNextMessage());
284284
EXPECT_SUBSTRING("\"error\"", handler.msg());
285285

286286
// The following test checks that a code object can be found only
287287
// at compile_timestamp()-code.EntryPoint().
288288
service_msg = EvalF(lib,
289-
"[0, port, '0', 'getObject', "
289+
"[0, port, '0', 'getObject', false, "
290290
"['objectId'], ['code/%" Px64 "-%" Px "']]",
291291
compile_timestamp, entry);
292292
HandleIsolateMessage(isolate, service_msg);
@@ -306,7 +306,7 @@ ISOLATE_UNIT_TEST_CASE(Service_Code) {
306306
// Expect this to fail because the address is not the entry point.
307307
uintptr_t address = entry + 16;
308308
service_msg = EvalF(lib,
309-
"[0, port, '0', 'getObject', "
309+
"[0, port, '0', 'getObject', false, "
310310
"['objectId'], ['code/%" Px64 "-%" Px "']]",
311311
compile_timestamp, address);
312312
HandleIsolateMessage(isolate, service_msg);
@@ -317,7 +317,7 @@ ISOLATE_UNIT_TEST_CASE(Service_Code) {
317317
// Expect this to fail because the timestamp is wrong.
318318
address = entry;
319319
service_msg = EvalF(lib,
320-
"[0, port, '0', 'getObject', "
320+
"[0, port, '0', 'getObject', false, "
321321
"['objectId'], ['code/%" Px64 "-%" Px "']]",
322322
compile_timestamp - 1, address);
323323
HandleIsolateMessage(isolate, service_msg);
@@ -327,7 +327,7 @@ ISOLATE_UNIT_TEST_CASE(Service_Code) {
327327
// Request native code at address. Expect the null code object back.
328328
address = last;
329329
service_msg = EvalF(lib,
330-
"[0, port, '0', 'getObject', "
330+
"[0, port, '0', 'getObject', false, "
331331
"['objectId'], ['code/native-%" Px "']]",
332332
address);
333333
HandleIsolateMessage(isolate, service_msg);
@@ -337,7 +337,7 @@ ISOLATE_UNIT_TEST_CASE(Service_Code) {
337337

338338
// Request malformed native code.
339339
service_msg = EvalF(lib,
340-
"[0, port, '0', 'getObject', ['objectId'], "
340+
"[0, port, '0', 'getObject', false, ['objectId'], "
341341
"['code/native%" Px "']]",
342342
address);
343343
HandleIsolateMessage(isolate, service_msg);
@@ -405,7 +405,7 @@ ISOLATE_UNIT_TEST_CASE(Service_PcDescriptors) {
405405

406406
// Fetch object.
407407
service_msg = EvalF(lib,
408-
"[0, port, '0', 'getObject', "
408+
"[0, port, '0', 'getObject', false, "
409409
"['objectId'], ['%s']]",
410410
id);
411411
HandleIsolateMessage(isolate, service_msg);
@@ -477,7 +477,7 @@ ISOLATE_UNIT_TEST_CASE(Service_LocalVarDescriptors) {
477477

478478
// Fetch object.
479479
service_msg = EvalF(lib,
480-
"[0, port, '0', 'getObject', "
480+
"[0, port, '0', 'getObject', false, "
481481
"['objectId'], ['%s']]",
482482
id);
483483
HandleIsolateMessage(isolate, service_msg);
@@ -538,7 +538,8 @@ ISOLATE_UNIT_TEST_CASE(Service_PersistentHandles) {
538538
Array& service_msg = Array::Handle();
539539

540540
// Get persistent handles.
541-
service_msg = Eval(lib, "[0, port, '0', '_getPersistentHandles', [], []]");
541+
service_msg =
542+
Eval(lib, "[0, port, '0', '_getPersistentHandles', false, [], []]");
542543
HandleIsolateMessage(isolate, service_msg);
543544
EXPECT_EQ(MessageHandler::kOK, handler.HandleNextMessage());
544545
// Look for a heart beat.
@@ -555,7 +556,8 @@ ISOLATE_UNIT_TEST_CASE(Service_PersistentHandles) {
555556
}
556557

557558
// Get persistent handles (again).
558-
service_msg = Eval(lib, "[0, port, '0', '_getPersistentHandles', [], []]");
559+
service_msg =
560+
Eval(lib, "[0, port, '0', '_getPersistentHandles', false, [], []]");
559561
HandleIsolateMessage(isolate, service_msg);
560562
EXPECT_EQ(MessageHandler::kOK, handler.HandleNextMessage());
561563
EXPECT_SUBSTRING("\"type\":\"_PersistentHandles\"", handler.msg());
@@ -620,12 +622,12 @@ ISOLATE_UNIT_TEST_CASE(Service_EmbedderRootHandler) {
620622
}
621623

622624
Array& service_msg = Array::Handle();
623-
service_msg = Eval(lib, "[0, port, '\"', 'alpha', [], []]");
625+
service_msg = Eval(lib, "[0, port, '\"', 'alpha', false, [], []]");
624626
HandleRootMessage(service_msg);
625627
EXPECT_EQ(MessageHandler::kOK, handler.HandleNextMessage());
626628
EXPECT_STREQ("{\"jsonrpc\":\"2.0\", \"result\":alpha,\"id\":\"\\\"\"}",
627629
handler.msg());
628-
service_msg = Eval(lib, "[0, port, 1, 'beta', [], []]");
630+
service_msg = Eval(lib, "[0, port, 1, 'beta', false, [], []]");
629631
HandleRootMessage(service_msg);
630632
EXPECT_EQ(MessageHandler::kOK, handler.HandleNextMessage());
631633
EXPECT_STREQ("{\"jsonrpc\":\"2.0\", \"error\":beta,\"id\":1}", handler.msg());
@@ -668,12 +670,12 @@ ISOLATE_UNIT_TEST_CASE(Service_EmbedderIsolateHandler) {
668670

669671
Isolate* isolate = thread->isolate();
670672
Array& service_msg = Array::Handle();
671-
service_msg = Eval(lib, "[0, port, '0', 'alpha', [], []]");
673+
service_msg = Eval(lib, "[0, port, '0', 'alpha', false, [], []]");
672674
HandleIsolateMessage(isolate, service_msg);
673675
EXPECT_EQ(MessageHandler::kOK, handler.HandleNextMessage());
674676
EXPECT_STREQ("{\"jsonrpc\":\"2.0\", \"result\":alpha,\"id\":\"0\"}",
675677
handler.msg());
676-
service_msg = Eval(lib, "[0, port, '0', 'beta', [], []]");
678+
service_msg = Eval(lib, "[0, port, '0', 'beta', false, [], []]");
677679
HandleIsolateMessage(isolate, service_msg);
678680
EXPECT_EQ(MessageHandler::kOK, handler.HandleNextMessage());
679681
EXPECT_STREQ("{\"jsonrpc\":\"2.0\", \"error\":beta,\"id\":\"0\"}",
@@ -725,7 +727,7 @@ ISOLATE_UNIT_TEST_CASE(Service_Profile) {
725727
}
726728

727729
Array& service_msg = Array::Handle();
728-
service_msg = Eval(lib, "[0, port, '0', 'getCpuSamples', [], []]");
730+
service_msg = Eval(lib, "[0, port, '0', 'getCpuSamples', false, [], []]");
729731
HandleIsolateMessage(isolate, service_msg);
730732
EXPECT_EQ(MessageHandler::kOK, handler.HandleNextMessage());
731733
// Expect profile

0 commit comments

Comments
 (0)