- Implement Isolate.pause and Isolate.resume.

- Implement Isolate.pause and Isolate.resume.
- Implement a minimum Capability functionality.

R=asiva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=37982

[siva, 2014-07-01 21:42:57]

LGTM with some comments.

https://codereview.chromium.org/354763004/diff/1/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://codereview.chromium.org/354763004/diff/1/runtime/lib/isolate.cc#newco...
runtime/lib/isolate.cc:169: static RawArray* Spawn(Isolate* isolate,
This isolate parameter is not being used anywhere in the function, if it is for
use in the Handle creation function maybe we should call the parameter
spawning_isolate to make things clear.

Also maybe a local variable:
Isolate* spawned_isolate = state->isolate();

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

https://codereview.chromium.org/354763004/diff/1/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:112: void IsolateMessageHandler::HandleLibMessage(const
Array& message) {
Would be good to document the message formats here, e.g:
Messages handled have the following format:

Pause an isolate => unused, kPauseMsg, pauseCapability, resumeCapability
Resume an isolate => unused, kResumeMsg, pauseCapability, resumeCapability

https://codereview.chromium.org/354763004/diff/1/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:142: #if DEBUG
#if defined(DEBUG) to be consistent.

https://codereview.chromium.org/354763004/diff/1/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:235: #if DEBUG
Ditto.

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

https://codereview.chromium.org/354763004/diff/1/runtime/vm/isolate.h#newcode364
runtime/vm/isolate.h:364: // Verify that the sender had the capability to pause
this isolate.
sender has

https://codereview.chromium.org/354763004/diff/1/runtime/vm/isolate.h#newcode365
runtime/vm/isolate.h:365: bool VerifyPauseCapability(const Capability&
capability);
) const;

https://codereview.chromium.org/354763004/diff/1/runtime/vm/message_handler.h
File runtime/vm/message_handler.h (right):

https://codereview.chromium.org/354763004/diff/1/runtime/vm/message_handler.h...
runtime/vm/message_handler.h:76: void increment_paused() { paused_++; }
How do we ensure paused does not overflow?

https://codereview.chromium.org/354763004/diff/1/runtime/vm/message_handler.h...
runtime/vm/message_handler.h:77: void decrement_paused() { paused_--; }
ASSERT(paused_ > 0);

https://codereview.chromium.org/354763004/diff/1/runtime/vm/raw_object_snapsh...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/354763004/diff/1/runtime/vm/raw_object_snapsh...
runtime/vm/raw_object_snapshot.cc:2550: writer->Write(ptr()->id_);
writer->Write<uint64_t>(ptr()->id_);

so it reads consistent with the ReadFrom code above.

[Lasse Reichstein Nielsen, 2014-07-02 08:11:55]

https://codereview.chromium.org/354763004/diff/1/runtime/lib/isolate_patch.dart
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/354763004/diff/1/runtime/lib/isolate_patch.da...
runtime/lib/isolate_patch.dart:15: /* patch */ factory Capability() =
_CapabilityImpl;
Could you make the patch constructor native:
  /*patch*/ factory Capability() native "Capability_factory";
and avoid the _CapabilityImpl class?

https://codereview.chromium.org/354763004/diff/1/runtime/vm/message_handler.h
File runtime/vm/message_handler.h (right):

https://codereview.chromium.org/354763004/diff/1/runtime/vm/message_handler.h...
runtime/vm/message_handler.h:76: void increment_paused() { paused_++; }
If paused_ is an intptr_t, and we store one resumeCapability per pause, we will
definitely run out of memory before it overflows.

[siva, 2014-07-02 14:52:10]

https://codereview.chromium.org/354763004/diff/1/runtime/vm/message_handler.h
File runtime/vm/message_handler.h (right):

https://codereview.chromium.org/354763004/diff/1/runtime/vm/message_handler.h...
runtime/vm/message_handler.h:76: void increment_paused() { paused_++; }
On 2014/07/02 08:11:55, Lasse Reichstein Nielsen wrote:
> If paused_ is an intptr_t, and we store one resumeCapability per pause, we
will
> definitely run out of memory before it overflows.

True but instead of relying on that one needs to set a max possible value for
this and check against that max like we do in all other similar scenarios in the
VM. That is the recommended guideline from the security folks.

[Ivan Posva, 2014-07-03 12:51:17]

Thanks,
-Ivan

