Bug 59604 - REGRESSION: r83938 abandons GC memory
Summary: REGRESSION: r83938 abandons GC memory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P1 Major
Assignee: Geoffrey Garen
URL:
Keywords: InRadar
: 40926 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-04-27 06:57 PDT by Michael Saboff
Modified: 2011-05-28 09:58 PDT (History)
6 users (show)

See Also:


Attachments
Memory Test webpage (421 bytes, text/html)
2011-04-27 06:57 PDT, Michael Saboff
no flags Details
Patch (4.06 KB, patch)
2011-04-29 00:18 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (9.62 KB, patch)
2011-04-29 15:01 PDT, Geoffrey Garen
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2011-04-27 06:57:44 PDT
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.
Comment 1 Michael Saboff 2011-04-27 06:58:57 PDT
<rdar://problem/9344346>
Comment 2 Geoffrey Garen 2011-04-29 00:18:50 PDT
Created attachment 91648 [details]
Patch
Comment 3 Alexander Romanovich 2011-04-29 04:44:33 PDT
Does this fix address anything from bug 40926 in any way?
Comment 4 Geoffrey Garen 2011-04-29 12:10:37 PDT
(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.
Comment 5 Alexey Proskuryakov 2011-04-29 14:41:54 PDT
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-.
Comment 6 Geoffrey Garen 2011-04-29 15:01:56 PDT
Created attachment 91745 [details]
Patch
Comment 7 Geoffrey Garen 2011-04-29 15:09:40 PDT
*** Bug 40926 has been marked as a duplicate of this bug. ***
Comment 8 Alexey Proskuryakov 2011-04-29 15:18:27 PDT
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().
Comment 9 Geoffrey Garen 2011-04-29 17:57:44 PDT
Committed r85375: <http://trac.webkit.org/changeset/85375>
Comment 10 Simon Fraser (smfr) 2011-05-28 09:58:00 PDT
Lack of constness on new methods in this patch makes me sad.