Release object URL after image for resize has loaded #35499

MichelJansson · 2021-08-19T13:33:49Z

Releases/revokes objectURL of original image (sent to toImageFile) after it has been loaded, as it is no longer needed after that point.

No automated tests added, as there is no simple way (without overriding APIs anyway) to find/get existing objectURL's.
Manual testing however shows that the URL's are released by;

Using the sample outlined in Blazor InputFile RequestImageFileAsync not revoking ObjectURL #35156
Inspect page with DevTools, finding object URL's in the sources tab
Trying to open any of the URL's (open in new tab) -> ERR_FILE_NOT_FOUND

Fixes #35156

dnfadmin · 2021-08-19T13:34:04Z

All CLA requirements met.

TanayParikh

Thanks @MichelJansson, this looks good to me. cc/ @MackinnonBuck as he was the original author in this area so would like to get his take.

MichelJansson · 2021-08-20T17:14:10Z

@TanayParikh, Thanks for checking. I read somewhere that the main branch now was targeting .NET 7, is that correct? If so, should we change the target branch to one relevant for 6-rc2?

TanayParikh · 2021-08-20T17:27:40Z

@TanayParikh, Thanks for checking. I read somewhere that the main branch now was targeting .NET 7, is that correct? If so, should we change the target branch to one relevant for 6-rc2?

From my understanding the main branch should still be appropriate for rc2 targeted patches, I'll update if I find out otherwise.

MackinnonBuck · 2021-08-23T17:22:52Z

@TanayParikh This looks good to me as well

TanayParikh · 2021-08-24T23:02:46Z

Did some quick additional manual validations, things are looking good, ready to merge! Thanks again @MichelJansson

pranavkm · 2021-08-24T23:06:22Z

