Bug 102775 – Loader: Store WeakRef CachedResourceHandle in CachedResourceLoader

• Component: Page Loading
• Status: UNCONFIRMED

[pdeng6]

Created attachment 175160 [details]
This pic shows expected result, BF4DD16647C553D78FAAED29C99B5174.cache.js available.

How to Reproduce
1, Launch chromium, and load page https://code.google.com/p/chromium/source/search?q=file%3Aeventhandler.cpp
2, after page full loaded, open Developer Tools, click Resources Panel
3, 'BF4DD16647C553D78FAAED29C99B5174.cache.js' is not available in Frames->search->cs_frame->Scripts. while, it's been loaded.

the reason is CachedResource of 'BF4DD16647C553D78FAAED29C99B5174.cache.js' is removed by CachedResourceLoader::garbageCollectDocumentResources before inspector-frontend ask for resources of frame.

attachment is an expected result.

[pdeng6]

Created attachment 175915 [details]
Patch

[Brent Fulgham]

Comment on attachment 175915 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        The root cause of bug 102775: m_documentResources is info pool for Inspector Resource Panel, some CachedResourceHandles are removed from m_documentResources in order to delete strong-refs to CachedResources they hold, so they won't be presented in Resource Panel even they are not actually deleted. In stead of removing from m_documentResources, this change unregisters handles to weak-ref them, and weak-ref handle will be deleted when CachedResource destroyed.

This line should be broken at roughly 80 columns, like the other ChangeLog entries.

[Pavel Feldman]

Comment on attachment 175915 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:719
> +    for (DocumentResourceMap::iterator it = m_documentResources.begin(); it != m_documentResources.end(); ++it)

Typically, we don't apply intrusive changes like that for the sake of inspector. Is is fixing anything other than inspector bug? Have you consulted with the garbageCollectDocumentResources owners? Adding Antti.

[Vsevolod Vlasov]

Comment on attachment 175915 [details]
Patch

I don't think this patch makes sense as currently resource is only removed from m_documentResources when it has one (last) handle. Hence weak-ref'ing will immediately remove the resource anyway.

[pdeng6]

(In reply to comment #4)
> (From update of attachment 175915 [details])
> I don't think this patch makes sense as currently resource is only removed from m_documentResources when it has one (last) handle. Hence weak-ref'ing will immediately remove the resource anyway.

thanks, this patch need improvement.
while, I think the idea is ok as a workaround. if we wek-ref the resource Handle, the resource won't be removed immediately if it is in memcache.(this is the case inspector should represent it in panel I think)
another fix may be: a WeakRefResourceHandle Class to satisfy  m_documentResources in CachedResourceLoader, how do you think of it? :)

[pdeng6]

@japhet

I find following in CachedResource.h
CachedResourceLoader* m_owningCachedResourceLoader; // only non-0 for resources that are not in the cache

the reason of setting m_owningCachedResourceLoader to 0 is preventing duplicate memoryUsage report? or other? thanks a lot.

[pdeng6]

Created attachment 176916 [details]
Patch

[WebKit Review Bot]

Comment on attachment 176916 [details]
Patch

Attachment 176916 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15060236

New failing tests:
http/tests/css/cross-fade-reload.html

[pdeng6]

Created attachment 178189 [details]
Patch

[WebKit Review Bot]

Comment on attachment 178189 [details]
Patch

Attachment 178189 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15191209

New failing tests:
http/tests/cookies/js-get-and-set-http-only-cookie.html

[pdeng6]

Created attachment 179253 [details]
Patch

[Pavel Feldman]

