Bug 31253 - Unbounded memory growth when adding and removing images
: Unbounded memory growth when adding and removing images
Status: NEW
: WebKit
WebCore JavaScript
: 528+ (Nightly build)
: All All
: P2 Major
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-11-09 05:15 PST by
Modified: 2014-01-19 21:46 PST (History)


Attachments
bug_memoryLeak.html (2.76 KB, text/html)
2009-11-09 05:15 PST, Flament David
no flags Details
Simple patch, possible solution to improve memory consumption (13.21 KB, patch)
2010-07-08 08:16 PST, Mihnea Ovidenie
ap: review-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-11-09 05:15:54 PST
Created an attachment (id=42748) [details]
bug_memoryLeak.html

Hi,

I use a test page that will continuously consume memory until there's no free memory.
I looked into the test page's source, the javascript code is doing such a process:

1) create an image with "new Image()";
2) set the image.src to one of the 12 test pictures;
3) append the image to document container;
4) continue the loop from 1)

This piece of test code consume more and more memory until there's no free memory.
So, I think, Garbage collector management failed.

Have you an idea ? Bug already exist ?
Exemple of page using in attachement.

Thank's for your help ...

Thank
------- Comment #1 From 2009-11-09 11:59:30 PST -------
I do see unbounded memory growth, and the memory isn't released even after closing the page.

