- Implement Isolate.ping.

- Implement Isolate.ping.
- Allow isolate library messages to be enqueued at the beginning
  of the message queue.

R=asiva@google.com

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

[Ivan Posva, 2014-11-24 22:02:43]

iposva@google.com changed reviewers:
+ asiva@google.com

[siva, 2014-12-01 19:48:05]

LGTM with some comments and spec question about Denial of Service issue on back
to back PING immediate messages.

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

https://codereview.chromium.org/749373002/diff/1/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:178: const Smi& ping_type = Smi::Cast(obj3);
why not
intptr_t ping_value = Smi::Cast(obj3).Value();

and use ping_value in the if .. else statements.
if (ping_value == kImmediateAction) { etc.

https://codereview.chromium.org/749373002/diff/1/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:185: Message::kNormalPriority));
How does the spec resolve a potential Denial of Service attack if repeated PING
messages with kImmediateAction setting are sent.

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

https://codereview.chromium.org/749373002/diff/1/runtime/vm/message.cc#newcode54
runtime/vm/message.cc:54: tail_ = msg;
The "only element in queue" case is common always why not hoist that out to make
the code simpler to read:

if (head_ == NULL) {
  // Only element in the queue.
  ASSERT(tail_ == NULL);
  head_= msg;
  tail_ = msg;
} else {
  if (!before_events) {
    ASSERT(tail_ != NULL);
    // Append at the tail.
    ...
    ...
  } else {
    ASSERT(head_ != NULL);
    if (head->dest_port() != Message::kIllegalPort) {
      ...
      ...
    } else {
      Message* cur = head_;
      ...
      ...
    }
  }
}

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

https://codereview.chromium.org/749373002/diff/1/runtime/vm/message_test.cc#n...
runtime/vm/message_test.cc:66: EXPECT(!queue.IsEmpty());
Maybe the test would be better if you Enqueued another library message here and
it would test the case when we have to loop through the message and split when
dest_port != KNormalPriority.
i.e:

const char* str6 = "msg6";
Message* msg6 = new Message(Message::kIllegalPort,
                            AllocMsg(str6), strlen(str6) + 1,
                            Message::kNormalPriority);
queue.Enqueue(msg6, true);
EXPECT(!queue.IsEmpty());

https://codereview.chromium.org/749373002/diff/1/runtime/vm/message_test.cc#n...
runtime/vm/message_test.cc:77: 
Over here:

msg = queue.Dequeue();
EXPECT(msg != NULL);
EXPECT_STREQ(str6, reinterpret_cast<char*>(msg->data()));
EXPECT(!queue.IsEmpty());

[siva, 2014-12-01 19:48:06]

LGTM with some comments and spec question about Denial of Service issue on back
to back PING immediate messages.

[Ivan Posva, 2014-12-08 22:45:27]

Committed patchset #2 (id:20001) manually as r42193 (presubmit successful).

[Ivan Posva, 2014-12-12 20:21:47]

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

https://codereview.chromium.org/749373002/diff/1/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:178: const Smi& ping_type = Smi::Cast(obj3);
On 2014/12/01 19:48:05, siva wrote:
> why not
> intptr_t ping_value = Smi::Cast(obj3).Value();
> 
> and use ping_value in the if .. else statements.
> if (ping_value == kImmediateAction) { etc.

Done.

https://codereview.chromium.org/749373002/diff/1/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:185: Message::kNormalPriority));
On 2014/12/01 19:48:05, siva wrote:
> How does the spec resolve a potential Denial of Service attack if repeated
PING
> messages with kImmediateAction setting are sent.

If you control the isolate, then you can do other kinds of Denial of Service,
such as terminating it.

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

https://codereview.chromium.org/749373002/diff/1/runtime/vm/message.cc#newcode54
runtime/vm/message.cc:54: tail_ = msg;
On 2014/12/01 19:48:05, siva wrote:
> The "only element in queue" case is common always why not hoist that out to
make
> the code simpler to read:
> 
> if (head_ == NULL) {
>   // Only element in the queue.
>   ASSERT(tail_ == NULL);
>   head_= msg;
>   tail_ = msg;
> } else {
>   if (!before_events) {
>     ASSERT(tail_ != NULL);
>     // Append at the tail.
>     ...
>     ...
>   } else {
>     ASSERT(head_ != NULL);
>     if (head->dest_port() != Message::kIllegalPort) {
>       ...
>       ...
>     } else {
>       Message* cur = head_;
>       ...
>       ...
>     }
>   }
> }

Done.

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

https://codereview.chromium.org/749373002/diff/1/runtime/vm/message_test.cc#n...
runtime/vm/message_test.cc:66: EXPECT(!queue.IsEmpty());
On 2014/12/01 19:48:05, siva wrote:
> Maybe the test would be better if you Enqueued another library message here
and
> it would test the case when we have to loop through the message and split when
> dest_port != KNormalPriority.
> i.e:
> 
> const char* str6 = "msg6";
> Message* msg6 = new Message(Message::kIllegalPort,
>                             AllocMsg(str6), strlen(str6) + 1,
>                             Message::kNormalPriority);
> queue.Enqueue(msg6, true);
> EXPECT(!queue.IsEmpty());

Done.

https://codereview.chromium.org/749373002/diff/1/runtime/vm/message_test.cc#n...
runtime/vm/message_test.cc:77: 
On 2014/12/01 19:48:05, siva wrote:
> Over here:
> 
> msg = queue.Dequeue();
> EXPECT(msg != NULL);
> EXPECT_STREQ(str6, reinterpret_cast<char*>(msg->data()));
> EXPECT(!queue.IsEmpty());

Done.