Avoid trusting the length encoded in the Snapshot if there is an

Avoid trusting the length encoded in the Snapshot if there is an
external length available.  We now pass a length with all messages in
the vm and verify that there is no mismatch with the length from the
Snapshot.

Fixed a bug in the use of ApiMessageReader.  We were always manually
adding Snapshot::kHeaderSize to the data, but neglecting to subtract
kHeaderSize from the message length.

Added FullSnapshotWriter and MessageWriter classes.
Committed: https://code.google.com/p/dart/source/detail?r=11413

[siva, 2012-08-22 01:57:49]

I have two meta level questions:

1. We wanted to support streaming of messages when doing isolate communications,
will the new API in Message hinder that? We were considering removing the length
encoding in the message and instead continue reading until we hit some sentinel
value.

2. When communicating across isolates in different processes can we send partial
messages across or are we supposed to wait until the other end is able to accept
a message of 'len' size?

[siva, 2012-08-22 23:30:39]

https://chromiumcodereview.appspot.com/10829444/diff/1/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://chromiumcodereview.appspot.com/10829444/diff/1/runtime/lib/isolate.cc...
runtime/lib/isolate.cc:142: intptr_t data_len = writer.FinalizeBuffer();
As discussed offline we could probably get rid of the call to FinalizeBuffer
here and instead set data_len to writer.bytes_written().

https://chromiumcodereview.appspot.com/10829444/diff/1/runtime/vm/dart_api_im...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/10829444/diff/1/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:826: *size = writer.WriteFullSnapshot();
I would prefer not returning the size as a return value from WriteFullSnapshot
because we may want to return an error once we beef it up for chunked writing.

https://chromiumcodereview.appspot.com/10829444/diff/1/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://chromiumcodereview.appspot.com/10829444/diff/1/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:92: FATAL("IsolateMessageHandler saw malformed message. 
Exiting.");
Do these really need to be fatal errors?

We could drop this garbage message, set a sticky error and return false, the way
it seems to do it in the code below.

https://chromiumcodereview.appspot.com/10829444/diff/1/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:103: FATAL("IsolateMessageHandler saw malformed message. 
Exiting.");
Ditto comment.

[turnidge, 2012-08-23 18:37:57]

Siva,

This is a pretty major revision.  I have dropped the header entirely for
messages and I've reworked how the header gets added for full and script
snapshots.  You'll probably want to look it all over again.

Todd

https://chromiumcodereview.appspot.com/10829444/diff/1/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://chromiumcodereview.appspot.com/10829444/diff/1/runtime/lib/isolate.cc...
runtime/lib/isolate.cc:142: intptr_t data_len = writer.FinalizeBuffer();
On 2012/08/22 23:30:39, asiva wrote:
> As discussed offline we could probably get rid of the call to FinalizeBuffer
> here and instead set data_len to writer.bytes_written().

Done.

https://chromiumcodereview.appspot.com/10829444/diff/1/runtime/vm/dart_api_im...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/10829444/diff/1/runtime/vm/dart_api_im...
runtime/vm/dart_api_impl.cc:826: *size = writer.WriteFullSnapshot();
On 2012/08/22 23:30:39, asiva wrote:
> I would prefer not returning the size as a return value from WriteFullSnapshot
> because we may want to return an error once we beef it up for chunked writing.

Done.

https://chromiumcodereview.appspot.com/10829444/diff/1/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://chromiumcodereview.appspot.com/10829444/diff/1/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:92: FATAL("IsolateMessageHandler saw malformed message. 
Exiting.");
I believe it should be fatal, yes.  Made this UNREACHABLE().

On 2012/08/22 23:30:39, asiva wrote:
> Do these really need to be fatal errors?
> 
> We could drop this garbage message, set a sticky error and return false, the
way
> it seems to do it in the code below.

https://chromiumcodereview.appspot.com/10829444/diff/1/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:103: FATAL("IsolateMessageHandler saw malformed message. 
Exiting.");
On 2012/08/22 23:30:39, asiva wrote:
> Ditto comment.

This goes away in new revision.

[siva, 2012-08-27 16:31:17]

lgtm

https://chromiumcodereview.appspot.com/10829444/diff/22001/runtime/vm/snapsho...
File runtime/vm/snapshot_test.cc (right):

