Isolates: allow sending of arbitrary objects in dart2js.

Isolates: allow sending of arbitrary objects in dart2js.

BUG= http://dartbug.com/12628
BUG= http://dartbug.com/9245
R=lrn@google.com, sigurdm@google.com, sra@google.com

Committed: https://code.google.com/p/dart/source/detail?r=42161
Reverted: https://code.google.com/p/dart/source/detail?r=42206

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

[floitsch, 2014-11-21 19:38:59]

floitsch@google.com changed reviewers:
+ lrn@google.com, sigurdm@google.com, sra@google.com

[floitsch, 2014-11-21 19:39:00]

@lrn: please review as much as possible, but at the very least the isolate and
test changes.

@sra: please review sdk/lib/_internal/compiler/js_lib/native_helper.dart

@sigurdm: please review the changes to the compiler.

@iposva: I have added some tests for serialization (VM and dart2js currently
fail mostly the same tests). If you have already bug-numbers, please let me
know.

[sigurdm, 2014-11-24 09:06:09]

lgtm

https://codereview.chromium.org/742873002/diff/80001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/742873002/diff/80001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:255: List get
defineClassFunction {
List<js.Expression> ?

https://codereview.chromium.org/742873002/diff/80001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:322: var
classFieldsExtractorAssignment = js('''
Give type

https://codereview.chromium.org/742873002/diff/80001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:345: # = function(name,
o, args) {
Perhaps name third argument 'fields'

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/collection_patch.dart (right):

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/collection_patch.dart:8: import
'dart:_internal' show InternalMapMarker;
Shouldn't this be:
 show InternalMap;
?

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/isolate_helper.dart (right):

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_helper.dart:992: // Make sure that the
args list is a generic list.
What happens if it is not?

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart (right):

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:88: if (indexable
is JSArray) return ["const", id, serialized];
Maybe use a different keyword. 'const' has a certain meaning in dart.

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:329:
deserializeAny(x) {
Maybe name this 'deserializeDartObject'

[Lasse Reichstein Nielsen, 2014-11-24 12:55:42]

lgtm

https://codereview.chromium.org/742873002/diff/80001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/742873002/diff/80001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:278: if
(#hasIsolateSupport) { var fieldNames = ""; }
Wouldn't it be better to not have this line at all if hasIsolateSupport is
false?

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/isolate_helper.dart (right):

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_helper.dart:992: // Make sure that the
args list is a generic list.
Also ensures that it is a fresh list.

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart (right):

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:88: if (indexable
is JSArray) return ["const", id, serialized];
This is a const array (created using `const[...]`), right?

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:110: return ['map',
id,
Do you have to send the id with the object - can't it be reconstructed based on
the order of objects in the serialized form? (I.e., a counter matching the
counter on the serializing side)?

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:116: //
TODO(floitsch): weed out more JS objects.
Document what you are trying to guard against. The code is *not* obvious.

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:118: JS('bool',
'(typeof #.__proto__) !== "object"', x)) {
Are there any prototypes that don't have typeof "object"?
For example `typeof new Date().__proto__` is "object".
And `typeof new Date()` is also "object", so using that as prototype won't be
caught.

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:272: return result;
This special casing of just a single Map implementation isn't really enough.
In practice we need fixes for HashMap, [Linked]IdentityHashMap, [Linked]HashSet,
[Linked]IdentityHashSet, and even [Linked]CustomHashSet/Map (if the custom
functions are serializable).

And then there's all the other things that don't work.

I'm a little wary at adding this, because I don't want anyone to think that it
is actually even half a solution. I'd almost rather that it just doesn't work
until we fix it properly. Half (or less) solutions can live for years with
people working around it or just not using it :(

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:301: return x[1];
This means the SendPort preserves its identity when sent through another
SendPort, unlike everything else.

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/internal/interna...
File sdk/lib/internal/internal.dart (right):

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/internal/interna...
sdk/lib/internal/internal.dart:53: }
This should be in a dart2js specific import like _isolate_helper, not in
_internal which is also imported by the VM.

(My argument for that is somewhat lessened by the table above only being used by
the VM)

[floitsch, 2014-11-24 15:06:58]

https://codereview.chromium.org/742873002/diff/80001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart (right):

https://codereview.chromium.org/742873002/diff/80001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:255: List get
defineClassFunction {
On 2014/11/24 09:06:09, sigurdm wrote:
> List<js.Expression> ?

List<jsAst.Node>. (some of the elements are not expressions).

https://codereview.chromium.org/742873002/diff/80001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:278: if
(#hasIsolateSupport) { var fieldNames = ""; }
On 2014/11/24 12:55:41, Lasse Reichstein Nielsen wrote:
> Wouldn't it be better to not have this line at all if hasIsolateSupport is
> false?

The js-pretty printer removes "if (false) ..." and removes the if-condition for
'if (true)'. It does this syntactically (so the 'var' is gone too).

https://codereview.chromium.org/742873002/diff/80001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:322: var
classFieldsExtractorAssignment = js('''
On 2014/11/24 09:06:09, sigurdm wrote:
> Give type

Done.

https://codereview.chromium.org/742873002/diff/80001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/old_emitter/emitter.dart:345: # = function(name,
o, args) {
On 2014/11/24 09:06:09, sigurdm wrote:
> Perhaps name third argument 'fields'

Done.

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/collection_patch.dart (right):

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/collection_patch.dart:8: import
'dart:_internal' show InternalMapMarker;
On 2014/11/24 09:06:09, sigurdm wrote:
> Shouldn't this be:
>  show InternalMap;
> ?

Weird. How could this work?

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/isolate_helper.dart (right):

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_helper.dart:992: // Make sure that the
args list is a generic list.
On 2014/11/24 09:06:09, sigurdm wrote:
> What happens if it is not?

Added more info to comment. It's mostly to make it easier for programmers. With
this line a newly spawned isolate can assume that the args-list is an extendable
list (of the native list type).

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_helper.dart:992: // Make sure that the
args list is a generic list.
On 2014/11/24 12:55:41, Lasse Reichstein Nielsen wrote:
> Also ensures that it is a fresh list.

added "fresh" to the comment.

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart (right):

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:88: if (indexable
is JSArray) return ["const", id, serialized];
On 2014/11/24 12:55:41, Lasse Reichstein Nielsen wrote:
> This is a const array (created using `const[...]`), right?

Yes. Since it is not a JSMutableArray it must be const.

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:88: if (indexable
is JSArray) return ["const", id, serialized];
On 2014/11/24 09:06:09, sigurdm wrote:
> Maybe use a different keyword. 'const' has a certain meaning in dart.

This is the const List (as in const [1, 2]). Since we already tested for the
Mutable version this one must be the const one. Added comment.

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:110: return ['map',
id,
On 2014/11/24 12:55:42, Lasse Reichstein Nielsen wrote:
> Do you have to send the id with the object - can't it be reconstructed based
on
> the order of objects in the serialized form? (I.e., a counter matching the
> counter on the serializing side)?

I thought it would be less brittle with the ids, but in the end we still rely on
the order of deserialization, since the 'ref' references code that must have
been deserialized before.
Changed the code and added test.

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:116: //
TODO(floitsch): weed out more JS objects.
On 2014/11/24 12:55:41, Lasse Reichstein Nielsen wrote:
> Document what you are trying to guard against. The code is *not* obvious.

Changed to checking the constructor, which is what I should have been doing in
the first place.

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:118: JS('bool',
'(typeof #.__proto__) !== "object"', x)) {
On 2014/11/24 12:55:41, Lasse Reichstein Nielsen wrote:
> Are there any prototypes that don't have typeof "object"?
> For example `typeof new Date().__proto__` is "object".
> And `typeof new Date()` is also "object", so using that as prototype won't be
> caught.
> 

Functions have "function" as type for the prototype.
But obsolete, since I changed the test to look for the constructor property.

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:272: return result;
On 2014/11/24 12:55:41, Lasse Reichstein Nielsen wrote:
> This special casing of just a single Map implementation isn't really enough.
> In practice we need fixes for HashMap, [Linked]IdentityHashMap,
[Linked]HashSet,
> [Linked]IdentityHashSet, and even [Linked]CustomHashSet/Map (if the custom
> functions are serializable).
> 
> And then there's all the other things that don't work.
> 
> I'm a little wary at adding this, because I don't want anyone to think that it
> is actually even half a solution. I'd almost rather that it just doesn't work
> until we fix it properly. Half (or less) solutions can live for years with
> people working around it or just not using it :(

This is just a special case for the extremely common hashmap. Otherwise sending
maps is very expensive. It wasn't added with the intent of fixing anything.

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:301: return x[1];
On 2014/11/24 12:55:41, Lasse Reichstein Nielsen wrote:
> This means the SendPort preserves its identity when sent through another
> SendPort, unlike everything else.

Yes. But anything else would have required big refactorings. Once we don't have
local isolates we can simplify lots of the code (including this part).

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:329:
deserializeAny(x) {
On 2014/11/24 09:06:09, sigurdm wrote:
> Maybe name this 'deserializeDartObject'

Done.

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/internal/interna...
File sdk/lib/internal/internal.dart (right):

https://codereview.chromium.org/742873002/diff/80001/sdk/lib/internal/interna...
sdk/lib/internal/internal.dart:53: }
On 2014/11/24 12:55:42, Lasse Reichstein Nielsen wrote:
> This should be in a dart2js specific import like _isolate_helper, not in
> _internal which is also imported by the VM.
> 
> (My argument for that is somewhat lessened by the table above only being used
by
> the VM)

moved to js_helper.

[Lasse Reichstein Nielsen, 2014-11-24 15:37:05]

lgtm

https://codereview.chromium.org/742873002/diff/90001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart (right):

https://codereview.chromium.org/742873002/diff/90001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:118: JS('bool',
'x.constructor !== Object')) {
Still document what the case is that you are guarding for/against.

I assume that what you want is to only allow objects with a prototype chain
containing only the (initial) value of Object.prototype (which is what you
create with `{}` or `new Object`).
The most precise test of that would be `#.__proto__ === Object.prototype`, but
using `# != null && #.constructor == Object` might be faster than using
__proto__, and almost as safe (nothing will prevent determined and malicious JS
code anyway).

(I assume `#.__proto__` is more efficient than `Object.getPrototype(#)` since
you are using that).

[sra1, 2014-11-24 18:18:06]

Can you refresh the patch?  I was unable to patch against HEAD.
What is the code size impact, especially for programs that don't use the
feature, or only use it 'lightly'?

https://codereview.chromium.org/742873002/diff/90001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/native_helper.dart (right):

https://codereview.chromium.org/742873002/diff/90001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/native_helper.dart:447: if (constructor) var
name = constructor.name;
While valid and compact JavaScript, this looks a little odd.

I'd prefer:

if (constructor) {
  var name = constructor.name;
  if (name) return name;
}

[floitsch, 2014-11-25 12:32:18]

https://codereview.chromium.org/742873002/diff/90001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart (right):

https://codereview.chromium.org/742873002/diff/90001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:118: JS('bool',
'x.constructor !== Object')) {
On 2014/11/24 15:37:05, Lasse Reichstein Nielsen wrote:
> Still document what the case is that you are guarding for/against.
> 
> I assume that what you want is to only allow objects with a prototype chain
> containing only the (initial) value of Object.prototype (which is what you
> create with `{}` or `new Object`).
> The most precise test of that would be `#.__proto__ === Object.prototype`, but
> using `# != null && #.constructor == Object` might be faster than using
> __proto__, and almost as safe (nothing will prevent determined and malicious
JS
> code anyway).
Changed comment to:
    // We only serialize direct properties, and no properties from prototypes.
    // Check for the constructor and see if it's Object or undefined/null. This
    // should have no inheritance, and can't be functions either.

I prefer not to access "__proto__".

> 
> (I assume `#.__proto__` is more efficient than `Object.getPrototype(#)` since
> you are using that).

I don't use "__proto__".

https://codereview.chromium.org/742873002/diff/90001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/native_helper.dart (right):

https://codereview.chromium.org/742873002/diff/90001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/native_helper.dart:447: if (constructor) var
name = constructor.name;
On 2014/11/24 18:18:06, sra1 wrote:
> While valid and compact JavaScript, this looks a little odd.
> 
> I'd prefer:
> 
> if (constructor) {
>   var name = constructor.name;
>   if (name) return name;
> }

Done.

[floitsch, 2014-11-25 12:55:03]

On 2014/11/24 18:18:06, sra1 wrote:
> Can you refresh the patch?  I was unable to patch against HEAD.
Done.

> What is the code size impact, especially for programs that don't use the
> feature, or only use it 'lightly'?

Depends. A very minimal example grows:
===
import 'dart:isolate';
main(x) { Isolate.spawn(main, []); }
===
-rw-r----- 1 floitsch eng 224838 Nov 25 13:34 /tmp/out_old.js
-rw-r----- 1 floitsch eng 229498 Nov 25 13:42 /tmp/out_new.js

However, the CL actually removes some code, and once other parts of the program
start to use the primitives that are used by the (de)serializer the output
shrinks.
I added a similar `Isolate.spawn(main, [])` to swarm and the new version is
smaller:

-rw-r----- 1 floitsch eng 723683 Nov 25 13:50 /tmp/swarm_old.js
-rw-r----- 1 floitsch eng 717475 Nov 25 13:49 /tmp/swarm_new.js


> 
>
https://codereview.chromium.org/742873002/diff/90001/sdk/lib/_internal/compil...
> File sdk/lib/_internal/compiler/js_lib/native_helper.dart (right):
> 
>
https://codereview.chromium.org/742873002/diff/90001/sdk/lib/_internal/compil...
> sdk/lib/_internal/compiler/js_lib/native_helper.dart:447: if (constructor) var
> name = constructor.name;
> While valid and compact JavaScript, this looks a little odd.
> 
> I'd prefer:
> 
> if (constructor) {
>   var name = constructor.name;
>   if (name) return name;
> }

[Lasse Reichstein Nielsen, 2014-11-25 13:07:53]

https://codereview.chromium.org/742873002/diff/90001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart (right):

https://codereview.chromium.org/742873002/diff/90001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:118: JS('bool',
'x.constructor !== Object')) {
Not sure what "direct properties" mean. Is it "own properties" (like in
Object.getOwnPropertyKeys)?

Not sure what "This" refers to in last sentence.

How about: 
Don't serialize objects if their `constructor` property isn't `Object` or
undefined/null. 
A different constructor is taken as a sign that the object has complex internal
state, 
or that it is a function, and won't be serialized.

[floitsch, 2014-11-25 13:39:19]

https://codereview.chromium.org/742873002/diff/90001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart (right):

https://codereview.chromium.org/742873002/diff/90001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:118: JS('bool',
'x.constructor !== Object')) {
On 2014/11/25 13:07:53, Lasse Reichstein Nielsen wrote:
> Not sure what "direct properties" mean. Is it "own properties" (like in
> Object.getOwnPropertyKeys)?
> 
> Not sure what "This" refers to in last sentence.
> 
> How about: 
> Don't serialize objects if their `constructor` property isn't `Object` or
> undefined/null. 
> A different constructor is taken as a sign that the object has complex
internal
> state, 
> or that it is a function, and won't be serialized.

Done.

[floitsch, 2014-12-02 12:16:00]

On 2014/11/25 13:39:19, floitsch wrote:
>
https://codereview.chromium.org/742873002/diff/90001/sdk/lib/_internal/compil...
> File sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart (right):
> 
>
https://codereview.chromium.org/742873002/diff/90001/sdk/lib/_internal/compil...
> sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:118: JS('bool',
> 'x.constructor !== Object')) {
> On 2014/11/25 13:07:53, Lasse Reichstein Nielsen wrote:
> > Not sure what "direct properties" mean. Is it "own properties" (like in
> > Object.getOwnPropertyKeys)?
> > 
> > Not sure what "This" refers to in last sentence.
> > 
> > How about: 
> > Don't serialize objects if their `constructor` property isn't `Object` or
> > undefined/null. 
> > A different constructor is taken as a sign that the object has complex
> internal
> > state, 
> > or that it is a function, and won't be serialized.
> 
> Done.

@sra: ping.

[sra1, 2014-12-03 00:53:52]

Sorry for the delay.

I think typed data is probably a bit messed up.

On the size front, this comes out ahead on swarm (-5k) because it avoids
instantiating _HashMap and some toList() methods and that counts for more than
(-12k) the added (+7k) serialization code in async lib.
Maybe we are just lucky there is no remaining calls to 'new Map()'.


The following issues should be fixed in a separate CL:

dart:svg expands by 2k (and html by 1k) due to 'is Interceptor'.
The size is (1) a bunch of $isInterceptor flags and (2) some loss of
class-sharing.  I'm surprised at this since I usually only see this effect for
'implemented' interfaces, not from extending a superclass.

There is some bloat from 'is TypedData' (an implemented interface) that should
be resolved by changing to 'is NativeTypedData'.

https://chromiumcodereview.appspot.com/742873002/diff/190001/sdk/lib/_interna...
File sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart (right):

https://chromiumcodereview.appspot.com/742873002/diff/190001/sdk/lib/_interna...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:50: if (x is
Interceptor) unsupported(x);
ByteData is a TypedData that is not indexable.
ByteData (in JS: DataView) can be sent via postMessage, like other TypedData.

