REGRESSION: r83938 abandons GC memory

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Michael Saboff]

Created attachment 91278 [details]
Memory Test webpage

While running a memory test, the amount of dirty pages attributed to the garbage collector grows unbounded.  This is evident by the output of "vmmap -resident <pid>":

REGION TYPE             [ VIRTUAL/RESIDENT]
===========             [ =======/========]
ATS (font support)      [   31.8M/    424K]
CG backing stores       [   5140K/   5140K]
CG image                [     32K/     32K]
CG raster data          [   1176K/   1168K]
CG shared images        [   1972K/   1276K]
Carbon                  [   1728K/   1728K]
CoreGraphics            [    832K/    820K]
IOKit                   [  258.7M/   2652K]
JS JIT generated code   [    1.0G/   30.9M]
JS VM register file     [   4160K/     12K]
JS garbage collector    [   22.2M/   22.2M]
LayerKit                [   2380K/   1332K]
MALLOC                  [  206.1M/  193.4M]
Memory tag=242          [     12K/     12K]
Memory tag=251          [     28K/     28K]
Memory tag=69           [    160K/    160K]
OpenGL GLSL             [   2048K/   1144K]
STACK GUARD             [   56.1M/      0K]
Stack                   [   13.7M/    596K]
VM_ALLOCATE             [   16.5M/   16.5M]
__DATA                  [   17.0M/   12.1M]
__IMAGE                 [   1240K/    932K]
__LINKEDIT              [  116.0M/  103.2M]
__NV_CUDA               [      4K/      4K]
__TEXT                  [  169.4M/  129.9M]
__UNICODE               [    536K/    368K]
mapped file             [   37.2M/   15.7M]
shared memory           [    832K/    732K]

The memory test webpage is attached.

This revision was identified by building & testing Safari with r83938 and r83937.

[Michael Saboff]

<rdar://problem/9344346>

[Geoffrey Garen]

Created attachment 91648 [details]
Patch

[Alexander Romanovich]

Does this fix address anything from bug 40926 in any way?

[Geoffrey Garen]

(In reply to comment #3)
> Does this fix address anything from bug 40926 in any way?

Yes, it seems to fix the test case attached there.

[Alexey Proskuryakov]

Comment on attachment 91648 [details]
Patch

Loks good to me. As discussed in person, the wrapper need to be protected if the load is over, but an event is still pending. As you are going to work on a regression test for that, I can as well mark this patch r-.

[Geoffrey Garen]

Created attachment 91745 [details]
Patch

[Geoffrey Garen]

*** Bug 40926 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

Comment on attachment 91745 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91745&action=review

> Source/WebCore/bindings/js/JSNodeCustom.cpp:108
>          // If a wrapper is the last reference to an image or script element

Please fix the comment, too.

> Source/WebCore/loader/ImageLoader.cpp:70
> +    bool hasPendingEvents(const ImageLoader* loader) { return m_dispatchSoonList.find(loader) != notFound; }

We don't normally use const with complex classes. In particular, "const ImageLoader" never appears in WebCore code now.

So, I would slightly prefer removing "const" from ImageLoader::hasPendingLoadEvent() instead.

> LayoutTests/fast/dom/gc-image-element.html:17
> +function gc()

You could also use js-test machinery for logging and gc().

[Geoffrey Garen]

Committed r85375: <http://trac.webkit.org/changeset/85375>

[Simon Fraser (smfr)]

Lack of constness on new methods in this patch makes me sad.