[iOS] Safety Checks To Avoid Gemini Calls After SceneDidDisconnect [chromium/src : main]
Scott Yoder (Gerrit)
Scott Yoder voted and added 3 comments![]( "Open in Gerrit")
Votes added by Scott Yoder
| Code-Review | +1 |
3 comments
_viewController.geminiHandler = HandlerForProtocol(
_regularBrowser->GetCommandDispatcher(), BWGCommands);Joemer RamosDoes this work? SceneCoordinator gets started very early, so I would be surprised if BWGCommands handler is already available. We probably need to somehow access the handler at the time that it is needed.
Scott YoderI tested locally and it looks like it works...which might make sense? I copy pasted the logic from `TabGridViewController` and if that was the legacy rootViewController and [setting geminiHandler](https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_grid_coordinator.mm;l=979-980) worked there, maybe it works here too. Feel free to close this out after reading/if you agree.
After searching through the code I see that BWGCommands is dispatched to the BrowserCoordinator, which is instantiated in the BrowserLifecycleManager before SceneCoordinator is started. So agree, this should be ok. If this wasn't ok, HandlerForProtocol() would crash and I think that would show up pretty quickly in EG tests.
if (!self.geminiHandler) {
return;
}You don't need to nil check it, the command will just not be dispatched below if it is nil anyway.
Related details
- Gauthier Ambard
- Joemer Ramos
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joemer Ramos (Gerrit)
Joemer Ramos added 1 comment![]( "Open in Gerrit")
if (!self.geminiHandler) {
return;
}You don't need to nil check it, the command will just not be dispatched below if it is nil anyway.
Yeah thats what i thought too, but i was looking at minicrash dumps and it still looks like it was trying to call geminiHandler. I think maybe the <id> wasn't being cleaned up properly, but generally I agree so removing.
Related details
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gauthier Ambard (Gerrit)
Gauthier Ambard voted and added 1 comment![]( "Open in Gerrit")
Votes added by Gauthier Ambard
| Code-Review | +0 |
1 comment
UISceneActivationStateForegroundActive) {How does that work when the scene is in background? I don't have a clear flow in mind, but let's assume you are on iPad with several scenes. This is called on a background scene. Is it ok to early return?
Related details
- Joemer Ramos
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joemer Ramos (Gerrit)
Joemer Ramos added 1 comment![]( "Open in Gerrit")
UISceneActivationStateForegroundActive) {How does that work when the scene is in background? I don't have a clear flow in mind, but let's assume you are on iPad with several scenes. This is called on a background scene. Is it ok to early return?
I'm not sure how `dismissViewControllerAnimated:completion:` can be called while a scene is in the background **and** foregroundActive. Maybe @scott...@google.com knows?
But for Gemini context, there is only one Gemini floaty instance and this command dispatch call gets sent to `GeminiBrowserAgent` (which is browser/window bound). So if there's other scenes, even if they remain active and this function is called, the Gemini floaty being invoked would be taken into account on the browser level and would likely early return if the floaty wasn't invoked in that window. If the floaty **was** invoked in that background scene and somehow passed the `UISceneActivationStateForegroundActive` check, then I suppose the Gemini floaty would show.
In any case, if we early return in this function, the worst case scenario is that the floaty doesn't reappear when expected, and on the next scroll event (not in this CL), the floaty will reappear for the user.
Related details
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Yoder (Gerrit)
Scott Yoder voted and added 1 comment![]( "Open in Gerrit")
Votes added by Scott Yoder
| Code-Review | +1 |
1 comment
UISceneActivationStateForegroundActive) {Joemer RamosHow does that work when the scene is in background? I don't have a clear flow in mind, but let's assume you are on iPad with several scenes. This is called on a background scene. Is it ok to early return?
I'm not sure how `dismissViewControllerAnimated:completion:` can be called while a scene is in the background **and** foregroundActive. Maybe @scott...@google.com knows?
But for Gemini context, there is only one Gemini floaty instance and this command dispatch call gets sent to `GeminiBrowserAgent` (which is browser/window bound). So if there's other scenes, even if they remain active and this function is called, the Gemini floaty being invoked would be taken into account on the browser level and would likely early return if the floaty wasn't invoked in that window. If the floaty **was** invoked in that background scene and somehow passed the `UISceneActivationStateForegroundActive` check, then I suppose the Gemini floaty would show.
In any case, if we early return in this function, the worst case scenario is that the floaty doesn't reappear when expected, and on the next scroll event (not in this CL), the floaty will reappear for the user.
`dismissViewControllerAnimated:` gets called in two places:
1. when a modal view is dismissed, generally in response to a user action of some sort. This is probably most likely to happen while the scene is in the foreground because the user wouldn't be interacting with a background scene. (Please correct me if I'm wrong about this).
2. When the scene is disconnected
I think Floaty only needs the opportunity to reappear in scenario 1 above.
Related details
- Gauthier Ambard
- Joemer Ramos
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joemer Ramos (Gerrit)
Joemer Ramos added 1 comment![]( "Open in Gerrit")
UISceneActivationStateForegroundActive) {Joemer RamosHow does that work when the scene is in background? I don't have a clear flow in mind, but let's assume you are on iPad with several scenes. This is called on a background scene. Is it ok to early return?
Scott YoderI'm not sure how `dismissViewControllerAnimated:completion:` can be called while a scene is in the background **and** foregroundActive. Maybe @scott...@google.com knows?
But for Gemini context, there is only one Gemini floaty instance and this command dispatch call gets sent to `GeminiBrowserAgent` (which is browser/window bound). So if there's other scenes, even if they remain active and this function is called, the Gemini floaty being invoked would be taken into account on the browser level and would likely early return if the floaty wasn't invoked in that window. If the floaty **was** invoked in that background scene and somehow passed the `UISceneActivationStateForegroundActive` check, then I suppose the Gemini floaty would show.
In any case, if we early return in this function, the worst case scenario is that the floaty doesn't reappear when expected, and on the next scroll event (not in this CL), the floaty will reappear for the user.
`dismissViewControllerAnimated:` gets called in two places:
1. when a modal view is dismissed, generally in response to a user action of some sort. This is probably most likely to happen while the scene is in the foreground because the user wouldn't be interacting with a background scene. (Please correct me if I'm wrong about this).
2. When the scene is disconnected
I think Floaty only needs the opportunity to reappear in scenario 1 above.
- Gauthier Ambard
- Scott Yoder
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gauthier Ambard (Gerrit)
Gauthier Ambard voted and added 1 comment![]( "Open in Gerrit")
Votes added by Gauthier Ambard
| Code-Review | +1 |
1 comment
UISceneActivationStateForegroundActive) {Joemer RamosHow does that work when the scene is in background? I don't have a clear flow in mind, but let's assume you are on iPad with several scenes. This is called on a background scene. Is it ok to early return?
Scott YoderI'm not sure how `dismissViewControllerAnimated:completion:` can be called while a scene is in the background **and** foregroundActive. Maybe @scott...@google.com knows?
But for Gemini context, there is only one Gemini floaty instance and this command dispatch call gets sent to `GeminiBrowserAgent` (which is browser/window bound). So if there's other scenes, even if they remain active and this function is called, the Gemini floaty being invoked would be taken into account on the browser level and would likely early return if the floaty wasn't invoked in that window. If the floaty **was** invoked in that background scene and somehow passed the `UISceneActivationStateForegroundActive` check, then I suppose the Gemini floaty would show.
In any case, if we early return in this function, the worst case scenario is that the floaty doesn't reappear when expected, and on the next scroll event (not in this CL), the floaty will reappear for the user.
Joemer Ramos`dismissViewControllerAnimated:` gets called in two places:
1. when a modal view is dismissed, generally in response to a user action of some sort. This is probably most likely to happen while the scene is in the foreground because the user wouldn't be interacting with a background scene. (Please correct me if I'm wrong about this).
2. When the scene is disconnected
I think Floaty only needs the opportunity to reappear in scenario 1 above.
Yes that is all correct
There are cases where we dismiss the presented view controller of background scenes (actually it was the case of floaty).
If you are ok with floaty not reappearing in those cases, I am good with it!
You could also try locally and check what is the status of the windowScene at this point, maybe there is a specific state.
Related details
- Joemer Ramos
- Scott Yoder
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joemer Ramos (Gerrit)
Joemer Ramos added 1 comment![]( "Open in Gerrit")
Leaving as unresolved for me: tests are failing because command dispatcher is not available at `[SceneCoordinator start]` so creating a view controller delegate and adding checks to not call the dispatcher until the CommandDispatcher is ready.
Related details
- Scott Yoder
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joemer Ramos (Gerrit)
Joemer Ramos added 3 comments![]( "Open in Gerrit")
Leaving as unresolved for me: tests are failing because command dispatcher is not available at `[SceneCoordinator start]` so creating a view controller delegate and adding checks to not call the dispatcher until the CommandDispatcher is ready.
Done
Can i get a second pass? More checks for command dispatcher to not throw unrecognized selector
Joemer RamosHow does that work when the scene is in background? I don't have a clear flow in mind, but let's assume you are on iPad with several scenes. This is called on a background scene. Is it ok to early return?
Scott YoderI'm not sure how `dismissViewControllerAnimated:completion:` can be called while a scene is in the background **and** foregroundActive. Maybe @scott...@google.com knows?
But for Gemini context, there is only one Gemini floaty instance and this command dispatch call gets sent to `GeminiBrowserAgent` (which is browser/window bound). So if there's other scenes, even if they remain active and this function is called, the Gemini floaty being invoked would be taken into account on the browser level and would likely early return if the floaty wasn't invoked in that window. If the floaty **was** invoked in that background scene and somehow passed the `UISceneActivationStateForegroundActive` check, then I suppose the Gemini floaty would show.
In any case, if we early return in this function, the worst case scenario is that the floaty doesn't reappear when expected, and on the next scroll event (not in this CL), the floaty will reappear for the user.
Joemer Ramos`dismissViewControllerAnimated:` gets called in two places:
1. when a modal view is dismissed, generally in response to a user action of some sort. This is probably most likely to happen while the scene is in the foreground because the user wouldn't be interacting with a background scene. (Please correct me if I'm wrong about this).
2. When the scene is disconnected
I think Floaty only needs the opportunity to reappear in scenario 1 above.
Gauthier AmbardYes that is all correct
There are cases where we dismiss the presented view controller of background scenes (actually it was the case of floaty).
If you are ok with floaty not reappearing in those cases, I am good with it!
You could also try locally and check what is the status of the windowScene at this point, maybe there is a specific state.
Apologies, maybe I'm forgetting one of the user journies (10+) but I'm not aware of any case where the floaty needs to listen to a presented view controller dismissing when a scene is backgrounded.
@scott...@google.com IIUC we only have one UIWindow and UIWindowScene for each instance of Chrome where we can have more if we have multiwindow on iPad. The only thing I could think of is dismissing a second iPad window. In that event, the floaty **would** be on the other window. If the floaty is in the dismissed window, then we don't care about the floaty UI update.
Related details
- Gauthier Ambard
- Scott Yoder
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gauthier Ambard (Gerrit)
Gauthier Ambard voted Code-Review+1![]( "Open in Gerrit")
- Joemer Ramos
- Scott Yoder
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gauthier Ambard (Gerrit)
Gauthier Ambard added 1 comment![]( "Open in Gerrit")
UISceneActivationStateForegroundActive) {Joemer RamosHow does that work when the scene is in background? I don't have a clear flow in mind, but let's assume you are on iPad with several scenes. This is called on a background scene. Is it ok to early return?
Scott YoderI'm not sure how `dismissViewControllerAnimated:completion:` can be called while a scene is in the background **and** foregroundActive. Maybe @scott...@google.com knows?
But for Gemini context, there is only one Gemini floaty instance and this command dispatch call gets sent to `GeminiBrowserAgent` (which is browser/window bound). So if there's other scenes, even if they remain active and this function is called, the Gemini floaty being invoked would be taken into account on the browser level and would likely early return if the floaty wasn't invoked in that window. If the floaty **was** invoked in that background scene and somehow passed the `UISceneActivationStateForegroundActive` check, then I suppose the Gemini floaty would show.
In any case, if we early return in this function, the worst case scenario is that the floaty doesn't reappear when expected, and on the next scroll event (not in this CL), the floaty will reappear for the user.
Joemer Ramos`dismissViewControllerAnimated:` gets called in two places:
1. when a modal view is dismissed, generally in response to a user action of some sort. This is probably most likely to happen while the scene is in the foreground because the user wouldn't be interacting with a background scene. (Please correct me if I'm wrong about this).
2. When the scene is disconnected
I think Floaty only needs the opportunity to reappear in scenario 1 above.
Gauthier AmbardYes that is all correct
Joemer RamosThere are cases where we dismiss the presented view controller of background scenes (actually it was the case of floaty).
If you are ok with floaty not reappearing in those cases, I am good with it!
You could also try locally and check what is the status of the windowScene at this point, maybe there is a specific state.
Apologies, maybe I'm forgetting one of the user journies (10+) but I'm not aware of any case where the floaty needs to listen to a presented view controller dismissing when a scene is backgrounded.
@scott...@google.com IIUC we only have one UIWindow and UIWindowScene for each instance of Chrome where we can have more if we have multiwindow on iPad. The only thing I could think of is dismissing a second iPad window. In that event, the floaty **would** be on the other window. If the floaty is in the dismissed window, then we don't care about the floaty UI update.
My example was:
1. I open floaty on window A
2. I background A
3. I foreground window B and open floaty
As floaty is (was?) a singleton, opening it in B dismisses it in A. I know here it doesn't really impact it as this is what we want, but technically a VC is dismissed on A while it is in background. I am sure there are several other occurrences with other VC, this is just the most recent one I reviewed.
So, the question is: how bad it is if we never bring back floaty? If it is just that the user needs to re-invoke it, I am fine. If it is breaking things durably, it is not great.
Related details
- Joemer Ramos
- Scott Yoder
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joemer Ramos (Gerrit)
Joemer Ramos added 1 comment![]( "Open in Gerrit")
UISceneActivationStateForegroundActive) {Ahh ok thanks for explaining Gauthier! In that case, it's not bad at all that we never bring back the floaty since it's a singleton. The user would just need to re-invoke it and this is an expected user experience!
Related details
- Scott Yoder
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Yoder (Gerrit)
Scott Yoder voted and added 1 comment![]( "Open in Gerrit")
Votes added by Scott Yoder
| Code-Review | +1 |
LGTM
- Joemer Ramos
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joemer Ramos (Gerrit)
Joemer Ramos voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joemer Ramos
| Commit-Queue | +2 |
1 comment
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[iOS] Safety Checks To Avoid Gemini Calls After SceneDidDisconnect
The TabGridViewController's `dismissViewControllerAnimated:completion:`
is called in response to the scene being disconnected. Therefore, we
should check if the scene activation state shows that the app is still
in an active foreground and not detached to avoid sending calls to
`geminiHandler`.
This change was also added in SceneViewController to ensure that this
functionality works with flag `UseSceneViewController` enabled as
TabGridViewController's `dismissViewControllerAnimated:completion:`
isn't triggered when this feature is enabled since TabGridViewController
is not the root view controller.
I was also investigating mini crash dumps and noticed that
`self.geminiHandler` was nil, so I also added a safety check to make
sure we're not calling a value that's nil. Obj-C objects generally don't
call the selector function if the object is nil, but perhaps this is
failing because the object is technically a CommandDispatcher/C++ object
and a zombie referencing is being called.
I'm not too sure about scene lifecycles so leaning on gambard@ and
scottyoder@ for some expertise if this would help avoid calling the
gemini handler which is causing crashes.
Note: This doesn't solve all the crashes in crbug.com/490834944 but it
should solve most of them.
- M ios/chrome/browser/scene/coordinator/BUILD.gn
- M ios/chrome/browser/scene/coordinator/DEPS
- M ios/chrome/browser/scene/coordinator/scene_coordinator.mm
- M ios/chrome/browser/scene/ui/BUILD.gn
- M ios/chrome/browser/scene/ui/scene_view_controller.h
- M ios/chrome/browser/scene/ui/scene_view_controller.mm
- A ios/chrome/browser/scene/ui/scene_view_controller_delegate.h
- M ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_grid_view_controller.mm
Code-Review: +1 by Gauthier Ambard, +1 by Scott Yoder
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |