Bug 31253 - Unbounded memory growth when adding and removing images
Summary: Unbounded memory growth when adding and removing images
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-09 05:15 PST by Flament David
Modified: 2023-10-29 19:54 PDT (History)
20 users (show)

See Also:


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 PDT, Mihnea Ovidenie
ap: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Flament David 2009-11-09 05:15:54 PST
Created attachment 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 Alexey Proskuryakov 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 Flament David 2009-11-10 00:31:36 PST
Comment on attachment 42748 [details]
bug_memoryLeak.html

<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 Mihnea Ovidenie 2010-07-08 06:33:00 PDT
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 Mihnea Ovidenie 2010-07-08 08:16:50 PDT
Created attachment 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 Alexey Proskuryakov 2010-07-08 10:20:18 PDT
Comment on attachment 60885 [details]
Simple patch, possible solution to improve memory consumption

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 Mihnea Ovidenie 2010-07-08 23:22:14 PDT
(In reply to comment #5)
> (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.

Thanks a lot for your comment.
Comment 7 Alexey Proskuryakov 2010-11-11 15:48:07 PST
See also: bug 39628.
Comment 8 Kevin Gadd 2010-12-16 16:57:36 PST
(In reply to comment #5)
> (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.

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 Alexey Proskuryakov 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 Chen Zhixiang 2013-08-14 22:43:20 PDT
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 Mihnea Ovidenie 2013-08-14 23:39:59 PDT
(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 Arunprasad Rajkumar 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.
Comment 13 Makivelli 2015-02-16 23:02:39 PST
Comment on attachment 42748 [details]
bug_memoryLeak.html

><html>
>  <head>
>
>   <script language="javascript">
>
>      var containerNode, compteur=0, lastImg, timer, dist=45, number=0;
>      var activateGfx = false;
>      var activateOpacity = false;
>
>      function init(){
>	var img = new Image();
>	img.src = "./resources/"+compteur+".jpg";
>	lastImg = img;
>	containerNode = document.getElementById("imgContainer");
>	containerNode.appendChild(img);
>      }
>
>      function start(){
>	  timer = window.setInterval(function(){
>	    rLaunch();
>	  },1);
>      }
>
>      function stop(){
>	  clearInterval(timer);
>      }
>
>      function rLaunch(){
>	document.getElementById("imgContainer").innerHTML = "";
>	//lastImg.style.right=dist+"px";
>	lastImg=null;
>	dist = dist+1;
>	number = number+1;
>	//lastImg.style.left="auto";
>	var img = new Image();
>	compteur = (compteur==12) ? 0 : compteur+1; 
>	var str = activateGfx ? "|com.motorola.gfxmem=true":"";
>	img.src = "./resources/"+compteur+".jpg?dist="+dist+str;
>	lastImg = img;
>	
>	containerNode.appendChild(img);
>	var desc = activateOpacity ? " opacity activated" :  "";
>	document.getElementById("textContainer").innerHTML = "NbImages chargees : "+ number + desc;
>     }
>
>    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();	  
>	  
>     }
>
>    function activOpacity(){
>	 activateOpacity = !activateOpacity;
>	  if(activateOpacity) {
>	      document.getElementById("imgContainer").className = "opacity";
>	      document.getElementById("isOpacityActivated").innerHTML = "(on)";		
>	  } else {
>	      document.getElementById("imgContainer").className = "";
>	      document.getElementById("isOpacityActivated").innerHTML = "(off)";		
>	  }
>
>	clean();
>    }
>  </script>
>
>  <style type="text/css">
>
>    img{
>      left:45px;
>      top:45px;
>      position:absolute;
>     /* opacity:0.5;*/
>    }
>
>    .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()">Start</a>
>	<a href="#" onclick="javascript:stop()">Stop</a>
>	<a href="#" onclick="javascript:activOpacity()">Opacity <span id="isOpacityActivated">(Off)</span></a>
>    </div>
>    <div id="imgContainer"></div>
>    <div id="textContainer" class="textContainer"></div>
>  </body>
></html>
Comment 14 Pierre Fritsch 2018-07-19 00:47:17 PDT
For others battling with the memory increase in Safari when changing an image src, there's a work around until this bug is fixed: 

Store the image binary data in a `Blob` object, `createObjectURL` to represent this blob, and assign this URL as the `Image.src`. Finally, `revokeObjectURL` before switching to the next image.

See also https://stackoverflow.com/a/38624675/4769344

Note that this method assumes the usage of Data URIs. If you need to read existing image files, you would have to load the images on some server and to pass them to the browser as data URIs in order to avoid the creation of `Image` objects in WebKit.
Comment 15 Avner 2022-02-15 22:51:24 PST
It seems like this is still a problem.
In my case I read a blob from a local file, by extracting a zip entry from a zip file, without the use of an "Image" object.
A blob is then created from the array, and managed with createObjectURL, and revokeObjectURL.
But memory leak still happens on browsers in iOS, so I assume this is a bug in Webkit.
The problem is described in 
https://stackoverflow.com/questions/71122643/webapp-fails-in-ios-when-using-revokeobjecturl
and
https://developer.apple.com/forums/thread/700639
and an online example that demonstrates the problem is in
https://jsfiddle.net/avnerm/vfgkdenp/18/
Comment 16 Avner 2022-02-16 19:26:29 PST
I simplified the problem (which happens on iOS):
- replaced reading-from-zip with reading-from-external-url
- mimicked the reading of blob by artificially using createObjectURL, and revokeObjectURL.
The problem is described and demonstrated in: https://stackoverflow.com/questions/71151958/memory-leak-in-ios-when-loading-images-from-external-url-and-using-createobjectu