[Webkit-unassigned] [Bug 154452] Memory leaks when autoLoadImages is off

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=154452

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #272033|review?                     |review-
              Flags|                            |

--- Comment #11 from Darin Adler <darin at apple.com> ---
Comment on attachment 272033
  --> https://bugs.webkit.org/attachment.cgi?id=272033
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160226/fcf74c62/attachment.html>