Move ResourceClient to Oilpan heap

Move ResourceClient to Oilpan heap

Now Resource has its clients as weak members.

BUG=587663
Committed: https://crrev.com/9c3053df29fa90acbd9c2187f94807ff546b0981
Cr-Commit-Position: refs/heads/master@{#415007}

[yhirano, 2016-07-28 05:56:13]

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

[commit-bot: I haz the power, 2016-07-28 05:56:30]

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

[yhirano, 2016-07-28 06:01:40]

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

[commit-bot: I haz the power, 2016-07-28 06:01:49]

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-07-28 06:05:33]

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

[commit-bot: I haz the power, 2016-07-28 06:05:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yhirano, 2016-07-28 06:10:26]

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

[commit-bot: I haz the power, 2016-07-28 06:10: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-07-28 06:14:16]

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

[commit-bot: I haz the power, 2016-07-28 06:14:17]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yhirano, 2016-07-28 06:41:09]

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

[commit-bot: I haz the power, 2016-07-28 06:45:59]

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-07-28 06:49:59]

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

[commit-bot: I haz the power, 2016-07-28 06:50:00]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yhirano, 2016-07-28 06:52:21]

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

[commit-bot: I haz the power, 2016-07-28 06:52:34]

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-07-28 07:40:39]

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

[commit-bot: I haz the power, 2016-07-28 07:40:40]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yhirano, 2016-07-28 11:16:32]

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

[commit-bot: I haz the power, 2016-07-28 11:16:45]

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-07-28 12:23:50]

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

[commit-bot: I haz the power, 2016-07-28 12:23:51]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[yhirano, 2016-07-29 12:37:04]

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

[commit-bot: I haz the power, 2016-07-29 12:37:18]

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-07-29 12:39:49]

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

[commit-bot: I haz the power, 2016-07-29 12:39:50]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[yhirano, 2016-08-01 06:07:58]

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

[commit-bot: I haz the power, 2016-08-01 06:08:16]

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

[yhirano, 2016-08-01 06:27:01]

Patchset #2 (id:20001) has been deleted

[yhirano, 2016-08-01 06:27:06]

Patchset #2 (id:40001) has been deleted

[yhirano, 2016-08-01 06:27:12]

Patchset #2 (id:60001) has been deleted

[yhirano, 2016-08-01 06:27:18]

Patchset #2 (id:80001) has been deleted

[yhirano, 2016-08-01 06:27:25]

Patchset #2 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-08-01 06:47:30]

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

[commit-bot: I haz the power, 2016-08-01 06:47:31]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[yhirano, 2016-08-01 14:20:06]

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

[commit-bot: I haz the power, 2016-08-01 14:20:17]

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-08-01 14:23:22]

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

[commit-bot: I haz the power, 2016-08-01 14:23:24]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yhirano, 2016-08-01 15:14:35]

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

[commit-bot: I haz the power, 2016-08-01 15:14:51]

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-08-01 15:17:47]

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

[commit-bot: I haz the power, 2016-08-01 15:17:48]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yhirano, 2016-08-02 00:58:04]

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

[commit-bot: I haz the power, 2016-08-02 00:58:25]

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-08-02 02:17:11]

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

[commit-bot: I haz the power, 2016-08-02 02:17:12]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[yhirano, 2016-08-02 03:21:58]

Description was changed from

==========
Move ResourceClient to Oilpan heap


wip

BUG=587663
==========

to

==========
Move ResourceClient to Oilpan heap

Now Resource has its clients as weak members.

BUG=587663
==========

[yhirano, 2016-08-02 03:21:59]

yhirano@chromium.org changed reviewers:
+ japhet@chromium.org, oilpan-reviews@chromium.org

[yhirano, 2016-08-02 03:22:10]

Patchset #3 (id:140001) has been deleted

[yhirano, 2016-08-02 03:22:15]

Patchset #2 (id:120001) has been deleted

[haraken, 2016-08-02 04:18:05]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-08-02 04:18:07]

