Bug 82287 – MemoryCache should adopt our standard RefCounted model for object lifetime

• Component: Layout and Rendering
• Status: UNCONFIRMED

[GaoXiong]

CachedResource doesn't support reference counting in the usual model.  It uses a somewhat confusing set of rules to decide when to delete itself. See canDelete() in CachedResource.h. It's better to figure out a way to make CachedResource use our normal reference counting model.

[Brady Eidson]

The title "There may be crashes on memory cache" does not accurately reflect what this bug is tracking.

If you are aware of any specific crashes related to the memory cache, each such crash should be filed as an individual bug.

It's long been a goal of various folks within the WebKit project to rewrite the memory cache to - amongst other things - adopt standard RefCounting.  So I would be surprised if we didn't already have a bug for this.

Changing title from "There may be crashes on memory cache" to "MemoryCache should adopt our standard RefCounted model for object lifetime" and CC'ing a few of those folks.

[Brady Eidson]

The people I was going to CC were already CC'ed.  Yay!

[Nate Chapin]

(In reply to comment #1)
> The title "There may be crashes on memory cache" does not accurately reflect what this bug is tracking.
> 
> If you are aware of any specific crashes related to the memory cache, each such crash should be filed as an individual bug.
> 
> It's long been a goal of various folks within the WebKit project to rewrite the memory cache to - amongst other things - adopt standard RefCounting.  So I would be surprised if we didn't already have a bug for this.
> 
> Changing title from "There may be crashes on memory cache" to "MemoryCache should adopt our standard RefCounted model for object lifetime" and CC'ing a few of those folks.

A few months ago I spent a couple hours writing a first draft of RefCounted CachedResources and it failed surprisingly few tests. That may be a function of our testing coverage in the loader, though. I'll try to get that patch up to trunk.

[Brady Eidson]

(In reply to comment #3)
> (In reply to comment #1)
> > 
> > Changing title from "There may be crashes on memory cache" to "MemoryCache should adopt our standard RefCounted model for object lifetime" and CC'ing a few of those folks.
> 
> A few months ago I spent a couple hours writing a first draft of RefCounted CachedResources and it failed surprisingly few tests. That may be a function of our testing coverage in the loader, though. I'll try to get that patch up to trunk.

I strongly suspect our tests aren't nearly enough to cover such a change with confidence without tons and tons of additional expert review and/or adhoc testing.

Antti would really need to take a look at such a patch.

[Nate Chapin]

Created attachment 143354 [details]
Attempt at RefCounted<CachedResource>

NOTE: This patch passes layout tests for me, but I have not yet done any testing beyond that.  It very well may have glaring deficiencies.

Also, I made no attempt to create a standard as when to use a RefPtr and when to use a CachedResourceHandle for a CachedResource.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/loader/cache/CachedResourceHandle.h:47:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 1 in 31 files


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

[Early Warning System Bot]

Comment on attachment 143354 [details]
Attempt at RefCounted<CachedResource>

Attachment 143354 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12748543

[Early Warning System Bot]

Comment on attachment 143354 [details]
Attempt at RefCounted<CachedResource>

Attachment 143354 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12748542

[Eric Seidel (no email)]

Comment on attachment 143354 [details]
Attempt at RefCounted<CachedResource>

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

I'm excited about this change, but unclear how this is going to work for clients?  Previously I thought clients were the only way that resources stayed alive.  Now I guess clients will hold a ref to the resource?  And the resource will keep weak pointers to the clients?

> Source/WebCore/loader/ImageLoader.cpp:116
> +    RefPtr<CachedImage> oldImage = m_image.get();

You could do .release() and unconditionally set m_image from newImage here?

[WebKit Review Bot]

Comment on attachment 143354 [details]
Attempt at RefCounted<CachedResource>

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

[Nate Chapin]

(In reply to comment #9)
> (From update of attachment 143354 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143354&action=review
> 
> I'm excited about this change, but unclear how this is going to work for clients?  Previously I thought clients were the only way that resources stayed alive.  Now I guess clients will hold a ref to the resource?  And the resource will keep weak pointers to the clients?

The existing model is that there is a long list of things that can keep a CachedResource alive:

bool canDelete() const { return !hasClients() && !m_loader && !m_preloadCount && !m_handleCount && !m_resourceToRevalidate && !m_proxyResource; }

The general strategy of my patch was to represent each of these clauses with a RefPtr somewhere.

Traditionally, clients kept a resource alive both by registering as a client and by holding a CachedResourceHandle (which increments m_handleCount). The model is now that CachedResourceClients are expected to hold a CachedResourceHandle, which in turn holds RefPtr to the CachedResource. I believe the CachedResourceHandle is still necessary for the time being, as it is a core part of 304 handling.



> 
> > Source/WebCore/loader/ImageLoader.cpp:116
> > +    RefPtr<CachedImage> oldImage = m_image.get();
> 
> You could do .release() and unconditionally set m_image from newImage here?

I'll look at it more, I had attempted to make this patch as un-clever as possible for the first pass.

[Eric Seidel (no email)]

I see.  So basically just replacing the m_handleCount with a real m_refCount.  The question to me is what happens when things hold onto a CachedResource w/o a full-blown handle.  Will things get confused.

If you could upload a patch which fixes the EWS I'm happy to take a second, longer look.

[Nate Chapin]

(In reply to comment #12)
> I see.  So basically just replacing the m_handleCount with a real m_refCount.  The question to me is what happens when things hold onto a CachedResource w/o a full-blown handle.  Will things get confused.

I think the rule for now is "If you aren't SubresourceLoader and you don't live in WebCore/lodaer/cache/, you need a CachedResourceHandle instead of a RefPtr for a class level variable".

I'm pondering how to do away with CachedResourceHandle entirely, but I think that's something to deal with after this bug.

> 
> If you could upload a patch which fixes the EWS I'm happy to take a second, longer look.

Will do.

[Nate Chapin]

Created attachment 143386 [details]
Make all the bots happy

[Eric Seidel (no email)]

Comment on attachment 143386 [details]
Make all the bots happy

LGTM.  Please add some explanation to the top of CachedResource.h as well as CachedResourceHandle.h to explain when one is supposed to use a CachedResourceHandle, vs. a RefPtr.  You can do that in a separate patch, but I believe it's important to document this distinction in the code now that it's possible to confuse these two.

[Brady Eidson]

(In reply to comment #15)
> (From update of attachment 143386 [details])
> LGTM.  Please add some explanation to the top of CachedResource.h as well as CachedResourceHandle.h to explain when one is supposed to use a CachedResourceHandle, vs. a RefPtr.  You can do that in a separate patch, but I believe it's important to document this distinction in the code now that it's possible to confuse these two.

If this is a concern, then why are we doing this bizarre halfway step?

Understanding the MemoryCache is already difficult enough.  Making it even more confusing (but relying on comments for clarity) doesn't seem worthwhile.

Why can't we replace all uses of CachedResourceHandle with RefPtrs, or go 100% CachedResourceHandle with no raw pointers?

Wouldn't either of those be better than "confusing"?

(full disclosure: have not looked at the patch, have just been paying attention to the worrisome comments)

[Nate Chapin]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 143386 [details] [details])
> > LGTM.  Please add some explanation to the top of CachedResource.h as well as CachedResourceHandle.h to explain when one is supposed to use a CachedResourceHandle, vs. a RefPtr.  You can do that in a separate patch, but I believe it's important to document this distinction in the code now that it's possible to confuse these two.
> 
> If this is a concern, then why are we doing this bizarre halfway step?
> 
> Understanding the MemoryCache is already difficult enough.  Making it even more confusing (but relying on comments for clarity) doesn't seem worthwhile.
> 
> Why can't we replace all uses of CachedResourceHandle with RefPtrs, or go 100% CachedResourceHandle with no raw pointers?
> 
> Wouldn't either of those be better than "confusing"?
> 
> (full disclosure: have not looked at the patch, have just been paying attention to the worrisome comments)

Seems worthwhile to remove cq+ for the moment.

As mentioned above, I'd be inclined to replace CachedResourceHandle with RefPtr, but I need to figure the right interactions between CachedResource and its clients in CachedResource::switchClientsToRevalidatedResource(). At the very least, I should probably include a FIXME to that effect when submitting this.

[Nate Chapin]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (From update of attachment 143386 [details] [details] [details])
> > > LGTM.  Please add some explanation to the top of CachedResource.h as well as CachedResourceHandle.h to explain when one is supposed to use a CachedResourceHandle, vs. a RefPtr.  You can do that in a separate patch, but I believe it's important to document this distinction in the code now that it's possible to confuse these two.
> > 
> > If this is a concern, then why are we doing this bizarre halfway step?
> > 
> > Understanding the MemoryCache is already difficult enough.  Making it even more confusing (but relying on comments for clarity) doesn't seem worthwhile.
> > 
> > Why can't we replace all uses of CachedResourceHandle with RefPtrs, or go 100% CachedResourceHandle with no raw pointers?
> > 
> > Wouldn't either of those be better than "confusing"?
> > 
> > (full disclosure: have not looked at the patch, have just been paying attention to the worrisome comments)
> 
> Seems worthwhile to remove cq+ for the moment.
> 
> As mentioned above, I'd be inclined to replace CachedResourceHandle with RefPtr, but I need to figure the right interactions between CachedResource and its clients in CachedResource::switchClientsToRevalidatedResource(). At the very least, I should probably include a FIXME to that effect when submitting this.

It appears to be pretty complicated to do away with CachedResourceHandle entirely (at least, not something I could throw together in a day). I'm inclined to set a rule of: RefPtrs are an implementation detail of the cache and parts of the loader below it (namely SubresourceLoader), CachedResourceHandles are the public tool for keeping CachedResources alive. Is that clear/simple enough that it might work in practice?

[Brady Eidson]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (In reply to comment #15)
> > > > (From update of attachment 143386 [details] [details] [details] [details])
> > > > LGTM.  Please add some explanation to the top of CachedResource.h as well as CachedResourceHandle.h to explain when one is supposed to use a CachedResourceHandle, vs. a RefPtr.  You can do that in a separate patch, but I believe it's important to document this distinction in the code now that it's possible to confuse these two.
> > > 
> > > If this is a concern, then why are we doing this bizarre halfway step?
> > > 
> > > Understanding the MemoryCache is already difficult enough.  Making it even more confusing (but relying on comments for clarity) doesn't seem worthwhile.
> > > 
> > > Why can't we replace all uses of CachedResourceHandle with RefPtrs, or go 100% CachedResourceHandle with no raw pointers?
> > > 
> > > Wouldn't either of those be better than "confusing"?
> > > 
> > > (full disclosure: have not looked at the patch, have just been paying attention to the worrisome comments)
> > 
> > Seems worthwhile to remove cq+ for the moment.
> > 
> > As mentioned above, I'd be inclined to replace CachedResourceHandle with RefPtr, but I need to figure the right interactions between CachedResource and its clients in CachedResource::switchClientsToRevalidatedResource(). At the very least, I should probably include a FIXME to that effect when submitting this.
> 
> It appears to be pretty complicated to do away with CachedResourceHandle entirely (at least, not something I could throw together in a day). I'm inclined to set a rule of: RefPtrs are an implementation detail of the cache and parts of the loader below it (namely SubresourceLoader), CachedResourceHandles are the public tool for keeping CachedResources alive. Is that clear/simple enough that it might work in practice?

It seems reasonable.  If completely replacing CachedResourceHandle really is a much more monumental task, then it's fine to take the intermediate step.

But it really would be great to get rid of it altogether sooner rather than later.

Don't let my principled objections hold this up any longer.  :)

[Antti Koivisto]

Comment on attachment 143386 [details]
Make all the bots happy

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

CachedResourceHandles are needed because the pointed CachedResource may change. When revalidating resources (using HTTP If-modified-since/If-none-match headers) we create a CachedResource for the validation request. If the validation fails those resources continue loading as normal. If it succeeds we switch all registered CachedResourceHandles to point to the existing CachedResource and throw the validation resource away.

A better way to do this would be to split the memory cache into data storage backend and front end resource parts. Then we could just switch the validated resource to point to the existing data (which would be shared) and CachedResourceHandles could become regular RefPtrs.

> Source/WebCore/loader/cache/CachedResourceHandle.h:77
>      template <class R> class CachedResourceHandle : public CachedResourceHandleBase {
>      public: 
>          CachedResourceHandle() { }
>          CachedResourceHandle(R* res) : CachedResourceHandleBase(res) { }
> +        CachedResourceHandle(PassRefPtr<R> res) : CachedResourceHandleBase(res) { }
>          CachedResourceHandle(const CachedResourceHandle<R>& o) : CachedResourceHandleBase(o) { }
>          template<typename U> CachedResourceHandle(const CachedResourceHandle<U>& o) : CachedResourceHandleBase(o.get()) { }
>  
>          R* get() const { return reinterpret_cast<R*>(CachedResourceHandleBase::get()); }
>          R* operator->() const { return get(); }
> -               
> +
> +        CachedResourceHandle& operator=(PassRefPtr<R> res) { setResource(res); return *this; } 
>          CachedResourceHandle& operator=(R* res) { setResource(res); return *this; } 

As long as we have CachedResourceHandle it would be safer to have a special type for passing around CachedResources. Basically we should have PassCachedResource that acts exactly like PassRefPtr but is only compatible with CachedResourceHandle constructor and operator=.

[Antti Koivisto]

or maybe PassCachedResourceHandle<>

[Antti Koivisto]

This patch is improvement over the current state though so feel free to land if you feel it is not worth doing.

[Nate Chapin]

Created attachment 144086 [details]
Add PassCachedResourceHandle

This patch required some extra shifting around (namely, I moved a bunch of CachedResourceHandleBase into CachedResourceHandle.cpp because of #include cycles).

I mostly tried to piggyback off of RefPtr and PassRefPtr, but I don't know this stuff well enough to be confident that I got it right and avoided refcount churn.

[Early Warning System Bot]

Comment on attachment 144086 [details]
Add PassCachedResourceHandle

Attachment 144086 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12808426

[Build Bot]

Comment on attachment 144086 [details]
Add PassCachedResourceHandle

Attachment 144086 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12800553

[Early Warning System Bot]

Comment on attachment 144086 [details]
Add PassCachedResourceHandle

Attachment 144086 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12808427

[WebKit Review Bot]

Comment on attachment 144086 [details]
Add PassCachedResourceHandle

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

[Build Bot]

Comment on attachment 144086 [details]
Add PassCachedResourceHandle

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

[Nate Chapin]

Created attachment 144100 [details]
PassCachedResourceHandle - try to fix compiles

[Gustavo Noronha (kov)]

Comment on attachment 144100 [details]
PassCachedResourceHandle - try to fix compiles

Attachment 144100 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12808444

[Early Warning System Bot]

Comment on attachment 144100 [details]
PassCachedResourceHandle - try to fix compiles

Attachment 144100 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12799597

[Early Warning System Bot]

Comment on attachment 144100 [details]
PassCachedResourceHandle - try to fix compiles

Attachment 144100 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12793529

[Build Bot]

Comment on attachment 144100 [details]
PassCachedResourceHandle - try to fix compiles

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

[Build Bot]

Comment on attachment 144100 [details]
PassCachedResourceHandle - try to fix compiles

Attachment 144100 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12794560

[Nate Chapin]

Created attachment 144133 [details]
PassCachedResourceHandle - trying again

[Eric Seidel (no email)]

Comment on attachment 144133 [details]
PassCachedResourceHandle - trying again

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

I really like the change, but I think the PassCachedResourceHandle bits is really a wholy separate change from the RefCounted bits.  I would have done the PassCachedResourceHande bit first, and then teh RefCounted part, were I writing this.  I'm happy to discuss this change at greater length with you over IRC or VC.  I may need a VC walkthrough to review the whole thing at once like this.

> Source/WebKit/mac/WebView/WebHTMLView.mm:535
> -    WebCore::CachedImage* promisedDragTIFFDataSource;
> +    WebCore::CachedResourceHandle<WebCore::CachedImage> promisedDragTIFFDataSource;

Would be nice to document the reasoning in the ChangeLog.

> Source/WebCore/ChangeLog:8
> +        No new tests, refactor only.

:(  Would be nice to explain the change in the ChangeLog.

> Source/WebCore/loader/cache/CachedResourceHandle.h:39
> +// CachedResourceHandle and PassCachedResourceHandle are like RefPtr and PassRefPtr, but specifically for CachedResources.
> +// If you are a CachedResourceClient, hold a CachedResourceHandle instead of a RefPtr, since a CachedResourceHandle will have its
> +// CachedResource pointer updated to the correct resource in the event of a 304, while a RefPtr will not.
> +// FIXME: It would be awesome if we could get rid of CachedResourceHandle and solve 304 handling with a different mechanism,
> +// so CachedResourceClients can just hold normal RefPtrs.`

Fantastic, thank you!

> Source/WebCore/loader/cache/CachedResourceHandle.h:46
> +    PassRefPtr<R> get() const { return m_resource; }

get() normally gives you a raw pointer.

> Source/WebCore/loader/cache/CachedResourceHandle.h:79
> +template <class R> class CachedResourceHandle : public CachedResourceHandleBase {

R?

> Source/WebCore/loader/cache/CachedResourceHandle.h:100
> +template <class R, class RR> bool operator==(const CachedResourceHandle<R>& h, const RR* res) 

I think we could have more descriptive names than R/RR?

[Nate Chapin]

Created attachment 145888 [details]
Another potential solution - add a CachedResourceClient callback for 304s

I'm not thrilled with my previous two solutions, so I put together a patch using the strategy of solving the 304 problem by adding a mandatory function, switchClientsToRevalidatedResource(), to CachedResourceClient.  I'm not totally sure that this is cleaner than the previous strategies, but I think it's pretty readable.

This patch doesn't actually include the final goal of making CachedResource RefCounted, it's just re-plumbing.

[WebKit Review Bot]

Comment on attachment 145888 [details]
Another potential solution - add a CachedResourceClient callback for 304s

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

[Eric Seidel (no email)]

Comment on attachment 145888 [details]
Another potential solution - add a CachedResourceClient callback for 304s

Eh.  I think I prefer the handle approach which makes 304's transparent.  This makes it easy for someone to forget to implement this callback.  (I don't think CachedResourceHandle does a very good job of making its purpose explicit, but it seems to do a decent job of abstracting the trouble of 304s. :)

[Eric Seidel (no email)]

Comment on attachment 144133 [details]
PassCachedResourceHandle - trying again

I'm ready to move forward on this approach.  Can you ping me over #webkit today and we'll sync up briefly before final review (today)?

[Eric Seidel (no email)]

Attachment 143386 [details] was posted by a committer and has review+, assigning to Nate Chapin for commit.

[Sangho Kim]

I prefer https://bug-82287-attachments.webkit.org/attachment.cgi?id=145888 solution. Because another patches was crashed when so many images is regenerated  by scrolling like google map.