Fix deferred load errors / bug #26482

Fix deferred load errors

R=hausner@google.com

Committed: https://github.com/dart-lang/sdk/commit/3b2e14ab56d2e5e9dbf660270153402e6689bd1e

[Cutch, 2016-06-07 21:15:12]

Description was changed from

==========
Fix deferred load errors / bug #26482

BUG=
==========

to

==========
Fix deferred load errors / bug #26482

Fixes #26482
==========

[Cutch, 2016-06-07 21:15:12]

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

[hausner, 2016-06-07 21:47:05]

I think the new LibraryPrefix_hasError function is not detecting trasitive load
errors, eg. if M imports A, A imports B, then the import of A must fail if B
fails.

Also, it would be nice to have a simple regression test.

https://codereview.chromium.org/2045023003/diff/1/runtime/bin/loader.cc
File runtime/bin/loader.cc (right):

https://codereview.chromium.org/2045023003/diff/1/runtime/bin/loader.cc#newco...
runtime/bin/loader.cc:235: if ((library != Dart_Null()) &&
!Dart_IsError(library)) {
Is there a difference between x != Dart_Null() and !Dart_IsNull(x)?

I thought we typically use Dart_IsNull(), as you do a couple of lines below.

https://codereview.chromium.org/2045023003/diff/1/runtime/lib/lib_prefix.dart
File runtime/lib/lib_prefix.dart (right):

https://codereview.chromium.org/2045023003/diff/1/runtime/lib/lib_prefix.dart...
runtime/lib/lib_prefix.dart:60: if (prefix.hasError()) {
I think you should use prefix._loadError() to determine whether there was an
error while loading. That function checks the library import graph transitively
for errors.

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

https://codereview.chromium.org/2045023003/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:10907: return deferred_lib.LoadFailed();
I'm not sure LoadFailed() is sufficient. You need to check transiently whether
any of the libraries imported by deferred_lib has failed.

[Cutch, 2016-06-07 22:29:26]

Patch Set 2 handles the transitive error case exactly as the old implementation
did (which is buggy because it doesn't complete the future with an error).

https://codereview.chromium.org/2045023003/diff/1/runtime/bin/loader.cc
File runtime/bin/loader.cc (right):

https://codereview.chromium.org/2045023003/diff/1/runtime/bin/loader.cc#newco...
runtime/bin/loader.cc:235: if ((library != Dart_Null()) &&
!Dart_IsError(library)) {
On 2016/06/07 21:47:05, hausner wrote:
> Is there a difference between x != Dart_Null() and !Dart_IsNull(x)?
> 
> I thought we typically use Dart_IsNull(), as you do a couple of lines below.

No difference, but I switched to Dart_IsNull().

https://codereview.chromium.org/2045023003/diff/1/runtime/lib/lib_prefix.dart
File runtime/lib/lib_prefix.dart (right):

https://codereview.chromium.org/2045023003/diff/1/runtime/lib/lib_prefix.dart...
runtime/lib/lib_prefix.dart:60: if (prefix.hasError()) {
On 2016/06/07 21:47:05, hausner wrote:
> I think you should use prefix._loadError() to determine whether there was an
> error while loading. That function checks the library import graph
transitively
> for errors. 

Done.

[hausner, 2016-06-07 23:36:07]

LGTM w/comment.

A regression test would still be nice.

https://codereview.chromium.org/2045023003/diff/40001/runtime/lib/object.cc
File runtime/lib/object.cc (right):

https://codereview.chromium.org/2045023003/diff/40001/runtime/lib/object.cc#n...
runtime/lib/object.cc:330: DEFINE_NATIVE_ENTRY(LibraryPrefix_hasError, 1) {
This function is no longer used and can be removed as well.

https://codereview.chromium.org/2045023003/diff/40001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2045023003/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:10939: Object& obj = Object::Handle();
Object::Handle(zone)

[Cutch, 2016-06-08 13:19:03]

https://codereview.chromium.org/2045023003/diff/40001/runtime/lib/object.cc
File runtime/lib/object.cc (right):

https://codereview.chromium.org/2045023003/diff/40001/runtime/lib/object.cc#n...
runtime/lib/object.cc:330: DEFINE_NATIVE_ENTRY(LibraryPrefix_hasError, 1) {
On 2016/06/07 23:36:07, hausner wrote:
> This function is no longer used and can be removed as well.

Done.

https://codereview.chromium.org/2045023003/diff/40001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2045023003/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:10939: Object& obj = Object::Handle();
On 2016/06/07 23:36:07, hausner wrote:
> Object::Handle(zone)

Done.

[Cutch, 2016-06-08 13:46:11]

Added test case.

[Cutch, 2016-06-08 13:53:31]

Description was changed from

==========
Fix deferred load errors / bug #26482

Fixes #26482
==========

to

==========
Fix deferred load errors
==========

[Cutch, 2016-06-08 14:12:32]

Included a test case.

[Cutch, 2016-06-08 14:13:00]

Description was changed from

==========
Fix deferred load errors
==========

to

==========
Fix deferred load errors

R=hausner@google.com

Committed:
https://github.com/dart-lang/sdk/commit/3b2e14ab56d2e5e9dbf660270153402e6689bd1e
==========

[Cutch, 2016-06-08 14:13:02]

Committed patchset #8 (id:140001) manually as
3b2e14ab56d2e5e9dbf660270153402e6689bd1e (presubmit successful).