-
-
Notifications
You must be signed in to change notification settings - Fork 518
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(iOS): Crash while pushing n different screens at the same time #2249
Conversation
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.
Looks good, left 2 comments
ios/RNSScreenStack.mm
Outdated
// Child update operation is performed in the mountingTransactionDidMount method to prevent error, while | ||
// navigating to `n` different screens at the same time. |
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 comment isn't very helpful outside the scope of this PR since the deleted lines won't be visible. WDYT?
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 just made it for the sake of questions like "why we cannot move this update method to the mount and unmount methods, if they're doing the same?" (especially, that removing this method won't be visible in git blame), but if you think this comment is unnecessary, I can remove it.
I'm starting to think if it is safe what we do in those observers. What I mean is that we don't check if the mutations concern this exact |
Now I can see that my PR with observer does it much better since it operates on |
I wonder whether we should pursue reporting this to RN team & trying to fix this upstream. Since there is a discrepancy between Paper & Fabric updates batching I believe this could be considered as an unintended change, similarly to this one. |
@kkafar Do you believe that's a batching issue? Between Paper and Fabric there are different methods for inserting and removing views and I believe that mistake lies on us and that we didn't fully covered when we should update parent containers on new architecture 🤔 |
That was my impression after we talked in the office ~2 weeks ago, that on Paper these updates are batched together & on Fabric these come separated.
If that's the case then I do not fully understand the error then. |
Note: I recently merged #2261 which removed |
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.
Great job on this one 💪🏻
Just short summary so that error mechanism is noted down & not lost for future generations 😄
Okay, so the crash happens because we do schedule container update each time a component is mounted under stack and when two components come in consecutively, one after another in single transaction the UIKit protests - to be honest I do not get why this is the case as the updates we schedule should be executed in serial & synchronous manner on main thread, but it is the case.
Thus mimicking logic on Paper, this patch moves container updates to a point after the transaction is completed.
Caution
One thing that bothers me is that during testing of this PR I observed that the Create
/ Insert
mutations are coming in separate transaction from Update
mutations when navigating, however I can not observe why this is the case in the native code. Most likely this is caused by setting props / state in useEffects and thus triggering consecutive renders, but I wanted this to be noted down here.
The fix makes sense & we will proceed with this approach.
The updateContainer
call is moved to mountingTransactionDidMount: withSurfaceTelemetry:
and I believe there is no better place to do it, since on Fabric there is no counterpart of didUpdateReactSubviews
(we have finalizeUpdates
, however it does not inform on changes in subviews - this would be something nice to add to RCTComponentViewProtocol
). Thus we're good on this front.
The call to updateContainer
is enqueued on main queue and not executed synchronously - and this is good, because we want to update container (present our view controllers) after they receive native frame (the one computed by native layout). I do not know what action triggers the native layout precisely, however it seems that it is already scheduled during react mounting. Thus we're good on this front also.
Left few remarks to consult.
Note
Before merging this PR we need to merge main & add the RCTMountingTransactionObserving
protocol back to the list of implemented protocols of RNSScreenStackView
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 we're good to go 🎉
Absolute agree here @WoLewicki . We can protect ourselves temporarily by filtering mutations by the tag <- we do have access to it. |
Please withhold merging right now, I'm preparing & testing a patch here |
With clearance from @tboba I allowed myself to push following changes:
Both changes aim to handle what @WoLewicki has noticed in #2249 (comment) By filtering by component tag we will dispatch update block only when there was a mutation concerning this particular stack view in given transaction. The nested stack screen was added to allow for testing this change (you can do so by adding a native log in line before block dispatch). Caution Filtering by tag might not be safe, because tag is invalidated (set to -1) when the view is deleted ( |
…oftware-mansion#2249) ## Description On Paper, while user tries to navigate to 2 or more screens at the same time, updates were batched (on async thread), resulting rerendering stack hierarchy on the next layout. In the same scenario, on Fabric, updates are not being batched, which causes crash: ``` "<RNSNavigationController> is pushing the same view controller instance (<RNSScreen>) more than once which is not supported and is most likely an error in the application" ``` since `maybeAddToParentAndUpdateContainer` is being called to early. This PR fixes this bug by moving the logic of calling that method on `mountingTransactionDidMount` method. I've also checked if this change does not call `maybeAddToParentAndUpdateContainer` too frequent and I can't see more than one correct call there (every doubled call is being catched by `return` in conditions). Fixes software-mansion#2235. ## Changes - Moved updating containers to `mountingTransactionDidMount` method. ## Screenshots / GIFs ### Before https://github.com/user-attachments/assets/24daf575-d06c-4972-9166-89454be60126 ### After https://github.com/user-attachments/assets/d377c527-0b2c-4850-b0b5-8e93dd5f25ac ## Test code and steps to reproduce You can use `Test2235.tsx` to test this change. ## Checklist - [X] Included code example that can be used to test this change - [x] Ensured that CI passes --------- Co-authored-by: Kacper Kafara <kacperkafara@gmail.com>
Description
On Paper, while user tries to navigate to 2 or more screens at the same time, updates were batched (on async thread), resulting rerendering stack hierarchy on the next layout. In the same scenario, on Fabric, updates are not being batched, which causes crash:
since
maybeAddToParentAndUpdateContainer
is being called to early. This PR fixes this bug by moving the logic of calling that method onmountingTransactionDidMount
method.I've also checked if this change does not call
maybeAddToParentAndUpdateContainer
too frequent and I can't see more than one correct call there (every doubled call is being catched byreturn
in conditions).Fixes #2235.
Changes
mountingTransactionDidMount
method.Screenshots / GIFs
Before
Screen.Recording.2024-07-16.at.16.51.22.mov
After
Screen.Recording.2024-07-16.at.16.52.05.mov
Test code and steps to reproduce
You can use
Test2235.tsx
to test this change.Checklist