Do not call MemoryCache::prune() when a client/observer is removed

Do not call MemoryCache::prune() when a client/observer is removed

- didRemoveClientOrObserver() can be called during a Oilpan prefinalizer,
  so it might be unsafe to call MemoryCache::prune() there.
- As WeakMemoryCache is enabled, Resource is expected to be garbage collected
  shortly after all clients/observers are removed, so there is less need for
  the pruning.

BUG=644058, 603462
Committed: https://crrev.com/b23d2fc9c472f2b89b1101bdcf38ac13f4756ef7
Cr-Commit-Position: refs/heads/master@{#421768}

[hiroshige, 2016-09-15 08:11:19]

Description was changed from

==========
Do not call MemoryCache::prune() when a client/observer is removed

BUG=
==========

to

==========
Do not call MemoryCache::prune() when a client/observer is removed

- didRemoveClientOrObserver() can be called during a Oilpan prefinalizer,
  so it might be unsafe to call MemoryCache::prune() there.
- As WeakMemoryCache is enabled, Resource is expected to be garbage collected
  shortly after all clients/observers are removed, so there is less need for
  the pruning.

BUG=644058, 603462
==========

[hiroshige, 2016-09-15 08:11:19]

hiroshige@chromium.org changed reviewers:
+ japhet@chromium.org, yhirano@chromium.org

[hiroshige, 2016-09-15 08:11:29]

PTAL.

[yhirano, 2016-09-15 08:30:43]

https://codereview.chromium.org/2326743003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2326743003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.cpp:733:
memoryCache()->remove(this);
Hm, is this statement still needed?

[hiroshige, 2016-09-15 08:45:33]

https://codereview.chromium.org/2326743003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2326743003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.cpp:733:
memoryCache()->remove(this);
On 2016/09/15 08:30:43, yhirano (slow) wrote:
> Hm, is this statement still needed?

I'm not sure.
I created a CL https://codereview.chromium.org/2346483005 and seeing whether
this is covered by tests.

(Anyway it should be handled in a separate CL because it affects caching
correctness, and MemoryCache::remove() is not harmful when executed in
prefinalizers)

[Nate Chapin, 2016-09-16 21:28:47]

LGTM with a nit...

https://codereview.chromium.org/2326743003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.cpp (left):

https://codereview.chromium.org/2326743003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.cpp:736:
memoryCache()->prune(this);
I think this is the only callsite that passes a non-null Resource* to prune().
Could we remove that unused logic, too?

[yhirano, 2016-09-20 07:26:11]

lgtm

[hiroshige, 2016-09-28 04:32:14]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 04:32:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 05:57:01]

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

[commit-bot: I haz the power, 2016-09-28 05:57:02]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-09-29 06:27:17]

https://codereview.chromium.org/2326743003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.cpp (left):

https://codereview.chromium.org/2326743003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.cpp:736:
memoryCache()->prune(this);
On 2016/09/16 21:28:47, Nate Chapin wrote:
> I think this is the only callsite that passes a non-null Resource* to prune().
> Could we remove that unused logic, too?

Done.

[hiroshige, 2016-09-29 06:48:42]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2016-09-29 06:48:44]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2326743003/#ps60001
(title: "Remove justReleasedResource")

[commit-bot: I haz the power, 2016-09-29 06:48:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 07:31:27]

Description was changed from

==========
Do not call MemoryCache::prune() when a client/observer is removed

- didRemoveClientOrObserver() can be called during a Oilpan prefinalizer,
  so it might be unsafe to call MemoryCache::prune() there.
- As WeakMemoryCache is enabled, Resource is expected to be garbage collected
  shortly after all clients/observers are removed, so there is less need for
  the pruning.

BUG=644058, 603462
==========

to

==========
Do not call MemoryCache::prune() when a client/observer is removed

- didRemoveClientOrObserver() can be called during a Oilpan prefinalizer,
  so it might be unsafe to call MemoryCache::prune() there.
- As WeakMemoryCache is enabled, Resource is expected to be garbage collected
  shortly after all clients/observers are removed, so there is less need for
  the pruning.

BUG=644058, 603462
==========

[commit-bot: I haz the power, 2016-09-29 07:31:28]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-29 07:33:36]

Description was changed from

==========
Do not call MemoryCache::prune() when a client/observer is removed

- didRemoveClientOrObserver() can be called during a Oilpan prefinalizer,
  so it might be unsafe to call MemoryCache::prune() there.
- As WeakMemoryCache is enabled, Resource is expected to be garbage collected
  shortly after all clients/observers are removed, so there is less need for
  the pruning.

BUG=644058, 603462
==========

to

==========
Do not call MemoryCache::prune() when a client/observer is removed

- didRemoveClientOrObserver() can be called during a Oilpan prefinalizer,
  so it might be unsafe to call MemoryCache::prune() there.
- As WeakMemoryCache is enabled, Resource is expected to be garbage collected
  shortly after all clients/observers are removed, so there is less need for
  the pruning.

BUG=644058, 603462

Committed: https://crrev.com/b23d2fc9c472f2b89b1101bdcf38ac13f4756ef7
Cr-Commit-Position: refs/heads/master@{#421768}
==========

[commit-bot: I haz the power, 2016-09-29 07:33:37]

Patchset 4 (id:??) landed as
https://crrev.com/b23d2fc9c472f2b89b1101bdcf38ac13f4756ef7
Cr-Commit-Position: refs/heads/master@{#421768}