Wrap GCed raw pointer parameters of WTF::bind with Persistent

Wrap GCed raw pointer parameters of WTF::bind with Persistent

This CL adds `wrapPersistent` to all GCed raw pointer prameters
of WTF::bind, as a preparation of raw pointer removal on WTF::bind.

WTF::bind and blink::threadSafeBind wrap raw pointers into Persistent if
it's a GCed type. However, the lifetime management based on the type get
considered harmful these days, and should eventually be removed.
This CL and following CLs annotate all raw pointers on WTF::bind, and
ban all raw pointers.

BUG=597856
Committed: https://crrev.com/8359caf0b6a4b86949be98b12147c926fb58d2a8
Cr-Commit-Position: refs/heads/master@{#401558}

[tzik, 2016-06-08 11:59:55]

Description was changed from

==========
Disallow raw pointers on WTF::bind

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

to

==========
Disallow raw pointers on WTF::bind

123456789012345678901234567890123456789012345678901234567890123456789012

This CL introduces WTF::unretained, which is the Blink version of
base::Unretained, wraps all raw pointers on WTF::bind with WTF::unretained
or blink::wrapPersistent, bans all raw pointers on WTF::bind.

This is a preparation to add base::Bind() based WTF::bind().

Note that this CL contains mostly no behavior change, except for changes
on RefCounted types in RepectedPromise and Worklet.

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

[tzik, 2016-06-08 11:59:55]

tzik@chromium.org changed reviewers:
+ haraken@chromium.org, hiroshige@chromium.org, yhirano@chromium.org

[tzik, 2016-06-08 12:01:34]

Description was changed from

==========
Disallow raw pointers on WTF::bind

123456789012345678901234567890123456789012345678901234567890123456789012

This CL introduces WTF::unretained, which is the Blink version of
base::Unretained, wraps all raw pointers on WTF::bind with WTF::unretained
or blink::wrapPersistent, bans all raw pointers on WTF::bind.

This is a preparation to add base::Bind() based WTF::bind().

Note that this CL contains mostly no behavior change, except for changes
on RefCounted types in RepectedPromise and Worklet.

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

to

==========
Disallow raw pointers on WTF::bind

123456789012345678901234567890123456789012345678901234567890123456789012

This CL introduces WTF::unretained, which is the Blink version of
base::Unretained, wraps all raw pointers on WTF::bind with WTF::unretained
or blink::wrapPersistent, bans all raw pointers on WTF::bind.

This is a preparation to add base::Bind() based WTF::bind().

Note that this CL contains mostly no behavior change, except for changes
on RefCounted types in RepectedPromise and Worklet.

BUG=597856
==========

[tzik, 2016-06-08 12:02:00]

Description was changed from

==========
Disallow raw pointers on WTF::bind

123456789012345678901234567890123456789012345678901234567890123456789012

This CL introduces WTF::unretained, which is the Blink version of
base::Unretained, wraps all raw pointers on WTF::bind with WTF::unretained
or blink::wrapPersistent, bans all raw pointers on WTF::bind.

This is a preparation to add base::Bind() based WTF::bind().

Note that this CL contains mostly no behavior change, except for changes
on RefCounted types in RepectedPromise and Worklet.

BUG=597856
==========

to

==========
Disallow raw pointers on WTF::bind

This CL introduces WTF::unretained, which is the Blink version of
base::Unretained, wraps all raw pointers on WTF::bind with WTF::unretained
or blink::wrapPersistent, bans all raw pointers on WTF::bind.

This is a preparation to add base::Bind() based WTF::bind().

Note that this CL contains mostly no behavior change, except for changes
on RefCounted types in RepectedPromise and Worklet.

BUG=597856
==========

[tzik, 2016-06-08 12:02:21]

PTAL

[Yuta Kitamura, 2016-06-09 04:20:20]

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

[Yuta Kitamura, 2016-06-09 04:20:21]

Didn't look into the actual code, but if you want to land
this, I think you need to give a PSA on blink-dev.

[yhirano, 2016-06-09 08:47:39]

bindings and modules/fetch lgtm.


The |this| pointer in the bind call in RejectedPromises  once had
PassRefPtrWillBeRawPtr, but it lost the PassRefPtr part when Oilpan is enabled
by default[1][2], and RejectedPromises was de-oilpanized[3] after that. So it
should be PassRefPtr, I think.

1: https://codereview.chromium.org/1853743005
2: https://codereview.chromium.org/1878943003
3: https://codereview.chromium.org/1601283003

[tzik, 2016-06-09 09:08:11]

On 2016/06/09 04:20:21, Yuta Kitamura wrote:
> Didn't look into the actual code, but if you want to land
> this, I think you need to give a PSA on blink-dev.

Hiroshige-san seems adding WTF::unretained() in the other CL.
Let me split the "unretained" part and "disallow pointers" part of this CL into
separate CLs, so that the "wrapPersistent" part can work independently.

[tzik, 2016-06-09 09:36:03]

Description was changed from

==========
Disallow raw pointers on WTF::bind

This CL introduces WTF::unretained, which is the Blink version of
base::Unretained, wraps all raw pointers on WTF::bind with WTF::unretained
or blink::wrapPersistent, bans all raw pointers on WTF::bind.

This is a preparation to add base::Bind() based WTF::bind().

Note that this CL contains mostly no behavior change, except for changes
on RefCounted types in RepectedPromise and Worklet.

BUG=597856
==========

to

==========
Wrap GCed raw pointer parameters of WTF::bind with CrossThreadPersistent

This CL adds `wrapCrossThreadPersistent` to all GCed raw pointer prameters
of WTF::bind, as a preparation of raw pointer removal on WTF::bind.

WTF::bind and blink::threadSafeBind wrap raw pointers into Persistent if
it's a GCed type. However, the lifetime management based on the type get
considered harmful these days, and should eventually be removed.
This CL and following CLs annotate all raw pointers on WTF::bind, and
ban all raw pointers.

BUG=597856
==========

[tzik, 2016-06-09 09:36:30]

Updated. PTAL.

[hiroshige, 2016-06-15 12:12:39]

About CL description:
> WTF::bind and blink::threadSafeBind wrap raw pointers into Persistent
s/Persistent/CrossThreadPersistent/.

Also, could you mention that why this CL applies wrapCrossThreadPersistent(),
not wrapPersistent(), e.g.
- to keep the behavior
- In most cases we can replace wrapCrossThreadPersistent() with wrapPersistent()
in the same-thread cases, but it is not always possible due to the reason we
discussed offline.

https://codereview.chromium.org/2049003002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/RejectedPromises.cpp (right):

https://codereview.chromium.org/2049003002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/RejectedPromises.cpp:213:
Platform::current()->currentThread()->scheduler()->timerTaskRunner()->postTask(BLINK_FROM_HERE,
bind(&RejectedPromises::revokeNow, RefPtr<RejectedPromises>(this),
passed(std::move(message))));
I think it is better to split this change into a separate CL, because this
changes the behavior while other changes don't change the behavior and quite
mechanical.
(this itself looks good according to yhirano@'s comment)

https://codereview.chromium.org/2049003002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right):

https://codereview.chromium.org/2049003002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:297:
Microtask::enqueueMicrotask(WTF::bind(&HTMLSlotElement::dispatchSlotChangeEvent,
wrapCrossThreadPersistent(this)));
Shouldn't this be |wrapPersistent(this)| to keep the current behavior, because
it is originally Persistent<HTMLSlotElement>?

[tzik, 2016-06-21 09:09:04]

https://codereview.chromium.org/2049003002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/RejectedPromises.cpp (right):

https://codereview.chromium.org/2049003002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/RejectedPromises.cpp:213:
Platform::current()->currentThread()->scheduler()->timerTaskRunner()->postTask(BLINK_FROM_HERE,
bind(&RejectedPromises::revokeNow, RefPtr<RejectedPromises>(this),
passed(std::move(message))));
On 2016/06/15 12:12:39, hiroshige (slow) wrote:
> I think it is better to split this change into a separate CL, because this
> changes the behavior while other changes don't change the behavior and quite
> mechanical.
> (this itself looks good according to yhirano@'s comment)

Done. I split this part into https://codereview.chromium.org/2083953002/

https://codereview.chromium.org/2049003002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right):

https://codereview.chromium.org/2049003002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:297:
Microtask::enqueueMicrotask(WTF::bind(&HTMLSlotElement::dispatchSlotChangeEvent,
wrapCrossThreadPersistent(this)));
On 2016/06/15 12:12:39, hiroshige (slow) wrote:
> Shouldn't this be |wrapPersistent(this)| to keep the current behavior, because
> it is originally Persistent<HTMLSlotElement>?

Done.

[kouhei (in TOK), 2016-06-22 06:27:25]

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

[kouhei (in TOK), 2016-06-22 06:27:26]

Source/core & Source/modules lgtm

[haraken, 2016-06-22 06:27:42]

haraken@chromium.org changed reviewers:
- kouhei@chromium.org

[haraken, 2016-06-22 06:27:43]

It looks like this CL is abusing CrossThreadPersistents. We should use
wrapPersistent() for tasks that run on the same thread.

https://codereview.chromium.org/2049003002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ExecutionContext.cpp (right):

https://codereview.chromium.org/2049003002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ExecutionContext.cpp:97:
postTask(BLINK_FROM_HERE,
createSameThreadTask(&ExecutionContext::runSuspendableTasks,
wrapCrossThreadPersistent(this)));

It looks strange that you use a CrossThreadPersistent for the same-thread task.

https://codereview.chromium.org/2049003002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ExecutionContext.cpp:123:
postTask(BLINK_FROM_HERE,
createSameThreadTask(&ExecutionContext::runSuspendableTasks,
wrapCrossThreadPersistent(this)));

Ditto.

https://codereview.chromium.org/2049003002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/MemoryCacheTest.cpp (right):

https://codereview.chromium.org/2049003002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/MemoryCacheTest.cpp:243:
Platform::current()->currentThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE,
bind(&runTask1, wrapCrossThreadPersistent(cachedLiveResource),
wrapCrossThreadPersistent(cachedDeadResource)));

Does this need to be CrossThreadPersistent?

https://codereview.chromium.org/2049003002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fileapi/FileReader.cpp (right):

https://codereview.chromium.org/2049003002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fileapi/FileReader.cpp:338: BLINK_FROM_HERE,
createSameThreadTask(&delayedAbort, wrapCrossThreadPersistent(this)));

Ditto.

https://codereview.chromium.org/2049003002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/2049003002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:411:
m_document->postTask(BLINK_FROM_HERE,
createSameThreadTask(&LocalDOMWindow::dispatchLoadEvent,
wrapCrossThreadPersistent(this)));

Ditto.

[sof, 2016-06-22 06:36:57]

On 2016/06/22 06:27:43, haraken wrote:
> It looks like this CL is abusing CrossThreadPersistents. We should use
> wrapPersistent() for tasks that run on the same thread.
> 

+1 - it would be rare for bind() to require cross-thread persistents.
CrossThreadPersistent<> is the heaviest smart pointer we have.

[Yuta Kitamura, 2016-06-22 07:12:47]

I felt the same way as haraken. Are those changes
functionally equivalent to what we have now?

[tzik, 2016-06-22 07:21:28]

Hmm, we currently use CrossThreadPersistent to them, and I wanted to avoid
functional changes in this CL...

OK, I'll switch them to Persistent. All but one can be just Persistent.

[tzik, 2016-06-22 08:24:41]

Description was changed from

==========
Wrap GCed raw pointer parameters of WTF::bind with CrossThreadPersistent

This CL adds `wrapCrossThreadPersistent` to all GCed raw pointer prameters
of WTF::bind, as a preparation of raw pointer removal on WTF::bind.

WTF::bind and blink::threadSafeBind wrap raw pointers into Persistent if
it's a GCed type. However, the lifetime management based on the type get
considered harmful these days, and should eventually be removed.
This CL and following CLs annotate all raw pointers on WTF::bind, and
ban all raw pointers.

BUG=597856
==========

to

==========
Wrap GCed raw pointer parameters of WTF::bind with Persistent

This CL adds `wrapPersistent` to all GCed raw pointer prameters
of WTF::bind, as a preparation of raw pointer removal on WTF::bind.

WTF::bind and blink::threadSafeBind wrap raw pointers into Persistent if
it's a GCed type. However, the lifetime management based on the type get
considered harmful these days, and should eventually be removed.
This CL and following CLs annotate all raw pointers on WTF::bind, and
ban all raw pointers.

BUG=597856
==========

[tzik, 2016-06-22 08:25:08]

Updated. Could you guys take another look?

[tzik, 2016-06-22 08:26:04]

https://codereview.chromium.org/2049003002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ExecutionContext.cpp (right):

https://codereview.chromium.org/2049003002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ExecutionContext.cpp:97:
postTask(BLINK_FROM_HERE,
createSameThreadTask(&ExecutionContext::runSuspendableTasks,
wrapCrossThreadPersistent(this)));
On 2016/06/22 06:27:43, haraken wrote:
> 
> It looks strange that you use a CrossThreadPersistent for the same-thread
task.

Done.

https://codereview.chromium.org/2049003002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ExecutionContext.cpp:123:
postTask(BLINK_FROM_HERE,
createSameThreadTask(&ExecutionContext::runSuspendableTasks,
wrapCrossThreadPersistent(this)));
On 2016/06/22 06:27:43, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/2049003002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/MemoryCacheTest.cpp (right):

https://codereview.chromium.org/2049003002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/MemoryCacheTest.cpp:243:
Platform::current()->currentThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE,
bind(&runTask1, wrapCrossThreadPersistent(cachedLiveResource),
wrapCrossThreadPersistent(cachedDeadResource)));
On 2016/06/22 06:27:43, haraken wrote:
> 
> Does this need to be CrossThreadPersistent?

