Display isolate message queue in Observatory debugger

Display isolate message queue in Observatory debugger

- Log the parameters to RPCs
- Display messages like stack frames
    - handler function and script visible
    - message preview instead of local variables
- Expanded and previewed messages do not collapse when stepping within the existing message
- Mark patch classes as finalized to avoid ASSERT on class_finalizer.cc:2358 **
- Support iterating over message queues
- Support printing the message queue as JSON
- Inhibit OOB messages from being handled if we are iterating over the message queue
    - avoids dead lock when looking up port handler (done in Dart code with message handler locked) and a stack overflow trigger to handle OOB messages
- make getObject understand message ids
- Fix dartbug.com/23355

** Need to discuss with Ivan and Matthias (reviewed code differs from committed code but matches my fix):
https://codereview.chromium.org//828353002
https://code.google.com/p/dart/source/detail?r=42612

R=turnidge@google.com

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

[Cutch, 2015-05-04 15:19:24]

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

[turnidge, 2015-05-04 17:47:13]

https://codereview.chromium.org/1122503003/diff/140001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/debugger.dart (right):

https://codereview.chromium.org/1122503003/diff/140001/runtime/observatory/li...
runtime/observatory/lib/src/elements/debugger.dart:1323: }
Nice.

https://codereview.chromium.org/1122503003/diff/140001/runtime/observatory/li...
runtime/observatory/lib/src/elements/debugger.dart:1452:
@CustomTag('debugger-message')
I wonder if we can share more code between debugger-frame and debuggr-message. 
Not for this CL.

https://codereview.chromium.org/1122503003/diff/140001/runtime/observatory/li...
runtime/observatory/lib/src/elements/debugger.dart:1490: bool should_reset =
camelCase

Also, could call it messageChanged instead of shouldReset.

https://codereview.chromium.org/1122503003/diff/140001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/debugger.html (right):

https://codereview.chromium.org/1122503003/diff/140001/runtime/observatory/li...
runtime/observatory/lib/src/elements/debugger.html:373: <!-- TODO(turnidge): Add
eval box here? -->
Drop this TODO from here.

https://codereview.chromium.org/1122503003/diff/140001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/script_ref.dart (right):

https://codereview.chromium.org/1122503003/diff/140001/runtime/observatory/li...
runtime/observatory/lib/src/elements/script_ref.dart:13: @published int pos =
-1;
Change to pos = null

https://codereview.chromium.org/1122503003/diff/140001/runtime/observatory/li...
File runtime/observatory/lib/src/service/object.dart (right):

https://codereview.chromium.org/1122503003/diff/140001/runtime/observatory/li...
runtime/observatory/lib/src/service/object.dart:83: // Is this an isolate
message?
Not sure that this comment adds, given that none of its peers are commented.

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

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message.cc#...
runtime/vm/message.cc:159: return current();
Your Next() and HasNext() are a bit different than those found, e.g., in Java. 
Do we use this pattern elsewhere in the VM already?  It is odd to me that I can
call Next() as many times as I like and it just sticks to the end of the queue
when HasNext is false, seems like it could hide errors.

I would be more used to seeing a Next() function that unconditionally returns
the next element and kills the VM if it goes out of bounds.  Loops would look
like this:

while (it.HasNext()) {
  Message* current = it.Next();
  ... do work here...
}

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message.cc#...
runtime/vm/message.cc:200: message.AddProperty("type", "IsolateMessage");
IsolateMessage?  I don't like the "Isolate".  Maybe just Message?

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message.cc#...
runtime/vm/message.cc:206: message.AddProperty("destinationPort",
Make this _destinationPort.

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message.cc#...
runtime/vm/message.cc:210: msg_handler =
DartLibraryCalls::LookupHandler(current->dest_port());
Add a TODO to move this map into the VM so we can avoid this call into Dart...
(as discussed offline w/ iposva).

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message.cc#...
runtime/vm/message.cc:222: message.AddProperty("handlerScript", script);
Consider dropping handlerScript.  Up to you.

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message_han...
File runtime/vm/message_handler.cc (right):

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message_han...
runtime/vm/message_handler.cc:351: }
Move the bulk of this into Reset.

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message_han...
runtime/vm/message_handler.cc:364: handler_->oob_message_handling_allowed_ =
false;
Does this need to be done under the monitor here?

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message_tes...
File runtime/vm/message_test.cc (right):

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message_tes...
runtime/vm/message_test.cc:40: }
Unroll, as suggested below.

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message_tes...
runtime/vm/message_test.cc:57: }
I would unroll this, I think it is shorter/simpler.

