MemoryCache: make sure that Resources are evicted only when they can be deleted

MemoryCache: make sure that Resources are evicted only when they can be deleted

This CL fixes a problem where Resources can be taken out of the cache but not
deleted (if their handle count is not 0 for example).

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

[clamy, 2014-02-25 16:02:16]

@pliard, japhet: PTAL. This makes sure that we only call evict on dead resources
when it will succeed. Otherwise the resource is taken out of the cache, but no
memory is actually regained.

[Philippe, 2014-02-25 17:11:10]

lgtm, thanks Camille and great catch!

[Philippe, 2014-03-10 14:48:01]

On 2014/02/25 17:11:10, Philippe wrote:
> lgtm, thanks Camille and great catch!

Nate: ping :)

[Nate Chapin, 2014-03-10 16:40:21]

LGTM

Sorry for the delay, I forgot this needed OWNERS.

[clamy, 2014-03-10 16:41:02]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2014-03-10 17:11:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clamy@chromium.org/179943002/30001

[commit-bot: I haz the power, 2014-03-10 17:28:11]

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

[commit-bot: I haz the power, 2014-03-10 17:28:12]

Try jobs failed on following builders: mac_blink_rel, win_blink_rel

[clamy, 2014-03-11 15:59:05]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2014-03-11 15:59:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clamy@chromium.org/179943002/30001

[commit-bot: I haz the power, 2014-03-11 15:59:26]

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

[commit-bot: I haz the power, 2014-03-11 15:59:27]

Failed to apply patch for Source/core/fetch/MemoryCache.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/fetch/MemoryCache.cpp
  Hunk #1 FAILED at 218.
  Hunk #2 FAILED at 260.
  2 out of 2 hunks FAILED -- saving rejects to file
Source/core/fetch/MemoryCache.cpp.rej

