Rework service extensions to be safe

Rework service extensions to be safe

- When scheduling a service extension to be invoked, store the closure in an array held in the isolate.
- At the tail of IsolateMessageHandler::HandleMessage, invoke all pending service extension closures.

R=iposva@google.com, turnidge@google.com

Committed: https://github.com/dart-lang/sdk/commit/7052bd6bed45b0f24b311562e1ae9b9d63554114

[Cutch, 2015-08-20 01:47:10]

johnmccutchan@google.com changed reviewers:
+ iposva@google.com, turnidge@google.com

[turnidge, 2015-08-20 17:34:39]

lgtm w/ cosmetic comments.

https://codereview.chromium.org/1299493007/diff/20001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1299493007/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:1873: RawGrowableObjectArray*
Isolate::PrunePendingExtensionCalls() {
Maybe GetAndClearPendingExtensionCalls?  Your choice.

https://codereview.chromium.org/1299493007/diff/20001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1299493007/diff/20001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:713: void set_pending_extension_calls(const
GrowableObjectArray& value);
Maybe make pending_extension_calls, set_pending_extension_calls, and
PrunePendingExtensionCalls private.

https://codereview.chromium.org/1299493007/diff/20001/sdk/lib/developer/exten...
File sdk/lib/developer/extension.dart (right):

https://codereview.chromium.org/1299493007/diff/20001/sdk/lib/developer/exten...
sdk/lib/developer/extension.dart:105: Object _scheduleExtension(String method,
Function?

[Cutch, 2015-08-20 18:09:37]

https://codereview.chromium.org/1299493007/diff/20001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1299493007/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:1873: RawGrowableObjectArray*
Isolate::PrunePendingExtensionCalls() {
On 2015/08/20 17:34:39, turnidge wrote:
> Maybe GetAndClearPendingExtensionCalls?  Your choice.

Done.

https://codereview.chromium.org/1299493007/diff/20001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1299493007/diff/20001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:713: void set_pending_extension_calls(const
GrowableObjectArray& value);
On 2015/08/20 17:34:39, turnidge wrote:
> Maybe make pending_extension_calls, set_pending_extension_calls, and
> PrunePendingExtensionCalls private.

Done.

https://codereview.chromium.org/1299493007/diff/20001/sdk/lib/developer/exten...
File sdk/lib/developer/extension.dart (right):

https://codereview.chromium.org/1299493007/diff/20001/sdk/lib/developer/exten...
sdk/lib/developer/extension.dart:105: Object _scheduleExtension(String method,
On 2015/08/20 17:34:39, turnidge wrote:
> Function?

Done.

[Ivan Posva, 2015-08-20 20:46:05]

-Ivan

https://codereview.chromium.org/1299493007/diff/40001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1299493007/diff/40001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:499: success =
ProcessUnhandledException(Error::Cast(result));
We should propagate errors to the service isolate here instead of the isolate
itself. Unless we have an UnwindError of course.

https://codereview.chromium.org/1299493007/diff/40001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:1862: return result.raw();
Maybe we want to requeue the non-handled extension calls in case errors are not
fatal? Actually, errors in extension calls should never be fatal as they come in
from a side-channel.

As discussed we should send these errors back to the VM service.

https://codereview.chromium.org/1299493007/diff/40001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/1299493007/diff/40001/runtime/vm/service.cc#n...
runtime/vm/service.cc:909: DartEntry::InvokeFunction(schedule_extension,
arguments));
This still executes Dart code at an arbitrary location. How about just
remembering all the parameters in the pending extension list? That will also
give you a reference to the reply port, which you need in case of an error.

[Cutch, 2015-08-21 14:18:56]

PTAL

https://codereview.chromium.org/1299493007/diff/40001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1299493007/diff/40001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:499: success =
ProcessUnhandledException(Error::Cast(result));
On 2015/08/20 20:46:05, Ivan Posva wrote:
> We should propagate errors to the service isolate here instead of the isolate
> itself. Unless we have an UnwindError of course.

Done.

https://codereview.chromium.org/1299493007/diff/40001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:1862: return result.raw();
On 2015/08/20 20:46:05, Ivan Posva wrote:
> Maybe we want to requeue the non-handled extension calls in case errors are
not
> fatal? Actually, errors in extension calls should never be fatal as they come
in
> from a side-channel.
> 
> As discussed we should send these errors back to the VM service.

Done.

https://codereview.chromium.org/1299493007/diff/40001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/1299493007/diff/40001/runtime/vm/service.cc#n...
runtime/vm/service.cc:909: DartEntry::InvokeFunction(schedule_extension,
arguments));
On 2015/08/20 20:46:05, Ivan Posva wrote:
> This still executes Dart code at an arbitrary location. How about just
> remembering all the parameters in the pending extension list? That will also
> give you a reference to the reply port, which you need in case of an error.

I now no longer invoke any Dart code from the OOB. It is all C++ and deferred.

[Ivan Posva, 2015-08-26 04:47:00]

-Ivan

https://codereview.chromium.org/1299493007/diff/100001/runtime/observatory/te...
File runtime/observatory/tests/service/developer_extension_test.dart (right):

https://codereview.chromium.org/1299493007/diff/100001/runtime/observatory/te...
runtime/observatory/tests/service/developer_extension_test.dart:52: klajsdlkjfad
Please add a comment that you did not just fat-finger here. This is intended,
no?

https://codereview.chromium.org/1299493007/diff/100001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1299493007/diff/100001/runtime/vm/isolate.h#n...
runtime/vm/isolate.h:892: // We use 2 list entries for each register extension
handler.
register -> registered

https://codereview.chromium.org/1299493007/diff/100001/sdk/lib/developer/exte...
File sdk/lib/developer/extension.dart (right):

https://codereview.chromium.org/1299493007/diff/100001/sdk/lib/developer/exte...
sdk/lib/developer/extension.dart:91: void registerExtension(String method,
ServiceExtensionHandler handler) {
Can you clear/remove an extension by registering a null handler?

https://codereview.chromium.org/1299493007/diff/100001/sdk/lib/developer/exte...
sdk/lib/developer/extension.dart:140: response.catchError((e, st) {
Usually I would expect to chain this in a normal try-catch fashion:
response.then(block, onError: catch_block)

Also, what happens if errors are thrown in the then or catchError blocks?

[Cutch, 2015-08-26 14:28:01]

https://codereview.chromium.org/1299493007/diff/100001/runtime/observatory/te...
File runtime/observatory/tests/service/developer_extension_test.dart (right):

https://codereview.chromium.org/1299493007/diff/100001/runtime/observatory/te...
runtime/observatory/tests/service/developer_extension_test.dart:52: klajsdlkjfad
On 2015/08/26 04:46:59, Ivan Posva wrote:
> Please add a comment that you did not just fat-finger here. This is intended,
> no?

Done.

https://codereview.chromium.org/1299493007/diff/100001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1299493007/diff/100001/runtime/vm/isolate.h#n...
runtime/vm/isolate.h:892: // We use 2 list entries for each register extension
handler.
On 2015/08/26 04:47:00, Ivan Posva wrote:
> register -> registered

Done.

https://codereview.chromium.org/1299493007/diff/100001/sdk/lib/developer/exte...
File sdk/lib/developer/extension.dart (right):

https://codereview.chromium.org/1299493007/diff/100001/sdk/lib/developer/exte...
sdk/lib/developer/extension.dart:91: void registerExtension(String method,
ServiceExtensionHandler handler) {
On 2015/08/26 04:47:00, Ivan Posva wrote:
> Can you clear/remove an extension by registering a null handler?

No. Registering a null handler is not allowed by the check:

if (handler is! ServiceExtensionHandler) 

I do not intend for extensions to be removable.

https://codereview.chromium.org/1299493007/diff/100001/sdk/lib/developer/exte...
sdk/lib/developer/extension.dart:140: response.catchError((e, st) {
On 2015/08/26 04:47:00, Ivan Posva wrote:
> Usually I would expect to chain this in a normal try-catch fashion:
> response.then(block, onError: catch_block)
> 
> Also, what happens if errors are thrown in the then or catchError blocks?

I'm eagerly catching errors and wrapping them in a ServiceExtensionResponse so
that .then branch can just post the error as a response. I've added comments to
explain this.

No errors can be thrown in the .then or the .catchError blocks. The code in
those blocks is internal library code that doesn't call any non-internal library
code. As a precaution, I've added a .catchError block at the end that silently
consumes the error.

Finally, if any errors do escape _runExtension the code that invokes them
(Isolate::InvokePendingExtensionCalls) handles the errors in C++ and sends them
over the service protocol.

[Ivan Posva, 2015-08-27 06:15:29]

-Ivan

https://codereview.chromium.org/1299493007/diff/140001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1299493007/diff/140001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1957: calls ^= GrowableObjectArray::New(Heap::kOld);
Why in old space?

https://codereview.chromium.org/1299493007/diff/140001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1962: calls.Add(closure, Heap::kOld);
ditto here and below. Why old?

https://codereview.chromium.org/1299493007/diff/140001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1976: void Isolate::RegisterExtensionHandler(const String&
name,
These are really VM service extension handlers, correct? Can we find a name that
reflects this a bit more clearly? Thanks!

https://codereview.chromium.org/1299493007/diff/140001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1999: RawInstance* Isolate::LookupExtensionHandler(const
String& name) {
Why isn't the extension list not maintained in the associated Dart code?
Actually in Dart code it would be trivial to store this in a Map<String,
Function>.

[Cutch, 2015-08-27 13:27:49]

https://codereview.chromium.org/1299493007/diff/140001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1299493007/diff/140001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1957: calls ^= GrowableObjectArray::New(Heap::kOld);
On 2015/08/27 06:15:29, Ivan Posva wrote:
> Why in old space?

Done.

https://codereview.chromium.org/1299493007/diff/140001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1962: calls.Add(closure, Heap::kOld);
On 2015/08/27 06:15:29, Ivan Posva wrote:
> ditto here and below. Why old?

Done.

https://codereview.chromium.org/1299493007/diff/140001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1976: void Isolate::RegisterExtensionHandler(const String&
name,
On 2015/08/27 06:15:29, Ivan Posva wrote:
> These are really VM service extension handlers, correct? Can we find a name
that
> reflects this a bit more clearly? Thanks!

Done.

https://codereview.chromium.org/1299493007/diff/140001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1999: RawInstance* Isolate::LookupExtensionHandler(const
String& name) {
On 2015/08/27 06:15:29, Ivan Posva wrote:
> Why isn't the extension list not maintained in the associated Dart code?
> Actually in Dart code it would be trivial to store this in a Map<String,
> Function>.

Because doing that in Dart code could lead to inconsistent states. If I use a
map, I could get an OOB in the middle of registering a service extension and
then depending on what the intermediate state, the lookup would fail or cause a
crash.

To be safe, everything is done in C++.

[Ivan Posva, 2015-08-27 18:12:26]

LGTM with comment added.

-Ivan

https://codereview.chromium.org/1299493007/diff/140001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1299493007/diff/140001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1999: RawInstance* Isolate::LookupExtensionHandler(const
String& name) {
On 2015/08/27 13:27:49, Cutch wrote:
> On 2015/08/27 06:15:29, Ivan Posva wrote:
> > Why isn't the extension list not maintained in the associated Dart code?
> > Actually in Dart code it would be trivial to store this in a Map<String,
> > Function>.
> 
> Because doing that in Dart code could lead to inconsistent states. If I use a
> map, I could get an OOB in the middle of registering a service extension and
> then depending on what the intermediate state, the lookup would fail or cause
a
> crash.
> 
> To be safe, everything is done in C++.

Many thanks for the explanation. Please add it as a comment so that we do not
mistakenly "simplify" the code and move the method_name to service extension
handler mapping into Dart code.

https://codereview.chromium.org/1299493007/diff/160001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1299493007/diff/160001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:516: } else  if (FLAG_trace_service) {
single space instead?

https://codereview.chromium.org/1299493007/diff/160001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:530: } else  if (FLAG_trace_service) {
ditto

[Cutch, 2015-08-27 18:23:29]

https://codereview.chromium.org/1299493007/diff/140001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1299493007/diff/140001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1999: RawInstance* Isolate::LookupExtensionHandler(const
String& name) {
On 2015/08/27 18:12:26, Ivan Posva wrote:
> On 2015/08/27 13:27:49, Cutch wrote:
> > On 2015/08/27 06:15:29, Ivan Posva wrote:
> > > Why isn't the extension list not maintained in the associated Dart code?
> > > Actually in Dart code it would be trivial to store this in a Map<String,
> > > Function>.
> > 
> > Because doing that in Dart code could lead to inconsistent states. If I use
a
> > map, I could get an OOB in the middle of registering a service extension and
> > then depending on what the intermediate state, the lookup would fail or
cause
> a
> > crash.
> > 
> > To be safe, everything is done in C++.
> 
> Many thanks for the explanation. Please add it as a comment so that we do not
> mistakenly "simplify" the code and move the method_name to service extension
> handler mapping into Dart code.

Done.

https://codereview.chromium.org/1299493007/diff/160001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1299493007/diff/160001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:516: } else  if (FLAG_trace_service) {
On 2015/08/27 18:12:26, Ivan Posva wrote:
> single space instead?

Done.

https://codereview.chromium.org/1299493007/diff/160001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:530: } else  if (FLAG_trace_service) {
On 2015/08/27 18:12:26, Ivan Posva wrote:
> ditto

Done.

[Cutch, 2015-08-27 18:23:44]

Committed patchset #10 (id:180001) manually as
7052bd6bed45b0f24b311562e1ae9b9d63554114 (presubmit successful).