(Here's how the code would look it you rewrite Next() and HasNext() as I
suggested elsewhere)

EXPECT(it.hasNext());
EXPECT(it.Next() == msg1);
EXPECT(it.hasNext());
EXPECT(it.Next() == msg2);
EXPECT(!it.hasNext());

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message_tes...
runtime/vm/message_test.cc:75: }
Unroll.

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

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/service.cc#...
runtime/vm/service.cc:1193: 
Maybe a short function-level comment.

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/service.cc#...
runtime/vm/service.cc:1203: }
I see that this code is very similar to code I wrote in LookupBreakpoint, above.
 I don't understand why I waiting to check for (rest == NULL) until after the
strncmp test.  it's kind of weird.  I'm guessing this code used to be embedded
in a larger control flow and then got abstracted into a function.

Once we hoist the "rest check" up, does it turn into something like this?:

size_t end_pos = strcspn(id, "/");
if (end_pos == strlen(id)) {
  return false;
}
const char* rest = id + end_pos + 1;    // +1 for '/'.

Do you mind fixing LookupBreakpoint as well, if you agree that the code is
weird?

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/service/vms...
File runtime/vm/service/vmservice.dart (right):

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/service/vms...
runtime/vm/service/vmservice.dart:70: scriptLoadPort.close();
Add a comment so I don't undo this fix again!

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/service_tes...
File runtime/vm/service_test.cc (right):

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/service_tes...
runtime/vm/service_test.cc:573: }
In addition to this test, we should test this code in a dart test.  Can you
either add a case to debugging_test.dart or add a new get_stack_rpc_test.dart
test for this?

[Cutch, 2015-05-04 19:56:52]

PTAL

https://codereview.chromium.org/1122503003/diff/140001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/debugger.dart (right):

https://codereview.chromium.org/1122503003/diff/140001/runtime/observatory/li...
runtime/observatory/lib/src/elements/debugger.dart:1323: }
On 2015/05/04 17:47:12, turnidge wrote:
> Nice.

Acknowledged.

https://codereview.chromium.org/1122503003/diff/140001/runtime/observatory/li...
runtime/observatory/lib/src/elements/debugger.dart:1452:
@CustomTag('debugger-message')
On 2015/05/04 17:47:12, turnidge wrote:
> I wonder if we can share more code between debugger-frame and debuggr-message.

> Not for this CL.

Acknowledged.

https://codereview.chromium.org/1122503003/diff/140001/runtime/observatory/li...
runtime/observatory/lib/src/elements/debugger.dart:1490: bool should_reset =
On 2015/05/04 17:47:12, turnidge wrote:
> camelCase
> 
> Also, could call it messageChanged instead of shouldReset.

Done.

https://codereview.chromium.org/1122503003/diff/140001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/debugger.html (right):

https://codereview.chromium.org/1122503003/diff/140001/runtime/observatory/li...
runtime/observatory/lib/src/elements/debugger.html:373: <!-- TODO(turnidge): Add
eval box here? -->
On 2015/05/04 17:47:12, turnidge wrote:
> Drop this TODO from here.

Done.

https://codereview.chromium.org/1122503003/diff/140001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/script_ref.dart (right):

https://codereview.chromium.org/1122503003/diff/140001/runtime/observatory/li...
runtime/observatory/lib/src/elements/script_ref.dart:13: @published int pos =
-1;
On 2015/05/04 17:47:12, turnidge wrote:
> Change to pos = null

Done.

https://codereview.chromium.org/1122503003/diff/140001/runtime/observatory/li...
File runtime/observatory/lib/src/service/object.dart (right):

https://codereview.chromium.org/1122503003/diff/140001/runtime/observatory/li...
runtime/observatory/lib/src/service/object.dart:83: // Is this an isolate
message?
On 2015/05/04 17:47:12, turnidge wrote:
> Not sure that this comment adds, given that none of its peers are commented.

