Fix VM not accepting static methods for Isolate.spawn.

Fix VM not accepting static methods for Isolate.spawn.

BUG= http://dartbug.com/3011

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

R=iposva@google.com

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

[Ivan Posva, 2013-11-12 07:19:48]

First round of comments.

We used to have this functionality in the past and it was removed. Adding Siva
to figure out reasons why.

-Ivan

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

https://codereview.chromium.org/68813002/diff/30001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:1012: class_name_ = strdup(class_name.ToCString());
Leak!

https://codereview.chromium.org/68813002/diff/30001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:1062: function_name(), (library_url() ? library_url() :
script_url())));
ditto (see below).

https://codereview.chromium.org/68813002/diff/30001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:1073: class_name(), (library_url() ? library_url() :
script_url())));
I know this was already bad in the original code, but a char* is not a boolean.
Please change to "(library_url() != NULL) ? library_url() : script_url()".

https://codereview.chromium.org/68813002/diff/30001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:1082: (library_url() ? library_url() : script_url())));
ditto

https://codereview.chromium.org/68813002/diff/30001/tests/isolate/static_func...
File tests/isolate/static_function_test.dart (right):

https://codereview.chromium.org/68813002/diff/30001/tests/isolate/static_func...
tests/isolate/static_function_test.dart:6: // VMOptions=--enable_type_checks
--enable_asserts
Why are you enforcing these VM options here?

https://codereview.chromium.org/68813002/diff/30001/tests/isolate/static_func...
File tests/isolate/static_function_testlib.dart (right):

https://codereview.chromium.org/68813002/diff/30001/tests/isolate/static_func...
tests/isolate/static_function_testlib.dart:5: library static_function_testlib;
We usually name these just static_function_lib in a file
static_function_lib.dart.

[Lasse Reichstein Nielsen, 2013-11-12 07:28:50]

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

https://codereview.chromium.org/68813002/diff/30001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:1012: class_name_ = strdup(class_name.ToCString());
Beacuse I'm not freeing it somewhere else? (which makes sense, I'll see if I can
find where the other fields are freed).
Or is there some other reason why this is a leak, and not function_name_ above?

https://codereview.chromium.org/68813002/diff/30001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:1062: function_name(), (library_url() ? library_url() :
script_url())));
On 2013/11/12 07:19:49, Ivan Posva wrote:
> ditto (see below).

Done.

https://codereview.chromium.org/68813002/diff/30001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:1073: class_name(), (library_url() ? library_url() :
script_url())));
On 2013/11/12 07:19:49, Ivan Posva wrote:
> I know this was already bad in the original code, but a char* is not a
boolean.
> Please change to "(library_url() != NULL) ? library_url() : script_url()".

Done.

https://codereview.chromium.org/68813002/diff/30001/runtime/vm/isolate.cc#new...
runtime/vm/isolate.cc:1082: (library_url() ? library_url() : script_url())));
On 2013/11/12 07:19:49, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/68813002/diff/30001/tests/isolate/static_func...
File tests/isolate/static_function_test.dart (right):

https://codereview.chromium.org/68813002/diff/30001/tests/isolate/static_func...
tests/isolate/static_function_test.dart:6: // VMOptions=--enable_type_checks
--enable_asserts
Not needed. They were in the test file I started out from, and I forgot to
remove them.

https://codereview.chromium.org/68813002/diff/30001/tests/isolate/static_func...
File tests/isolate/static_function_testlib.dart (right):

https://codereview.chromium.org/68813002/diff/30001/tests/isolate/static_func...
tests/isolate/static_function_testlib.dart:5: library static_function_testlib;
On 2013/11/12 07:19:49, Ivan Posva wrote:
> We usually name these just static_function_lib in a file
> static_function_lib.dart.

Done.

[Lasse Reichstein Nielsen, 2013-11-12 07:34:05]

https://codereview.chromium.org/68813002/diff/110001/tests/isolate/static_fun...
File tests/isolate/static_function_test.dart (right):

https://codereview.chromium.org/68813002/diff/110001/tests/isolate/static_fun...
tests/isolate/static_function_test.dart:67: throwsTest("static closure",
staticClosure);
This test, and the next one, fails in the VM.
I'll try to figure out how to distinguish these from declared top-level
functions.

[Lasse Reichstein Nielsen, 2013-11-12 07:51:00]

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

