Remove get from CrossThreadPersistent to avoid accidental use

Remove get from CrossThreadPersistent to avoid accidental use

BUG=591606

[keishi, 2016-06-01 13:37:54]

Description was changed from

==========
Enable per thread heap for database thread (+check)

BUG=591606

patch from issue 1909813002 at patchset 160001
(http://crrev.com/1909813002#ps160001)
==========

to

==========
Remove get from CrossThreadPersistent to avoid accidental use

BUG=591606
==========

[keishi, 2016-06-02 02:09:55]

keishi@chromium.org changed reviewers:
+ haraken@chromium.org, oilpan-reviews@chromium.org

[keishi, 2016-06-02 02:09:56]

Not ready to land, but could you take a look if this is the right direction?

In crrev.com/1909813002/#msg18 I said that adding a DCHECK might be better but
on second thought I realized, we already have the necessary DCHECKs in marking,
so limiting access to the raw pointer is more about avoiding programmer error.
In that case removing CrossThreadPersistent::get seems like the right thing to
do.

I've moved get to protected so only classes that need to have access.
If raw pointer access is necessary for test/logging we can use
CrossThreadPersistent::unsafeGet(). I think it makes it clear that the raw
pointer shouldn't be kept.

[haraken, 2016-06-02 02:17:05]

On 2016/06/02 02:09:56, keishi wrote:
> Not ready to land, but could you take a look if this is the right direction?
> 
> In crrev.com/1909813002/#msg18 I said that adding a DCHECK might be better but
> on second thought I realized, we already have the necessary DCHECKs in
marking,
> so limiting access to the raw pointer is more about avoiding programmer error.
> In that case removing CrossThreadPersistent::get seems like the right thing to
> do.
> 
> I've moved get to protected so only classes that need to have access.
> If raw pointer access is necessary for test/logging we can use
> CrossThreadPersistent::unsafeGet(). I think it makes it clear that the raw
> pointer shouldn't be kept.

Yes, I think this is a good way to go.

The DCHECK is great but wouldn't be sufficient to catch bugs caused by on-stack,
raw, cross-thread pointers in a real world, because the bugs happen only when a
conservative GC is triggered at a certain point (i.e., it's really
timing-dependent) but DCHECK is enabled only on Debug builds. So it's important
to verify that there is no on-stack, raw, cross-thread pointers in the other way
(i.e., remove CrossThreadPersistent::get()).

[sof, 2016-06-02 07:00:11]

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

[sof, 2016-06-02 07:00:12]

If we disregard the issue of wanting to have heap-movable objects, why is it a
problem to have on-stack copies (in unattached threads) of pointers that a
CrossThreadPersistent<> already keeps alive?

https://codereview.chromium.org/2019963002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webdatabase/DatabaseTask.h (right):

https://codereview.chromium.org/2019963002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webdatabase/DatabaseTask.h:121:
CrossThreadPersistent<SQLTransactionBackend> transaction() const { return
m_transaction; }
You're creating a CrossThreadPersistent<> here, is that what you want & need?

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

https://codereview.chromium.org/2019963002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Persistent.h:425: T* get() const {
return Parent::get(); }
Could you add a comment (here or next to unsafeGet()) why this has limited
visibility?

https://codereview.chromium.org/2019963002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebPrivatePtr.h (right):

https://codereview.chromium.org/2019963002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebPrivatePtr.h:159: T* get() const { return
m_handle ? m_handle->unsafeGet() : nullptr; }
You can use a trait (over WebPrivatePtrDestruction) to avoid having to duplicate
a class specialization.

[haraken, 2016-06-02 07:31:42]

On 2016/06/02 07:00:12, sof wrote:
> If we disregard the issue of wanting to have heap-movable objects, why is it a
> problem to have on-stack copies (in unattached threads) of pointers that a
> CrossThreadPersistent<> already keeps alive?

If we do that, the semantics of on-stack pointers will be confusing:

- If the on-stack pointer is pointing to cross-thread objects, you need to keep
it alive by CrossThreadPersistent somewhere.
- Otherwise, you don't need to keep it alive.

[sof, 2016-06-02 07:44:54]

On 2016/06/02 07:31:42, haraken wrote:
> On 2016/06/02 07:00:12, sof wrote:
> > If we disregard the issue of wanting to have heap-movable objects, why is it
a
> > problem to have on-stack copies (in unattached threads) of pointers that a
> > CrossThreadPersistent<> already keeps alive?
> 
> If we do that, the semantics of on-stack pointers will be confusing:
> 
> - If the on-stack pointer is pointing to cross-thread objects, you need to
keep
> it alive by CrossThreadPersistent somewhere.
> - Otherwise, you don't need to keep it alive.

If the thread is ever going to be able to (safely) consult what the CTP refers
to, I don't see how you can avoid such stack/register references. But pushing
"outwards" the use of CrossThreadPersistent<>s like here to make it clear that
you are doing something requiring extra care, makes some sense.

[haraken, 2016-06-02 07:48:51]

On 2016/06/02 07:44:54, sof wrote:
> On 2016/06/02 07:31:42, haraken wrote:
> > On 2016/06/02 07:00:12, sof wrote:
> > > If we disregard the issue of wanting to have heap-movable objects, why is
it
> a
> > > problem to have on-stack copies (in unattached threads) of pointers that a
> > > CrossThreadPersistent<> already keeps alive?
> > 
> > If we do that, the semantics of on-stack pointers will be confusing:
> > 
> > - If the on-stack pointer is pointing to cross-thread objects, you need to
> keep
> > it alive by CrossThreadPersistent somewhere.
> > - Otherwise, you don't need to keep it alive.
> 
> If the thread is ever going to be able to (safely) consult what the CTP refers
> to, I don't see how you can avoid such stack/register references. But pushing
> "outwards" the use of CrossThreadPersistent<>s like here to make it clear that
> you are doing something requiring extra care, makes some sense.

Yeah, your point makes sense.