Repository navigation
[web] Clear HTMLImageElement.src when WebImageInfo and ImgElementPlatformView are disposed - #193408
Conversation
…formView are disposed When `Image.network` uses `WebHtmlElementStrategy.prefer` or `.fallback`, `loadViaImgElement` decodes an off-DOM `HTMLImageElement` wrapped in `WebImageInfo`, and `RawWebImage` mounts a second `<img>` element inside `ImgElementPlatformView`. Previously: - `WebImageInfo.dispose()` was a no-op, leaving `htmlImage.src` set on the off-DOM `HTMLImageElement` (which pins non-purgeable `VM_ALLOCATE (graphics)` memory in WebKit on iOS until full JS garbage collection). - `ImgElementPlatformView` neither updated its `<img>` element's `src` when updated in-place nor cleared `img.src = ''` when unmounted. This change: - Reference-counts the underlying `HTMLImageElement` across `WebImageInfo` clones and clears `htmlImage.src = ''` when the last clone is disposed. - Converts `ImgElementPlatformView` to a `StatefulWidget` that updates `img.src` in `didUpdateWidget` and clears `img.src = ''` on `dispose`. Issue: flutter#152709
There was a problem hiding this comment.
Code Review
This pull request introduces reference counting to WebImageInfo to eagerly release the decoded image buffer by clearing the HTML image element's src attribute when all clones are disposed. It also converts ImgElementPlatformView to a StatefulWidget to manage and clear the image src attribute on widget updates and disposal. Feedback was provided regarding a potential memory leak in ImgElementPlatformView if widget.src is updated to null before the platform view is fully created, suggesting an early return and clearing of the src attribute in _onPlatformViewCreated.
|
autosubmit label was removed for flutter/flutter/193408, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
autosubmit label was removed for flutter/flutter/193408, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
autosubmit label was removed for flutter/flutter/193408, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If both you and the PR author believe this PR qualifies for a test exemption, escalate to the tech lead for the changed area to review and apply the |
|
autosubmit label was removed for flutter/flutter/193408, because - This pull request has the |
|
PR already includes tests but cc @eyebrowsoffire can we loosen the test file detection a bit? |
…formView are disposed (flutter#193408) ### Description When `Image.network` is configured with `WebHtmlElementStrategy.prefer` or `WebHtmlElementStrategy.fallback`, `NetworkImage._loadAsync` (`loadViaImgElement`) creates an off-DOM `HTMLImageElement`, awaits `imageElement.decode()`, and wraps it in `WebImageInfo`. When rendered by `RawWebImage`, `ImgElementPlatformView` creates a second `HTMLImageElement` inside an `HtmlElementView`. Previously: - `WebImageInfo.dispose()` was a no-op, leaving `htmlImage.src` set on the off-DOM `HTMLImageElement` even after `ImageCache` eviction and widget disposal. In WebKit on iOS, `imageElement.decode()` pins full-resolution decoded RGBA8 bitmaps in non-purgeable `VM_ALLOCATE (graphics)` memory (`PURGE=N`) until `src` is cleared or a full JS garbage collection finalizes the detached wrapper. - `ImgElementPlatformView` was a `StatelessWidget` that neither updated its `<img>` element's `src` when `RawWebImage` updated in-place (for example with `gaplessPlayback: true`) nor cleared `img.src = ''` when unmounted. This PR: - Reference-counts the underlying `HTMLImageElement` across `WebImageInfo` clones (`_HtmlImageElementHandle` / `_HtmlImageElementRef`) and sets `htmlImage.src = ''` when the last clone is disposed (matching the engine's `ImageElementImageSource._doClose()` behavior in `dart:ui_web`). - Converts `ImgElementPlatformView` to a `StatefulWidget` that tracks the created `HTMLImageElement` via `onPlatformViewCreated`, updates `img.src` in `didUpdateWidget`, and clears `img.src = ''` in `dispose()`. - Adds unit and widget tests in `test/painting/_network_image_test_web.dart` verifying reference-counted `WebImageInfo` disposal across clones, in-place `ImgElementPlatformView` `src` updates, and `src` cleanup on unmount. ### Empirical Benchmark (`iPhone 17 Pro` Simulator, `iOS 26.0.1` Mobile Safari, Flutter Web `--release`) Stepping through 5 high-resolution JPEGs (`4672x7008` to `6215x9322`, `677.5 MB` total uncompressed RGBA8) with `WebHtmlElementStrategy.prefer` and unmounting: | Metric (`com.apple.WebKit.WebContent`, `--release`) | Pre-Change (`master`) | Post-Change (This PR) | Delta | | :--- | :--- | :--- | :--- | | **Post-Unmount `VM_ALLOCATE (graphics)` (before `lowMemory`)** | `221.0 MB` | `0.0 MB` | `-221.0 MB` (`-100.0%`) | | **Post-Unmount `phys_footprint` (before `lowMemory`)** | `542.1 MB` | `315.7 MB` | `-226.4 MB` (`-41.8%`) | | **Step 5 Active `phys_footprint`** | `616.5 MB` | `390.1 MB` | `-226.4 MB` (`-36.7%`) | | **Kernel Peak `phys_footprint` (`ri_lifetime_max_phys_footprint`)** | `1,159.8 MB` | `972.4 MB` | `-187.4 MB` (`-16.2%`) | Fixes part of flutter#152709 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant in-code documentation (doc comments with `///`). - [x] If this PR introduces a new feature or capability, I created and linked a website documentation issue or PR in [flutter/website] (or verified none is needed). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [flutter/website]: https://github.com/flutter/website [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Description
When
Image.networkis configured withWebHtmlElementStrategy.preferorWebHtmlElementStrategy.fallback,NetworkImage._loadAsync(loadViaImgElement) creates an off-DOMHTMLImageElement, awaitsimageElement.decode(), and wraps it inWebImageInfo. When rendered byRawWebImage,ImgElementPlatformViewcreates a secondHTMLImageElementinside anHtmlElementView.Previously:
WebImageInfo.dispose()was a no-op, leavinghtmlImage.srcset on the off-DOMHTMLImageElementeven afterImageCacheeviction and widget disposal. In WebKit on iOS,imageElement.decode()pins full-resolution decoded RGBA8 bitmaps in non-purgeableVM_ALLOCATE (graphics)memory (PURGE=N) untilsrcis cleared or a full JS garbage collection finalizes the detached wrapper.ImgElementPlatformViewwas aStatelessWidgetthat neither updated its<img>element'ssrcwhenRawWebImageupdated in-place (for example withgaplessPlayback: true) nor clearedimg.src = ''when unmounted.This PR:
HTMLImageElementacrossWebImageInfoclones (_HtmlImageElementHandle/_HtmlImageElementRef) and setshtmlImage.src = ''when the last clone is disposed (matching the engine'sImageElementImageSource._doClose()behavior indart:ui_web).ImgElementPlatformViewto aStatefulWidgetthat tracks the createdHTMLImageElementviaonPlatformViewCreated, updatesimg.srcindidUpdateWidget, and clearsimg.src = ''indispose().test/painting/_network_image_test_web.dartverifying reference-countedWebImageInfodisposal across clones, in-placeImgElementPlatformViewsrcupdates, andsrccleanup on unmount.Empirical Benchmark (
iPhone 17 ProSimulator,iOS 26.0.1Mobile Safari, Flutter Web--release)Stepping through 5 high-resolution JPEGs (
4672x7008to6215x9322,677.5 MBtotal uncompressed RGBA8) withWebHtmlElementStrategy.preferand unmounting:com.apple.WebKit.WebContent,--release)master)VM_ALLOCATE (graphics)(beforelowMemory)221.0 MB0.0 MB-221.0 MB(-100.0%)phys_footprint(beforelowMemory)542.1 MB315.7 MB-226.4 MB(-41.8%)phys_footprint616.5 MB390.1 MB-226.4 MB(-36.7%)phys_footprint(ri_lifetime_max_phys_footprint)1,159.8 MB972.4 MB-187.4 MB(-16.2%)Fixes part of #152709
Pre-launch Checklist
///).