Please add a test for serializing ByteData.

I think maybe ByteBuffer can also be sent via postMessage.

https://chromiumcodereview.appspot.com/742873002/diff/190001/sdk/lib/_interna...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:79: if (indexable
is TypedData) {
There are classes that implement this that are not postable (e.g.
Float32x4List).

NativeTypedData is the root for all these and can be used as a marker.
See above that this includes non-indexables.

https://chromiumcodereview.appspot.com/742873002/diff/190001/sdk/lib/_interna...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:113:
x.values.map(serialize).toList()];
Unfortunately this creates a second tear-off closure.

[floitsch, 2014-12-04 16:16:19]

PTAL.

https://chromiumcodereview.appspot.com/742873002/diff/190001/sdk/lib/_interna...
File sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart (right):

https://chromiumcodereview.appspot.com/742873002/diff/190001/sdk/lib/_interna...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:50: if (x is
Interceptor) unsupported(x);
On 2014/12/03 00:53:52, sra1 wrote:
> ByteData is a TypedData that is not indexable.
> ByteData (in JS: DataView) can be sent via postMessage, like other TypedData.
> 
> Please add a test for serializing ByteData.
> 
> I think maybe ByteBuffer can also be sent via postMessage.

Done.

https://chromiumcodereview.appspot.com/742873002/diff/190001/sdk/lib/_interna...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:79: if (indexable
is TypedData) {
On 2014/12/03 00:53:52, sra1 wrote:
> There are classes that implement this that are not postable (e.g.
> Float32x4List).
> 
> NativeTypedData is the root for all these and can be used as a marker.
> See above that this includes non-indexables.

Done.

https://chromiumcodereview.appspot.com/742873002/diff/190001/sdk/lib/_interna...
sdk/lib/_internal/compiler/js_lib/isolate_serialization.dart:113:
x.values.map(serialize).toList()];
On 2014/12/03 00:53:52, sra1 wrote:
> Unfortunately this creates a second tear-off closure.