Done.

https://codereview.chromium.org/2049003002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fileapi/FileReader.cpp (right):

https://codereview.chromium.org/2049003002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fileapi/FileReader.cpp:338: BLINK_FROM_HERE,
createSameThreadTask(&delayedAbort, wrapCrossThreadPersistent(this)));
On 2016/06/22 06:27:43, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/2049003002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/2049003002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:411:
m_document->postTask(BLINK_FROM_HERE,
createSameThreadTask(&LocalDOMWindow::dispatchLoadEvent,
wrapCrossThreadPersistent(this)));
On 2016/06/22 06:27:43, haraken wrote:
> 
> Ditto.

Done.

[haraken, 2016-06-22 09:35:55]

LGTM

[sof, 2016-06-22 09:39:20]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2016-06-22 09:39:22]

lgtm

Could we remove WTF::PointerParamStorageTraits<T*, true> following these
changes?

https://codereview.chromium.org/2049003002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webdatabase/DatabaseThread.cpp (right):

https://codereview.chromium.org/2049003002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webdatabase/DatabaseThread.cpp:117:
m_thread->postTask(BLINK_FROM_HERE,
WTF::bind(&DatabaseThread::cleanupDatabaseThreadCompleted,
wrapCrossThreadPersistent(this)));
Isn't this happening on the same thread?

