Fix incorrect handling of error handling in case an isolate initialization fails

[mkustermann]

For isolates spawned by the application via Isolate.spawn()ed, the VM
will create a "lightweight" isolate and invoke the "initialize_isolate"
embedder callback to initialize it.

The embedder-provided callback will be invoked with the active isolate
and is expected to return with that active isolate - irrespective of
whether it succeeded to initialize or not.
=> The unsuccessful path was using Dart_ExitIsolate() - which is
incorrect.

This PR fixes that by not exiting the isolate. As a side-effect of the
fix, we also do less Dart_EnterIsolate()/Dart_ExitIsolate() calls in
initialization (which makes it faster) and handle failure to spawn the
root isolate. Furthermore this PR removes some dead code and replaces it
with FML_DCHECK()s instead.

Due to no way to reproduce this failure to initialize isolate in normal
circumstances, this PR was tested manually by injecting a return false
into the non-root isolate case of DartIsolate::InitializeIsolate() and
ensuring that an appropriate exception gets thrown on the Dart
side (i.e. the await Isolate.spawn() call).

Fixes flutter/flutter#90478

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[aam]

From the code below do you have some sense as to which method might end up returning false?
None of the Dart_SetLibraryTagHandler, Dart_SetDeferredLoadHandler, UpdateThreadPoolNames can seem to do that.

[mkustermann]

It may not be this method that returns false. As mentioned in CL description we know (from crash stack trace) it's DartIsolate::InitializeIsolate() which returns false - which looks like this:

bool DartIsolate::InitializeIsolate(...) {
  ...
  if (!embedder_isolate->Initialize(isolate)) { ... }

  if (!embedder_isolate->LoadLibraries()) {
    ... 
    return false;
  }
  ...
  return true;
}

My suspicion is that LoadLibraries() - which does all kinds of stuff, including calling into Dart code when setting up dart:io ... - fails.

From flutter/flutter#90478 (comment):

crash log:
...
I/scudo   (28282): Scudo OOM: The process has Exhausted 256M for size class 262160.

it indicates that this app is running in low-memory situations. So one option I can see is that when we initialize the libraries we get into a OOM situation.

Just a suspicion - really hard to verify.

Though the bug itself is quite obvious when reading the code and I've verified that it fixes the error handling path.

[aam]

It looks like this pull request may not have tests. Please make sure to add tests before

Potentially we could add a test here that would spawn two isolates in sequence and first one on a shutdown would set root library to null forcing initialization of the second isolate to fail(return false).

[zanderso]

A test like @aam suggests would be helpful here to avoid future breakage and to help folks understand what the problem is that is being fixed.

[mkustermann]

Potentially we could add a test here that would spawn two isolates in sequence and first one on a shutdown would set root library to null forcing initialization of the second isolate to fail(return false).

Thanks for the suggestion, @aam !

A test like @aam suggests would be helpful here to avoid future breakage and to help folks understand what the problem is that is being fixed.

Will give it a try.

[mkustermann]

Indeed, setting the root library to null apparently makes it fail. Flutter engine seemingly checks for it being non-null.
Have added at test, PTAL

[aam]

Indeed, setting the root library to null apparently makes it fail. Flutter engine seemingly checks for it being non-null. Have added at test, PTAL

Cool, lgtm!

[zanderso]

Thanks!

[zanderso]

It looks like this landed when the tree was red. As a reminder, please only land changes when the tree is red if they will help the tree be green again.

[mkustermann]

It looks like this landed when the tree was red. As a reminder, please only land changes when the tree is red if they will
help the tree be green again.

Thanks for the reminder. It didn't seem red but rather purple due to some infra failures which I assumed are temporary.