NEW247984
References to iframes seem do not get garbage collected 
https://bugs.webkit.org/show_bug.cgi?id=247984
Summary References to iframes seem do not get garbage collected
Alex Christensen
Reported 2022-11-16 09:32:31 PST
This uses lots of memory: for (var i = 0; i < 100000; i++) { document.body.appendChild(document.createElement('iframe')); document.querySelector('iframe').remove(); } But this does not: for (var i = 0; i < 100000; i++) { new ArrayBuffer(1000000); } It seems like we are keeping references to iframes alive. Reported by Mark in the Ask Apple event.
Attachments
Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2022-11-16 09:33:48 PST
<rdar://problem/102423946>
Alex Christensen
Comment 2 2022-11-16 09:40:40 PST
Chrome and Firefox seem to have the same behavior.
Alex Christensen
Comment 3 2022-11-16 09:46:15 PST
Mark reported that WebAssembly.Memory does have a similar issue unique to WebKit. It's possible we're missing a call to reportExtraMemoryAllocated or something in that case.
Yusuke Suzuki
Comment 4 2022-11-16 13:15:41 PST
(In reply to Alex Christensen from comment #3) > Mark reported that WebAssembly.Memory does have a similar issue unique to > WebKit. It's possible we're missing a call to reportExtraMemoryAllocated or > something in that case. WebAssembly.Memory already has this. And I cannot reproduce this issue. for (var i = 0; i < 1000000; i++) { new WebAssembly.Memory({ initial: 1024 }); } Just works as the same way to `new ArrayBuffer` one.
dmt021
Comment 5 2022-11-16 13:31:35 PST
(In reply to Yusuke Suzuki from comment #4) > (In reply to Alex Christensen from comment #3) > > Mark reported that WebAssembly.Memory does have a similar issue unique to > > WebKit. It's possible we're missing a call to reportExtraMemoryAllocated or > > something in that case. > > WebAssembly.Memory already has this. And I cannot reproduce this issue. > > for (var i = 0; i < 1000000; i++) { new WebAssembly.Memory({ initial: 1024 > }); } > > Just works as the same way to `new ArrayBuffer` one. https://dmt021.github.io/grow_index.html Sample with WebAssembly.Memory leak Basic flow: 1. alloc WebAssembly.Memory within the iframe 2. delete iframe from the main frame
Yusuke Suzuki
Comment 6 2022-11-16 21:27:41 PST
(In reply to dmt021 from comment #5) > (In reply to Yusuke Suzuki from comment #4) > > (In reply to Alex Christensen from comment #3) > > > Mark reported that WebAssembly.Memory does have a similar issue unique to > > > WebKit. It's possible we're missing a call to reportExtraMemoryAllocated or > > > something in that case. > > > > WebAssembly.Memory already has this. And I cannot reproduce this issue. > > > > for (var i = 0; i < 1000000; i++) { new WebAssembly.Memory({ initial: 1024 > > }); } > > > > Just works as the same way to `new ArrayBuffer` one. > > https://dmt021.github.io/grow_index.html > Sample with WebAssembly.Memory leak > Basic flow: > 1. alloc WebAssembly.Memory within the iframe > 2. delete iframe from the main frame This means that iframe is alive and WebAssembly.Memory is kept alive. Not particularly related to WebAssembly.Memory implementation.
Yusuke Suzuki
Comment 7 2022-11-16 21:28:33 PST
<!DOCTYPE html> <html lang="en"> <head> <button onclick="allocate_memory();"> Allocate Memory </button> <p>Memory bytes: </p> <p id="memory_text">65536</p> </head> <script> let ctx = { memory: new WebAssembly.Memory({ initial: 1, }) }; const bytesPerPage = 64 * 1024; function allocate_memory() { let memory = ctx.memory; memory.grow(1000); document.getElementById("memory_text").innerHTML = memory.buffer.byteLength; } </script> Yeah, allocate_memory function in the global variable, and it is capturing WebAssembly.Memory. So, so long as iframe is alive, then WebAssembly.Memory is also alive.
Mark Agranat
Comment 8 2023-01-20 02:55:06 PST
The bug with memory (In reply to Yusuke Suzuki from comment #7) > <!DOCTYPE html> > <html lang="en"> > > <head> > <button onclick="allocate_memory();"> > Allocate Memory > </button> > <p>Memory bytes: </p> > <p id="memory_text">65536</p> > </head> > > <script> > let ctx = { > memory: new WebAssembly.Memory({ > initial: 1, > }) > }; > const bytesPerPage = 64 * 1024; > function allocate_memory() { > let memory = ctx.memory; > memory.grow(1000); > document.getElementById("memory_text").innerHTML = > memory.buffer.byteLength; > } > </script> > > Yeah, allocate_memory function in the global variable, and it is capturing > WebAssembly.Memory. So, so long as iframe is alive, then WebAssembly.Memory > is also alive. (In reply to Yusuke Suzuki from comment #6) > (In reply to dmt021 from comment #5) > > (In reply to Yusuke Suzuki from comment #4) > > > (In reply to Alex Christensen from comment #3) > > > > Mark reported that WebAssembly.Memory does have a similar issue unique to > > > > WebKit. It's possible we're missing a call to reportExtraMemoryAllocated or > > > > something in that case. > > > > > > WebAssembly.Memory already has this. And I cannot reproduce this issue. > > > > > > for (var i = 0; i < 1000000; i++) { new WebAssembly.Memory({ initial: 1024 > > > }); } > > > > > > Just works as the same way to `new ArrayBuffer` one. > > > > https://dmt021.github.io/grow_index.html > > Sample with WebAssembly.Memory leak > > Basic flow: > > 1. alloc WebAssembly.Memory within the iframe > > 2. delete iframe from the main frame > > This means that iframe is alive and WebAssembly.Memory is kept alive. Not > particularly related to WebAssembly.Memory implementation. How it could be alive, if we create and delete it?
Mark Agranat
Comment 9 2023-02-14 06:12:56 PST
Are any news here?
Mark Agranat
Comment 10 2023-03-15 02:39:05 PDT
I want to mention that we don't have such problem in android, for example.
Mark Agranat
Comment 11 2023-03-15 04:28:36 PDT
I've duplicated that bug here https://bugs.webkit.org/show_bug.cgi?id=253950
Alexey Proskuryakov
Comment 12 2023-03-15 12:16:12 PDT
*** Bug 253950 has been marked as a duplicate of this bug. ***
Maciej Stachowiak
Comment 13 2023-04-24 14:51:54 PDT
Is there an impact on Yandex? If so, what is it?
Mark Agranat
Comment 14 2023-04-25 02:57:38 PDT
(In reply to Maciej Stachowiak from comment #13) > Is there an impact on Yandex? If so, what is it? We have app(https://igrotok.yandex.ru/games/?utm_source=igrotok), that works nicely on another platforms (all kind of desktops and android), but always reloads page after few swipes on iOS. After spending some time with measurement tools (safari web inspector) and compiling+debugging webkit we discovered that there is a huge memory allocations related to web assembly and iframe specific classes. I think that that may be webkit have different garbage collection policy, but i don't have time for investigate it.
Mark Agranat
Comment 15 2023-04-25 04:03:54 PDT
(In reply to Mark Agranat from comment #14) > (In reply to Maciej Stachowiak from comment #13) > > Is there an impact on Yandex? If so, what is it? > > We have app(https://igrotok.yandex.ru/games/?utm_source=igrotok), that works > nicely on another platforms (all kind of desktops and android), but always > reloads page after few swipes on iOS. After spending some time with > measurement tools (safari web inspector) and compiling+debugging webkit we > discovered that there is a huge memory allocations related to web assembly > and iframe specific classes. I think that that may be webkit have different > garbage collection policy, but i don't have time for investigate it. about working platforms i made mistake. it's work well only on android (desktop is now unsupported).
Chris Dumez
Comment 16 2023-04-26 08:28:55 PDT
``` for (var i = 0; i < 100000; i++) { document.body.appendChild(document.createElement('iframe')); document.querySelector('iframe').remove(); } ``` is a tight loop. As a result, our GC wouldn't free the frames until the loop is over and we return to the runloop I believe. This is expected behavior AFAIK. Written like this, it should avoid crazy memory growth: ``` for (var i = 0; i < 100000; i++) { setTimeout(() => { document.body.appendChild(document.createElement('iframe')); document.querySelector('iframe').remove(); }, 0); } ``` Ff the frames don't get freed after the loop though, then we have a leak. I'll verify.
Chris Dumez
Comment 17 2023-04-26 08:48:55 PDT
I have tried the following test case: ``` for (var i = 0; i < 100000; i++) { setTimeout(() => { document.body.appendChild(document.createElement('iframe')); document.querySelector('iframe').remove(); }, 0); } ``` We do NOT have a leak. I added logging and I see iframes getting destroyed (The HTMLIFrameElement destructor is getting called). However, it is true that GC is not destroying frames as fast as they are constructed. I see the number of HTMLIframeElement instances steadily growing (despite some frames getting destroyed along the way). Given that this is not a leak, I am not sure what to do on WebCore side. To me, this looks like GC not being aggressive enough.
Mark Agranat
Comment 18 2023-05-02 04:18:18 PDT
(In reply to Chris Dumez from comment #17) > I have tried the following test case: > ``` > for (var i = 0; i < 100000; i++) { > setTimeout(() => { > document.body.appendChild(document.createElement('iframe')); > document.querySelector('iframe').remove(); > }, 0); > } > ``` > > We do NOT have a leak. I added logging and I see iframes getting destroyed > (The HTMLIFrameElement destructor is getting called). > > However, it is true that GC is not destroying frames as fast as they are > constructed. I see the number of HTMLIframeElement instances steadily > growing (despite some frames getting destroyed along the way). > > Given that this is not a leak, I am not sure what to do on WebCore side. To > me, this looks like GC not being aggressive enough. 1) Do you know, do apple have own implementation of GC? 2) Maybe it is possible to make the GC mode more aggressive? 3) Are WebContent process receiving a memory pressure in case of HTMLIframeElement instances growing? Is it possible to force garbage collection in such case?
Note You need to log in before you can comment on or make changes to this bug.