[webkit-reviews] review denied: [Bug 61006] REGRESSION (r39725?): Resources removed from document can not be freed until the document is deleted : [Attachment 101045] run a garbage collection on m_documentResources when a load finishes

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Scott Graham
<scottmg at chromium.org>'s request for review:
Bug 61006: REGRESSION (r39725?): Resources removed from document can not be
freed until the document is deleted
https://bugs.webkit.org/show_bug.cgi?id=61006

Attachment 101045: run a garbage collection on m_documentResources when a load
finishes
https://bugs.webkit.org/attachment.cgi?id=101045&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=101045&action=review


> Source/WebCore/loader/cache/CachedResource.h:185
>      bool canDelete() const { return !hasClients() && !m_request &&
!m_preloadCount && !m_handleCount && !m_resourceToRevalidate &&
!m_proxyResource; }
> +    unsigned getHandleCount() const { return m_handleCount; }

get* naming is not according to coding style. Please replace this with
something like hasOneHandle(), no need to expose the count.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:565
> +    garbageCollectDocumentResources();

This should start a timer (zero duration or more) so we don't get to O(n^2).

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:577
> +    for (DocumentResourceMap::iterator it = m_documentResources.begin(); it
!= m_documentResources.end(); ++it) {
> +	   if (it->second->getHandleCount() == 1) {
> +	       m_documentResources.remove(it);
> +	       // Iterators invalidated. We don't want to copy keys, because
they
> +	       // maybe be very large 'data:...'. Rather than restart
traversal,
> +	       // simply only remove the first free-able resource, and get
> +	       // another next time.
> +	       return;

This is unnecessary. Map keys here are Strings which are really just pointers
to StringImpls. Copying a String is just a pointer copy. You should garbage
collect the entire map.