https://codereview.chromium.org/354763004/diff/1/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://codereview.chromium.org/354763004/diff/1/runtime/lib/isolate.cc#newco...
runtime/lib/isolate.cc:169: static RawArray* Spawn(Isolate* isolate,
On 2014/07/01 21:42:56, siva wrote:
> This isolate parameter is not being used anywhere in the function, if it is
for
> use in the Handle creation function maybe we should call the parameter
> spawning_isolate to make things clear.
> 
> Also maybe a local variable:
> Isolate* spawned_isolate = state->isolate();

- Removed the unnecessary parameter, it was used in a intermediate
implementation.
- Introduced the local variable spawned_isolate to make it clear what is
happening.

https://codereview.chromium.org/354763004/diff/1/runtime/lib/isolate_patch.dart
File runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/354763004/diff/1/runtime/lib/isolate_patch.da...
runtime/lib/isolate_patch.dart:15: /* patch */ factory Capability() =
_CapabilityImpl;
On 2014/07/02 08:11:55, Lasse Reichstein Nielsen wrote:
> Could you make the patch constructor native:
>   /*patch*/ factory Capability() native "Capability_factory";
> and avoid the _CapabilityImpl class?

Since I will need to implement hash code and equality, I prefer not to patch the
interface class.

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

https://codereview.chromium.org/354763004/diff/1/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:112: void IsolateMessageHandler::HandleLibMessage(const
Array& message) {
On 2014/07/01 21:42:56, siva wrote:
> Would be good to document the message formats here, e.g:
> Messages handled have the following format:
> 
> Pause an isolate => unused, kPauseMsg, pauseCapability, resumeCapability
> Resume an isolate => unused, kResumeMsg, pauseCapability, resumeCapability

Done.

https://codereview.chromium.org/354763004/diff/1/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:142: #if DEBUG
On 2014/07/01 21:42:56, siva wrote:
> #if defined(DEBUG) to be consistent.

Done.

https://codereview.chromium.org/354763004/diff/1/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:235: #if DEBUG
On 2014/07/01 21:42:56, siva wrote:
> Ditto.

Done.

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

https://codereview.chromium.org/354763004/diff/1/runtime/vm/isolate.h#newcode364
runtime/vm/isolate.h:364: // Verify that the sender had the capability to pause
this isolate.
On 2014/07/01 21:42:56, siva wrote:
> sender has

Done.

https://codereview.chromium.org/354763004/diff/1/runtime/vm/isolate.h#newcode365
runtime/vm/isolate.h:365: bool VerifyPauseCapability(const Capability&
capability);
On 2014/07/01 21:42:56, siva wrote:
> ) const;

Done.

https://codereview.chromium.org/354763004/diff/1/runtime/vm/isolate.h#newcode368
runtime/vm/isolate.h:368: bool AddPauseCapability(const Capability& capability);
Renamed to resume capabilities and refactored the rest of the code as these are
really capabilities for resume and not pause. I found it confusing to have
VerifyPauseCapability sitting right next to this.

https://codereview.chromium.org/354763004/diff/1/runtime/vm/message_handler.h
File runtime/vm/message_handler.h (right):

https://codereview.chromium.org/354763004/diff/1/runtime/vm/message_handler.h...
runtime/vm/message_handler.h:76: void increment_paused() { paused_++; }
On 2014/07/02 14:52:10, siva wrote:
> On 2014/07/02 08:11:55, Lasse Reichstein Nielsen wrote:
> > If paused_ is an intptr_t, and we store one resumeCapability per pause, we
> will
> > definitely run out of memory before it overflows.
> 
> True but instead of relying on that one needs to set a max possible value for
> this and check against that max like we do in all other similar scenarios in
the
> VM. That is the recommended guideline from the security folks.

Isolate::AddResumeCapability now prevents overflows of this counter by
calculating a maximum allowable number of capabilities recorded and not growing
once that number is reached.

https://codereview.chromium.org/354763004/diff/1/runtime/vm/message_handler.h...
runtime/vm/message_handler.h:77: void decrement_paused() { paused_--; }
On 2014/07/01 21:42:56, siva wrote:
> ASSERT(paused_ > 0);

Done.

https://codereview.chromium.org/354763004/diff/1/runtime/vm/raw_object_snapsh...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/354763004/diff/1/runtime/vm/raw_object_snapsh...
runtime/vm/raw_object_snapshot.cc:2550: writer->Write(ptr()->id_);
On 2014/07/01 21:42:56, siva wrote:
> writer->Write<uint64_t>(ptr()->id_);
> 
> so it reads consistent with the ReadFrom code above.

Done. Also below in SendPort::WriteTo.

[Ivan Posva, 2014-07-03 12:56:16]

Committed patchset #1 manually as r37982 (presubmit successful).