https://codereview.chromium.org/68813002/diff/150001/runtime/lib/isolate.cc#n...
runtime/lib/isolate.cc:224: func.name() == Symbols::AnonymousClosure().raw()) {
This feels a little hackish. Is there a better way to only allow *declared*
top-level/static methods?

[Lasse Reichstein Nielsen, 2013-11-14 08:32:32]

ping

[Ivan Posva, 2013-11-14 17:51:01]

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

https://codereview.chromium.org/68813002/diff/150001/runtime/lib/isolate.cc#n...
runtime/lib/isolate.cc:224: func.name() == Symbols::AnonymousClosure().raw()) {
On 2013/11/12 07:51:00, Lasse Reichstein Nielsen wrote:
> This feels a little hackish. Is there a better way to only allow *declared*
> top-level/static methods?

You are not protecting against this here:

foo() {
  bar() { ...}
  return bar;
}

main() {
  Isolate.spawn(foo());
}

[Ivan Posva, 2013-11-14 18:11:45]

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

https://codereview.chromium.org/68813002/diff/150001/runtime/lib/isolate.cc#n...
runtime/lib/isolate.cc:224: func.name() == Symbols::AnonymousClosure().raw()) {
On 2013/11/14 17:51:01, Ivan Posva wrote:
> On 2013/11/12 07:51:00, Lasse Reichstein Nielsen wrote:
> > This feels a little hackish. Is there a better way to only allow *declared*
> > top-level/static methods?
> 
> You are not protecting against this here:
> 
> foo() {
>   bar() { ...}
>   return bar;
> }
> 
> main() {
>   Isolate.spawn(foo());
> }

I think what you want here is:

if (!func.IsImplicitClosureFunction || !func.is_static()) {
  throw_exception = true;
}

[Ivan Posva, 2013-11-14 18:16:47]

Please add a test that ensures that named inner functions are not sneaking
through, so that we do not end up trying to use the name of the function as a
signal for acceptance.

Thanks,
-Ivan

[Lasse Reichstein Nielsen, 2013-11-15 10:10:03]

Your suggested fix worked.
It looks like it is all working now.

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

https://codereview.chromium.org/68813002/diff/150001/runtime/lib/isolate.cc#n...
runtime/lib/isolate.cc:224: func.name() == Symbols::AnonymousClosure().raw()) {
Good catch!
I had thought the !func.is_static() would prohibit that case, the suggested
change works like a charm.

Added test for named function inside function, and functions in constructors
too.

[Lasse Reichstein Nielsen, 2013-11-15 10:28:42]

Committed patchset #5 manually as r30300 (presubmit successful).

[Lasse Reichstein Nielsen, 2013-11-15 11:22:27]

Ick! Committed wrong workspace. Will revert.

[Lasse Reichstein Nielsen, 2013-11-15 11:45:43]

Reverted, please continue looking at CL.

[Lasse Reichstein Nielsen, 2013-11-19 07:43:34]

ping

[Ivan Posva, 2013-11-19 22:20:15]

LGTM -ip

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

https://codereview.chromium.org/68813002/diff/310002/runtime/lib/isolate.cc#n...
runtime/lib/isolate.cc:218: GET_NON_NULL_NATIVE_ARGUMENT(Instance, closure,
arguments->NativeArgAt(0));
This whole control flow can be simplified. Feel free to address or add a
TODO(iposva):

if (closure.IsClosure()) {
  const Function& func = Function::Handle(Closure::function(closure));
  if (func.IsImplicitClosureFunction() && func.is_static()) {
    ASSERT(...)
    return Spawn(...);
  }
}
Exceptions::ThrowArgumentException(...);
return Object::null();

[Lasse Reichstein Nielsen, 2013-11-20 09:09:09]

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

https://codereview.chromium.org/68813002/diff/310002/runtime/lib/isolate.cc#n...
runtime/lib/isolate.cc:218: GET_NON_NULL_NATIVE_ARGUMENT(Instance, closure,
arguments->NativeArgAt(0));
Done. Just rearranged the blocks, so if you know a way to simplyfy the "#ifdef
DEBUG" block into a single ASSERT, you can still do that.

[Lasse Reichstein Nielsen, 2013-11-20 09:16:12]

Committed patchset #7 manually as r30442 (presubmit successful).