Bug 154452 – Memory leaks when autoLoadImages is off

• Status: NEW
• Component: WebCore Misc.

[Marc Epard]

When autoLoadImages is off, an HTMLImageElement (along with a memory cache entry) leaks for each image. 

I first noticed continuously increasing memory consumption using phantomjs to visit lots of webpages with autoLoadImages=false. phantomjs uses an older version of WebKit and I've implemented a fix and test case to verify the problem and the fix there.

I also have a fix for current WebKit and will post it shortly. Unfortunately, I don't see an easy way to craft a WebKit test to reproduce this. This lack of testing support was also mentioned in https://bugs.webkit.org/show_bug.cgi?id=124411#c5

To verify that the leaks still occur, I hacked the MiniBrowser to set autoLoadImages to true. The leaks are reported at the end of execution when compiled --debug. I've also set breakpoints in the HTMLImageElement and ImageLoader destructors and observed that they don't get called when autoLoadImages is true and do get called when autoLoadImages is false.

The cause of the problem is that the ImageLoader protects the HTMLImageElement from deletion while a load event is pending, but when autoLoadImages is false, no load event will happen. The protection is implemented by incrementing the reference count of the HTMLImageElement. The reference count was manipulated explicitly, but previously explicitly, now by keeping a RefPtr<Element> (see https://bugs.webkit.org/show_bug.cgi?id=146538). 

The leak occurs because of a circular reference. The ImageLoader's destructor removes the reference, but the destructor can't run because HTMLImageElement contains the ImageLoader.

Two closed bugs have changesets that attempt to stop the leaks:

https://bugs.webkit.org/show_bug.cgi?id=124411 and http://trac.webkit.org/changeset/171036 are specifically about autoLoadImages. This patch may have been sufficient at the time, but is not now.

https://bugs.webkit.org/show_bug.cgi?id=146538 and http://trac.webkit.org/changeset/186267 don't mention autoLoadImages, but are about ImageLoader leaks. This change converted the explicit reference count manipulation to implicit, but didn't change the semantics of the circular reference.

[Marc Epard]

Created attachment 271768 [details]
Patch

[Build Bot]

Comment on attachment 271768 [details]
Patch

Attachment 271768 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/855644

New failing tests:
http/tests/cache/display-image-unset-allows-cached-image-load.html
http/tests/misc/window-open-then-write.html
fast/loader/display-image-unset-can-block-image-and-can-reload-in-place.html

[Build Bot]

Created attachment 271781 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 271768 [details]
Patch

Attachment 271768 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/855656

New failing tests:
http/tests/cache/display-image-unset-allows-cached-image-load.html
fast/loader/display-image-unset-can-block-image-and-can-reload-in-place.html

[Build Bot]

Created attachment 271784 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 271768 [details]
Patch

Attachment 271768 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/855640

New failing tests:
http/tests/cache/display-image-unset-allows-cached-image-load.html
http/tests/misc/window-open-then-write.html
fast/loader/display-image-unset-can-block-image-and-can-reload-in-place.html

[Build Bot]

Created attachment 271786 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Brady Eidson]

Comment on attachment 271768 [details]
Patch

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

r- because of layout test regressions.

> Source/WebCore/ChangeLog:17
> +        Unable to test.

But good thing existing tests caught regressions! :)

[Marc Epard]

Created attachment 272033 [details]
Patch

[Marc Epard]

> But good thing existing tests caught regressions! :)

Indeed! The 2nd take looks better.

[Darin Adler]

Comment on attachment 272033 [details]
Patch

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

Thanks for tackling this.

By checking autoLoadImages() directly, I think this creates code that is quite fragile, relying on effects that are far away from the code making the decision. We should seek a revised solution that doesn’t require ImageLoader to separately second guess decisions that CachedResourceLoader will make about doing image loads.

We also need to make a regression test for this or explain why creating one was impossible. A subtle bug like this needs a test even more than a more straightforard one.

> Source/WebCore/loader/ImageLoader.cpp:94
> +    , m_postponeLoadImage(false)

We generally try to name boolean data members with predicates that fit the phrase "loader <xxx>".

In particular we try to avoid verb phrases, since booleans are not themselves actions. This would lead to a name like "is postponing image load", which would be m_isPostponingImageLoad.

> Source/WebCore/loader/ImageLoader.cpp:238
> +        // Remember if we're (auto) loading the image right away. See updatedHasPendingEvent().

This comment is confusing to anyone who doesn’t already know what the code should do. I suggest a different comment.

> Source/WebCore/loader/ImageLoader.cpp:239
> +        m_postponeLoadImage = !document.cachedResourceLoader().autoLoadImages();

Checking autoLoadImages seems likely to give an incorrect result for data URLs. Can we change this code so it relies on on the result of CachedResourceLoader::shouldPerformImageLoad or CachedResourceLoader::shouldDeferImageLoad or something along those lines? If not, then this code is going to give a bad result.

> Source/WebCore/loader/ImageLoader.cpp:373
> +    // https://bugs.webkit.org/show_bug.cgi?id=17469 (maybe)
> +    // https://bugs.webkit.org/show_bug.cgi?id=146538
> +    // https://bugreports.qt.io/browse/QTBUG-38857
> +    // https://github.com/ariya/phantomjs/issues/12903

This list of URLs is not helpful. I don’t think we need to mention even one of these, since we normally rely on the change log for that, but if we absolutely feel the need to mention a specific bug report, we should point to just one of these, the one from the WebKit project, not from other projects, and it, in turn, can point to all the others.