[Yuta Kitamura, 2016-06-22 10:14:11]

lgtm modulo sof's comments

[tzik, 2016-06-22 10:16:02]

https://codereview.chromium.org/2049003002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webdatabase/DatabaseThread.cpp (right):

https://codereview.chromium.org/2049003002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webdatabase/DatabaseThread.cpp:117:
m_thread->postTask(BLINK_FROM_HERE,
WTF::bind(&DatabaseThread::cleanupDatabaseThreadCompleted,
wrapCrossThreadPersistent(this)));
On 2016/06/22 09:39:22, sof wrote:
> Isn't this happening on the same thread?

Yes, this task is posted to the same thread.
However, we need to use CrossThread here, since |this| is created on the main
thread and posted to the database thread.

[sof, 2016-06-22 13:24:28]

https://codereview.chromium.org/2049003002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webdatabase/DatabaseThread.cpp (right):

https://codereview.chromium.org/2049003002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webdatabase/DatabaseThread.cpp:117:
m_thread->postTask(BLINK_FROM_HERE,
WTF::bind(&DatabaseThread::cleanupDatabaseThreadCompleted,
wrapCrossThreadPersistent(this)));
On 2016/06/22 10:16:01, tzik wrote:
> On 2016/06/22 09:39:22, sof wrote:
> > Isn't this happening on the same thread?
> 
> Yes, this task is posted to the same thread.
> However, we need to use CrossThread here, since |this| is created on the main
> thread and posted to the database thread.