(In reply to comment #0)
> Created an attachment (id=175160) [details]
> This pic shows expected result, BF4DD16647C553D78FAAED29C99B5174.cache.js available.
> 
> How to Reproduce
> 1, Launch chromium, and load page https://code.google.com/p/chromium/source/search?q=file%3Aeventhandler.cpp
> 2, after page full loaded, open Developer Tools, click Resources Panel
> 3, 'BF4DD16647C553D78FAAED29C99B5174.cache.js' is not available in Frames->search->cs_frame->Scripts. while, it's been loaded.

That's working as intended. Inspector does not intend to hold the resources that would otherwise be collected until its front-end is opened. Fixing this will classify as a memory leak.

[Pavel Feldman]

Comment on attachment 179253 [details]
Patch

Clearing r? from the closed bug.

[pdeng6]

(In reply to comment #12)
> (In reply to comment #0)
> > Created an attachment (id=175160) [details] [details]
> > This pic shows expected result, BF4DD16647C553D78FAAED29C99B5174.cache.js available.
> > 
> > How to Reproduce
> > 1, Launch chromium, and load page https://code.google.com/p/chromium/source/search?q=file%3Aeventhandler.cpp
> > 2, after page full loaded, open Developer Tools, click Resources Panel
> > 3, 'BF4DD16647C553D78FAAED29C99B5174.cache.js' is not available in Frames->search->cs_frame->Scripts. while, it's been loaded.
> That's working as intended. Inspector does not intend to hold the resources that would otherwise be collected until its front-end is opened. 
Pavel, thanks for your review :)
Previous workaround: CachedResourceHandle may be removed from CachedResourceLoader even its CachedResource will be still in memcache, the iniatior of gc this is a timer of CachedResourceLoader, not really GC, I thinks it is not easy to understand even resources can be removed after that.
WeakHandle solution: WeakCachedResourceHandle stored in CachedResourceLoader, their resources can be removed by really GC(GCed resource will notify its weakHandles 'die'). when inspector request resources from CachedResourceLoader, WeakCachedResourceHandle will be checked if "died" or not, if it's died, it won't be represented in front-end. that's to say, inspecotr represents 'resource' accroding to the resource live or not. 
any problem?

>Fixing this will classify as a memory leak.
I don't understand this, do you mean WeakCachedResourceHandle is leak?

[Pavel Feldman]

I see. In that case, it should be formulated as a more generic issue. Now I see that your change makes almost no changes to the inspector, but rather changes loaders. I'd suggest that you change the bug title, its component and reach out to loader infrastructure reviewers for the feedback.

[pdeng6]

(In reply to comment #15)
> I see. In that case, it should be formulated as a more generic issue. Now I see that your change makes almost no changes to the inspector, but rather changes loaders. I'd suggest that you change the bug title, its component and reach out to loader infrastructure reviewers for the feedback.
thanks for your suggestion:)

[pdeng6]

Currently, Strong referred CachedResourceHandles are stored in CachedResourceLoader, so corresponding CachedResources won’t be GCed unless the strong handles are removed.

As a workaround, a timer is used in CachedResourceLoader, it will do the GC job, remove CachedResourceHandle which is only-handle one, and try to release its resource, even there is no GC need. This solution did solve memory leak problem, while, generally, I think the initiator of GC should be MemoryCache rather than timer in CachedResourceLoader.

Furthermore, web inspector is a victim. Sometimes resource of a simple page won’t be presented in ResourcePanel, because the timer fired and removed the harmless CachedResourceHandle, although the resource still lives in MemoryCache.

In this patch, WeakCachedResourceHandles are stored in CachedResourceLoader, timer is removed, MemoryCache init the GC, and living resource will be presented in web inspector.

@Antti @japhet, could you please help review this patch? :)

Thanks!
Pan 

[pdeng6]

Created attachment 190715 [details]
Patch

[pdeng6]

Comment on attachment 190715 [details]
Patch

Introduce WeakPtr in WTF to update this patch.

[Adam Barth]

Comment on attachment 190715 [details]
Patch

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

> Source/WTF/wtf/WeakPtr.h:118
> +    // It somewhat breaks the type system, however to allow transfer of ownership
> +    // out of a const WeakPtrFactory is really a need.
> +    WeakPtrFactory<T>(const WeakPtrFactory<T>& factory) : m_ref(factory.leakRef()) { }

I don't think we want to break the type system in this way.

[pdeng6]

Created attachment 190896 [details]
Patch

[pdeng6]

(In reply to comment #20)
> (From update of attachment 190715 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190715&action=review
> 
> > Source/WTF/wtf/WeakPtr.h:118
> > +    // It somewhat breaks the type system, however to allow transfer of ownership
> > +    // out of a const WeakPtrFactory is really a need.
> > +    WeakPtrFactory<T>(const WeakPtrFactory<T>& factory) : m_ref(factory.leakRef()) { }
> 
> I don't think we want to break the type system in this way.

Thanks, a "transfer" method added to avoid this. how do you think of this? :)

Pan

[Adam Barth]

Comment on attachment 190896 [details]
Patch

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

This patch makes the lifetime of CachedResources too complicated.

WeakPtr is a new concept in WebCore.  One of the risks of introducing WeakPtr.h into the project is that folks will use it to build complex ownership relationships.  We avoided adding it for a long to for just that reason.  Now that we have it, we're planning to use it sparingly, at first only for cross-thread communication where RefPtr and OwnPtr are either inappropriate or lead to worse problems.

Overall, this patch feels like a band-aid that layers on more complexity instead of rationalizing the ownership semantics of CachedResources.

> Source/WTF/wtf/WeakPtr.h:72
> +    void reset(T* ptr)
> +    {
> +#ifndef NDEBUG
> +        m_boundThread = currentThread();
> +#endif
> +        m_ptr = ptr;
> +    }

If we're going to write m_ptr, we need to assert that we're on the bound thread.

> Source/WTF/wtf/WeakPtr.h:120
> +        m_ref = WeakReference<T>::create(0);

ditto

[pdeng6]

Created attachment 191417 [details]
Patch

[pdeng6]

(In reply to comment #23)
> (From update of attachment 190896 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190896&action=review
> 
> This patch makes the lifetime of CachedResources too complicated.
> 
> WeakPtr is a new concept in WebCore.  One of the risks of introducing WeakPtr.h into the project is that folks will use it to build complex ownership relationships.  We avoided adding it for a long to for just that reason.  Now that we have it, we're planning to use it sparingly, at first only for cross-thread communication where RefPtr and OwnPtr are either inappropriate or lead to worse problems.
> 
> Overall, this patch feels like a band-aid that layers on more complexity instead of rationalizing the ownership semantics of CachedResources.

Thanks for your comments. This time, CachedResourceLoader is notified by SubresourceLoader to reset weakPtr when 304 comes, then ownership of WeakFactory and lifetime of CachedResource won't be hurt. :)

Pan

[Adam Barth]

Comment on attachment 191417 [details]
Patch

I don't think we should use WeakPtr in the loader yet.  The loader's lifetime management is already very complex and error-prone.  We don't have enough experience with WeakPtr as a project yet to make good decisions about how to use WeakPtr in such complex environments.