Tested with a local debug build of r50532 on Mac OS X 10.5.8 (so, it's cross-platform).
------- Comment #2 From 2009-11-10 00:31:36 PST -------
(From update of attachment 42748 [details])
<html>
  <head>

   <script language="javascript">

      var containerNode, compteur=0, lastImg, timer, dist=45, number=0;

      function init(){
    var img = new Image();
    img.src = "./resources/"+compteur+".jpg";
    lastImg = img;
    containerNode = document.getElementById("imgContainer");
    containerNode.appendChild(img);
      }

      function start_loop(){
      timer = window.setInterval(function(){
        rLaunch();
      },1);
      }

      function stop_loop(){
      clearInterval(timer);
      }

      function rLaunch(){
    document.getElementById("imgContainer").innerHTML = "";
    lastImg=null;
    dist = dist+1;
    number = number+1;
    var img = new Image();
    compteur = (compteur==12) ? 0 : compteur+1; 
    img.src = "./resources/"+compteur+".jpg?dist="+dist;
    lastImg = img;

    containerNode.appendChild(img);
    document.getElementById("textContainer").innerHTML = "Images loaded : "+ number;
     }

    function clean(){
      document.getElementById("imgContainer").innerHTML = "";
      number = 0;
      clearInterval(timer);

      // add by DFL
    while(containerNode.hasChildNodes()) {
    containerNode.removeChild(containerNode.lastChild) ;
    }

     // Remove element nodes and prevent memory leaks
     jQuery(this).children().remove();
     // Remove any remaining nodes
     while (this.firstChild)
     this.removeChild(this.firstChild);    

     containerNode.clear();
     containerNode.empty();      

     }

  </script>

  <style type="text/css">

    img{
      left:45px;
      top:45px;
      position:absolute;
    }

    .opacity img{
    opacity:0.5;
     }

    div.controls{
      position:absolute;
      bottom:20px;
      left:45px;

    }

    div.textContainer{
      position:absolute;
      top: 150px;
      left: 400px;
    }


  </style>


  </head>
  <body onload="init()">
    <div id="controls" class="controls">
    <a href="#" onclick="javascript:start_loop()">Start_loop</a>
    <a href="#" onclick="javascript:stop_loop()">Stop_loop</a>
    </div>
    <div id="imgContainer"></div>
    <div id="textContainer" class="textContainer"></div>
  </body>
</html>
------- Comment #3 From 2010-07-08 06:33:00 PST -------
Hi,

I have seen the same behavior while trying to add/remove several images into DOM tree. After some time spent in debugging, i have noticed that DocLoader is keeping references to CachedResources in its member m_documentResources. The type of m_documentResources is typedef HashMap<String, CachedResourceHandle<CachedResource> > and it was introduced with changelists 39725 and 39304(bug 17998). When an image is removed from DOM, DocLoader is still keeping a reference to it even when the resource is purged from cache. It is true that when DocLoader is destroyed, the CachedResources from m_documentResources are removed since their reference count is dropping to 0.

Using a similar test as the one already posted in the bug (adding/removing a lot of images) both WebKit nightly and Safari 5 have their memory grow to 1GB on Mac/Win after 1 day of running. Chrome does not have this problem, Firefox 3.6.6 does not have this problem, IE 8 also does not have this problem.

I was wondering if the DocLoader still keeping a reference to all CachedResources could be the cause for this memory growth. I was experimenting a small patch with DocLoader keeping a weak reference to its CachedResources, so that these resources get the chance to be deleted when they are removed from cache. 

I also saw that there is another bug logged to an issue that may be related to this one: https://bugs.webkit.org/show_bug.cgi?id=23372 Repeatedly overwriting img.src can cause memory exhaustion.

Could someone comment on this?

Regards,
Mihnea Ovidenie
------- Comment #4 From 2010-07-08 08:16:50 PST -------
Created an attachment (id=60885) [details]
Simple patch, possible solution to improve memory consumption

Here is a simple patch that implements the idea of DocLoader keeping a weak reference to its CachedResources. When this resources are removed from cache, they are also deleted since DocLoader will not keep a reference to them. Taking into account the complexity of such issue, it would be great if somebody with expertise could take a look and share an opinion about this patch.

Regards,
Mihnea
------- Comment #5 From 2010-07-08 10:20:18 PST -------
(From update of attachment 60885 [details])
I didn't look at the patch, but I assume that it implements what the above comment describes.

The reason for m_documentResources existence is so that we could provide a list of resources to clients via -[WebDataSource subresources], store them in Web archives, and show them in Web Inspector. Thus, removing items just because the resource data is no longer cached is inappropriate.
------- Comment #6 From 2010-07-08 23:22:14 PST -------
(In reply to comment #5)
> (From update of attachment 60885 [details] [details])
> I didn't look at the patch, but I assume that it implements what the above comment describes.
> 
> The reason for m_documentResources existence is so that we could provide a list of resources to clients via -[WebDataSource subresources], store them in Web archives, and show them in Web Inspector. Thus, removing items just because the resource data is no longer cached is inappropriate.

Thanks a lot for your comment.
------- Comment #7 From 2010-11-11 15:48:07 PST -------
See also: bug 39628.
------- Comment #8 From 2010-12-16 16:57:36 PST -------
(In reply to comment #5)
> (From update of attachment 60885 [details] [details])
> I didn't look at the patch, but I assume that it implements what the above comment describes.
> 
> The reason for m_documentResources existence is so that we could provide a list of resources to clients via -[WebDataSource subresources], store them in Web archives, and show them in Web Inspector. Thus, removing items just because the resource data is no longer cached is inappropriate.

I have recently discovered that this bug makes an application I'm building unusable on any modern WebKit-based browser, as it exhausts available memory within minutes. I've yet to find a workaround, so I decided to come see if perhaps I could at least submit a patch that fixes the bug and hope that it makes its way to devices like the iPhone in a reasonable amount of time.

Am I interpreting your statement correctly that you believe that this memory leak is intended behavior and should be preserved simply to keep Web Inspector's current behavior? I understand the desire to have web inspector offer a list of all resources ever loaded by a page, but doing it in this way breaks a large set of web development use cases. This seems an extreme cost to pay merely to get a feature of questionable value within Web Inspector. At the very least, this unbounded memory consumption should only be triggered when resource tracking has been explicitly requested by the user.

Is there a way that this bug can be fixed and still meet your functionality requirements for Web Inspector? If you can describe the requirements for an acceptable fix, I can attempt to create that fix.
------- Comment #9 From 2010-12-16 17:37:00 PST -------
Note that Web Inspector was not the only client that I mentioned as needing access to document resources. Anyway, I don't think that anyone will argue that secondary functionality (like Activity window or web archives) should prevent web content from working.

It may be just fine to remove resources from m_documentResources when all elements using them are known to have been removed from the document, and won't come back from a detached tree. It's not very easy though, as external resources can also be referenced by style rules, so counting only elements isn't sufficient.

There is some surprising code in CachedResourceLoader today. The attached patch implies that CachedResourceLoader holds a strong reference to document resources - but why does this class have a removeCachedResource() method called from CachedResource destructor then?

It's not obvious that all bugs with memory growth when adding and removing images are the same internally. There may be loader problems, but there may also be JavaScript garbage collection problems. Unless you're 100% sure that you are seeing an identical issue, it may be best to file a separate bug - even if it's found to be a duplicate later, that's much better than discussing two issues in one bug.
------- Comment #10 From 2013-08-14 22:43:20 PST -------
The patch doesn't work for our WinCE port which is based on a 2012/8 nearby WebKit.

Test code doesn't add/remove <img> nodes, but just keep changing <img>'s src attribute to load different images.

The debug output says: "LEAK: 85 CachedResource", which is the all image resources.

Also, the newest Chrome 28 and Firefox 23 also both have this problem.
------- Comment #11 From 2013-08-14 23:39:59 PST -------
(In reply to comment #10)
> The patch doesn't work for our WinCE port which is based on a 2012/8 nearby WebKit.
> 
> Test code doesn't add/remove <img> nodes, but just keep changing <img>'s src attribute to load different images.
> 
> The debug output says: "LEAK: 85 CachedResource", which is the all image resources.
> 
> Also, the newest Chrome 28 and Firefox 23 also both have this problem.

Well, you can always propose a new patch/approach if the problem still persist.
------- Comment #12 From 2014-01-19 21:45:53 PST -------
(In reply to comment #11)
> (In reply to comment #10)
> > The patch doesn't work for our WinCE port which is based on a 2012/8 nearby WebKit.
> > 
> > Test code doesn't add/remove <img> nodes, but just keep changing <img>'s src attribute to load different images.
> > 
> > The debug output says: "LEAK: 85 CachedResource", which is the all image resources.
> > 
> > Also, the newest Chrome 28 and Firefox 23 also both have this problem.
> 
> Well, you can always propose a new patch/approach if the problem still persist.

I think http://trac.webkit.org/changeset/92143 fixed some part of this issue.