https://chromiumcodereview.appspot.com/10829444/diff/22001/runtime/vm/snapsho...
runtime/vm/snapshot_test.cc:595: }
This has changed the model of the test itself. It used to serialize all
singleton objects into one snapshot and read from that snapshot but now it seems
to create individual snapshots for each of them and read them.

[turnidge, 2012-08-27 17:43:05]

https://chromiumcodereview.appspot.com/10829444/diff/22001/runtime/vm/snapsho...
File runtime/vm/snapshot_test.cc (right):

https://chromiumcodereview.appspot.com/10829444/diff/22001/runtime/vm/snapsho...
runtime/vm/snapshot_test.cc:595: }
On 2012/08/27 16:31:17, asiva wrote:
> This has changed the model of the test itself. It used to serialize all
> singleton objects into one snapshot and read from that snapshot but now it
seems
> to create individual snapshots for each of them and read them.

Yes, it has.  I didn't think that putting them in the same snapshot was an
important aspect of the test.  My opinion was that if they can
serialize/deserialize individually, they can probably do it together.  I figured
that the existing FullSnapshot test provided a fine test of multiple object
serialization/deserialization.

Is it important to you to have it as it originally was?  If so, I either need to
(a) throw all of the singletons into an object array and serialize that as a
single message or  (b) make my own testing subclass of SnapshotWriter that
exposes UnmarkAll() publicly.

Let me know what you'd like me to do.

[siva, 2012-08-27 18:41:03]

https://chromiumcodereview.appspot.com/10829444/diff/22001/runtime/vm/snapsho...
File runtime/vm/snapshot_test.cc (right):

https://chromiumcodereview.appspot.com/10829444/diff/22001/runtime/vm/snapsho...
runtime/vm/snapshot_test.cc:595: }
This test was trying to test both the singleton aspect as well as the
possibility of having only singletons (more than one) in the snapshot. (i.e no
code in writer assumed it will see an object to be serialized).

On 2012/08/27 17:43:05, turnidge wrote:
> On 2012/08/27 16:31:17, asiva wrote:
> > This has changed the model of the test itself. It used to serialize all
> > singleton objects into one snapshot and read from that snapshot but now it
> seems
> > to create individual snapshots for each of them and read them.
> 
> Yes, it has.  I didn't think that putting them in the same snapshot was an
> important aspect of the test.  My opinion was that if they can
> serialize/deserialize individually, they can probably do it together.  I
figured
> that the existing FullSnapshot test provided a fine test of multiple object
> serialization/deserialization.
> 
> Is it important to you to have it as it originally was?  If so, I either need
to
> (a) throw all of the singletons into an object array and serialize that as a
> single message or  (b) make my own testing subclass of SnapshotWriter that
> exposes UnmarkAll() publicly.
> 
> Let me know what you'd like me to do.

[turnidge, 2012-08-27 20:09:16]

https://chromiumcodereview.appspot.com/10829444/diff/22001/runtime/vm/snapsho...
File runtime/vm/snapshot_test.cc (right):

https://chromiumcodereview.appspot.com/10829444/diff/22001/runtime/vm/snapsho...
runtime/vm/snapshot_test.cc:595: }
Ok, I've revert it to its original form.

On 2012/08/27 18:41:03, asiva wrote:
> This test was trying to test both the singleton aspect as well as the
> possibility of having only singletons (more than one) in the snapshot. (i.e no
> code in writer assumed it will see an object to be serialized).
> 
> On 2012/08/27 17:43:05, turnidge wrote:
> > On 2012/08/27 16:31:17, asiva wrote:
> > > This has changed the model of the test itself. It used to serialize all
> > > singleton objects into one snapshot and read from that snapshot but now it
> > seems
> > > to create individual snapshots for each of them and read them.
> > 
> > Yes, it has.  I didn't think that putting them in the same snapshot was an
> > important aspect of the test.  My opinion was that if they can
> > serialize/deserialize individually, they can probably do it together.  I
> figured
> > that the existing FullSnapshot test provided a fine test of multiple object
> > serialization/deserialization.
> > 
> > Is it important to you to have it as it originally was?  If so, I either
need
> to
> > (a) throw all of the singletons into an object array and serialize that as a
> > single message or  (b) make my own testing subclass of SnapshotWriter that
> > exposes UnmarkAll() publicly.
> > 
> > Let me know what you'd like me to do.
>