This is amazing! You're getting pretty close :)

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:47: struct
ImageResourceObserverWalker {

Shall we add a TODO to replace this with ResourceClientWalker?

Also as I commented below, I think we can just remove the walkers if we have
copyToVector(HashCountedSet<>). Then the caller site can just copyToVector the
hash counted set and iterate the vector.

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:725: if (!hasClient(client)) {

Would you help me understand when this can happen? It seems problematic that the
client is already removed from the weak hash counted set while you're still
holding a reference to the client...

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:820:
clientsToNotify.reserveCapacity(m_clientsAwaitingCallback.size());

Can we implement copyToVector for a hash counted set?

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:823: if (client)

I don't think this can happen. See my comment below.

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceClientWalker.h (right):

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceClientWalker.h:37: struct
ResourceClientWalker {

I'd prefer using a class.

(Honestly speaking, I'm not quite sure of the benefit of having the
ResourceClientWalker class. The caller sites can just copyToVector the hash
counted set and iterate the vector.)

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceClientWalker.h:47: // |client| can
be null, as the container holds weak pointers.

I guess |client| cannot be null because null-ed entries are automatically
removed from the hash counted set.

[yhirano, 2016-08-03 08:56:11]

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

[commit-bot: I haz the power, 2016-08-03 08:56:19]

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

[yhirano, 2016-08-03 09:08:31]

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

[commit-bot: I haz the power, 2016-08-03 09:10:07]

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-08-03 09:25:42]

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

[commit-bot: I haz the power, 2016-08-03 09:25:43]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[yhirano, 2016-08-03 10:40:28]

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:47: struct
ImageResourceObserverWalker {
On 2016/08/02 04:18:06, haraken wrote:
> 
> Shall we add a TODO to replace this with ResourceClientWalker?
> 
> Also as I commented below, I think we can just remove the walkers if we have
> copyToVector(HashCountedSet<>). Then the caller site can just copyToVector the
> hash counted set and iterate the vector.

Thanks, done.

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:725: if (!hasClient(client)) {
On 2016/08/02 04:18:06, haraken wrote:
> 
> Would you help me understand when this can happen? It seems problematic that
the
> client is already removed from the weak hash counted set while you're still
> holding a reference to the client...

This function can be called from a pre-finalizer (see ResourceOwner). In such a
case weak pointers are cleared just before the pre-finalization step.

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:820:
clientsToNotify.reserveCapacity(m_clientsAwaitingCallback.size());
On 2016/08/02 04:18:06, haraken wrote:
> 
> Can we implement copyToVector for a hash counted set?

We already have it, but as I said in the other place, it's not usable to convert
HeapHashCountedSet<WeakMember<T>> to HeapVector<Member<T>>.

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:823: if (client)
On 2016/08/02 04:18:06, haraken wrote:
> 
> I don't think this can happen. See my comment below.

Done.

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceClientWalker.h (right):

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceClientWalker.h:37: struct
ResourceClientWalker {
On 2016/08/02 04:18:06, haraken wrote:
> 
> I'd prefer using a class.
> 
> (Honestly speaking, I'm not quite sure of the benefit of having the
> ResourceClientWalker class. The caller sites can just copyToVector the hash
> counted set and iterate the vector.)

Done.

This class is doing two extra conversions: One is WeakMember to Member and
another is ResourceClient* to T*. copyToVector can handle neither conversions.

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceClientWalker.h:47: // |client| can
be null, as the container holds weak pointers.
On 2016/08/02 04:18:06, haraken wrote:
> 
> I guess |client| cannot be null because null-ed entries are automatically
> removed from the hash counted set.

Done.

[haraken, 2016-08-03 11:11:40]

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:725: if (!hasClient(client)) {
On 2016/08/03 10:40:28, yhirano wrote:
> On 2016/08/02 04:18:06, haraken wrote:
> > 
> > Would you help me understand when this can happen? It seems problematic that
> the
> > client is already removed from the weak hash counted set while you're still
> > holding a reference to the client...
> 
> This function can be called from a pre-finalizer (see ResourceOwner). In such
a
> case weak pointers are cleared just before the pre-finalization step.

Can we avoid calling Resource::removeClient() in the pre-finalizer? Alternately,
we can call didRemoveClientOrObserver() in the pre-finalizer.

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:725: if (!hasClient(client)) {
On 2016/08/03 10:40:28, yhirano wrote:
> On 2016/08/02 04:18:06, haraken wrote:
> > 
> > Would you help me understand when this can happen? It seems problematic that
> the
> > client is already removed from the weak hash counted set while you're still
> > holding a reference to the client...
> 
> This function can be called from a pre-finalizer (see ResourceOwner). In such
a
> case weak pointers are cleared just before the pre-finalization step.

Can you avoid calling Resource::removeClient in the pre-finalizer? You can call
didRemoveClientOrObserver() instead.

[Nate Chapin, 2016-08-03 19:40:58]

LGTM!

https://codereview.chromium.org/2191633003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorResourceContentLoader.cpp
(right):

https://codereview.chromium.org/2191633003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorResourceContentLoader.cpp:45:
RawResourceClient::trace(visitor);
Why change the ordering here?

[yhirano, 2016-08-09 07:38:21]

Sorry for the late response.

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:725: if (!hasClient(client)) {
On 2016/08/03 11:11:40, haraken wrote:
> On 2016/08/03 10:40:28, yhirano wrote:
> > On 2016/08/02 04:18:06, haraken wrote:
> > > 
> > > Would you help me understand when this can happen? It seems problematic
that
> > the
> > > client is already removed from the weak hash counted set while you're
still
> > > holding a reference to the client...
> > 
> > This function can be called from a pre-finalizer (see ResourceOwner). In
such
> a
> > case weak pointers are cleared just before the pre-finalization step.
> 
> Can you avoid calling Resource::removeClient in the pre-finalizer? You can
call
> didRemoveClientOrObserver() instead.

It's hard because callers generally don't distinguish them. For example,
PendingScript::stopWatchingForLoad calls Resource::removeClient and there are 8
callers who call PendingScript::stopWatchingForLoad. At least one of them
(ScriptLoader::notifyFinished) can be called as a non-prefinalizer and at least
one of them (HTMLScriptRunner::detach) can be called in the prefinalization
step.

[yhirano, 2016-08-09 07:42:42]

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

[commit-bot: I haz the power, 2016-08-09 07:43:03]

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-08-09 09:07:58]

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

[commit-bot: I haz the power, 2016-08-09 09:07:59]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[yhirano, 2016-08-12 09:44:43]

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

[yhirano, 2016-08-12 09:44:52]

haraken@: ping

[commit-bot: I haz the power, 2016-08-12 09:44:57]

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

[haraken, 2016-08-12 10:45:20]

LGTM

https://codereview.chromium.org/2191633003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2191633003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:50: Vector<T*>
asVector(const HashCountedSet<T*>& set)

I'd move this helper function to HashCountedSet.

https://codereview.chromium.org/2191633003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2191633003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:727: // hold clients as weak
member.

// This code may be called in a pre-finalizer, where weak members in the
HashCountedSet are already swept out. In that case, we just need to call
didRemoveClientOrObserver().

https://codereview.chromium.org/2191633003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:819:
HeapVector<Member<ResourceClient>> clientsToNotify;

Can we use the asVector?

https://codereview.chromium.org/2191633003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceClientWalker.h (right):

https://codereview.chromium.org/2191633003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceClientWalker.h:37: class
ResourceClientWalker {

I'd prefer removing this class in a follow-up CL in favor of
HashCountedSet.asVector(), just like you did in ImageResource.cpp.

[commit-bot: I haz the power, 2016-08-12 11:12:19]

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

[commit-bot: I haz the power, 2016-08-12 11:12:20]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[yhirano, 2016-08-16 05:53:57]

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

[commit-bot: I haz the power, 2016-08-16 05:54:10]

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-08-16 07:22:56]

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

[commit-bot: I haz the power, 2016-08-16 07:22:57]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[yhirano, 2016-08-16 08:28:15]

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

[commit-bot: I haz the power, 2016-08-16 08:28:27]

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

[yhirano, 2016-08-16 08:57:25]

https://codereview.chromium.org/2191633003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2191633003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:50: Vector<T*>
asVector(const HashCountedSet<T*>& set)
On 2016/08/12 10:45:20, haraken wrote:
> 
> I'd move this helper function to HashCountedSet.

Done.

https://codereview.chromium.org/2191633003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2191633003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:727: // hold clients as weak
member.
On 2016/08/12 10:45:20, haraken wrote:
> 
> // This code may be called in a pre-finalizer, where weak members in the
> HashCountedSet are already swept out. In that case, we just need to call
> didRemoveClientOrObserver().

Done.

https://codereview.chromium.org/2191633003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:819:
HeapVector<Member<ResourceClient>> clientsToNotify;
On 2016/08/12 10:45:20, haraken wrote:
> 
> Can we use the asVector?

Using asVector means making clientsToNotify a
HeapVector<WeakMember<ResourceClient>>. I think keeping clientsToNotify's type
is more important.

https://codereview.chromium.org/2191633003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceClientWalker.h (right):

https://codereview.chromium.org/2191633003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceClientWalker.h:37: class
ResourceClientWalker {
On 2016/08/12 10:45:20, haraken wrote:
> 
> I'd prefer removing this class in a follow-up CL in favor of
> HashCountedSet.asVector(), just like you did in ImageResource.cpp.
> 
> 

Please see
https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou....

[commit-bot: I haz the power, 2016-08-16 09:45:28]

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

[commit-bot: I haz the power, 2016-08-16 09:45:29]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[haraken, 2016-08-16 11:55:44]

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceClientWalker.h (right):

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceClientWalker.h:37: struct
ResourceClientWalker {
On 2016/08/03 10:40:28, yhirano wrote:
> On 2016/08/02 04:18:06, haraken wrote:
> > 
> > I'd prefer using a class.
> > 
> > (Honestly speaking, I'm not quite sure of the benefit of having the
> > ResourceClientWalker class. The caller sites can just copyToVector the hash
> > counted set and iterate the vector.)
> 
> Done.
> 
> This class is doing two extra conversions: One is WeakMember to Member and
> another is ResourceClient* to T*. copyToVector can handle neither conversions.

I'm just curious but why do you need to convert ResourceClient* to T*? Can we
avoid the conversion by adding some virtual method on ResourceClient?

https://codereview.chromium.org/2191633003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2191633003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:819:
HeapVector<Member<ResourceClient>> clientsToNotify;
On 2016/08/16 08:57:24, yhirano wrote:
> On 2016/08/12 10:45:20, haraken wrote:
> > 
> > Can we use the asVector?
> 
> Using asVector means making clientsToNotify a
> HeapVector<WeakMember<ResourceClient>>. I think keeping clientsToNotify's type
> is more important.

asVector should convert HeapHashCountedSet<WeakMember> to HeapVector<Member>.
Remember that copyToVector is converting HeapHashSet<WeakMember> to
HeapVector<Member>.

Nit: I'd prefer introducing copyToVector(HeapHashCountedSet<WeakMember/Member>,
HeapVector<Member>) to make the type conversion explicit. Then the call site
needs to create HeapVector<Member> before calling copyToVector, which makes the
type conversion explicit.

I'm wondering why you want to keep the type as WeakMembers.

[yhirano, 2016-08-17 06:20:53]

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

[commit-bot: I haz the power, 2016-08-17 06:21:05]

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-08-17 06:36:27]

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

[commit-bot: I haz the power, 2016-08-17 06:36:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yhirano, 2016-08-17 06:40:55]

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

[commit-bot: I haz the power, 2016-08-17 06:41:14]

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-08-17 07:58:32]

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

[commit-bot: I haz the power, 2016-08-17 07:58:33]

Dry run: Exceeded global retry quota

[yhirano, 2016-08-17 08:21:27]

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

[commit-bot: I haz the power, 2016-08-17 08:21:50]

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

[yhirano, 2016-08-17 08:32:20]

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

[commit-bot: I haz the power, 2016-08-17 08:32:41]

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

[yhirano, 2016-08-17 08:36:09]

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceClientWalker.h (right):

https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceClientWalker.h:37: struct
ResourceClientWalker {
On 2016/08/16 11:55:44, haraken wrote:
> On 2016/08/03 10:40:28, yhirano wrote:
> > On 2016/08/02 04:18:06, haraken wrote:
> > > 
> > > I'd prefer using a class.
> > > 
> > > (Honestly speaking, I'm not quite sure of the benefit of having the
> > > ResourceClientWalker class. The caller sites can just copyToVector the
hash
> > > counted set and iterate the vector.)
> > 
> > Done.
> > 
> > This class is doing two extra conversions: One is WeakMember to Member and
> > another is ResourceClient* to T*. copyToVector can handle neither
conversions.
> 
> I'm just curious but why do you need to convert ResourceClient* to T*? Can we
> avoid the conversion by adding some virtual method on ResourceClient?

The right direction would be making each Resource subclass have its
ResourceClient.

https://codereview.chromium.org/2191633003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2191633003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:819:
HeapVector<Member<ResourceClient>> clientsToNotify;
On 2016/08/16 11:55:44, haraken wrote:
> On 2016/08/16 08:57:24, yhirano wrote:
> > On 2016/08/12 10:45:20, haraken wrote:
> > > 
> > > Can we use the asVector?
> > 
> > Using asVector means making clientsToNotify a
> > HeapVector<WeakMember<ResourceClient>>. I think keeping clientsToNotify's
type
> > is more important.
> 
> asVector should convert HeapHashCountedSet<WeakMember> to HeapVector<Member>.
> Remember that copyToVector is converting HeapHashSet<WeakMember> to
> HeapVector<Member>.
> 
> Nit: I'd prefer introducing
copyToVector(HeapHashCountedSet<WeakMember/Member>,
> HeapVector<Member>) to make the type conversion explicit. Then the call site
> needs to create HeapVector<Member> before calling copyToVector, which makes
the
> type conversion explicit.
> 
> I'm wondering why you want to keep the type as WeakMembers.
> 
> 
> 
> 

I think HashCountedSet<T, ...>.asVector() should return Vector<T, ...>. WTF
containers don't know about Oilpan, so I feel it confusing if
HeapHashCountedSet<T, ...>.asVector() doesn't return Vector<T, ...>.

It's possible to introduce a type parameter, like 

clientsToNotify =
m_clientsAwaitingClalback.asVector<HeapVector<Member<ResourceClient>>>();

but it is too verbose.

PS11 uses copyToVector.

[haraken, 2016-08-17 08:57:22]

LGTM

On 2016/08/17 08:36:09, yhirano wrote:
>
https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/fetch/ResourceClientWalker.h (right):
> 
>
https://codereview.chromium.org/2191633003/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/fetch/ResourceClientWalker.h:37: struct
> ResourceClientWalker {
> On 2016/08/16 11:55:44, haraken wrote:
> > On 2016/08/03 10:40:28, yhirano wrote:
> > > On 2016/08/02 04:18:06, haraken wrote:
> > > > 
> > > > I'd prefer using a class.
> > > > 
> > > > (Honestly speaking, I'm not quite sure of the benefit of having the
> > > > ResourceClientWalker class. The caller sites can just copyToVector the
> hash
> > > > counted set and iterate the vector.)
> > > 
> > > Done.
> > > 
> > > This class is doing two extra conversions: One is WeakMember to Member and
> > > another is ResourceClient* to T*. copyToVector can handle neither
> conversions.
> > 
> > I'm just curious but why do you need to convert ResourceClient* to T*? Can
we
> > avoid the conversion by adding some virtual method on ResourceClient?
> 
> The right direction would be making each Resource subclass have its
> ResourceClient.
> 
>
https://codereview.chromium.org/2191633003/diff/240001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/fetch/Resource.cpp (right):
> 
>
https://codereview.chromium.org/2191633003/diff/240001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/fetch/Resource.cpp:819:
> HeapVector<Member<ResourceClient>> clientsToNotify;
> On 2016/08/16 11:55:44, haraken wrote:
> > On 2016/08/16 08:57:24, yhirano wrote:
> > > On 2016/08/12 10:45:20, haraken wrote:
> > > > 
> > > > Can we use the asVector?
> > > 
> > > Using asVector means making clientsToNotify a
> > > HeapVector<WeakMember<ResourceClient>>. I think keeping clientsToNotify's
> type
> > > is more important.
> > 
> > asVector should convert HeapHashCountedSet<WeakMember> to
HeapVector<Member>.
> > Remember that copyToVector is converting HeapHashSet<WeakMember> to
> > HeapVector<Member>.
> > 
> > Nit: I'd prefer introducing
> copyToVector(HeapHashCountedSet<WeakMember/Member>,
> > HeapVector<Member>) to make the type conversion explicit. Then the call site
> > needs to create HeapVector<Member> before calling copyToVector, which makes
> the
> > type conversion explicit.
> > 
> > I'm wondering why you want to keep the type as WeakMembers.
> > 
> > 
> > 
> > 
> 
> I think HashCountedSet<T, ...>.asVector() should return Vector<T, ...>. WTF
> containers don't know about Oilpan, so I feel it confusing if
> HeapHashCountedSet<T, ...>.asVector() doesn't return Vector<T, ...>.
> 
> It's possible to introduce a type parameter, like 
> 
> clientsToNotify =
> m_clientsAwaitingClalback.asVector<HeapVector<Member<ResourceClient>>>();
> 
> but it is too verbose.
> 
> PS11 uses copyToVector.

My preference was just to use copyToVector in the caller side without
introducing a helper function like asVector (it's a well-established pattern in
the Blink code base), but this looks fine.

[commit-bot: I haz the power, 2016-08-17 09:41:10]

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

[commit-bot: I haz the power, 2016-08-17 09:41:11]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[yhirano, 2016-08-17 12:28:44]

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

[commit-bot: I haz the power, 2016-08-17 12:28:52]

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-08-17 12:54:56]

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

[commit-bot: I haz the power, 2016-08-17 12:54:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yhirano, 2016-08-18 01:53:38]

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

[commit-bot: I haz the power, 2016-08-18 01:54:12]

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

[yhirano, 2016-08-18 05:21:24]

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

[commit-bot: I haz the power, 2016-08-18 05:21:40]

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-08-18 07:41:20]

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

[commit-bot: I haz the power, 2016-08-18 07:41:21]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-08-18 07:43:15]

PS15 introduces Resource::m_isAlive and Resource::isAlive() because we cannot
rely on Resource::hasClientsOrObservers() in the pre-finalization step.

[haraken, 2016-08-18 08:06:17]

https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:727: if (!hasClient(client)) {

I begin to think this is problematic...

If Resource and ResoureClient die at the same time, the weak processing doesn't
run. However, pre-finalizer runs. Then hasClient() returns true and we end up
with executing line 734 - 744. Which should be problematic... :/

[yhirano, 2016-08-18 08:31:53]

https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:727: if (!hasClient(client)) {
On 2016/08/18 08:06:17, haraken wrote:
> 
> I begin to think this is problematic...
> 
> If Resource and ResoureClient die at the same time, the weak processing
doesn't
> run. However, pre-finalizer runs. Then hasClient() returns true and we end up
> with executing line 734 - 744. Which should be problematic... :/

Hmm, I did think the weak processing is executed in such a case...

Let me confirm, an entry |e| in a HeapVector<WeakMember<E>> |v| is cleared by
the weak processing when |e| is no longer reachable but |v| is still reachable,
is that correct?

[yhirano, 2016-08-18 08:34:40]

On 2016/08/18 08:31:53, yhirano wrote:
>
https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/fetch/Resource.cpp (right):
> 
>
https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/fetch/Resource.cpp:727: if (!hasClient(client))
{
> On 2016/08/18 08:06:17, haraken wrote:
> > 
> > I begin to think this is problematic...
> > 
> > If Resource and ResoureClient die at the same time, the weak processing
> doesn't
> > run. However, pre-finalizer runs. Then hasClient() returns true and we end
up
> > with executing line 734 - 744. Which should be problematic... :/
> 
> Hmm, I did think the weak processing is executed in such a case...
> 
> Let me confirm, an entry |e| in a HeapVector<WeakMember<E>> |v| is cleared by
> the weak processing when |e| is no longer reachable but |v| is still
reachable,
> is that correct?

I meant "when and only when"

[haraken, 2016-08-18 08:35:12]

On 2016/08/18 08:31:53, yhirano wrote:
>
https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/fetch/Resource.cpp (right):
> 
>
https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/fetch/Resource.cpp:727: if (!hasClient(client))
{
> On 2016/08/18 08:06:17, haraken wrote:
> > 
> > I begin to think this is problematic...
> > 
> > If Resource and ResoureClient die at the same time, the weak processing
> doesn't
> > run. However, pre-finalizer runs. Then hasClient() returns true and we end
up
> > with executing line 734 - 744. Which should be problematic... :/
> 
> Hmm, I did think the weak processing is executed in such a case...
> 
> Let me confirm, an entry |e| in a HeapVector<WeakMember<E>> |v| is cleared by
> the weak processing when |e| is no longer reachable but |v| is still
reachable,
> is that correct?

Correct.

If you have objects A and B, and A has a WeakMember to B, the weak processing
runs when B dies before A.

[haraken, 2016-08-18 08:35:47]

On 2016/08/18 08:35:12, haraken wrote:
> On 2016/08/18 08:31:53, yhirano wrote:
> >
>
https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/fetch/Resource.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/fetch/Resource.cpp:727: if
(!hasClient(client))
> {
> > On 2016/08/18 08:06:17, haraken wrote:
> > > 
> > > I begin to think this is problematic...
> > > 
> > > If Resource and ResoureClient die at the same time, the weak processing
> > doesn't
> > > run. However, pre-finalizer runs. Then hasClient() returns true and we end
> up
> > > with executing line 734 - 744. Which should be problematic... :/
> > 
> > Hmm, I did think the weak processing is executed in such a case...
> > 
> > Let me confirm, an entry |e| in a HeapVector<WeakMember<E>> |v| is cleared
by
> > the weak processing when |e| is no longer reachable but |v| is still
> reachable,
> > is that correct?
> 
> Correct.
> 
> If you have objects A and B, and A has a WeakMember to B, the weak processing
> runs when B dies before A.

Right. s/when/when and only when/.

[yhirano, 2016-08-18 08:53:15]

https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:727: if (!hasClient(client)) {
On 2016/08/18 08:06:17, haraken wrote:
> 
> I begin to think this is problematic...
> 
> If Resource and ResoureClient die at the same time, the weak processing
doesn't
> run. However, pre-finalizer runs. Then hasClient() returns true and we end up
> with executing line 734 - 744. Which should be problematic... :/

What problems do we have in such a case? I think L734-L739 (accessing
unreachable heap vectors in a prefinalizer) is safe to process and
didRemoveClientOrObserver is called in any case.

I'm not so sure about ResourceCallback::cancel but it looks consisting of
HeapHashSet operations and CancellableTaskFactory which is not related with
Oilpan.

[haraken, 2016-08-18 09:11:01]

https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:727: if (!hasClient(client)) {
On 2016/08/18 08:53:15, yhirano wrote:
> On 2016/08/18 08:06:17, haraken wrote:
> > 
> > I begin to think this is problematic...
> > 
> > If Resource and ResoureClient die at the same time, the weak processing
> doesn't
> > run. However, pre-finalizer runs. Then hasClient() returns true and we end
up
> > with executing line 734 - 744. Which should be problematic... :/
> 
> What problems do we have in such a case? I think L734-L739 (accessing
> unreachable heap vectors in a prefinalizer) is safe to process and
> didRemoveClientOrObserver is called in any case.
> 
> I'm not so sure about ResourceCallback::cancel but it looks consisting of
> HeapHashSet operations and CancellableTaskFactory which is not related with
> Oilpan.
> 
> 

Does it mean that it's always safe to run line 734 - 744 regardless of whether
we're in a pre-finalizer or not? Then maybe can we just simply the hasClient()
check entirely?

https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:765: // This object may be
dead here.

I think we can now remove this comment because Resource is no longer
ref-counted, right?

[yhirano, 2016-08-23 01:42:34]

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

[commit-bot: I haz the power, 2016-08-23 01:43:28]

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-08-23 03:57:52]

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

[commit-bot: I haz the power, 2016-08-23 03:57:53]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yhirano, 2016-08-24 09:06:26]

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

[commit-bot: I haz the power, 2016-08-24 09:06:57]

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-08-24 11:08:52]

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

[commit-bot: I haz the power, 2016-08-24 11:08:53]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-08-26 01:58:03]

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

[commit-bot: I haz the power, 2016-08-26 01:58:42]

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-08-26 04:31:41]

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

[commit-bot: I haz the power, 2016-08-26 04:31:43]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[yhirano, 2016-08-26 09:12:16]

I'm sorry for the late response, I thought I have replied.

https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:727: if (!hasClient(client)) {
On 2016/08/18 09:11:01, haraken wrote:
> On 2016/08/18 08:53:15, yhirano wrote:
> > On 2016/08/18 08:06:17, haraken wrote:
> > > 
> > > I begin to think this is problematic...
> > > 
> > > If Resource and ResoureClient die at the same time, the weak processing
> > doesn't
> > > run. However, pre-finalizer runs. Then hasClient() returns true and we end
> up
> > > with executing line 734 - 744. Which should be problematic... :/
> > 
> > What problems do we have in such a case? I think L734-L739 (accessing
> > unreachable heap vectors in a prefinalizer) is safe to process and
> > didRemoveClientOrObserver is called in any case.
> > 
> > I'm not so sure about ResourceCallback::cancel but it looks consisting of
> > HeapHashSet operations and CancellableTaskFactory which is not related with
> > Oilpan.
> > 
> > 
> 
> Does it mean that it's always safe to run line 734 - 744 regardless of whether
> we're in a pre-finalizer or not? Then maybe can we just simply the hasClient()
> check entirely?

Even without this change removeClient can be called in the pre-finalization
step. What this CL changes is that m_clients (and other heap collections) will
be swept automatically just before the pre-finalization step. L734-L744 is not
affected by the change because when |client| is swept this branch is taken.

https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:765: // This object may be
dead here.
On 2016/08/18 09:11:01, haraken wrote:
> 
> I think we can now remove this comment because Resource is no longer
> ref-counted, right?
> 

Yes, there are many stale comments but I don't want to fix them in this CL.

[haraken, 2016-08-26 09:49:29]

https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:727: if (!hasClient(client)) {
On 2016/08/26 09:12:16, yhirano wrote:
> On 2016/08/18 09:11:01, haraken wrote:
> > On 2016/08/18 08:53:15, yhirano wrote:
> > > On 2016/08/18 08:06:17, haraken wrote:
> > > > 
> > > > I begin to think this is problematic...
> > > > 
> > > > If Resource and ResoureClient die at the same time, the weak processing
> > > doesn't
> > > > run. However, pre-finalizer runs. Then hasClient() returns true and we
end
> > up
> > > > with executing line 734 - 744. Which should be problematic... :/
> > > 
> > > What problems do we have in such a case? I think L734-L739 (accessing
> > > unreachable heap vectors in a prefinalizer) is safe to process and
> > > didRemoveClientOrObserver is called in any case.
> > > 
> > > I'm not so sure about ResourceCallback::cancel but it looks consisting of
> > > HeapHashSet operations and CancellableTaskFactory which is not related
with
> > > Oilpan.
> > > 
> > > 
> > 
> > Does it mean that it's always safe to run line 734 - 744 regardless of
whether
> > we're in a pre-finalizer or not? Then maybe can we just simply the
hasClient()
> > check entirely?
> 
> Even without this change removeClient can be called in the pre-finalization
> step. What this CL changes is that m_clients (and other heap collections) will
> be swept automatically just before the pre-finalization step. L734-L744 is not
> affected by the change because when |client| is swept this branch is taken.

My question is: Why do we need to have the branch at 727?

If Resource and ResourceClient die at the same time, the weak processing doesn't
run. Then the pre-finalizer will run line 734 - 744. Hence, line 734 - 744 must
be anyway written in a way that doesn't touch any other on-heap objects. If
that's the case, we can just unconditionally run line 734 - 744. Hence we don't
need the branch at 727.

[yhirano, 2016-08-26 10:08:55]

https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:727: if (!hasClient(client)) {
On 2016/08/26 09:49:29, haraken wrote:
> On 2016/08/26 09:12:16, yhirano wrote:
> > On 2016/08/18 09:11:01, haraken wrote:
> > > On 2016/08/18 08:53:15, yhirano wrote:
> > > > On 2016/08/18 08:06:17, haraken wrote:
> > > > > 
> > > > > I begin to think this is problematic...
> > > > > 
> > > > > If Resource and ResoureClient die at the same time, the weak
processing
> > > > doesn't
> > > > > run. However, pre-finalizer runs. Then hasClient() returns true and we
> end
> > > up
> > > > > with executing line 734 - 744. Which should be problematic... :/
> > > > 
> > > > What problems do we have in such a case? I think L734-L739 (accessing
> > > > unreachable heap vectors in a prefinalizer) is safe to process and
> > > > didRemoveClientOrObserver is called in any case.
> > > > 
> > > > I'm not so sure about ResourceCallback::cancel but it looks consisting
of
> > > > HeapHashSet operations and CancellableTaskFactory which is not related
> with
> > > > Oilpan.
> > > > 
> > > > 
> > > 
> > > Does it mean that it's always safe to run line 734 - 744 regardless of
> whether
> > > we're in a pre-finalizer or not? Then maybe can we just simply the
> hasClient()
> > > check entirely?
> > 
> > Even without this change removeClient can be called in the pre-finalization
> > step. What this CL changes is that m_clients (and other heap collections)
will
> > be swept automatically just before the pre-finalization step. L734-L744 is
not
> > affected by the change because when |client| is swept this branch is taken.
> 
> My question is: Why do we need to have the branch at 727?
> 
> If Resource and ResourceClient die at the same time, the weak processing
doesn't
> run. Then the pre-finalizer will run line 734 - 744. Hence, line 734 - 744
must
> be anyway written in a way that doesn't touch any other on-heap objects. If
> that's the case, we can just unconditionally run line 734 - 744. Hence we
don't
> need the branch at 727.
> 
> 

Because it's possible that a Resource is reachable but all ResourceClients
registered are unreachable and hence swept. Imagine a ResourceOwner |o| is
registered to a Resource |r| and the memory cache holds |r|. When |o| becomes
unreachable, this function will be called in |o|'s pre-finalizer, but |o| is
swept from |r->m_clients|. Note that |r| is still reachable via the memory
cache.

[haraken, 2016-08-26 10:19:51]

https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:727: if (!hasClient(client)) {
On 2016/08/26 10:08:55, yhirano wrote:
> On 2016/08/26 09:49:29, haraken wrote:
> > On 2016/08/26 09:12:16, yhirano wrote:
> > > On 2016/08/18 09:11:01, haraken wrote:
> > > > On 2016/08/18 08:53:15, yhirano wrote:
> > > > > On 2016/08/18 08:06:17, haraken wrote:
> > > > > > 
> > > > > > I begin to think this is problematic...
> > > > > > 
> > > > > > If Resource and ResoureClient die at the same time, the weak
> processing
> > > > > doesn't
> > > > > > run. However, pre-finalizer runs. Then hasClient() returns true and
we
> > end
> > > > up
> > > > > > with executing line 734 - 744. Which should be problematic... :/
> > > > > 
> > > > > What problems do we have in such a case? I think L734-L739 (accessing
> > > > > unreachable heap vectors in a prefinalizer) is safe to process and
> > > > > didRemoveClientOrObserver is called in any case.
> > > > > 
> > > > > I'm not so sure about ResourceCallback::cancel but it looks consisting
> of
> > > > > HeapHashSet operations and CancellableTaskFactory which is not related
> > with
> > > > > Oilpan.
> > > > > 
> > > > > 
> > > > 
> > > > Does it mean that it's always safe to run line 734 - 744 regardless of
> > whether
> > > > we're in a pre-finalizer or not? Then maybe can we just simply the
> > hasClient()
> > > > check entirely?
> > > 
> > > Even without this change removeClient can be called in the
pre-finalization
> > > step. What this CL changes is that m_clients (and other heap collections)
> will
> > > be swept automatically just before the pre-finalization step. L734-L744 is
> not
> > > affected by the change because when |client| is swept this branch is
taken.
> > 
> > My question is: Why do we need to have the branch at 727?
> > 
> > If Resource and ResourceClient die at the same time, the weak processing
> doesn't
> > run. Then the pre-finalizer will run line 734 - 744. Hence, line 734 - 744
> must
> > be anyway written in a way that doesn't touch any other on-heap objects. If
> > that's the case, we can just unconditionally run line 734 - 744. Hence we
> don't
> > need the branch at 727.
> > 
> > 
> 
> Because it's possible that a Resource is reachable but all ResourceClients
> registered are unreachable and hence swept. Imagine a ResourceOwner |o| is
> registered to a Resource |r| and the memory cache holds |r|. When |o| becomes
> unreachable, this function will be called in |o|'s pre-finalizer, but |o| is
> swept from |r->m_clients|. Note that |r| is still reachable via the memory
> cache.

It will happen, but what's a problem of running line 734 - 744 then?

[yhirano, 2016-08-26 10:26:12]

https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:727: if (!hasClient(client)) {
On 2016/08/26 10:19:51, haraken wrote:
> On 2016/08/26 10:08:55, yhirano wrote:
> > On 2016/08/26 09:49:29, haraken wrote:
> > > On 2016/08/26 09:12:16, yhirano wrote:
> > > > On 2016/08/18 09:11:01, haraken wrote:
> > > > > On 2016/08/18 08:53:15, yhirano wrote:
> > > > > > On 2016/08/18 08:06:17, haraken wrote:
> > > > > > > 
> > > > > > > I begin to think this is problematic...
> > > > > > > 
> > > > > > > If Resource and ResoureClient die at the same time, the weak
> > processing
> > > > > > doesn't
> > > > > > > run. However, pre-finalizer runs. Then hasClient() returns true
and
> we
> > > end
> > > > > up
> > > > > > > with executing line 734 - 744. Which should be problematic... :/
> > > > > > 
> > > > > > What problems do we have in such a case? I think L734-L739
(accessing
> > > > > > unreachable heap vectors in a prefinalizer) is safe to process and
> > > > > > didRemoveClientOrObserver is called in any case.
> > > > > > 
> > > > > > I'm not so sure about ResourceCallback::cancel but it looks
consisting
> > of
> > > > > > HeapHashSet operations and CancellableTaskFactory which is not
related
> > > with
> > > > > > Oilpan.
> > > > > > 
> > > > > > 
> > > > > 
> > > > > Does it mean that it's always safe to run line 734 - 744 regardless of
> > > whether
> > > > > we're in a pre-finalizer or not? Then maybe can we just simply the
> > > hasClient()
> > > > > check entirely?
> > > > 
> > > > Even without this change removeClient can be called in the
> pre-finalization
> > > > step. What this CL changes is that m_clients (and other heap
collections)
> > will
> > > > be swept automatically just before the pre-finalization step. L734-L744
is
> > not
> > > > affected by the change because when |client| is swept this branch is
> taken.
> > > 
> > > My question is: Why do we need to have the branch at 727?
> > > 
> > > If Resource and ResourceClient die at the same time, the weak processing
> > doesn't
> > > run. Then the pre-finalizer will run line 734 - 744. Hence, line 734 - 744
> > must
> > > be anyway written in a way that doesn't touch any other on-heap objects.
If
> > > that's the case, we can just unconditionally run line 734 - 744. Hence we
> > don't
> > > need the branch at 727.
> > > 
> > > 
> > 
> > Because it's possible that a Resource is reachable but all ResourceClients
> > registered are unreachable and hence swept. Imagine a ResourceOwner |o| is
> > registered to a Resource |r| and the memory cache holds |r|. When |o|
becomes
> > unreachable, this function will be called in |o|'s pre-finalizer, but |o| is
> > swept from |r->m_clients|. Note that |r| is still reachable via the memory
> > cache.
> 
> It will happen, but what's a problem of running line 734 - 744 then?

Hm, I didn't think of that. It looks safe as HashCountedSet::remove is no-op in
such cases.

[haraken, 2016-08-26 10:29:29]

On 2016/08/26 10:26:12, yhirano wrote:
>
https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/fetch/Resource.cpp (right):
> 
>
https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/fetch/Resource.cpp:727: if (!hasClient(client))
{
> On 2016/08/26 10:19:51, haraken wrote:
> > On 2016/08/26 10:08:55, yhirano wrote:
> > > On 2016/08/26 09:49:29, haraken wrote:
> > > > On 2016/08/26 09:12:16, yhirano wrote:
> > > > > On 2016/08/18 09:11:01, haraken wrote:
> > > > > > On 2016/08/18 08:53:15, yhirano wrote:
> > > > > > > On 2016/08/18 08:06:17, haraken wrote:
> > > > > > > > 
> > > > > > > > I begin to think this is problematic...
> > > > > > > > 
> > > > > > > > If Resource and ResoureClient die at the same time, the weak
> > > processing
> > > > > > > doesn't
> > > > > > > > run. However, pre-finalizer runs. Then hasClient() returns true
> and
> > we
> > > > end
> > > > > > up
> > > > > > > > with executing line 734 - 744. Which should be problematic... :/
> > > > > > > 
> > > > > > > What problems do we have in such a case? I think L734-L739
> (accessing
> > > > > > > unreachable heap vectors in a prefinalizer) is safe to process and
> > > > > > > didRemoveClientOrObserver is called in any case.
> > > > > > > 
> > > > > > > I'm not so sure about ResourceCallback::cancel but it looks
> consisting
> > > of
> > > > > > > HeapHashSet operations and CancellableTaskFactory which is not
> related
> > > > with
> > > > > > > Oilpan.
> > > > > > > 
> > > > > > > 
> > > > > > 
> > > > > > Does it mean that it's always safe to run line 734 - 744 regardless
of
> > > > whether
> > > > > > we're in a pre-finalizer or not? Then maybe can we just simply the
> > > > hasClient()
> > > > > > check entirely?
> > > > > 
> > > > > Even without this change removeClient can be called in the
> > pre-finalization
> > > > > step. What this CL changes is that m_clients (and other heap
> collections)
> > > will
> > > > > be swept automatically just before the pre-finalization step.
L734-L744
> is
> > > not
> > > > > affected by the change because when |client| is swept this branch is
> > taken.
> > > > 
> > > > My question is: Why do we need to have the branch at 727?
> > > > 
> > > > If Resource and ResourceClient die at the same time, the weak processing
> > > doesn't
> > > > run. Then the pre-finalizer will run line 734 - 744. Hence, line 734 -
744
> > > must
> > > > be anyway written in a way that doesn't touch any other on-heap objects.
> If
> > > > that's the case, we can just unconditionally run line 734 - 744. Hence
we
> > > don't
> > > > need the branch at 727.
> > > > 
> > > > 
> > > 
> > > Because it's possible that a Resource is reachable but all ResourceClients
> > > registered are unreachable and hence swept. Imagine a ResourceOwner |o| is
> > > registered to a Resource |r| and the memory cache holds |r|. When |o|
> becomes
> > > unreachable, this function will be called in |o|'s pre-finalizer, but |o|
is
> > > swept from |r->m_clients|. Note that |r| is still reachable via the memory
> > > cache.
> > 
> > It will happen, but what's a problem of running line 734 - 744 then?
> 
> Hm, I didn't think of that. It looks safe as HashCountedSet::remove is no-op
in
> such cases.

Look at my comment #132. My point is that if it's not safe to run line 734 - 744
unconditionally, it should be problematic.

[yhirano, 2016-08-26 10:30:04]

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

[commit-bot: I haz the power, 2016-08-26 10:30:18]

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

[yhirano, 2016-08-26 10:32:56]

On 2016/08/26 10:29:29, haraken wrote:
> On 2016/08/26 10:26:12, yhirano wrote:
> >
>
https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/fetch/Resource.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/fetch/Resource.cpp:727: if
(!hasClient(client))
> {
> > On 2016/08/26 10:19:51, haraken wrote:
> > > On 2016/08/26 10:08:55, yhirano wrote:
> > > > On 2016/08/26 09:49:29, haraken wrote:
> > > > > On 2016/08/26 09:12:16, yhirano wrote:
> > > > > > On 2016/08/18 09:11:01, haraken wrote:
> > > > > > > On 2016/08/18 08:53:15, yhirano wrote:
> > > > > > > > On 2016/08/18 08:06:17, haraken wrote:
> > > > > > > > > 
> > > > > > > > > I begin to think this is problematic...
> > > > > > > > > 
> > > > > > > > > If Resource and ResoureClient die at the same time, the weak
> > > > processing
> > > > > > > > doesn't
> > > > > > > > > run. However, pre-finalizer runs. Then hasClient() returns
true
> > and
> > > we
> > > > > end
> > > > > > > up
> > > > > > > > > with executing line 734 - 744. Which should be problematic...
:/
> > > > > > > > 
> > > > > > > > What problems do we have in such a case? I think L734-L739
> > (accessing
> > > > > > > > unreachable heap vectors in a prefinalizer) is safe to process
and
> > > > > > > > didRemoveClientOrObserver is called in any case.
> > > > > > > > 
> > > > > > > > I'm not so sure about ResourceCallback::cancel but it looks
> > consisting
> > > > of
> > > > > > > > HeapHashSet operations and CancellableTaskFactory which is not
> > related
> > > > > with
> > > > > > > > Oilpan.
> > > > > > > > 
> > > > > > > > 
> > > > > > > 
> > > > > > > Does it mean that it's always safe to run line 734 - 744
regardless
> of
> > > > > whether
> > > > > > > we're in a pre-finalizer or not? Then maybe can we just simply the
> > > > > hasClient()
> > > > > > > check entirely?
> > > > > > 
> > > > > > Even without this change removeClient can be called in the
> > > pre-finalization
> > > > > > step. What this CL changes is that m_clients (and other heap
> > collections)
> > > > will
> > > > > > be swept automatically just before the pre-finalization step.
> L734-L744
> > is
> > > > not
> > > > > > affected by the change because when |client| is swept this branch is
> > > taken.
> > > > > 
> > > > > My question is: Why do we need to have the branch at 727?
> > > > > 
> > > > > If Resource and ResourceClient die at the same time, the weak
processing
> > > > doesn't
> > > > > run. Then the pre-finalizer will run line 734 - 744. Hence, line 734 -
> 744
> > > > must
> > > > > be anyway written in a way that doesn't touch any other on-heap
objects.
> > If
> > > > > that's the case, we can just unconditionally run line 734 - 744. Hence
> we
> > > > don't
> > > > > need the branch at 727.
> > > > > 
> > > > > 
> > > > 
> > > > Because it's possible that a Resource is reachable but all
ResourceClients
> > > > registered are unreachable and hence swept. Imagine a ResourceOwner |o|
is
> > > > registered to a Resource |r| and the memory cache holds |r|. When |o|
> > becomes
> > > > unreachable, this function will be called in |o|'s pre-finalizer, but
|o|
> is
> > > > swept from |r->m_clients|. Note that |r| is still reachable via the
memory
> > > > cache.
> > > 
> > > It will happen, but what's a problem of running line 734 - 744 then?
> > 
> > Hm, I didn't think of that. It looks safe as HashCountedSet::remove is no-op
> in
> > such cases.
> 
> Look at my comment #132. My point is that if it's not safe to run line 734 -
744
> unconditionally, it should be problematic.

PS18 removes the branch (it removes the DCHECK from the original code). Waiting
for the trybots results.

[haraken, 2016-08-26 10:38:14]

On 2016/08/26 10:32:56, yhirano wrote:
> On 2016/08/26 10:29:29, haraken wrote:
> > On 2016/08/26 10:26:12, yhirano wrote:
> > >
> >
>
https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/fetch/Resource.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2191633003/diff/420001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/fetch/Resource.cpp:727: if
> (!hasClient(client))
> > {
> > > On 2016/08/26 10:19:51, haraken wrote:
> > > > On 2016/08/26 10:08:55, yhirano wrote:
> > > > > On 2016/08/26 09:49:29, haraken wrote:
> > > > > > On 2016/08/26 09:12:16, yhirano wrote:
> > > > > > > On 2016/08/18 09:11:01, haraken wrote:
> > > > > > > > On 2016/08/18 08:53:15, yhirano wrote:
> > > > > > > > > On 2016/08/18 08:06:17, haraken wrote:
> > > > > > > > > > 
> > > > > > > > > > I begin to think this is problematic...
> > > > > > > > > > 
> > > > > > > > > > If Resource and ResoureClient die at the same time, the weak
> > > > > processing
> > > > > > > > > doesn't
> > > > > > > > > > run. However, pre-finalizer runs. Then hasClient() returns
> true
> > > and
> > > > we
> > > > > > end
> > > > > > > > up
> > > > > > > > > > with executing line 734 - 744. Which should be
problematic...
> :/
> > > > > > > > > 
> > > > > > > > > What problems do we have in such a case? I think L734-L739
> > > (accessing
> > > > > > > > > unreachable heap vectors in a prefinalizer) is safe to process
> and
> > > > > > > > > didRemoveClientOrObserver is called in any case.
> > > > > > > > > 
> > > > > > > > > I'm not so sure about ResourceCallback::cancel but it looks
> > > consisting
> > > > > of
> > > > > > > > > HeapHashSet operations and CancellableTaskFactory which is not
> > > related
> > > > > > with
> > > > > > > > > Oilpan.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > Does it mean that it's always safe to run line 734 - 744
> regardless
> > of
> > > > > > whether
> > > > > > > > we're in a pre-finalizer or not? Then maybe can we just simply
the
> > > > > > hasClient()
> > > > > > > > check entirely?
> > > > > > > 
> > > > > > > Even without this change removeClient can be called in the
> > > > pre-finalization
> > > > > > > step. What this CL changes is that m_clients (and other heap
> > > collections)
> > > > > will
> > > > > > > be swept automatically just before the pre-finalization step.
> > L734-L744
> > > is
> > > > > not
> > > > > > > affected by the change because when |client| is swept this branch
is
> > > > taken.
> > > > > > 
> > > > > > My question is: Why do we need to have the branch at 727?
> > > > > > 
> > > > > > If Resource and ResourceClient die at the same time, the weak
> processing
> > > > > doesn't
> > > > > > run. Then the pre-finalizer will run line 734 - 744. Hence, line 734
-
> > 744
> > > > > must
> > > > > > be anyway written in a way that doesn't touch any other on-heap
> objects.
> > > If
> > > > > > that's the case, we can just unconditionally run line 734 - 744.
Hence
> > we
> > > > > don't
> > > > > > need the branch at 727.
> > > > > > 
> > > > > > 
> > > > > 
> > > > > Because it's possible that a Resource is reachable but all
> ResourceClients
> > > > > registered are unreachable and hence swept. Imagine a ResourceOwner
|o|
> is
> > > > > registered to a Resource |r| and the memory cache holds |r|. When |o|
> > > becomes
> > > > > unreachable, this function will be called in |o|'s pre-finalizer, but
> |o|
> > is
> > > > > swept from |r->m_clients|. Note that |r| is still reachable via the
> memory
> > > > > cache.
> > > > 
> > > > It will happen, but what's a problem of running line 734 - 744 then?
> > > 
> > > Hm, I didn't think of that. It looks safe as HashCountedSet::remove is
no-op
> > in
> > > such cases.
> > 
> > Look at my comment #132. My point is that if it's not safe to run line 734 -
> 744
> > unconditionally, it should be problematic.
> 
> PS18 removes the branch (it removes the DCHECK from the original code).
Waiting
> for the trybots results.

Thanks, LGTM.

Please just double-check that ResourceCallback::callbackHandler().cancel()
doesn't touch any on-heap objects.

[yhirano, 2016-08-26 11:15:38]

On 2016/08/26 10:38:14, haraken wrote:

(snip)
> 
> Please just double-check that ResourceCallback::callbackHandler().cancel()
> doesn't touch any on-heap objects.

It sometimes creates an on-heap object. Is it a problem? If so, I'll fix it
separately as this CL doesn't change the situation.

[haraken, 2016-08-26 13:20:41]

On 2016/08/26 11:15:38, yhirano wrote:
> On 2016/08/26 10:38:14, haraken wrote:
> 
> (snip)
> > 
> > Please just double-check that ResourceCallback::callbackHandler().cancel()
> > doesn't touch any on-heap objects.
> 
> It sometimes creates an on-heap object. Is it a problem? If so, I'll fix it
> separately as this CL doesn't change the situation.

Yeah, it's not allowed to create an on-heap object during a pre-finalizer. I
think it will already hit DCHECK though..

[yhirano, 2016-08-26 13:31:30]

On 2016/08/26 13:20:41, haraken wrote:
> On 2016/08/26 11:15:38, yhirano wrote:
> > On 2016/08/26 10:38:14, haraken wrote:
> > 
> > (snip)
> > > 
> > > Please just double-check that ResourceCallback::callbackHandler().cancel()
> > > doesn't touch any on-heap objects.
> > 
> > It sometimes creates an on-heap object. Is it a problem? If so, I'll fix it
> > separately as this CL doesn't change the situation.
> 
> Yeah, it's not allowed to create an on-heap object during a pre-finalizer. I
> think it will already hit DCHECK though..

It's a static variable initialization so I think it's very rare if possible. But
I can't prove it's impossible.

[commit-bot: I haz the power, 2016-08-26 14:04:19]

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

[commit-bot: I haz the power, 2016-08-26 14:04:21]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-08-26 15:52:20]

On 2016/08/26 13:31:30, yhirano wrote:
> On 2016/08/26 13:20:41, haraken wrote:
> > On 2016/08/26 11:15:38, yhirano wrote:
> > > On 2016/08/26 10:38:14, haraken wrote:
> > > 
> > > (snip)
> > > > 
> > > > Please just double-check that
ResourceCallback::callbackHandler().cancel()
> > > > doesn't touch any on-heap objects.
> > > 
> > > It sometimes creates an on-heap object. Is it a problem? If so, I'll fix
it
> > > separately as this CL doesn't change the situation.
> > 
> > Yeah, it's not allowed to create an on-heap object during a pre-finalizer. I
> > think it will already hit DCHECK though..
> 
> It's a static variable initialization so I think it's very rare if possible.
But
> I can't prove it's impossible.

https://codereview.chromium.org/2287483003/

[yhirano, 2016-08-29 04:07:14]

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

[commit-bot: I haz the power, 2016-08-29 04:07:32]

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-08-29 04:18:43]

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

[commit-bot: I haz the power, 2016-08-29 04:18:45]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[yhirano, 2016-08-29 04:31:41]

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

[commit-bot: I haz the power, 2016-08-29 04:31:56]

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-08-29 06:22:48]

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

[commit-bot: I haz the power, 2016-08-29 06:22:50]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[yhirano, 2016-08-29 09:39:00]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-08-29 09:39:01]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2191633003/#ps520001
(title: "build fix")

[commit-bot: I haz the power, 2016-08-29 09:39:17]

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-08-29 11:07:59]

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

[commit-bot: I haz the power, 2016-08-29 11:08:02]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[yhirano, 2016-08-29 11:19:47]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2016-08-29 11:20:07]

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-08-29 12:03:27]

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

[commit-bot: I haz the power, 2016-08-29 12:03:30]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[yhirano, 2016-08-29 13:52:53]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2016-08-29 13:53:12]

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-08-29 14:35:59]

Description was changed from

==========
Move ResourceClient to Oilpan heap

Now Resource has its clients as weak members.

BUG=587663
==========

to

==========
Move ResourceClient to Oilpan heap

Now Resource has its clients as weak members.

BUG=587663
==========

[commit-bot: I haz the power, 2016-08-29 14:36:03]

Committed patchset #20 (id:520001)

[commit-bot: I haz the power, 2016-08-29 14:37:18]

Description was changed from

==========
Move ResourceClient to Oilpan heap

Now Resource has its clients as weak members.

BUG=587663
==========

to

==========
Move ResourceClient to Oilpan heap

Now Resource has its clients as weak members.

BUG=587663

Committed: https://crrev.com/9c3053df29fa90acbd9c2187f94807ff546b0981
Cr-Commit-Position: refs/heads/master@{#415007}
==========

[commit-bot: I haz the power, 2016-08-29 14:37:20]

Patchset 20 (id:??) landed as
https://crrev.com/9c3053df29fa90acbd9c2187f94807ff546b0981
Cr-Commit-Position: refs/heads/master@{#415007}