src/Components/Web.JS/src/InputFile.ts

  const loadedImage = await new Promise(function(resolve: (loadedImage: HTMLImageElement) => void): void {
    const originalFileImage = new Image();
    originalFileImage.onload = function(): void {
+      URL.revokeObjectURL(originalFileImage.src);


Should we also do this onerror? Is there an opportunity for that to happen?

Sure, can't hurt. Note, we'll have to onerror = null first though: https://stackoverflow.com/questions/8124866/how-does-one-use-the-onerror-attribute-of-an-img-element

Good idea. I guess it would error out if the file sent wasn't an image?

@TanayParikh - Looks like the compiled dist/Release/blazor.*.js files didn't get this last update. From my understanding those also has to be committed?

Good idea. I guess it would error out if the file sent wasn't an image?

More so if the image doesn't exist at all:
https://www.w3schools.com/jsref/event_onerror.asp

@TanayParikh - Looks like the compiled dist/Release/blazor.*.js files didn't get this last update. From my understanding those also has to be committed?

Usually yes, however given the final change wouldn't have any effect on tests, updating the file isn't strictly necessary here (I also didn't have push permissions via git due to this branch being on a separate fork, only could make changes via GitHub UI). Regardless, the update will be picked up automatically via the next change-set. Good eye though! 😄

src/Components/Web.JS/src/InputFile.ts

TanayParikh · 2021-08-25T16:52:56Z

/backport to release/6.0

github-actions · 2021-08-25T16:53:11Z

Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/1167375618

github-actions · 2021-08-25T16:53:37Z

@TanayParikh backporting to release/6.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Release object URL after image for resize has loaded
warning: Cannot merge binary files: src/Components/Web.JS/dist/Release/blazor.server.js (HEAD vs. Release object URL after image for resize has loaded)
Using index info to reconstruct a base tree...
M	src/Components/Web.JS/dist/Release/blazor.server.js
Falling back to patching base and 3-way merge...
Auto-merging src/Components/Web.JS/dist/Release/blazor.server.js
CONFLICT (content): Merge conflict in src/Components/Web.JS/dist/Release/blazor.server.js
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Release object URL after image for resize has loaded
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Co-authored-by: Tanay Parikh <[email protected]>

…` to 6.0 (#35712) * Release object URL after image for resize has loaded (#35499) Co-authored-by: Tanay Parikh <[email protected]> * Updated release.js files Co-authored-by: Michel Jansson <[email protected]>

MikeReedGH · 2024-02-27T22:26:09Z

Hi All,

This error still appears to be happening in a .Net 7 Blazor WASM PWA app :(

After calling e.File.RequestImageFileAsync I can see the blob is created in the chrome sources tab.
For each file I select a blob is created. They never disappear from the sources tab unless I refresh (f5) the page.
If I don't use the e.File.RequestImageFileAsync but rather just the e.File.OpenReadStream(e.File.Size) then the blob is not created at all, and so the memory leak does not occur.

Will the fix mentioned above made it's way into the .Net 7 release?

I am running as a client side Blazor WASM/PWA and therefore internally navigating between pages rather than just a traditional redirect in the browser, could that mean that the revokeObjectURL is never called?

Thanks and Regards

MichelJansson · 2024-02-28T22:10:24Z

Hi All,

This error still appears to be happening in a .Net 7 Blazor WASM PWA app :(

After calling e.File.RequestImageFileAsync I can see the blob is created in the chrome sources tab. For each file I select a blob is created. They never disappear from the sources tab unless I refresh (f5) the page. If I don't use the e.File.RequestImageFileAsync but rather just the e.File.OpenReadStream(e.File.Size) then the blob is not created at all, and so the memory leak does not occur.

Will the fix mentioned above made it's way into the .Net 7 release?

I am running as a client side Blazor WASM/PWA and therefore internally navigating between pages rather than just a traditional redirect in the browser, could that mean that the revokeObjectURL is never called?

Thanks and Regards

@MikeReedGH The fix was implemented into .NET6 so should also have been released with .NET7. I can still see the fix in the main branch.

If i remember correctly, the blobs will still be visible under the sources tab even though they have been released. Try and open the URL's from the sources tab to see if they actually still exist (.3 of the manual testing suggested in the PR description)

MikeReedGH · 2024-02-29T02:25:17Z

@MichelJansson. Thanks for replying. Ah ok, so the blobs still showing may just be a red herring!? I'll check.

I do still have the problem....as soon a user selects a photo/or takes a camera photo that the WASM app just immediately crashes and restarts. It's not a silly bug in my handling of the IBrowserFile. I have quadrupled checked.
It happens seemingly randomly, it will work fine for hours and then suddenly just crash.
It just smells like a memory leak, and I really thought I had found it when reading about this bug.
I am running out of ideas :(

Thanks

Release object URL after image for resize has loaded

ab85b93

MichelJansson requested a review from a team as a code owner August 19, 2021 13:33

ghost added area-blazor Includes: Blazor, Razor Components community-contribution Indicates that the PR has been added by a community member labels Aug 19, 2021

TanayParikh self-assigned this Aug 19, 2021

TanayParikh reviewed Aug 20, 2021

View reviewed changes

Merge branch 'main' into blazor-revoke-object-url

64b7c18

TanayParikh enabled auto-merge (squash) August 24, 2021 23:01

pranavkm reviewed Aug 24, 2021

View reviewed changes

TanayParikh reviewed Aug 24, 2021

View reviewed changes

src/Components/Web.JS/src/InputFile.ts Show resolved Hide resolved

Update src/Components/Web.JS/src/InputFile.ts

4bb17a6

TanayParikh approved these changes Aug 24, 2021

View reviewed changes

TanayParikh merged commit 4b48d6f into dotnet:main Aug 25, 2021

ghost added this to the 7.0-preview1 milestone Aug 25, 2021

TanayParikh added a commit that referenced this pull request Aug 25, 2021

Release object URL after image for resize has loaded (#35499)

ff14c3d

Co-authored-by: Tanay Parikh <[email protected]>

TanayParikh mentioned this pull request Aug 25, 2021

Backport Release object URL after image for resize has loaded (#35499) to 6.0 #35712

Merged

Release object URL after image for resize has loaded #35499

Release object URL after image for resize has loaded #35499

Uh oh!

Conversation

MichelJansson commented Aug 19, 2021

Uh oh!

dnfadmin commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TanayParikh left a comment

Choose a reason for hiding this comment

Uh oh!

MichelJansson commented Aug 20, 2021

Uh oh!

TanayParikh commented Aug 20, 2021

Uh oh!

MackinnonBuck commented Aug 23, 2021

Uh oh!

TanayParikh commented Aug 24, 2021

Uh oh!

pranavkm Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

TanayParikh Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

TanayParikh Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

MichelJansson Aug 25, 2021

Choose a reason for hiding this comment

Uh oh!

TanayParikh Aug 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TanayParikh commented Aug 25, 2021

Uh oh!

github-actions bot commented Aug 25, 2021

Uh oh!

github-actions bot commented Aug 25, 2021

Uh oh!

MikeReedGH commented Feb 27, 2024

Uh oh!

MichelJansson commented Feb 28, 2024

Uh oh!

MikeReedGH commented Feb 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dnfadmin commented Aug 19, 2021 •

edited

Loading