Bug 247984
| Summary: | References to iframes seem do not get garbage collected | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> |
| Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> |
| Status: | NEW | ||
| Severity: | Normal | CC: | cdumez, dmt021, magranat, mjs, webkit-bug-importer, ysuzuki |
| Priority: | P2 | Keywords: | InRadar |
| Version: | WebKit Local Build | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
Alex Christensen
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
<rdar://problem/102423946>
Alex Christensen
Chrome and Firefox seem to have the same behavior.
Alex Christensen
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
(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
(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
(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
<!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
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
Are any news here?
Mark Agranat
I want to mention that we don't have such problem in android, for example.
Mark Agranat
I've duplicated that bug here https://bugs.webkit.org/show_bug.cgi?id=253950
Alexey Proskuryakov
*** Bug 253950 has been marked as a duplicate of this bug. ***
Maciej Stachowiak
Is there an impact on Yandex? If so, what is it?
Mark Agranat
(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
(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
```
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
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
(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?