Bug 247984 - References to iframes seem do not get garbage collected
Summary: References to iframes seem do not get garbage collected
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 253950 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-11-16 09:32 PST by Alex Christensen
Modified: 2023-05-02 04:18 PDT (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 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.
Comment 1 Radar WebKit Bug Importer 2022-11-16 09:33:48 PST
<rdar://problem/102423946>
Comment 2 Alex Christensen 2022-11-16 09:40:40 PST
Chrome and Firefox seem to have the same behavior.
Comment 3 Alex Christensen 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.
Comment 4 Yusuke Suzuki 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.
Comment 5 dmt021 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
Comment 6 Yusuke Suzuki 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.
Comment 7 Yusuke Suzuki 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.
Comment 8 Mark Agranat 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?
Comment 9 Mark Agranat 2023-02-14 06:12:56 PST
Are any news here?
Comment 10 Mark Agranat 2023-03-15 02:39:05 PDT
I want to mention that we don't have such problem in android, for example.
Comment 11 Mark Agranat 2023-03-15 04:28:36 PDT
I've duplicated that bug here https://bugs.webkit.org/show_bug.cgi?id=253950
Comment 12 Alexey Proskuryakov 2023-03-15 12:16:12 PDT
*** Bug 253950 has been marked as a duplicate of this bug. ***
Comment 13 Maciej Stachowiak 2023-04-24 14:51:54 PDT
Is there an impact on Yandex? If so, what is it?
Comment 14 Mark Agranat 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.
Comment 15 Mark Agranat 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).
Comment 16 Chris Dumez 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.
Comment 17 Chris Dumez 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.
Comment 18 Mark Agranat 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?