Improve CancellableTaskFactory handling and Oilpan usage.

Improve CancellableTaskFactory handling and Oilpan usage.

If a CancellableTaskFactory is used by an Oilpan heap object, the
closure that the factory works with/over, cannot embed a reference
back to that object by way of an off-heap Persistent<> (WTF::Closure
is not on the heap.) If it does, such a reference will keep the heap
object alive, without it ever being released. Memory leaks are very likely.

This is too easy a slip-up to make with the current CancellableTaskFactory
constructor, so rephrase the constructor so as to make leaks no longer (easily)
possible. For Oilpan heap objects baked into the closure, the persistent reference
now held will be weak.

At the same time, take the opportunity to have this object no longer be a part object,
but a separate (off-heap) allocation. This lets us drop the ad-hoc ASan unpoisoning
support that was previously needed if CancellableTaskFactory was a part object
of an Oilpan heap object. Less magic.

R=haraken
BUG=

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

[sof, 2015-09-03 13:49:14]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-09-03 13:49:15]

please take a look.

Still checking with the bots + verifying w/ ASan locally, but believe this is
ready.

[sof, 2015-09-03 20:28:01]

On 2015/09/03 13:49:15, sof wrote:
> please take a look.
> 
> Still checking with the bots + verifying w/ ASan locally, but believe this is
> ready.

Both reporting no problems.

[haraken, 2015-09-03 23:39:33]

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

[haraken, 2015-09-03 23:39:33]

LGTM

https://codereview.chromium.org/1312843009/diff/60001/Source/platform/heap/Ha...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/1312843009/diff/60001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:999: CrossThreadWeakPersistent<T> value() const {
return m_value; }

Just to confirm: All call sites of value() are written assuming that it may
return 0, right?

[sof, 2015-09-04 05:21:04]

https://codereview.chromium.org/1312843009/diff/60001/Source/platform/heap/Ha...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/1312843009/diff/60001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:999: CrossThreadWeakPersistent<T> value() const {
return m_value; }
On 2015/09/03 23:39:33, haraken wrote:
> 
> Just to confirm: All call sites of value() are written assuming that it may
> return 0, right?

No, doing so would require teaching wtf/Functional.h about
blink::(CrossThread)WeakPersistent<>.

For CancellableTaskFactory, weakness is checked for & handled before the closure
is executed/applied.

[haraken, 2015-09-04 05:23:39]

https://codereview.chromium.org/1312843009/diff/60001/Source/platform/heap/Ha...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/1312843009/diff/60001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:999: CrossThreadWeakPersistent<T> value() const {
return m_value; }
On 2015/09/04 05:21:04, sof wrote:
> On 2015/09/03 23:39:33, haraken wrote:
> > 
> > Just to confirm: All call sites of value() are written assuming that it may
> > return 0, right?
> 
> No, doing so would require teaching wtf/Functional.h about
> blink::(CrossThread)WeakPersistent<>.
> 
> For CancellableTaskFactory, weakness is checked for & handled before the
closure
> is executed/applied.

Makes sense.

[sof, 2015-09-04 06:50:21]

Thanks for the review; I'm glad this wrinkle has been smoothed out somewhat.

https://codereview.chromium.org/1312843009/diff/60001/Source/platform/heap/Ha...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/1312843009/diff/60001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:999: CrossThreadWeakPersistent<T> value() const {
return m_value; }
On 2015/09/04 05:23:38, haraken wrote:
> On 2015/09/04 05:21:04, sof wrote:
> > On 2015/09/03 23:39:33, haraken wrote:
> > > 
> > > Just to confirm: All call sites of value() are written assuming that it
may
> > > return 0, right?
> > 
> > No, doing so would require teaching wtf/Functional.h about
> > blink::(CrossThread)WeakPersistent<>.
> > 
> > For CancellableTaskFactory, weakness is checked for & handled before the
> closure
> > is executed/applied.
> 
> Makes sense.

Added a comment + TODO next to the relevant unwrap() here.

Having wtf/ abstractions work over types that are part of the Oilpan
infrastructure is becoming increasingly worthwhile.

[sof, 2015-09-04 07:09:03]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-09-04 07:09:04]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1312843009/#ps80001
(title: "add unwrap() clarification")

[commit-bot: I haz the power, 2015-09-04 07:09:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312843009/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312843009/80001

[commit-bot: I haz the power, 2015-09-04 09:35:40]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201787