A Persistent<> is allowed to contain a reference to an object in another
thread's heap -- it only must be created & destroyed on the same thread. Which I
think will hold here + be a simpler rule to follow. Or did you discover
otherwise?

[tzik, 2016-06-22 13:56:51]

https://codereview.chromium.org/2049003002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webdatabase/DatabaseThread.cpp (right):

https://codereview.chromium.org/2049003002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webdatabase/DatabaseThread.cpp:117:
m_thread->postTask(BLINK_FROM_HERE,
WTF::bind(&DatabaseThread::cleanupDatabaseThreadCompleted,
wrapCrossThreadPersistent(this)));
On 2016/06/22 13:24:27, sof wrote:
> On 2016/06/22 10:16:01, tzik wrote:
> > On 2016/06/22 09:39:22, sof wrote:
> > > Isn't this happening on the same thread?
> > 
> > Yes, this task is posted to the same thread.
> > However, we need to use CrossThread here, since |this| is created on the
main
> > thread and posted to the database thread.
> 
> A Persistent<> is allowed to contain a reference to an object in another
> thread's heap -- it only must be created & destroyed on the same thread. Which
I
> think will hold here + be a simpler rule to follow. Or did you discover
> otherwise?

That's true on other threads than DatabaseThread.
According to keishi@, DatabaseThread recently starts using per-thread heap. That
requires all references from foreign threads to be CrossThread, so that GC
system can find it.

