Prevent OOM tab reload when attaching multiple files on mWeb#93234
Conversation
- Trust pre-validated files in ReportAddAttachmentModalContent instead of re-running validateAttachmentFile on every modal mount (each image was decoded twice, doubling peak memory at the exact moment the carousel decodes the same images for display) - Validate files sequentially in useFilesValidation so peak memory is bounded to a single decoded image regardless of how many were selected - Revoke superseded blob object URLs in validateAttachmentFile so orphaned Blobs can be garbage-collected - Remove duplicate useNavigateToReportOnRefresh call
|
@ZhenjaHorbach 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] |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
Pre-existing no-unsafe-type-assertion drift in this test file (450 -> 466) surfaced by a fresh CI lint run; unrelated to this PR's attachment changes. Main's cached lint runs were masking it.
|
Sorry for the delay |
# Conflicts: # config/eslint/eslint.seatbelt.tsv
|
@ZhenjaHorbach done 👍 — merged |
JmillsExpensify
left a comment
There was a problem hiding this comment.
I don't think a product review is required for this one.
|
Hi @ZhenjaHorbach 👋 Friendly bump — I resolved the conflicts you flagged (head |
|
Will check tomorrow morning! |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppIMG_2685.MP4Android: mWeb ChromeIMG_2684.MP4iOS: HybridAppIMG_2685.MP4iOS: mWeb SafariIMG_2684.MP4MacOS: Chrome / Safari2026-07-08.10.44.31.mov |
|
Looks like on iOS web attachments don't work 2026-06-18.14.32.59.mov |
|
Thanks for testing this and grabbing the video, @ZhenjaHorbach 🙏 I dug into that red screen. It's a It isn't coming from this PR's changes: the diff only touches validation scheduling (sequential vs The attachments themselves do work — in your own video the carousel renders the photos both before and after the overlay (waterfall → flowers), and I reproduced the flow on iOS Safari/WebKit on this branch with attachments rendering correctly and no error. Could you double-check whether the same |
I'm not talking about the red screen 😅 |
|
Ah — the Send action itself, not the overlay. Got it, and sorry for chasing the red herring there 😄 Let me reproduce the Send path on this branch (and compare against |
|
Thanks for flagging the Send issue — I reproduced it on the iOS Simulator (Mobile Safari) and dug in:
So I don't think this PR should be blocked on it. I'm happy to file a separate issue for the pre-existing iOS-Safari Send behavior and take a crack at the fix. Let me know if the repro details/screenshots would help. |
|
Quick correction on my last comment — I want to be accurate here. The send itself works on iOS: a real click on the Send button fires the whole flow and the attachment posts (I verified this on the simulator with instrumentation on The key point still stands: this PR doesn't touch the send path, so it isn't a regression from these changes. If you're still seeing Send do nothing with a normal tap on your device, that would be a separate touch-level thing and I'm glad to dig into it with your exact steps. |
|
@ZhenjaHorbach — gentle bump. On the Send behavior you flagged: on my end a normal tap fires the full send flow and the attachment posts, and this PR doesn't touch the send path — but I want to be sure we're not missing something device-specific. If you're still seeing Send do nothing, could you share your device + exact steps and I'll dig back in? Otherwise the OOM reload fix is ready for another review. Thanks! 🙏 |
|
Sorry |
|
@ZhenjaHorbach — no rush, just keeping this on your radar 🙏 Whenever you get a chance to wrap the review, I'm around to turn anything you find around quickly. Thanks! |
|
@robertjchen @ZhenjaHorbach — two updates: 1. Conflicts resolved ✅ — 2. Important: yesterday's adhoc build didn't contain this fix. The build in run 28650145991 was dispatched from Could you trigger a fresh adhoc build now that the branch is mergeable? Happy to record the before/after on our side as well once the build's up. |
|
🚧 robertjchen has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
Still have this issue |
|
Thanks for the careful retest @ZhenjaHorbach — confirmed, and since this build did contain the fix, this is a real second front we hadn't closed. Traced it: the merged fix targets the image decode path (
Fix incoming on this PR: mount the PDF viewer only for the currently-visible carousel item (lightweight placeholder for off-screen ones, gated on the |
…d PDF validation Each mounted PDF viewer/thumbnail is a full PDF.js document parse, so both the preview carousel (initialNumToRender=3, no focus gate) and the pre-navigation validation pass scaled peak memory linearly with the number of picked PDFs — enough to OOM the WebContent process on iOS Safari and reload the tab. AttachmentView now defers the PDF viewer for non-focused carousel items (placeholder until swiped to; single- attachment hosts pass no isFocused and mount immediately), and useFilesValidation renders validation thumbnails one at a time, mirroring the sequential image validation.
|
Pushed
Net effect: at most one PDF is parsed at any moment during add-and-preview, regardless of how many are picked. tsc/lint/tests green. @robertjchen could you trigger a fresh adhoc when you get a chance? @ZhenjaHorbach the repro to beat is your exact one — 3 file-picker PDFs on iPhone. |
|
🚧 robertjchen has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
Let me know how the new adhoc tests 🤞 |
| if (updatedFile.name !== cleanName) { | ||
| updatedFile = new File([updatedFile], cleanName, {type: updatedFile.type}); | ||
| } | ||
| const previousUri = updatedFile.uri; |
There was a problem hiding this comment.
previousUri is read from updatedFile after it may have been reassigned to a new File via new File([...], cleanName, ...) on line 71, and the new File doesn't carry over the custom .uri property
So the revoke never fires whenever the filename needs cleaning (cleanFileName replaces spaces/unicode/special chars with _ for example, default macOS screenshot names)
In that case, the previous blob URL from AttachmentPicker is never released — part of the memory-leak fix silently doesn't work
So I think it's better to use normalizedFile.uri here
There was a problem hiding this comment.
What about this comment?
There was a problem hiding this comment.
Great catch — you're right, and it was worse than cosmetic: the new File(...) reassignment drops the custom .uri, so the revoke silently skipped exactly the cleaned-name files. Fixed in 1ea534d (previousUri now reads from normalizedFile) + added a regression test with a macOS-screenshot-style name that forces the cleaning path. Apologies for missing this thread earlier — the two bumps were on the review thread while I was watching the main conversation.
|
But overall, the changes look good |
|
@ZhenjaHorbach Glad the changes look good! Did the 3-PDF retest on the new adhoc come back clean on your iPhone? If so, would you mind completing the reviewer checklist so this can head to merge? Happy to address anything else first. |
…es filename cleaning new File(...) during name cleaning doesn't carry the custom .uri property, so reading previousUri from updatedFile skipped the revoke for exactly the files whose names get cleaned (e.g. default macOS screenshot names) — the original AttachmentPicker blob stayed resident. Regression test added for the cleaned-name path.
|
@ZhenjaHorbach Checking in — the |
|
Will recheck today before approving! |
|
🚧 robertjchen has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
LGTM |
This comment has been minimized.
This comment has been minimized.
|
Congrats, that's your 5th PR merged! 🎉 Do you know about the ContributorPlus role? It's an opportunity to earn more in the Expensify Open Source community. Keep up the great work - thanks! |
|
🚧 robertjchen has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/robertjchen in version: 9.4.36-0 🚀
|
Help site review: no changes requiredI reviewed the changes in this PR against the help site content under Why: This PR is an internal memory/performance fix. It prevents the mWeb (iOS Safari) WebContent process from OOM-ing and reloading the tab when several full-resolution photos or PDFs are attached at once. The changes are entirely in the attachment validation/rendering internals:
None of this changes any documented, user-facing behavior — no new features, no renamed tabs/settings/buttons, and no change to the steps a user follows to add an attachment. The "Add attachment" flow works exactly the same from the user's perspective; it just no longer crashes. There is no help article documenting the attachment-selection memory internals, and the attachment-related articles that do exist (e.g.
Since no docs changes are needed, there is no linked help site PR to review. |
Explanation of Change
Selecting multiple full-resolution photos could kill the browser's WebContent process on memory-constrained mobile Safari and reload the tab. The picked files only live in an in-memory ref, so after the reload
useNavigateToReportOnRefreshsees no file params and navigates back to the report — the attachments are silently dropped. SinceFile/blob:handles cannot survive a WebContent reload, the fix prevents the reload by removing three compounding memory costs in the selection chain:ReportAddAttachmentModalContentre-ranvalidateAttachmentFileon files thatuseFilesValidationhad already validated, HEIC-converted and resized before navigation — decoding every image a second time (at the same moment the carousel decodes them for display) and allocating a duplicate set of object URLs. The screen is only reachable from the pre-validated picker path (useAttachmentPicker.onFilesValidated), so it now trusts the validated params and derivesvalidFiles/sourcedirectly. This also removes a duplicateduseNavigateToReportOnRefreshcall — the same hook was invoked twice, and two identical effects racingNavigation.goBackcan over-pop the stack on the refresh path.useFilesValidationvalidated the whole batch withPromise.all, so peak memory scaled with the number of selected files (3 concurrent full-size decodes instead of one). Validation now runs sequentially, bounding the peak to a single decoded image regardless of how many files were selected.validateAttachmentFileoverwrotefile.uriwith a fresh object URL without revoking the previous one, so each supersededblob:URL kept its full-size Blob resident until the document died. The superseded URL is now revoked.Fixed Issues
$ #92108
PROPOSAL: #92108 (comment)
Tests
Offline tests
QA Steps
Same as Tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
N/A — the only available test account is SAML-required, and a known unrelated app bug (#86705) signs the session out ~15s after login, before the multi-step attachment flow completes, so a full native recording can't be captured. This fix is platform-agnostic JavaScript (attachment validation/memory handling); the behavior is demonstrated on Web, both mWeb platforms, and iOS Native above.
Android: mWeb Chrome
92108_android_mweb.mp4
iOS: Native
92108_ios_native_small.mp4
iOS: mWeb Safari
92108_ios_mweb_small.mp4
MacOS: Chrome / Safari
https://github.com/user-attachments/assets/c441b646-7817-4ede-bc20-d9d307e73c8b Offline test: https://github.com/user-attachments/assets/c4942472-829e-427c-a65b-763ac2118678