Make pruning the memory cache when a resource becomes dead consistent in case of no-store

Make pruning the memory cache when a resource becomes dead consistent in case of no-store

Hard to find any reason why it was written the way it was. We just need
to make sure not use-after-free the Resource object but otherwise it is
fine pruning the cache after a no-store resource becomes dead.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167454

[kbalazs, 2014-02-18 21:29:23]

ptal

[kbalazs, 2014-02-18 21:32:18]

About testing, maybe something can be added to MemoryCacheTest but honestly I'm
not sure it's worth it.

[Nate Chapin, 2014-02-18 21:35:54]

https://codereview.chromium.org/171303003/diff/1/Source/core/fetch/Resource.cpp
File Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/171303003/diff/1/Source/core/fetch/Resource.c...
Source/core/fetch/Resource.cpp:517: memoryCache()->prune();
I think it's safe to call prune(this) after removing, which would allow us to
remove the else{} condition.

Alternately, maybe we could prune(this), then check the no-store case only if
this is still in the cache?

[kbalazs, 2014-02-18 22:27:09]

On 2014/02/18 21:35:54, Nate Chapin wrote:
>
https://codereview.chromium.org/171303003/diff/1/Source/core/fetch/Resource.cpp
> File Source/core/fetch/Resource.cpp (right):
> 
>
https://codereview.chromium.org/171303003/diff/1/Source/core/fetch/Resource.c...
> Source/core/fetch/Resource.cpp:517: memoryCache()->prune();
> I think it's safe to call prune(this) after removing, which would allow us to
> remove the else{} condition.
> 
> Alternately, maybe we could prune(this), then check the no-store case only if
> this is still in the cache?

I don't think we can do either of that. Both remove(this) and prune(this) can
call evict which in turn calls deleteIfPossible() so doing one after the other
(no matter the order) is a potential use-after-free. Only remove(this) followed
by prune() is safe as there is no chance the prune will touch the object if it's
deleted.

[Nate Chapin, 2014-02-18 22:29:35]

Ok, LGTM

I was thinking that remove() had a bool return value indicating whether the
removed element was also deleted. It doesn't, and I don't think this is a good
enough reason to add it.

[kbalazs, 2014-02-18 23:40:41]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-02-18 23:41:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/171303003/1

[commit-bot: I haz the power, 2014-02-19 02:23:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-19 02:23:04]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[kbalazs, 2014-02-19 17:47:12]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-02-19 21:49:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/171303003/1

[commit-bot: I haz the power, 2014-02-19 22:54:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/171303003/1

[commit-bot: I haz the power, 2014-02-20 06:54:32]

Change committed as 167454