VM: Fix bug in deferred loading.

VM: Fix bug in deferred loading.

In some cases when loading a deferred library twice, the VM would crash/throw.

Make sure we generate the correct static NoSuchMethod error when referring to
a deferred library that is not loaded yet.

Fixes #28278.

R=rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/9c82d88d67b44457eaaa70d80f1fd15288b8ee05

[Florian Schneider, 2017-01-05 21:17:21]

fschneider@google.com changed reviewers:
+ rmacnak@google.com

[hausner, 2017-01-05 21:50:43]

hausner@google.com changed reviewers:
+ hausner@google.com

[hausner, 2017-01-05 21:50:44]

DBC

https://codereview.chromium.org/2618823002/diff/1/runtime/vm/ast.h
File runtime/vm/ast.h (right):

https://codereview.chromium.org/2618823002/diff/1/runtime/vm/ast.h#newcode587
runtime/vm/ast.h:587: const LibraryPrefix* prefix_;
We don't typically store pointers to handles in the ast.

https://codereview.chromium.org/2618823002/diff/1/tests/language/regress_2827...
File tests/language/regress_28278_test.dart (right):

https://codereview.chromium.org/2618823002/diff/1/tests/language/regress_2827...
tests/language/regress_28278_test.dart:14: def.foo(print("here"));
Can you make the test self-checking, e.g. by concatenating the strings in a
trace that can be compared to an expected value?

[Florian Schneider, 2017-01-05 22:07:16]

https://codereview.chromium.org/2618823002/diff/1/runtime/vm/ast.h
File runtime/vm/ast.h (right):

https://codereview.chromium.org/2618823002/diff/1/runtime/vm/ast.h#newcode587
runtime/vm/ast.h:587: const LibraryPrefix* prefix_;
On 2017/01/05 21:50:44, hausner wrote:
> We don't typically store pointers to handles in the ast.

I'd have to allocate an extra handle per PrimaryNode, right?
Also, we do store pointers to handles in a few places in
intermediate_language.h, so I don't see why not in the AST.

Would it be ok if I add an assertion that the pointer points to a ZoneHandle?
(i.e. not a scoped handle)

https://codereview.chromium.org/2618823002/diff/1/tests/language/regress_2827...
File tests/language/regress_28278_test.dart (right):

https://codereview.chromium.org/2618823002/diff/1/tests/language/regress_2827...
tests/language/regress_28278_test.dart:14: def.foo(print("here"));
On 2017/01/05 21:50:44, hausner wrote:
> Can you make the test self-checking, e.g. by concatenating the strings in a
> trace that can be compared to an expected value?

Done.

[hausner, 2017-01-05 22:39:41]

https://codereview.chromium.org/2618823002/diff/1/runtime/vm/ast.h
File runtime/vm/ast.h (right):

https://codereview.chromium.org/2618823002/diff/1/runtime/vm/ast.h#newcode587
runtime/vm/ast.h:587: const LibraryPrefix* prefix_;
On 2017/01/05 22:07:16, Florian Schneider wrote:
> On 2017/01/05 21:50:44, hausner wrote:
> > We don't typically store pointers to handles in the ast.
> 
> I'd have to allocate an extra handle per PrimaryNode, right?
> Also, we do store pointers to handles in a few places in
> intermediate_language.h, so I don't see why not in the AST.
> 
> Would it be ok if I add an assertion that the pointer points to a ZoneHandle?
> (i.e. not a scoped handle)

Fine with me. Yes, otherwise you'd have to allocate a handle. Or else you
introduce a null handle in the VM isolate, but that's overkill I think.

[rmacnak, 2017-01-05 22:50:11]

lgtm w/c

https://codereview.chromium.org/2618823002/diff/1/runtime/vm/ast.h
File runtime/vm/ast.h (right):

https://codereview.chromium.org/2618823002/diff/1/runtime/vm/ast.h#newcode573
runtime/vm/ast.h:573: void set_prefix(const LibraryPrefix* prefix) { prefix_ =
prefix; }
prefix -> deferred_prefix

ASSERT(prefix.is_deferred_load());

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

https://codereview.chromium.org/2618823002/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:11260: prefix);
Please add an assert in ThrowNoSuchMethodError that if a prefix is provided it
is deferred.

[Florian Schneider, 2017-01-06 02:12:13]

https://codereview.chromium.org/2618823002/diff/1/runtime/vm/ast.h
File runtime/vm/ast.h (right):

https://codereview.chromium.org/2618823002/diff/1/runtime/vm/ast.h#newcode573
runtime/vm/ast.h:573: void set_prefix(const LibraryPrefix* prefix) { prefix_ =
prefix; }
On 2017/01/05 22:50:11, rmacnak wrote:
> prefix -> deferred_prefix
> 
> ASSERT(prefix.is_deferred_load());

Done.

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

https://codereview.chromium.org/2618823002/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:11260: prefix);
On 2017/01/05 22:50:11, rmacnak wrote:
> Please add an assert in ThrowNoSuchMethodError that if a prefix is provided it
> is deferred. 

Done.

[Florian Schneider, 2017-01-06 02:20:59]

Description was changed from

==========
VM: Fix bug in deferred loading.

In some cases when loading a deferred library twice, the VM would crash/throw.

Make sure we generate the correct static NoSuchMethod error when referring to
a deferred library that is not loaded yet.

Fixes #28278.
==========

to

==========
VM: Fix bug in deferred loading.

In some cases when loading a deferred library twice, the VM would crash/throw.

Make sure we generate the correct static NoSuchMethod error when referring to
a deferred library that is not loaded yet.

Fixes #28278.

R=rmacnak@google.com

Committed:
https://github.com/dart-lang/sdk/commit/9c82d88d67b44457eaaa70d80f1fd15288b8ee05
==========

[Florian Schneider, 2017-01-06 02:21:00]

Committed patchset #3 (id:40001) manually as
9c82d88d67b44457eaaa70d80f1fd15288b8ee05 (presubmit successful).