Done.

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

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message.cc#...
runtime/vm/message.cc:159: return current();
On 2015/05/04 17:47:13, turnidge wrote:
> Your Next() and HasNext() are a bit different than those found, e.g., in Java.

> Do we use this pattern elsewhere in the VM already?  It is odd to me that I
can
> call Next() as many times as I like and it just sticks to the end of the queue
> when HasNext is false, seems like it could hide errors.
> 
> I would be more used to seeing a Next() function that unconditionally returns
> the next element and kills the VM if it goes out of bounds.  Loops would look
> like this:
> 
> while (it.HasNext()) {
>   Message* current = it.Next();
>   ... do work here...
> }

Done.

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message.cc#...
runtime/vm/message.cc:200: message.AddProperty("type", "IsolateMessage");
On 2015/05/04 17:47:13, turnidge wrote:
> IsolateMessage?  I don't like the "Isolate".  Maybe just Message?

Done.

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message.cc#...
runtime/vm/message.cc:206: message.AddProperty("destinationPort",
On 2015/05/04 17:47:13, turnidge wrote:
> Make this _destinationPort.

Done.

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message.cc#...
runtime/vm/message.cc:210: msg_handler =
DartLibraryCalls::LookupHandler(current->dest_port());
On 2015/05/04 17:47:13, turnidge wrote:
> Add a TODO to move this map into the VM so we can avoid this call into Dart...
> (as discussed offline w/ iposva).

Done.

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message.cc#...
runtime/vm/message.cc:222: message.AddProperty("handlerScript", script);
On 2015/05/04 17:47:13, turnidge wrote:
> Consider dropping handlerScript.  Up to you.

Done.

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message_han...
File runtime/vm/message_handler.cc (right):

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message_han...
runtime/vm/message_handler.cc:351: }
On 2015/05/04 17:47:13, turnidge wrote:
> Move the bulk of this into Reset.

Done.

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message_han...
runtime/vm/message_handler.cc:364: handler_->oob_message_handling_allowed_ =
false;
On 2015/05/04 17:47:13, turnidge wrote:
> Does this need to be done under the monitor here?

Done.

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message_tes...
File runtime/vm/message_test.cc (right):

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message_tes...
runtime/vm/message_test.cc:40: }
On 2015/05/04 17:47:13, turnidge wrote:
> Unroll, as suggested below.

Done.

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message_tes...
runtime/vm/message_test.cc:57: }
On 2015/05/04 17:47:13, turnidge wrote:
> I would unroll this, I think it is shorter/simpler.
> 
> (Here's how the code would look it you rewrite Next() and HasNext() as I
> suggested elsewhere)
> 
> EXPECT(it.hasNext());
> EXPECT(it.Next() == msg1);
> EXPECT(it.hasNext());
> EXPECT(it.Next() == msg2);
> EXPECT(!it.hasNext());

Done.

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/message_tes...
runtime/vm/message_test.cc:75: }
On 2015/05/04 17:47:13, turnidge wrote:
> Unroll.

Done.

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

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/service.cc#...
runtime/vm/service.cc:1193: 
On 2015/05/04 17:47:13, turnidge wrote:
> Maybe a short function-level comment.

Done.

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/service.cc#...
runtime/vm/service.cc:1203: }
On 2015/05/04 17:47:13, turnidge wrote:
> I see that this code is very similar to code I wrote in LookupBreakpoint,
above.
>  I don't understand why I waiting to check for (rest == NULL) until after the
> strncmp test.  it's kind of weird.  I'm guessing this code used to be embedded
> in a larger control flow and then got abstracted into a function.
> 
> Once we hoist the "rest check" up, does it turn into something like this?:
> 
> size_t end_pos = strcspn(id, "/");
> if (end_pos == strlen(id)) {
>   return false;
> }
> const char* rest = id + end_pos + 1;    // +1 for '/'.
> 
> Do you mind fixing LookupBreakpoint as well, if you agree that the code is
> weird?

Done.

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/service/vms...
File runtime/vm/service/vmservice.dart (right):

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/service/vms...
runtime/vm/service/vmservice.dart:70: scriptLoadPort.close();
On 2015/05/04 17:47:13, turnidge wrote:
> Add a comment so I don't undo this fix again!

Done.

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/service_tes...
File runtime/vm/service_test.cc (right):

