Specialize base::IsWeakReceiver for Blink weak pointers to support base::Bind (1/5)

Specialize base::IsWeakReceiver for Blink weak pointers to support base::Bind

* Specialize base::IsWeakReceiver for Blink weak pointers, so that
  base::Bind can handle them as weak pointers.
* Unify method invocation forms in WTF::FunctionWrapper using
  base::IsWeakReceiver.
* Add wrapWeakPersistent() and wrapCrossThreadWeakPersistent().
* Remove WeakPersistentThisPointer and CrossThreadWeakPersistentThisPointer,
  which are no longer needed.

BUG=597856
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/4b2899d40575fd381ba861b4a711a20ab39bc1d3
Cr-Commit-Position: refs/heads/master@{#401878}

[tzik, 2016-06-23 09:30:29]

Description was changed from

==========
Remove WeakPersistentThisPointer

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

to

==========
Remove WeakPersistentThisPointer

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[tzik, 2016-06-23 09:42:37]

Description was changed from

==========
Remove WeakPersistentThisPointer

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Specialize base::IsWeakReceiver for Blink weak pointers to support base::Bind

* Specialize base::IsWeakReceiver for Blink weak pointers, so that
  base::Bind can handle them as weak pointers.
* Unify method invocation forms in WTF::FunctionWrapper using
  base::IsWeakReceiver.
* Add wrapWeakPersistent() and wrapCrossThreadWeakPersistent().
* Remove WeakPersistentThisPointer and CrossThreadWeakPersistentThisPointer,
  which are no longer needed.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[tzik, 2016-06-23 09:43:33]

tzik@chromium.org changed reviewers:
+ haraken@chromium.org, sigbjornf@opera.com, yutak@chromium.org

[tzik, 2016-06-23 09:44:59]

Description was changed from

==========
Specialize base::IsWeakReceiver for Blink weak pointers to support base::Bind

* Specialize base::IsWeakReceiver for Blink weak pointers, so that
  base::Bind can handle them as weak pointers.
* Unify method invocation forms in WTF::FunctionWrapper using
  base::IsWeakReceiver.
* Add wrapWeakPersistent() and wrapCrossThreadWeakPersistent().
* Remove WeakPersistentThisPointer and CrossThreadWeakPersistentThisPointer,
  which are no longer needed.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Specialize base::IsWeakReceiver for Blink weak pointers to support base::Bind

* Specialize base::IsWeakReceiver for Blink weak pointers, so that
  base::Bind can handle them as weak pointers.
* Unify method invocation forms in WTF::FunctionWrapper using
  base::IsWeakReceiver.
* Add wrapWeakPersistent() and wrapCrossThreadWeakPersistent().
* Remove WeakPersistentThisPointer and CrossThreadWeakPersistentThisPointer,
  which are no longer needed.

BUG=597856
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[tzik, 2016-06-23 09:47:45]

tzik@chromium.org changed reviewers:
+ hiroshige@chromium.org

[tzik, 2016-06-23 09:47:46]

PTAL

[hiroshige, 2016-06-23 09:56:18]

Basically looks good, great!

https://codereview.chromium.org/2091713002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/CancellableTaskFactory.h
(right):

https://codereview.chromium.org/2091713002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/CancellableTaskFactory.h:42: return
wrapUnique(new CancellableTaskFactory(WTF::bind(method,
wrapCrossThreadWeakPersistent(thisObject))));
This is WTF::bind() (not threadSafeBind()), so can we use wrapPersistent() here?

(CrossThreadWeakPersistentThisPointer is older than WeakPersistentThisPointer
and there might be same-thread usage of CrossThreadWeakPersistentThisPointer, I
suspect, but not sure)

[sof, 2016-06-23 11:07:48]

some offscreencanvas/ImageBitmap failures reported by the bots.

[tzik, 2016-06-23 11:47:49]

On 2016/06/23 11:07:48, sof wrote:
> some offscreencanvas/ImageBitmap failures reported by the bots.

That's due to a CrossThreadWeakPersistent-to-WeakPersistent replacement in
WebGLRenderingContext. Let me revert it and see the try result.

[tzik, 2016-06-23 11:48:01]

https://codereview.chromium.org/2091713002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/CancellableTaskFactory.h
(right):

https://codereview.chromium.org/2091713002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/CancellableTaskFactory.h:42: return
wrapUnique(new CancellableTaskFactory(WTF::bind(method,
wrapCrossThreadWeakPersistent(thisObject))));
On 2016/06/23 09:56:18, hiroshige (slow) wrote:
> This is WTF::bind() (not threadSafeBind()), so can we use wrapPersistent()
here?
> 
> (CrossThreadWeakPersistentThisPointer is older than WeakPersistentThisPointer
> and there might be same-thread usage of CrossThreadWeakPersistentThisPointer,
I
> suspect, but not sure)

Done.

[tzik, 2016-06-24 05:15:36]

PTAL

[sof, 2016-06-24 05:24:08]

lgtm, happy to see Weak*ThisPointer<> go.

https://codereview.chromium.org/2091713002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2091713002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1161:
drawingBuffer()->addNewMailboxCallback(nullptr);
It makes sense, but why is this needed now?

[haraken, 2016-06-24 05:32:57]

LGTM

[tzik, 2016-06-24 05:57:06]

https://codereview.chromium.org/2091713002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2091713002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1161:
drawingBuffer()->addNewMailboxCallback(nullptr);
On 2016/06/24 05:24:08, sof wrote:
> It makes sense, but why is this needed now?

The error caught a Persistent leak after the termination GC.
That's due to DrawingBuffer holds a WeakPersintent of WebGLRendingContextBase
through the WTF::Function, and probably lives after the heap shutdown.
I don't know why the test didn't catch CrossThreadWeakPersistent...

[sof, 2016-06-24 06:45:32]

https://codereview.chromium.org/2091713002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2091713002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1161:
drawingBuffer()->addNewMailboxCallback(nullptr);
On 2016/06/24 05:57:06, tzik wrote:
> On 2016/06/24 05:24:08, sof wrote:
> > It makes sense, but why is this needed now?
> 
> The error caught a Persistent leak after the termination GC.
> That's due to DrawingBuffer holds a WeakPersintent of WebGLRendingContextBase
> through the WTF::Function, and probably lives after the heap shutdown.
> I don't know why the test didn't catch CrossThreadWeakPersistent...

CT(W)Ps are exempt from checking for leaks when running termination GCs.

But why would the main thread be running a termination GC?

[tzik, 2016-06-24 07:04:58]

https://codereview.chromium.org/2091713002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2091713002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1161:
drawingBuffer()->addNewMailboxCallback(nullptr);
On 2016/06/24 06:45:32, sof wrote:
> On 2016/06/24 05:57:06, tzik wrote:
> > On 2016/06/24 05:24:08, sof wrote:
> > > It makes sense, but why is this needed now?
> > 
> > The error caught a Persistent leak after the termination GC.
> > That's due to DrawingBuffer holds a WeakPersintent of
WebGLRendingContextBase
> > through the WTF::Function, and probably lives after the heap shutdown.
> > I don't know why the test didn't catch CrossThreadWeakPersistent...
> 
> CT(W)Ps are exempt from checking for leaks when running termination GCs.
Makes sense.

> 
> But why would the main thread be running a termination GC?

The instance seems to be created on other thread than main according to line
578.

[sof, 2016-06-24 07:07:48]

On 2016/06/24 07:04:58, tzik wrote:
>
https://codereview.chromium.org/2091713002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
> (right):
> 
>
https://codereview.chromium.org/2091713002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1161:
> drawingBuffer()->addNewMailboxCallback(nullptr);
> On 2016/06/24 06:45:32, sof wrote:
> > On 2016/06/24 05:57:06, tzik wrote:
> > > On 2016/06/24 05:24:08, sof wrote:
> > > > It makes sense, but why is this needed now?
> > > 
> > > The error caught a Persistent leak after the termination GC.
> > > That's due to DrawingBuffer holds a WeakPersintent of
> WebGLRendingContextBase
> > > through the WTF::Function, and probably lives after the heap shutdown.
> > > I don't know why the test didn't catch CrossThreadWeakPersistent...
> > 
> > CT(W)Ps are exempt from checking for leaks when running termination GCs.
> Makes sense.
> 
> > 
> > But why would the main thread be running a termination GC?
> 
> The instance seems to be created on other thread than main according to line
> 578.

ok, but then it ought to be a cross-thread weak persistent?

[tzik, 2016-06-24 07:32:36]

On 2016/06/24 07:07:48, sof wrote:
> On 2016/06/24 07:04:58, tzik wrote:
> >
>
https://codereview.chromium.org/2091713002/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2091713002/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1161:
> > drawingBuffer()->addNewMailboxCallback(nullptr);
> > On 2016/06/24 06:45:32, sof wrote:
> > > On 2016/06/24 05:57:06, tzik wrote:
> > > > On 2016/06/24 05:24:08, sof wrote:
> > > > > It makes sense, but why is this needed now?
> > > > 
> > > > The error caught a Persistent leak after the termination GC.
> > > > That's due to DrawingBuffer holds a WeakPersintent of
> > WebGLRendingContextBase
> > > > through the WTF::Function, and probably lives after the heap shutdown.
> > > > I don't know why the test didn't catch CrossThreadWeakPersistent...
> > > 
> > > CT(W)Ps are exempt from checking for leaks when running termination GCs.
> > Makes sense.
> > 
> > > 
> > > But why would the main thread be running a termination GC?
> > 
> > The instance seems to be created on other thread than main according to line
> > 578.
> 
> ok, but then it ought to be a cross-thread weak persistent?

Once the instance is created, the instance will stick in the thread, as we use
non-thread-safe bind and RefCounted around here.

[sof, 2016-06-24 08:55:34]

On 2016/06/24 07:32:36, tzik wrote:
> On 2016/06/24 07:07:48, sof wrote:
> > On 2016/06/24 07:04:58, tzik wrote:
> > >
> >
>
https://codereview.chromium.org/2091713002/diff/60001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2091713002/diff/60001/third_party/WebKit/Sour...
> > >
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1161:
> > > drawingBuffer()->addNewMailboxCallback(nullptr);
> > > On 2016/06/24 06:45:32, sof wrote:
> > > > On 2016/06/24 05:57:06, tzik wrote:
> > > > > On 2016/06/24 05:24:08, sof wrote:
> > > > > > It makes sense, but why is this needed now?
> > > > > 
> > > > > The error caught a Persistent leak after the termination GC.
> > > > > That's due to DrawingBuffer holds a WeakPersintent of
> > > WebGLRendingContextBase
> > > > > through the WTF::Function, and probably lives after the heap shutdown.
> > > > > I don't know why the test didn't catch CrossThreadWeakPersistent...
> > > > 
> > > > CT(W)Ps are exempt from checking for leaks when running termination GCs.
> > > Makes sense.
> > > 
> > > > 
> > > > But why would the main thread be running a termination GC?
> > > 
> > > The instance seems to be created on other thread than main according to
line
> > > 578.
> > 
> > ok, but then it ought to be a cross-thread weak persistent?
> 
> Once the instance is created, the instance will stick in the thread, as we use
> non-thread-safe bind and RefCounted around here.

I see what's happening now, this is for a worker thread creating offscreen
canvases.

But WeakPersistent<>s are cleared during weak processing, but not deallocated,
so the persistent (node) count won't decrease, causing the termination GC to
assert about persistents leaking. I think we should fix that, but I'm fine with
doing the explicit reset of the callback as done here regardless.

[sof, 2016-06-24 08:56:02]

https://codereview.chromium.org/2091713002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Persistent.h (right):

https://codereview.chromium.org/2091713002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Persistent.h:711: struct
IsWeakReceiver<blink::WeakPersistent<T>> : std::true_type {};
It would be good to have (HeapTest) unit tests for this.

[tzik, 2016-06-24 09:41:41]

https://codereview.chromium.org/2091713002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Persistent.h (right):

https://codereview.chromium.org/2091713002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Persistent.h:711: struct
IsWeakReceiver<blink::WeakPersistent<T>> : std::true_type {};
On 2016/06/24 08:56:02, sof wrote:
> It would be good to have (HeapTest) unit tests for this.

Done.

[sof, 2016-06-24 09:45:14]

lgtm, thanks.

(i'll follow up on the clearing of weak persistents.)

[tzik, 2016-06-24 14:20:03]

haraken: Just in case, could you take another look to diff to PS4? I added a
test file there.

[haraken, 2016-06-24 14:25:08]

LGTM

[tzik, 2016-06-24 14:26:14]

The CQ bit was checked by tzik@chromium.org

[tzik, 2016-06-24 14:26:15]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/2091713002/#ps120001
(title: "+#include for build fix")

[commit-bot: I haz the power, 2016-06-24 14:26:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2091713002/120001

[commit-bot: I haz the power, 2016-06-24 16:33:45]

Description was changed from

==========
Specialize base::IsWeakReceiver for Blink weak pointers to support base::Bind

* Specialize base::IsWeakReceiver for Blink weak pointers, so that
  base::Bind can handle them as weak pointers.
* Unify method invocation forms in WTF::FunctionWrapper using
  base::IsWeakReceiver.
* Add wrapWeakPersistent() and wrapCrossThreadWeakPersistent().
* Remove WeakPersistentThisPointer and CrossThreadWeakPersistentThisPointer,
  which are no longer needed.

BUG=597856
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Specialize base::IsWeakReceiver for Blink weak pointers to support base::Bind

* Specialize base::IsWeakReceiver for Blink weak pointers, so that
  base::Bind can handle them as weak pointers.
* Unify method invocation forms in WTF::FunctionWrapper using
  base::IsWeakReceiver.
* Add wrapWeakPersistent() and wrapCrossThreadWeakPersistent().
* Remove WeakPersistentThisPointer and CrossThreadWeakPersistentThisPointer,
  which are no longer needed.

BUG=597856
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-06-24 16:33:47]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-24 16:37:09]

Description was changed from

==========
Specialize base::IsWeakReceiver for Blink weak pointers to support base::Bind

* Specialize base::IsWeakReceiver for Blink weak pointers, so that
  base::Bind can handle them as weak pointers.
* Unify method invocation forms in WTF::FunctionWrapper using
  base::IsWeakReceiver.
* Add wrapWeakPersistent() and wrapCrossThreadWeakPersistent().
* Remove WeakPersistentThisPointer and CrossThreadWeakPersistentThisPointer,
  which are no longer needed.

BUG=597856
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Specialize base::IsWeakReceiver for Blink weak pointers to support base::Bind

* Specialize base::IsWeakReceiver for Blink weak pointers, so that
  base::Bind can handle them as weak pointers.
* Unify method invocation forms in WTF::FunctionWrapper using
  base::IsWeakReceiver.
* Add wrapWeakPersistent() and wrapCrossThreadWeakPersistent().
* Remove WeakPersistentThisPointer and CrossThreadWeakPersistentThisPointer,
  which are no longer needed.

BUG=597856
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/4b2899d40575fd381ba861b4a711a20ab39bc1d3
Cr-Commit-Position: refs/heads/master@{#401878}
==========

[commit-bot: I haz the power, 2016-06-24 16:37:12]

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

[sof, 2016-06-27 08:53:17]

On 2016/06/24 08:55:34, sof wrote:
> On 2016/06/24 07:32:36, tzik wrote:
> > On 2016/06/24 07:07:48, sof wrote:
> > > On 2016/06/24 07:04:58, tzik wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2091713002/diff/60001/third_party/WebKit/Sour...
> > > > File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2091713002/diff/60001/third_party/WebKit/Sour...
> > > >
> third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1161:
> > > > drawingBuffer()->addNewMailboxCallback(nullptr);
> > > > On 2016/06/24 06:45:32, sof wrote:
> > > > > On 2016/06/24 05:57:06, tzik wrote:
> > > > > > On 2016/06/24 05:24:08, sof wrote:
> > > > > > > It makes sense, but why is this needed now?
> > > > > > 
> > > > > > The error caught a Persistent leak after the termination GC.
> > > > > > That's due to DrawingBuffer holds a WeakPersintent of
> > > > WebGLRendingContextBase
> > > > > > through the WTF::Function, and probably lives after the heap
shutdown.
> > > > > > I don't know why the test didn't catch CrossThreadWeakPersistent...
> > > > > 
> > > > > CT(W)Ps are exempt from checking for leaks when running termination
GCs.
> > > > Makes sense.
> > > > 
> > > > > 
> > > > > But why would the main thread be running a termination GC?
> > > > 
> > > > The instance seems to be created on other thread than main according to
> line
> > > > 578.
> > > 
> > > ok, but then it ought to be a cross-thread weak persistent?
> > 
> > Once the instance is created, the instance will stick in the thread, as we
use
> > non-thread-safe bind and RefCounted around here.
> 
> I see what's happening now, this is for a worker thread creating offscreen
> canvases.
> 
> But WeakPersistent<>s are cleared during weak processing, but not deallocated,
> so the persistent (node) count won't decrease, causing the termination GC to
> assert about persistents leaking. I think we should fix that, but I'm fine
with
> doing the explicit reset of the callback as done here regardless.

ftr, https://codereview.chromium.org/2094973002/ addressed weak persistent nodes
not being freed & with it in scope confirmed not to require the clearing of the
mailbox callback here. (But it makes sense to do so regardless.)