[sof, 2016-06-22 14:06:50]

https://codereview.chromium.org/2049003002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webdatabase/DatabaseThread.cpp (right):

https://codereview.chromium.org/2049003002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webdatabase/DatabaseThread.cpp:117:
m_thread->postTask(BLINK_FROM_HERE,
WTF::bind(&DatabaseThread::cleanupDatabaseThreadCompleted,
wrapCrossThreadPersistent(this)));
On 2016/06/22 13:56:51, tzik wrote:
> On 2016/06/22 13:24:27, sof wrote:
> > On 2016/06/22 10:16:01, tzik wrote:
> > > On 2016/06/22 09:39:22, sof wrote:
> > > > Isn't this happening on the same thread?
> > > 
> > > Yes, this task is posted to the same thread.
> > > However, we need to use CrossThread here, since |this| is created on the
> main
> > > thread and posted to the database thread.
> > 
> > A Persistent<> is allowed to contain a reference to an object in another
> > thread's heap -- it only must be created & destroyed on the same thread.
Which
> I
> > think will hold here + be a simpler rule to follow. Or did you discover
> > otherwise?
> 
> That's true on other threads than DatabaseThread.
> According to keishi@, DatabaseThread recently starts using per-thread heap.
That
> requires all references from foreign threads to be CrossThread, so that GC
> system can find it.

If that's the case, how would |this| be discovered as a root for a main thread
GC while (e.g.) this method (cleanupDatabaseThread()) is executed by the
database thread?

[tzik, 2016-06-22 14:32:49]

On 2016/06/22 14:06:50, sof wrote:
>
https://codereview.chromium.org/2049003002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/webdatabase/DatabaseThread.cpp (right):
> 
>
https://codereview.chromium.org/2049003002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/webdatabase/DatabaseThread.cpp:117:
> m_thread->postTask(BLINK_FROM_HERE,
> WTF::bind(&DatabaseThread::cleanupDatabaseThreadCompleted,
> wrapCrossThreadPersistent(this)));
> On 2016/06/22 13:56:51, tzik wrote:
> > On 2016/06/22 13:24:27, sof wrote:
> > > On 2016/06/22 10:16:01, tzik wrote:
> > > > On 2016/06/22 09:39:22, sof wrote:
> > > > > Isn't this happening on the same thread?
> > > > 
> > > > Yes, this task is posted to the same thread.
> > > > However, we need to use CrossThread here, since |this| is created on the
> > main
> > > > thread and posted to the database thread.
> > > 
> > > A Persistent<> is allowed to contain a reference to an object in another
> > > thread's heap -- it only must be created & destroyed on the same thread.
> Which
> > I
> > > think will hold here + be a simpler rule to follow. Or did you discover
> > > otherwise?
> > 
> > That's true on other threads than DatabaseThread.
> > According to keishi@, DatabaseThread recently starts using per-thread heap.
> That
> > requires all references from foreign threads to be CrossThread, so that GC
> > system can find it.
> 
> If that's the case, how would |this| be discovered as a root for a main thread
> GC while (e.g.) this method (cleanupDatabaseThread()) is executed by the
> database thread?

The caller of cleanupDatabaseThread() should hold the reference as
CrossThreadPersistent, while it's running. In this case,
WTF::PartBoundFunctionImpl does.

[sof, 2016-06-22 14:49:22]