Done.

[sra1, 2014-12-04 19:02:12]

lgtm

https://chromiumcodereview.appspot.com/742873002/diff/210001/tests/isolate/is...
File tests/isolate/isolate.status (right):

https://chromiumcodereview.appspot.com/742873002/diff/210001/tests/isolate/is...
tests/isolate/isolate.status:8: message3_test/constList_identical: RuntimeError
# Issue TODO
If possible create the issue and update TODOs before committing.

[floitsch, 2014-12-08 13:28:43]

Committed patchset #15 (id:270001) manually as 42161 (presubmit successful).

[floitsch, 2014-12-09 14:11:07]

Had to revert.
Patch set 16 contains a rebased version of the CL.
This CL now also includes the changes to the status files and to the tests of
the follow-up commits: r42161, r42163, r42165, r42166.

[floitsch, 2014-12-09 14:11:52]

On 2014/12/09 14:11:07, floitsch wrote:
> Had to revert.
> Patch set 16 contains a rebased version of the CL.
> This CL now also includes the changes to the status files and to the tests of
> the follow-up commits: r42161, r42163, r42165, r42166.

Note: other than the rebase, nothing new in Patch set 16.
I will fix CSP now, and ask for new reviews when its ready.

[floitsch, 2014-12-09 14:51:41]

@lrn, sigurdm: PTAL. (Use diff between Patchset 16 and 17 to see the changes
needed for CSP).

[floitsch, 2014-12-09 14:53:00]

@zarah: fyi. I'm maybe conflicting with your changes.

[sigurdm, 2014-12-09 15:34:18]

lgtm

[floitsch, 2014-12-09 16:42:14]

Committed patchset #17 (id:310001) manually as 42209 (presubmit successful).