Conversation
|
|
|
@abzokhattab Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@abzokhattab pls, ignore, @nkdengineer is going to review this. thanks |
|
@MelvinBot can u remove me from reviewers and assign @nkdengineer instead |
|
Removed
For contrast, C+ reviewers who can be requested here (for example To finish this: someone with admin on |
|
cc @luacmartins on the above #100016 (comment) |
|
@nkdengineer can you comment on this PR please? |
|
@luacmartins I'm here |
|
Hmm GH isn't finding you. Maybe the comment below is accurate. Did your access change recently?
|
|
i think it's enough to assign @nkdengineer on issue and can review PR? |
|
@mukhrr Please merge main. |
|
@nkdengineer done |
|
@luacmartins Currently, I can only reproduce the crash after reverting this PR and have verified that the patch is the correct fix for this crash. Do we still want to continue applying this patch, or close it as it's no longer reproducible in production? |
|
Let's close it if the issues doesn't occur anymore. Before closing, I asked QA to retest it too. I'll close the issue if they can't reproduce it either |
|
closing per #100008 (comment). issue is not reproducible now |
Explanation of Change
-[RCTViewComponentView unmountChildComponentView:index:]asserts that the child being unmounted sits insideself.currentContainerView, butcurrentContainerViewis not a pure getter. When_useCustomContainerViewflips it creates or tears down_containerViewand moves the existing subviews across, so reading it mutates the very hierarchy the assertion is inspecting. The order in which the two operands of==are evaluated is unspecified, sosuperviewcan be read before that migration runs and the check then compares a pre-migration superview against the post-migration container view. The tell is in the crash message: the reportedexisting parenttag equals the parent's own tag, meaning the child really is inside the parent, just not inside the parent's container view._useCustomContainerViewis recomputed on everyfinalizeUpdatesfromstyleWouldClipOverflowInk, so any view whose style crosses that threshold while it has mounted children is exposed. Bulk-duplicating reports mounts and unmounts enough of those at once to hit it, but the defect is not specific to that flow.This adds
react-native+0.86.0+042+fix-unmount-assert-container-view.patch, which resolves the container view into a local before the assertions readsuperview, so every operand comes from the same side of the migration. The[childComponentView removeFromSuperview]that follows was always correct, which is why this surfaced only as an abort and never as broken rendering. No app code changes.Fixed Issues
$ #100008
PROPOSAL: #100008 (comment)
Tests
Precondition: an iOS build of this branch with assertions enabled, signed in to an account that has more than 15 reports containing 50+ expenses each.
NSInternalInconsistencyException: Attempt to unmount a view which is mounted inside a different view.Copy of ...report appears for every report that was selected.Offline tests
This changes native view mounting only and has no network surface, so it cannot behave differently by connection state. To confirm no regression, run the Tests steps with the network turned off at step 2: the duplicates still appear optimistically and the app still does not abort. Turning the network back on syncs them as before.
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
642124834-de0fac1d-6aff-464f-af02-d6df9d295086.mp4
Android: mWeb Chrome
642124797-ebb1023b-6b39-4848-a334-2ad99b7ba344.mp4
iOS: Native
642124764-4e079a1e-937f-4e5b-b0a4-7bcefd538882.mp4
iOS: mWeb Safari
642124725-38906582-b61b-4565-985d-899fc28d36d7.mp4
MacOS: Chrome / Safari
641998971-d00da669-e0d7-4142-aea9-89042aea11f4.mp4