On 2016/06/22 14:32:49, tzik wrote:
> On 2016/06/22 14:06:50, sof wrote:
> >
>
https://codereview.chromium.org/2049003002/diff/140001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/modules/webdatabase/DatabaseThread.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/2049003002/diff/140001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/modules/webdatabase/DatabaseThread.cpp:117:
> > m_thread->postTask(BLINK_FROM_HERE,
> > WTF::bind(&DatabaseThread::cleanupDatabaseThreadCompleted,
> > wrapCrossThreadPersistent(this)));
> > On 2016/06/22 13:56:51, tzik wrote:
> > > On 2016/06/22 13:24:27, sof wrote:
> > > > On 2016/06/22 10:16:01, tzik wrote:
> > > > > On 2016/06/22 09:39:22, sof wrote:
> > > > > > Isn't this happening on the same thread?
> > > > > 
> > > > > Yes, this task is posted to the same thread.
> > > > > However, we need to use CrossThread here, since |this| is created on
the
> > > main
> > > > > thread and posted to the database thread.
> > > > 
> > > > A Persistent<> is allowed to contain a reference to an object in another
> > > > thread's heap -- it only must be created & destroyed on the same thread.
> > Which
> > > I
> > > > think will hold here + be a simpler rule to follow. Or did you discover
> > > > otherwise?
> > > 
> > > That's true on other threads than DatabaseThread.
> > > According to keishi@, DatabaseThread recently starts using per-thread
heap.
> > That
> > > requires all references from foreign threads to be CrossThread, so that GC
> > > system can find it.
> > 
> > If that's the case, how would |this| be discovered as a root for a main
thread
> > GC while (e.g.) this method (cleanupDatabaseThread()) is executed by the
> > database thread?
> 
> The caller of cleanupDatabaseThread() should hold the reference as
> CrossThreadPersistent, while it's running. In this case,
> WTF::PartBoundFunctionImpl does.

I'm not quite convinced that's workable scheme for maintaining a precise root
set from another thread in a per-thread heap setting, but we're drifting topic.

I'm fine with using wrapCrossThreadPersistent() here.

[hiroshige, 2016-06-23 04:48:27]

lgtm.

Use of CrossThreadPersistent with same-thread WTF::bind() looks complicated, but
it is a general issue of per-thread heap, not bind-specific.

[tzik, 2016-06-23 05:30:27]

The CQ bit was checked by tzik@chromium.org

[tzik, 2016-06-23 05:30:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
yhirano@chromium.org, kouhei@chromium.org, hiroshige@chromium.org,
sigbjornf@opera.com, yutak@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2049003002/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-23 05:30:37]

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

[commit-bot: I haz the power, 2016-06-23 07:46:18]

Description was changed from

==========
Wrap GCed raw pointer parameters of WTF::bind with Persistent

This CL adds `wrapPersistent` to all GCed raw pointer prameters
of WTF::bind, as a preparation of raw pointer removal on WTF::bind.

WTF::bind and blink::threadSafeBind wrap raw pointers into Persistent if
it's a GCed type. However, the lifetime management based on the type get
considered harmful these days, and should eventually be removed.
This CL and following CLs annotate all raw pointers on WTF::bind, and
ban all raw pointers.

BUG=597856
==========

to

==========
Wrap GCed raw pointer parameters of WTF::bind with Persistent

This CL adds `wrapPersistent` to all GCed raw pointer prameters
of WTF::bind, as a preparation of raw pointer removal on WTF::bind.

WTF::bind and blink::threadSafeBind wrap raw pointers into Persistent if
it's a GCed type. However, the lifetime management based on the type get
considered harmful these days, and should eventually be removed.
This CL and following CLs annotate all raw pointers on WTF::bind, and
ban all raw pointers.

BUG=597856
==========

[commit-bot: I haz the power, 2016-06-23 07:46:21]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-06-23 07:48:45]

Description was changed from

==========
Wrap GCed raw pointer parameters of WTF::bind with Persistent

This CL adds `wrapPersistent` to all GCed raw pointer prameters
of WTF::bind, as a preparation of raw pointer removal on WTF::bind.

WTF::bind and blink::threadSafeBind wrap raw pointers into Persistent if
it's a GCed type. However, the lifetime management based on the type get
considered harmful these days, and should eventually be removed.
This CL and following CLs annotate all raw pointers on WTF::bind, and
ban all raw pointers.

BUG=597856
==========

to

==========
Wrap GCed raw pointer parameters of WTF::bind with Persistent

This CL adds `wrapPersistent` to all GCed raw pointer prameters
of WTF::bind, as a preparation of raw pointer removal on WTF::bind.

WTF::bind and blink::threadSafeBind wrap raw pointers into Persistent if
it's a GCed type. However, the lifetime management based on the type get
considered harmful these days, and should eventually be removed.
This CL and following CLs annotate all raw pointers on WTF::bind, and
ban all raw pointers.

BUG=597856

Committed: https://crrev.com/8359caf0b6a4b86949be98b12147c926fb58d2a8
Cr-Commit-Position: refs/heads/master@{#401558}
==========

[commit-bot: I haz the power, 2016-06-23 07:48:49]

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