https://codereview.chromium.org/1122503003/diff/140001/runtime/vm/service_tes...
runtime/vm/service_test.cc:573: }
On 2015/05/04 17:47:13, turnidge wrote:
> In addition to this test, we should test this code in a dart test.  Can you
> either add a case to debugging_test.dart or add a new get_stack_rpc_test.dart
> test for this?

Acknowledged.

[turnidge, 2015-05-04 20:11:52]

lgtm w/ some comments below.

https://codereview.chromium.org/1122503003/diff/160001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/script_ref.dart (right):

https://codereview.chromium.org/1122503003/diff/160001/runtime/observatory/li...
runtime/observatory/lib/src/elements/script_ref.dart:13: @published int pos =
null;
I guess it is null by default, huh.

https://codereview.chromium.org/1122503003/diff/160001/runtime/observatory/te...
File runtime/observatory/tests/service/get_stack_rpc_test.dart (right):

https://codereview.chromium.org/1122503003/diff/160001/runtime/observatory/te...
runtime/observatory/tests/service/get_stack_rpc_test.dart:21: counter++;
I had 3 counter++'s so that I could test stepping.  You probably only need one.

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

https://codereview.chromium.org/1122503003/diff/160001/runtime/vm/message.cc#...
runtime/vm/message.cc:144: current_ = NULL;
next_ = queue_->head_;

https://codereview.chromium.org/1122503003/diff/160001/runtime/vm/message.cc#...
runtime/vm/message.cc:148: bool MessageQueue::Iterator::HasNext() {
return next_ != NULL

https://codereview.chromium.org/1122503003/diff/160001/runtime/vm/message.cc#...
runtime/vm/message.cc:161: Message* MessageQueue::Iterator::Next() {
ASSERT(next_ != NULL);
Message* current = next_;
next_ = next_->next_;
return current;

https://codereview.chromium.org/1122503003/diff/160001/runtime/vm/message.h
File runtime/vm/message.h (right):

https://codereview.chromium.org/1122503003/diff/160001/runtime/vm/message.h#n...
runtime/vm/message.h:116: // returns false when there are no more messages left.
Capitalize "returns" for consistency.

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

https://codereview.chromium.org/1122503003/diff/160001/runtime/vm/service.cc#...
runtime/vm/service.cc:1196: const char* rest = id + end_pos + 1;  // +1 for '/'.
Move the "rest" line below the end_pos bailout, here and above.

[Cutch, 2015-05-04 20:18:58]

https://codereview.chromium.org/1122503003/diff/160001/runtime/observatory/li...
File runtime/observatory/lib/src/elements/script_ref.dart (right):

https://codereview.chromium.org/1122503003/diff/160001/runtime/observatory/li...
runtime/observatory/lib/src/elements/script_ref.dart:13: @published int pos =
null;
On 2015/05/04 20:11:51, turnidge wrote:
> I guess it is null by default, huh.

Done.

https://codereview.chromium.org/1122503003/diff/160001/runtime/observatory/te...
File runtime/observatory/tests/service/get_stack_rpc_test.dart (right):

https://codereview.chromium.org/1122503003/diff/160001/runtime/observatory/te...
runtime/observatory/tests/service/get_stack_rpc_test.dart:21: counter++;
On 2015/05/04 20:11:51, turnidge wrote:
> I had 3 counter++'s so that I could test stepping.  You probably only need
one.

Acknowledged.

https://codereview.chromium.org/1122503003/diff/160001/runtime/vm/message.h
File runtime/vm/message.h (right):

https://codereview.chromium.org/1122503003/diff/160001/runtime/vm/message.h#n...
runtime/vm/message.h:116: // returns false when there are no more messages left.
On 2015/05/04 20:11:51, turnidge wrote:
> Capitalize "returns" for consistency.

Done.

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

https://codereview.chromium.org/1122503003/diff/160001/runtime/vm/service.cc#...
runtime/vm/service.cc:1196: const char* rest = id + end_pos + 1;  // +1 for '/'.
On 2015/05/04 20:11:51, turnidge wrote:
> Move the "rest" line below the end_pos bailout, here and above.

Done.

[Cutch, 2015-05-04 20:21:07]

Committed patchset #11 (id:200001) manually as 45505 (presubmit successful).