Repository navigation
ios: Return NO from run* calls on a running/terminated engine - #192465
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates FlutterEngine to prevent running the engine again after it has been destroyed, making destroyContext idempotent and adding safety checks to ignore texture callbacks after destruction. It also updates the documentation and adds comprehensive unit tests for these behaviors. The review feedback suggests caching self.platformView in a local variable within -unregisterTexture: and -textureFrameAvailable: to avoid redundant getter calls and protect against potential TOCTOU issues.
| - (void)unregisterTexture:(int64_t)textureId { | ||
| _shell->GetPlatformView()->UnregisterTexture(textureId); | ||
| if (!self.platformView) { | ||
| return; | ||
| } | ||
| self.platformView->UnregisterTexture(textureId); | ||
| } |
There was a problem hiding this comment.
To prevent redundant getter calls and protect against potential TOCTOU (Time-of-Check to Time-of-Use) issues if self.platformView is modified or cleared concurrently, consider caching the platform view pointer in a local variable.
- (void)unregisterTexture:(int64_t)textureId {
auto* platformView = self.platformView;
if (!platformView) {
return;
}
platformView->UnregisterTexture(textureId);
}
There was a problem hiding this comment.
There is not TOCTOU issue here. Both this and destroyContext run on the platform thread. They can't be interleaved.
That said I'm doing it anyway cause self.platformView does a null check fml::WeakPtr get() so there's no sense in doing that twice.
| - (void)textureFrameAvailable:(int64_t)textureId { | ||
| _shell->GetPlatformView()->MarkTextureFrameAvailable(textureId); | ||
| if (!self.platformView) { | ||
| return; | ||
| } | ||
| self.platformView->MarkTextureFrameAvailable(textureId); | ||
| } |
There was a problem hiding this comment.
Similarly, cache the platform view pointer in a local variable to avoid redundant getter calls and potential TOCTOU issues.
- (void)textureFrameAvailable:(int64_t)textureId {
auto* platformView = self.platformView;
if (!platformView) {
return;
}
platformView->MarkTextureFrameAvailable(textureId);
}
There was a problem hiding this comment.
In this case you're right about the TOCTOU behaviour since this callback happens on the raster thread.
Edit: nope my memory is wrong we're on the platform thread here too.
void Shell::OnPlatformViewMarkTextureFrameAvailable(int64_t texture_id) {
FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread());Alright updated:
There is not TOCTOU issue here. Both this and destroyContext run on the platform thread. They can't be interleaved.
That said I'm doing it anyway cause self.platformView does a null check fml::WeakPtr get() so there's no sense in doing that twice.
52aefa1 to
72ced12
Compare
We explicitly document in FlutterEngine's `run*` methods that calling run more than once is will return without doing anything. In `[FlutterEngine destroyContext]` we specifically document that the engine is in an unusable state, that the engine will be in an unusable state until it is deallocated, and that accessing properties or sending messages to it will result in undefined behavior or runtime errors. By design, all calls to `run*` methods after the first invocation returned `NO`. This was unintentionally changed in flutter-team-archive/engine#6774 such that calling `run` on an already-running engine returns `YES` instead of `false` as it was prior to that patch. That's incorrect behaviour since it suggests that a new engine was spun up. A particularly obvious incorrect behaviour that this triggered was returning `YES` in the second invocation in a case like this: ```objc [engine runWithEntryPoint:@"main" ...]; [engine runWithEntryPoint:@"notMain" ...]; ``` This restores the original behaviour and prevents spinning up a new shell on a destroyed engine. We only ever documented that calling `run` a second time was unsupported and would return immediately with no effect, but never documented the return value on a second invocation, this updates that to document (and test) the intended behaviour. This also makes destroyContext idempotent such that calling it a second time is a no-op. This can happen if destroyContext is called directly as well as via e.g. `appOrSceneWillTerminate` or on view controller deallocation. This also adds null guards to unregisterTexture: and textureFrameAvailable: which would previously have triggered a null dereference. Related: flutter-team-archive/engine#6774 Related: flutter-team-archive/engine#7610 Issue: flutter#185608 Issue: flutter#115157 (b/256086107)
72ced12 to
087e77c
Compare
|
CI had a failure that stopped further tests from running. We need to investigate to determine the root cause. SHA at time of execution: 087e77c. Possible causes:
A blank commit, or merging to head, will be required to resume running CI for this PR. Error Details: Stack trace: |
|
CI had a failure that stopped further tests from running. We need to investigate to determine the root cause. SHA at time of execution: 087e77c. Possible causes:
A blank commit, or merging to head, will be required to resume running CI for this PR. Error Details: Stack trace: |
|
|
||
| return _shell != nullptr; | ||
| [self launchEngine:entrypoint libraryURI:libraryURI entrypointArgs:entrypointArgs]; | ||
| return YES; |
There was a problem hiding this comment.
don't we still want return _shell != nullptr;? I guessing that checks problems with launchEngine
There was a problem hiding this comment.
If shell is nullptr, [FlutterEngine launchEngine:libraryURI:entryPointArgs:] already crashes; it's got an FML_DCHECK and it dereferences it:
The good news is, it won't be nullptr... because if we fail to launch, we don't actually reset shell, so it's not a good indicator of success either way:
flutter/engine/src/flutter/shell/common/shell.cc
Lines 807 to 810 in 4e6768e
I guess the more interesting point is that we currently have no mechanism in the code to let the embedder know that launch failed. We should probably fix that but it's probably out of scope for this guy since that might happen on another thread (the UI thread). For iOS we have merged threads so this should be more than possible.
I can send a followup that deals with that.
There was a problem hiding this comment.
Okay, a followup sounds good to me. A FML_DCHECK will only catch in debug builds. Can you please file a brief followup issue. No need to add a TODO comment in the code unless you want to.
There was a problem hiding this comment.
FML_DCHECK will only fire in debug builds but dereferencing the pointer on the next line will fire in release builds :)
Will file an issue and send a patch.
|
CI had a failure that stopped further tests from running. We need to investigate to determine the root cause. SHA at time of execution: 087e77c. Possible causes:
A blank commit, or merging to head, will be required to resume running CI for this PR. Error Details: Stack trace: |
We explicitly document in FlutterEngine's
run*methods that calling run more than once is will return without doing anything. In[FlutterEngine destroyContext]we specifically document that the engine is in an unusable state, that the engine will be in an unusable state until it is deallocated, and that accessing properties or sending messages to it will result in undefined behavior or runtime errors.By design, all calls to
run*methods after the first invocation returnedNO. This was unintentionally changed in flutter-team-archive/engine#6774 such that callingrunon an already-running engine returnsYESinstead offalseas it was prior to that patch. That's incorrect behaviour since it suggests that a new engine was spun up. A particularly obvious incorrect behaviour that this triggered was returningYESin the second invocation in a case like this:This restores the original behaviour and prevents spinning up a new shell on a destroyed engine. We only ever documented that calling
runa second time was unsupported and would return immediately with no effect, but never documented the return value on a second invocation, this updates that to document (and test) the intended behaviour.This also makes destroyContext idempotent such that calling it a second time is a no-op. This can happen if destroyContext is called directly as well as via e.g.
appOrSceneWillTerminateor on view controller deallocation.This also adds null guards to unregisterTexture: and textureFrameAvailable: which would previously have triggered a null dereference.
Related: flutter-team-archive/engine#6774
Related: flutter-team-archive/engine#7610
Fixes: #185608
Fixes: #115157 (b/256086107)
Issue: #112232
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.