Patch:       Source/core/fetch/MemoryCache.cpp
Index: Source/core/fetch/MemoryCache.cpp
diff --git a/Source/core/fetch/MemoryCache.cpp
b/Source/core/fetch/MemoryCache.cpp
index
489d62b398bf6a1ebd8b57a01089b38f74ad6c42..5e12ba3009133a4ac586e9a5276fddf615e8f27d
100644
--- a/Source/core/fetch/MemoryCache.cpp
+++ b/Source/core/fetch/MemoryCache.cpp
@@ -218,10 +218,11 @@ void MemoryCache::pruneDeadResources()
         Resource* current = m_allResources[i].m_tail;
         while (current) {
             Resource* prev = current->m_prevInAllResourcesList;
-            if (current->wasPurged()) {
+            if (current->wasPurged() && current->canDelete()) {
                 ASSERT(!current->hasClients());
                 ASSERT(!current->isPreloaded());
-                evict(current);
+                bool wasEvicted = evict(current);
+                ASSERT_UNUSED(wasEvicted, wasEvicted);
             }
             current = prev;
         }
@@ -260,8 +261,10 @@ void MemoryCache::pruneDeadResources()
         while (current) {
             ResourcePtr<Resource> previous = current->m_prevInAllResourcesList;
             ASSERT(!previous || previous->inCache());
-            if (!current->hasClients() && !current->isPreloaded() &&
!current->isCacheValidator()) {
-                evict(current);
+            if (!current->hasClients() && !current->isPreloaded()
+                && !current->isCacheValidator() && current->canDelete()) {
+                bool wasEvicted = evict(current);
+                ASSERT_UNUSED(wasEvicted, wasEvicted);
                 if (targetSize && m_deadSize <= targetSize)
                     return;
             }

[clamy, 2014-03-13 15:05:54]

@eustas: PTAL at the change in the layout test. With this change, the script is
not evicted from the MemoryCache (since it has dangling handlers). So on reload,
it is served from the cache and not from the network, and no fallback content is
actually requested.

[Philippe, 2014-03-14 09:30:25]

@eustas: ping :)


On Thu, Mar 13, 2014 at 4:05 PM, <clamy@chromium.org> wrote:

> @eustas: PTAL at the change in the layout test. With this change, the
> script is
> not evicted from the MemoryCache (since it has dangling handlers). So on
> reload,
> it is served from the cache and not from the network, and no fallback
> content is
> actually requested.
>
> https://chromiumcodereview.appspot.com/179943002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[eustas, 2014-03-14 09:48:57]

On 2014/03/14 09:30:25, Philippe wrote:
> @eustas: ping :)
> 
> 
> On Thu, Mar 13, 2014 at 4:05 PM, <mailto:clamy@chromium.org> wrote:
> 
> > @eustas: PTAL at the change in the layout test. With this change, the
> > script is
> > not evicted from the MemoryCache (since it has dangling handlers). So on
> > reload,
> > it is served from the cache and not from the network, and no fallback
> > content is
> > actually requested.
> >
> > https://chromiumcodereview.appspot.com/179943002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

long-script-content test specifically tested the behavior when resource is
removed from cache.

Now this doesn't happen.

Looking on the patch I may suggest that this is because resources in cache
are not deleted anymore (even if handle count is zero).

So, to fix test we need new scenario in which script resource could be deleted
from memory cache.

[Philippe, 2014-03-19 10:09:50]

The CQ bit was checked by pliard@chromium.org

[commit-bot: I haz the power, 2014-03-19 10:09:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clamy@chromium.org/179943002/110001

[Philippe, 2014-03-19 10:11:13]

The CQ bit was unchecked by pliard@chromium.org

[Philippe, 2014-03-26 16:04:06]

Thanks Camille! I'm glad that we will be able to land this!

https://chromiumcodereview.appspot.com/179943002/diff/370001/Source/core/fetc...
File Source/core/fetch/Resource.cpp (right):

https://chromiumcodereview.appspot.com/179943002/diff/370001/Source/core/fetc...
Source/core/fetch/Resource.cpp:370: return (inCache && m_handleCount ==
targetCount + 1) || (!inCache && m_handleCount == targetCount);
Nit: not sure this is more readable but in case you think it is, you can replace
this I believe with:

return m_handleCount == targetCount + !!memoryCache()->contains(this);

https://chromiumcodereview.appspot.com/179943002/diff/370001/Source/core/fetc...
Source/core/fetch/Resource.cpp:791: if (!hasClients())
Nit: Do we still need this change?

https://chromiumcodereview.appspot.com/179943002/diff/370001/Source/core/fetc...
File Source/core/fetch/Resource.h (right):

https://chromiumcodereview.appspot.com/179943002/diff/370001/Source/core/fetc...
Source/core/fetch/Resource.h:200: bool canDelete();
Nit: did you drop the 'const' here intentionally? We should also be able to add
it back on line 201 (right below) and line 345.

I see that MemoryCache::contains() is not only not const but also takes a
mutable pointer to Resource which is why you had to drop the const here I guess.
I think this is a mistake, please consider making it have the following
signature instead: "bool MemoryCache::contains(const Resource*) const".

[Philippe, 2014-03-26 17:08:32]

https://chromiumcodereview.appspot.com/179943002/diff/370001/Source/core/fetc...
File Source/core/fetch/Resource.cpp (right):

https://chromiumcodereview.appspot.com/179943002/diff/370001/Source/core/fetc...
Source/core/fetch/Resource.cpp:370: return (inCache && m_handleCount ==
targetCount + 1) || (!inCache && m_handleCount == targetCount);
On 2014/03/26 16:04:07, Philippe wrote:
> Nit: not sure this is more readable but in case you think it is, you can
replace
> this I believe with:
> 
> return m_handleCount == targetCount + !!memoryCache()->contains(this);

You might not even need the '!!' though. We usually use this pattern to convert
pointers to integers but contains() already returns a bool here.

https://chromiumcodereview.appspot.com/179943002/diff/370001/Source/core/fetc...
File Source/core/fetch/Resource.h (right):

https://chromiumcodereview.appspot.com/179943002/diff/370001/Source/core/fetc...
Source/core/fetch/Resource.h:200: bool canDelete();
On 2014/03/26 16:04:07, Philippe wrote:
> Nit: did you drop the 'const' here intentionally? We should also be able to
add
> it back on line 201 (right below) and line 345.
> 
> I see that MemoryCache::contains() is not only not const but also takes a
> mutable pointer to Resource which is why you had to drop the const here I
guess.
> I think this is a mistake, please consider making it have the following
> signature instead: "bool MemoryCache::contains(const Resource*) const".

Just sent this CL to Nate so that you can add the 'const' back here:
https://chromiumcodereview.appspot.com/213193002/ :)

[clamy, 2014-03-27 10:47:16]

Thanks Philippe!

https://chromiumcodereview.appspot.com/179943002/diff/370001/Source/core/fetc...
File Source/core/fetch/Resource.cpp (right):

https://chromiumcodereview.appspot.com/179943002/diff/370001/Source/core/fetc...
Source/core/fetch/Resource.cpp:370: return (inCache && m_handleCount ==
targetCount + 1) || (!inCache && m_handleCount == targetCount);
On 2014/03/26 17:08:33, Philippe wrote:
> On 2014/03/26 16:04:07, Philippe wrote:
> > Nit: not sure this is more readable but in case you think it is, you can
> replace
> > this I believe with:
> > 
> > return m_handleCount == targetCount + !!memoryCache()->contains(this);
> 
> You might not even need the '!!' though. We usually use this pattern to
convert
> pointers to integers but contains() already returns a bool here.

Done.

https://chromiumcodereview.appspot.com/179943002/diff/370001/Source/core/fetc...
Source/core/fetch/Resource.cpp:791: if (!hasClients())
On 2014/03/26 16:04:07, Philippe wrote:
> Nit: Do we still need this change?

Yes this is what is making it work with the rebase.

https://chromiumcodereview.appspot.com/179943002/diff/370001/Source/core/fetc...
File Source/core/fetch/Resource.h (right):

https://chromiumcodereview.appspot.com/179943002/diff/370001/Source/core/fetc...
Source/core/fetch/Resource.h:200: bool canDelete();
On 2014/03/26 17:08:33, Philippe wrote:
> On 2014/03/26 16:04:07, Philippe wrote:
> > Nit: did you drop the 'const' here intentionally? We should also be able to
> add
> > it back on line 201 (right below) and line 345.
> > 
> > I see that MemoryCache::contains() is not only not const but also takes a
> > mutable pointer to Resource which is why you had to drop the const here I
> guess.
> > I think this is a mistake, please consider making it have the following
> > signature instead: "bool MemoryCache::contains(const Resource*) const".
> 
> Just sent this CL to Nate so that you can add the 'const' back here:
> https://chromiumcodereview.appspot.com/213193002/ :)

Thanks! I will make them const again.

[clamy, 2014-03-27 10:59:37]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2014-03-27 10:59:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clamy@chromium.org/179943002/440001

[commit-bot: I haz the power, 2014-03-27 11:06:03]

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

[commit-bot: I haz the power, 2014-03-27 11:06:03]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_compile_dbg

[Philippe, 2014-03-27 11:58:52]

The CQ bit was checked by pliard@chromium.org

[commit-bot: I haz the power, 2014-03-27 11:59:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clamy@chromium.org/179943002/440001

[commit-bot: I haz the power, 2014-03-27 12:06:57]

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

[commit-bot: I haz the power, 2014-03-27 12:06:58]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[Philippe, 2014-03-27 12:59:46]

The CQ bit was checked by pliard@chromium.org

[commit-bot: I haz the power, 2014-03-27 12:59:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clamy@chromium.org/179943002/440001

[commit-bot: I haz the power, 2014-03-27 14:14:00]

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

[commit-bot: I haz the power, 2014-03-27 14:14:01]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[clamy, 2014-03-27 14:54:50]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2014-03-27 14:54:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clamy@chromium.org/179943002/440001

[commit-bot: I haz the power, 2014-03-27 15:16:29]

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

[commit-bot: I haz the power, 2014-03-27 15:16:29]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[clamy, 2014-03-27 16:05:12]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2014-03-27 16:05:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clamy@chromium.org/179943002/440001

[commit-bot: I haz the power, 2014-03-27 16:14:43]

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

[commit-bot: I haz the power, 2014-03-27 16:14:45]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[Philippe, 2014-03-28 08:29:12]

The CQ bit was checked by pliard@chromium.org

[commit-bot: I haz the power, 2014-03-28 08:29:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clamy@chromium.org/179943002/440001

[commit-bot: I haz the power, 2014-03-28 08:29:29]

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

[commit-bot: I haz the power, 2014-03-28 08:29:30]

Failed to apply patch for Source/core/fetch/MemoryCache.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/fetch/MemoryCache.cpp
  Hunk #1 FAILED at 223.
  Hunk #2 FAILED at 265.
  2 out of 2 hunks FAILED -- saving rejects to file
Source/core/fetch/MemoryCache.cpp.rej

Patch:       Source/core/fetch/MemoryCache.cpp
Index: Source/core/fetch/MemoryCache.cpp
diff --git a/Source/core/fetch/MemoryCache.cpp
b/Source/core/fetch/MemoryCache.cpp
index
7deda544049639131b00ff35e0a4a91cb35e2408..5a9e7bde46cd4dba67a0a5e927ec025dd7a62b63
100644
--- a/Source/core/fetch/MemoryCache.cpp
+++ b/Source/core/fetch/MemoryCache.cpp
@@ -223,10 +223,11 @@ void MemoryCache::pruneDeadResources()
         MemoryCacheEntry* current = m_allResources[i].m_tail;
         while (current) {
             MemoryCacheEntry* previous = current->m_previousInAllResourcesList;
-            if (current->m_resource->wasPurged()) {
+            if (current->m_resource->wasPurged() &&
current->m_resource->canDelete()) {
                 ASSERT(!current->m_resource->hasClients());
                 ASSERT(!current->m_resource->isPreloaded());
-                evict(current->m_resource.get());
+                bool wasEvicted = evict(current->m_resource.get());
+                ASSERT_UNUSED(wasEvicted, wasEvicted);
             }
             current = previous;
         }
@@ -265,8 +266,10 @@ void MemoryCache::pruneDeadResources()
         while (current) {
             MemoryCacheEntry* previous = current->m_previousInAllResourcesList;
             ASSERT(!previous || contains(previous->m_resource.get()));
-            if (!current->m_resource->hasClients() &&
!current->m_resource->isPreloaded() && !current->m_resource->isCacheValidator())
{
-                evict(current->m_resource.get());
+            if (!current->m_resource->hasClients() &&
!current->m_resource->isPreloaded()
+                && !current->m_resource->isCacheValidator() &&
current->m_resource->canDelete()) {
+                bool wasEvicted = evict(current->m_resource.get());
+                ASSERT_UNUSED(wasEvicted, wasEvicted);
                 if (targetSize && m_deadSize <= targetSize)
                     return;
             }

[clamy, 2014-03-28 09:49:46]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2014-03-28 09:49:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clamy@chromium.org/179943002/460001

[commit-bot: I haz the power, 2014-03-28 10:32:43]

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

[commit-bot: I haz the power, 2014-03-28 10:32:44]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[Philippe, 2014-03-28 10:40:46]

The CQ bit was checked by pliard@chromium.org

[commit-bot: I haz the power, 2014-03-28 10:40:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clamy@chromium.org/179943002/460001

[commit-bot: I haz the power, 2014-03-28 11:39:02]

Change committed as 170311

[enne (OOO), 2014-03-28 23:21:15]

A revert of this CL has been created in
https://codereview.chromium.org/217643003/ by enne@chromium.org.

The reason for reverting is: Possibly causing crashes in
MultiProfileFileManagerBrowserTest

See:
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome....