-
Notifications
You must be signed in to change notification settings - Fork 6k
Avoid crashing and display error if the process cannot be prepared for JIT mode Dart VM. #20980
Conversation
…r JIT mode Dart VM. On iOS, A `FlutterEngine` instantiation requires the initialization of the Dart VM. In the `debug` Flutter runtime mode, a JIT mode Dart VM is created. For a functional JIT, code pages need to be marked as executable at runtime. This is disallowed for applications released on the iOS App Store. So, Flutter uses an AOT mode Dart VM in the `profile` and `release` Flutter runtime modes. In the `debug` mode, Flutter used to depend on enabling tracing in the process to enable the JIT. This is the same mechanism used by debuggers. With recent releases of iOS, this mechanism is no longer reliable when applications are launched from the home-screen. This is not entirely surprising because the process was using the private `ptrace` API to enable tracing in the process. When tracing cannot be enabled in the process in `debug` runtime modes, there used to be no error handling with the process crashing. There were no clear messages or logs either. This patch adds said error handling. In place or the Flutter contents, a message will be displayed saying with guidance on why the launch failed. An appropriate log will be added as well. To enable applications launched from the home-screen, JIT mode Dart VMs will have to be avoided in favor of the old bytecode interpreter. That was decommissioned because of performance and upkeep cost concerns (context in go/decommissioning-dbc). The stakeholders in that document will have to decide if that decision needs to be revisited given this deterioration in user experience. On the patch itself, in cases where tracing cannot be enabled in the process, the FlutterEngine instantiation will return `nil` and log a message. A more prominent assertion cannot be added as this will break existing applications and take down the process. The `FlutterViewController` was not resilient to an engine being `nil` and so had to be updated. It is unclear if I missed any spots still because this is a change in the core assumption in the embedding implementation. This is a best effort attempt at converting crashes to user visible messages and logs. Fixes flutter/flutter#62518
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. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
runtime/dart_vm.cc
Outdated
@@ -329,7 +329,12 @@ DartVM::DartVM(std::shared_ptr<const DartVMData> vm_data, | |||
PushBackAll(&args, kDartWriteProtectCodeArgs, | |||
fml::size(kDartWriteProtectCodeArgs)); | |||
#else | |||
EnsureDebuggedIOS(settings_); | |||
const bool tracing_result = EnableTracingIfNecessary(settings_); | |||
// This check should only trip if the embedding made no attempts enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sentence has some issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
|
||
if (ptrace(PT_TRACE_ME, 0, nullptr, 0) == -1) { | ||
static bool EnableTracingManually(const Settings& vm_settings) { | ||
if (::ptrace(PT_TRACE_ME, 0, nullptr, 0) == -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some code comments for readers (e.g. these 2 ifs are expected to fail on iOS 14, the rest should let people jit on iOS 13 etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can expect that. It's a private API and all implementation detail that can change at anytime without warning. I'd rather just be resilient to errors.
return true; | ||
} | ||
|
||
static bool EnableTracingIfNecessaryOnce(const Settings& vm_settings) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the name have once in it? Nothing here enforces the once-ness here it seems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, seems like the name is reflects the expectation rather than the implementation... Seems a bit strange but I don't have a strong opinion. If you expect something, you should probably assert it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was following the usual C convention of of calling a method that is supposed to be called when a mutex is held with the Locked
suffix.
(FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_DEBUG) | ||
|
||
enum class TracingResult { | ||
kNotAttempted, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What part would you like clarified?
NSLog(@"Cannot create a FlutterEngine instance in debug mode without Flutter tooling or Xcode. " | ||
@"To create FlutterEngine instances from applications launched from the home-screen, use " | ||
@"profile or release modes instead."); | ||
[self release]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we assert here or do something more eye-catching than a console message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we assert, we will take down the process. We don't want to do that. Instead, we want to display an error message in existing applications.
NSLog(@"Cannot create a FlutterEngine instance in debug mode without Flutter tooling or Xcode. " | ||
@"To create FlutterEngine instances from applications launched from the home-screen, use " | ||
@"profile or release modes instead."); | ||
[self release]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@To launch in debug mode, run flutter run from Flutter tools, run from an IDE with a Flutter IDE plugin or run the iOS project from Xcode
It's implicit but we should describe the solution explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
@@ -310,8 +318,37 @@ - (void)pushRoute:(NSString*)route { | |||
|
|||
#pragma mark - Loading the view | |||
|
|||
static UIView* GetViewOrPlaceholder(UIView* existing_view) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: for consistency with the rest of the file, could this be an objective-c selector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The selector will make it an instance method. This utility is not tied to any instance of the view controller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't this just be a + (UIView*) getViewOrPlaceholder:?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getViewOrPlaceholder:
isn't related in any way to the meta-class either. I'd rather keep it this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<feel free to ignore me / just water cooler chatting> isn't that generally true for most private objective-c class methods? What's the advantage for this micro-optimization?
- (void)loadView { | ||
self.view = _flutterView.get(); | ||
self.view = GetViewOrPlaceholder(_flutterView.get()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit derived. i.e. it's not a very clear, direct causation chain between the EnableTracingIfNecessary failing, the engine being nil, the view being nil etc to the ultimate text shown: "Flutter application in debug mode can only be launched from Flutter tooling or Xcode". If we show a specific solution, let's double check a specific diagnosis, i.e. engine or view is nil AND GetTracingResult failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe checking the engine directly is a bit cleaner. Otherwise there's still an implementation leak here where the vc knows that the view implementation returns nil when the delegate is nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, you did inside the placeholder method. Are we adding new vectors to end up in a white placeholder view which I think shouldn't be possible before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's double check a specific diagnosis,
This check is being done in the method GetViewOrPlaceholder
method. The comment above that check explicitly mentions the case you described.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we adding new vectors to end up in a white placeholder view which I think shouldn't be possible before?
I couldn't think of any but wanted to only add the message if we were sure of the cause of the failure.
project:project | ||
allowHeadlessExecution:self.engineAllowHeadlessExecution]}; | ||
|
||
if (!engine) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is ok but maybe this adds maintenance cost since we have to check everywhere that we respect this new contract and we should test all of these. Could we just assert in the engine init? There's no real point limping around further if the engine can't run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Umm I suppose the thinking here is that the user's not plugged in and will never see console logs by launching from home screen? Ya, never mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just assert in the engine init?
We cannot assert as this will cause existing applications to crash when running on iOS 14. We limp here by displaying the placeholder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, your point about add-to-app makes sense, SG
messageLabel.autoresizingMask = | ||
UIViewAutoresizingFlexibleWidth | UIViewAutoresizingFlexibleHeight; | ||
messageLabel.text = | ||
@"Flutter application in debug mode can only be launched from Flutter tooling " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDEs with Flutter plugins
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
Approach LG. We should add some tests to FlutterViewControllerTest, FlutterEngineTest etc by injecting mock values. |
Where and what would I assert? |
Do you mean where to inject the test only mock return values for GetTracingResult or where to put the test? I'm thinking somewhere in FlutterViewControllerTest.m, you can set something in "flutter/runtime/ptrace_check.h" then assert on the UIView's UILabel |
A single VM is launched in the process. Mocking the value in ptrace_check will cause DartVM initialization caused by other tests to fail. I can try to mock the FlutterEngine designated initializer to return nil and ensure that a placeholder is added instead of a FlutterView I suppose. |
Good point. SG for cutting in at engine level. |
Actually that probably doesn't work since you're still calling flutter::GetTracingResult() in your view controller code. Can't ptrace_check still work as is in the single VM session but you set and unset an override test value at the beginning and at the end of the test block? |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
1 similar comment
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
…pared for JIT mode Dart VM. (flutter/engine#20980)
…pared for JIT mode Dart VM. (flutter/engine#20980)
…pared for JIT mode Dart VM. (flutter/engine#20980)
…pared for JIT mode Dart VM. (flutter/engine#20980)
…pared for JIT mode Dart VM. (flutter/engine#20980)
…pared for JIT mode Dart VM. (flutter/engine#20980)
…pared for JIT mode Dart VM. (flutter/engine#20980)
…pared for JIT mode Dart VM. (flutter/engine#20980)
…pared for JIT mode Dart VM. (flutter/engine#20980)
* 556cb23 Roll Skia from 6763a713f957 to d91cd6b5ee2b (3 revisions) (flutter/engine#20989) * b6a3c54 Roll Fuchsia Linux SDK from A0PKwETay... to gfAt63Ezd... (flutter/engine#21005) * cceb733 Roll Fuchsia Mac SDK from sih5f60Gt... to 9pfHLZEFU... (flutter/engine#21006) * 5539820 Roll Skia from d91cd6b5ee2b to a73a84f9b8e3 (1 revision) (flutter/engine#21007) * b4cc631 Roll Dart SDK from f3a9ca88b664 to e59935669cb0 (1 revision) (flutter/engine#21008) * 6cf0cc4 Roll Skia from a73a84f9b8e3 to d0fe7d37d678 (1 revision) (flutter/engine#21011) * 5b055bb Roll Skia from d0fe7d37d678 to 611a52108b01 (2 revisions) (flutter/engine#21012) * 575a519 Enable lazy-async-stacks by-default in all modes (Take 3) (flutter/engine#20895) * 040a794 Roll Fuchsia Mac SDK from 9pfHLZEFU... to tUwahggJ8... (flutter/engine#21013) * 22cca4c Roll Dart SDK from e59935669cb0 to f745f9447ddf (1 revision) (flutter/engine#21014) * 539cb69 Roll Fuchsia Linux SDK from gfAt63Ezd... to Ta3F40BV6... (flutter/engine#21015) * 7d927dd Roll Dart SDK from f745f9447ddf to b88c06c314f4 (1 revision) (flutter/engine#21016) * 09a5bf7 Tweak the mdns error message (flutter/engine#20991) * d0d9ce6 Roll Fuchsia Linux SDK from Ta3F40BV6... to coVjRTWSf... (flutter/engine#21018) * 808bb85 Roll Fuchsia Mac SDK from tUwahggJ8... to TyNHQXzNU... (flutter/engine#21019) * 5aa6921 Roll Skia from 611a52108b01 to cd54c8385c31 (2 revisions) (flutter/engine#21021) * e7d558f Roll Dart SDK from b88c06c314f4 to 33b6c95936e0 (2 revisions) (flutter/engine#21023) * af90dd3 Roll Skia from cd54c8385c31 to c0d3495e1ee2 (12 revisions) (flutter/engine#21024) * f0fb74b Avoid crashing and display error if the process cannot be prepared for JIT mode Dart VM. (flutter/engine#20980) * 6a6c23a Roll Skia from c0d3495e1ee2 to cf1a4f50121f (6 revisions) (flutter/engine#21026) * 716dce0 Android 30r3 (flutter/engine#21025) * 7431070 Roll Dart SDK from 33b6c95936e0 to a2c9cae4dcd8 (1 revision) (flutter/engine#21027) * cef383d Roll Skia from cf1a4f50121f to 04b9443274cf (2 revisions) (flutter/engine#21028) * cf8c6b8 Update web lerpDouble to match C++ behaviour (flutter/engine#21010) * 6866675 Roll Skia from 04b9443274cf to b8ae7fa12aa0 (1 revision) (flutter/engine#21030) * c538f40 Roll Dart SDK from a2c9cae4dcd8 to ffbfa2000435 (1 revision) (flutter/engine#21031) * 89d34b0 Roll Skia from b8ae7fa12aa0 to 445c8ebcb710 (1 revision) (flutter/engine#21032) * 7766d2e Roll Fuchsia Mac SDK from TyNHQXzNU... to Phn3nF_BJ... (flutter/engine#21034) * e3de8d0 Roll Fuchsia Linux SDK from coVjRTWSf... to eBus_y4DN... (flutter/engine#21035) * beb7df5 Roll Skia from 445c8ebcb710 to f9d5940fef55 (3 revisions) (flutter/engine#21037) * f3a17b6 Roll Skia from f9d5940fef55 to 81c6d6eeb4cf (1 revision) (flutter/engine#21038) * bbcc495 Roll Dart SDK from ffbfa2000435 to 2e838b7b4503 (2 revisions) (flutter/engine#21039) * 61062fd Roll Skia from 81c6d6eeb4cf to 81606b5d9774 (5 revisions) (flutter/engine#21041) * f7c7b41 Revert "Enable lazy-async-stacks by-default in all modes (Take 3) (#20895)" (flutter/engine#21043)
On Flutter 3.22, this warning error does not show anymore, and instead, the app simply crashes if launched from the home screen in Debug mode, when attempting to register iOS plugins. fluttercommunity/plus_plugins#2912 |
On iOS, A
FlutterEngine
instantiation requires the initialization of the DartVM. In the
debug
Flutter runtime mode, a JIT mode Dart VM is created. For afunctional JIT, code pages need to be marked as executable at runtime. This is
disallowed for applications released on the iOS App Store. So, Flutter uses an
AOT mode Dart VM in the
profile
andrelease
Flutter runtime modes. In thedebug
mode, Flutter used to depend on enabling tracing in the process toenable the JIT. This is the same mechanism used by debuggers. With recent
releases of iOS, this mechanism is no longer reliable when applications are
launched from the home-screen. This is not entirely surprising because the
process was using the private
ptrace
API to enable tracing in the process.When tracing cannot be enabled in the process in
debug
runtime modes, thereused to be no error handling with the process crashing. There were no clear
messages or logs either. This patch adds said error handling. In place or the
Flutter contents, a message will be displayed saying with guidance on why the
launch failed. An appropriate log will be added as well.
To enable applications launched from the home-screen, JIT mode Dart VMs will
have to be avoided in favor of the old bytecode interpreter. That was
decommissioned because of performance and upkeep cost concerns (context in
go/decommissioning-dbc). The stakeholders in that document will have to decide
if that decision needs to be revisited given this deterioration in user
experience.
On the patch itself, in cases where tracing cannot be enabled in the process,
the FlutterEngine instantiation will return
nil
and log a message. A moreprominent assertion cannot be added as this will break existing applications and
take down the process. The
FlutterViewController
was not resilient to anengine being
nil
and so had to be updated. It is unclear if I missed any spotsstill because this is a change in the core assumption in the embedding
implementation. This is a best effort attempt at converting crashes to user
visible messages and logs.
Fixes flutter/flutter#62518