Skip to content

Conversation

@butonic
Copy link
Member

@butonic butonic commented Jun 18, 2018

Edge has a hard 5GB memory limit. currently chunked uploading seems to use one indirection too many so edge will never garbage collect the chunks Blob objects.

Unsetting them manually after a chunk is completed is not enough. The memory tab in edge won't show you the objects on the heap, but overall memory consumption will still increase.

leak

This pr allows cs to happen by removing the actual data before passing the options to the anonymous handler: https://github.com/owncloud/core/compare/update-jqfu-fix-edge-oom?expand=1#diff-9e1e26ed9e6e26bd68c321b010edc0d9R772

gc

The other changes stem from a bump to the latest version, which has some cool things like blueimp/jQuery-File-Upload#3486 that would allow us to use dynamic chunk sizes based on the network bandwith.

  • create PR against upstream
  • use proper dependency management

@butonic
Copy link
Member Author

butonic commented Jun 18, 2018

maybe this even prevents ie from freezing as well.

@butonic
Copy link
Member Author

butonic commented Jun 18, 2018

upstream PR: blueimp/jQuery-File-Upload#3508

@butonic butonic added this to the development milestone Jun 18, 2018
@butonic butonic requested a review from PVince81 June 18, 2018 15:20
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@butonic butonic force-pushed the update-jqfu-fix-edge-oom branch from 21fd58d to d472c6a Compare June 19, 2018 07:22
@codecov
Copy link

codecov bot commented Jun 19, 2018

Codecov Report

Merging #31825 into master will decrease coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #31825     +/-   ##
===========================================
- Coverage     63.45%   63.25%   -0.2%     
+ Complexity    18785    18457    -328     
===========================================
  Files          1158     1158             
  Lines         69718    69319    -399     
  Branches       1261     1261             
===========================================
- Hits          44237    43846    -391     
+ Misses        25111    25103      -8     
  Partials        370      370
Flag Coverage Δ Complexity Δ
#javascript 52.47% <ø> (-47.53%) 0 <ø> (ø)
#phpunit 64.48% <ø> (-1.53%) 18457 <ø> (-2329)
Impacted Files Coverage Δ Complexity Δ
apps/files_external/lib/Lib/Backend/SFTP.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
apps/files_external/lib/Lib/Backend/Swift.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
apps/files_external/lib/Lib/Backend/Local.php 76.92% <0%> (ø) 0% <0%> (ø) ⬇️
core/js/systemtags/systemtagmodel.js 85.71% <0%> (ø) 0% <0%> (ø) ⬇️
apps/files_external/lib/Command/Export.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
apps/files_external/lib/Lib/Backend/SFTP_Key.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
...s/files_external/lib/Lib/Storage/StreamWrapper.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
core/js/sharedialoglinksocialview.js 51.72% <0%> (ø) 0% <0%> (ø) ⬇️
core/js/config.js 3.33% <0%> (ø) 0% <0%> (ø) ⬇️
core/js/octemplate.js 11.11% <0%> (ø) 0% <0%> (ø) ⬇️
... and 100 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66029f1...d472c6a. Read the comment docs.

@butonic butonic changed the title update jquery.fileupload.js to 1d733f8a, allow edge to gc chunks allow edge to gc chunks Jun 20, 2018
@PVince81
Copy link
Contributor

seems this PR changed approach completely? @butonic

did the new approach work ?

at least it looks much less intrusive!

@butonic
Copy link
Member Author

butonic commented Jun 21, 2018

It works for EdgeHTML 17 (the current stable version for Win 10). It does not work for EdgeHTML 15. Which is what is being used. Still an improvement. Without this Edge will eat memory the size of every upload and restart the process when 5GB have been reached. You can monitor the memory being eaten live in the dev tools memory tab. https://www.browserstack.com/test-on-microsoft-edge-browser allows you to test EdgeHTML 15 and 17 in your browser for free.

@PVince81
Copy link
Contributor

We could still put this in though ?

@butonic
Copy link
Member Author

butonic commented Jun 22, 2018

Yes, should be put in. Prevents memory leak and crash when uploading large file with EdgeHTML 16 or greater.

@butonic
Copy link
Member Author

butonic commented Jun 22, 2018

backport to 10 needed

@butonic
Copy link
Member Author

butonic commented Jun 22, 2018

failing test unrelated

@phil-davis
Copy link
Contributor

failing test unrelated

caldav/carddav drone jobs changed a few days ago (last week?). A rebase here will pick all that up and the jobs will pass.

@PVince81 PVince81 self-assigned this Jun 22, 2018
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 looks safe, merging

@PVince81 PVince81 merged commit b2a0c67 into master Jun 25, 2018
@PVince81 PVince81 deleted the update-jqfu-fix-edge-oom branch June 25, 2018 08:45
@PVince81
Copy link
Contributor

@butonic please backport

@PVince81
Copy link
Contributor

stable10: #31884

@lock lock bot locked as resolved and limited conversation to collaborators Aug 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants