Bug 39460 – Javascript image prefetching does not report to squirrelfish GC

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Nathan Lawrence]

When an image is preloaded in javascript, the size of the image is never reported to the javascript garbage collector.  If a large image is repeatedly preloaded but never used, it will not free any of the images until the garbage collector has run out of space to allocate any new free javascript variables.

[Nathan Lawrence]

Created attachment 56651 [details]
Suggested patch

Adds a check to see if an image is loaded but is not part of the document.

[WebKit Review Bot]

Attachment 56651 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/manual-tests/image-prefetch-stress.html:6:  Line contains tab character.  [whitespace/tab] [5]
WebCore/manual-tests/image-prefetch-stress.html:7:  Line contains tab character.  [whitespace/tab] [5]
WebCore/manual-tests/image-prefetch-stress.html:8:  Line contains tab character.  [whitespace/tab] [5]
WebCore/manual-tests/image-prefetch-stress.html:9:  Line contains tab character.  [whitespace/tab] [5]
WebCore/manual-tests/image-prefetch-stress.html:10:  Line contains tab character.  [whitespace/tab] [5]
WebCore/manual-tests/image-prefetch-stress.html:14:  Line contains tab character.  [whitespace/tab] [5]
WebCore/manual-tests/image-prefetch-stress.html:15:  Line contains tab character.  [whitespace/tab] [5]
WebCore/manual-tests/image-prefetch-stress.html:16:  Line contains tab character.  [whitespace/tab] [5]
WebCore/manual-tests/image-prefetch-stress.html:17:  Line contains tab character.  [whitespace/tab] [5]
WebCore/manual-tests/image-prefetch-stress.html:18:  Line contains tab character.  [whitespace/tab] [5]
WebCore/manual-tests/image-prefetch-stress.html:19:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 11 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 56651 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2301384

[Nathan Lawrence]

Created attachment 56654 [details]
Updated 56651

Updates style changes and adds ifdefs around JSC specific code.

[Mark Rowe (bdash)]

Comment on attachment 56654 [details]
Updated 56651

Your manual test references a file URL that exists only on your machine.  You should either update the test to use an image that exists in the SVN repository or add a suitable image alongside the test.

[Geoffrey Garen]

> JSC::JSGlobalData *globalData

WebKit style puts the "*" next to the type name, since "pointer" is logically a party of the type.

> +i = 0;
> +setInterval(function(){
> +    x = new Image();

Need 'var' in front of these two variable declarations for "i" and "x".

With this patch, how many images load / how much memory is used before GC triggers?

[Nathan Lawrence]

Created attachment 56749 [details]
more style changes.

re ggaren:
It should garbage collect after maxExtraCost additional memory has been reported to the garbage collector (what appears to be a megabyte).  On my system, with only the manual test running in the browser, the usage doesn't seem to get above 130MB.

[Geoffrey Garen]

Comment on attachment 56749 [details]
more style changes.

I asked Nathan to take another look at this because the patch doesn't seem to fully solve the problem. In his test, about 130MB of image data is still reported as allocated, and the number of live JSHTMLImageElements still increases linearly.

[Nathan Lawrence]

Created attachment 57280 [details]
Fixed test case.

It appears that the number of live HTMLImageElements is still increasing because the image elements are being created faster than they can load creating a backlog of images to be loaded.  The new test case doesn't load images as quickly, showing that the patch does in fact work.

[Geoffrey Garen]

Comment on attachment 57280 [details]
Fixed test case.

r=me

[WebKit Commit Bot]

Comment on attachment 57280 [details]
Fixed test case.

Clearing flags on attachment: 57280

Committed r60337: <http://trac.webkit.org/changeset/60337>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Is a similar fix needed for Audio and Video elements?

[Geoffrey Garen]

(In reply to comment #13)
> Is a similar fix needed for Audio and Video elements?

Do those elements preload while outside the document? If so, yes.

[Geoffrey Garen]

Dan pointed out on IRC that checking just for the <img> tag is probably sufficient, since that's what new Image uses.

[Simon Fraser (smfr)]

(In reply to comment #14)
> Do those elements preload while outside the document? If so, yes.

Indeed they do. I filed bug 39858.

[Nathan Lawrence]

Because not just <img> and <image> elements can preload images, we don't want to restrict based on the element associated with the loader.

[Nathan Lawrence]

Created attachment 57287 [details]
removed <img> and <image> tag checks.

[Geoffrey Garen]

Comment on attachment 57287 [details]
removed <img> and <image> tag checks.

r=me

[WebKit Commit Bot]

Comment on attachment 57287 [details]
removed <img> and <image> tag checks.

Clearing flags on attachment: 57287

Committed r60345: <http://trac.webkit.org/changeset/60345>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.