Oilpan: add explicit Persistent<T>(T&) and Member<T>(T&) constructors.

Oilpan: add explicit Persistent<T>(T&) and Member<T>(T&) constructors.

Synchronize with RefPtr<T>(T&)'s constructor offering and provide same,
explicit constructors taking a reference.

Make use of it in PostMessageTimer and WorkerThreadableWebSocketChannel.

R=haraken@chromium.org,ager@chromium.org
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170792

[sof, 2014-04-02 07:46:42]

Please take a look.

Following up on an earlier issue - no harm in doing so, but saw no compelling
use-case for adding these constructors to a compiler's pool of implicit
conversions.

(Will delay landing this until the platform/heap/ switch has happened.)

[haraken, 2014-04-02 07:49:41]

https://codereview.chromium.org/221823002/diff/1/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/221823002/diff/1/Source/heap/Handle.h#newcode450
Source/heap/Handle.h:450: COMPILE_ASSERT_IS_GARBAGE_COLLECTED(T,
NonGarbageCollectedObjectInMember);

Can we implement ASSERT_IS_VALID_MEMBER_POINTER as well?

[sof, 2014-04-02 08:05:49]

https://codereview.chromium.org/221823002/diff/1/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/221823002/diff/1/Source/heap/Handle.h#newcode450
Source/heap/Handle.h:450: COMPILE_ASSERT_IS_GARBAGE_COLLECTED(T,
NonGarbageCollectedObjectInMember);
On 2014/04/02 07:49:41, haraken wrote:
> 
> Can we implement ASSERT_IS_VALID_MEMBER_POINTER as well?

Catching out cross-heap references would be useful.

[haraken, 2014-04-02 08:09:29]

LGTM

https://codereview.chromium.org/221823002/diff/1/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/221823002/diff/1/Source/heap/Handle.h#newcode450
Source/heap/Handle.h:450: COMPILE_ASSERT_IS_GARBAGE_COLLECTED(T,
NonGarbageCollectedObjectInMember);
On 2014/04/02 08:05:50, sof wrote:
> On 2014/04/02 07:49:41, haraken wrote:
> > 
> > Can we implement ASSERT_IS_VALID_MEMBER_POINTER as well?
> 
> Catching out cross-heap references would be useful.

Oh, sorry. Cross-heap references are allowed, so we cannot add the ASSERT.

If we want to add some ASSERT, we need to check if the pointer is in either of
the thread heaps. However, we cannot check that without stopping all threads.
Thus we cannot add the ASSERT.

[sof, 2014-04-02 08:12:54]

https://codereview.chromium.org/221823002/diff/1/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/221823002/diff/1/Source/heap/Handle.h#newcode450
Source/heap/Handle.h:450: COMPILE_ASSERT_IS_GARBAGE_COLLECTED(T,
NonGarbageCollectedObjectInMember);
On 2014/04/02 08:09:29, haraken wrote:
> On 2014/04/02 08:05:50, sof wrote:
> > On 2014/04/02 07:49:41, haraken wrote:
> > > 
> > > Can we implement ASSERT_IS_VALID_MEMBER_POINTER as well?
> > 
> > Catching out cross-heap references would be useful.
> 
> Oh, sorry. Cross-heap references are allowed, so we cannot add the ASSERT.
> 
> If we want to add some ASSERT, we need to check if the pointer is in either of
> the thread heaps. However, we cannot check that without stopping all threads.
> Thus we cannot add the ASSERT.

Interesting, what code relies on creating such cross references?

[haraken, 2014-04-02 08:15:41]

https://codereview.chromium.org/221823002/diff/1/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/221823002/diff/1/Source/heap/Handle.h#newcode450
Source/heap/Handle.h:450: COMPILE_ASSERT_IS_GARBAGE_COLLECTED(T,
NonGarbageCollectedObjectInMember);
On 2014/04/02 08:12:54, sof wrote:
> On 2014/04/02 08:09:29, haraken wrote:
> > On 2014/04/02 08:05:50, sof wrote:
> > > On 2014/04/02 07:49:41, haraken wrote:
> > > > 
> > > > Can we implement ASSERT_IS_VALID_MEMBER_POINTER as well?
> > > 
> > > Catching out cross-heap references would be useful.
> > 
> > Oh, sorry. Cross-heap references are allowed, so we cannot add the ASSERT.
> > 
> > If we want to add some ASSERT, we need to check if the pointer is in either
of
> > the thread heaps. However, we cannot check that without stopping all
threads.
> > Thus we cannot add the ASSERT.
> 
> Interesting, what code relies on creating such cross references?

I think some classes in webdatabase/ are using cross-thread Members.

tkent-san: Can you point out any example?

[haraken, 2014-04-02 08:21:12]

The only thing we have to care about is that a Member must not outlive a thread
that holds the persistent handle of the Member.

This is already checked in the "empty-check" ASSERT in ThreadState::detach(). If
a Member in some thread is going to outlive a thread X that holds the persistent
handle of the Member, we will hit the ASSERT because the thread X calls
ThreadState::detach() with the live persistent handle.

[Mads Ager (chromium), 2014-04-02 08:22:03]

LGTM

https://codereview.chromium.org/221823002/diff/1/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/221823002/diff/1/Source/heap/Handle.h#newcode450
Source/heap/Handle.h:450: COMPILE_ASSERT_IS_GARBAGE_COLLECTED(T,
NonGarbageCollectedObjectInMember);
On 2014/04/02 08:15:41, haraken wrote:
> On 2014/04/02 08:12:54, sof wrote:
> > On 2014/04/02 08:09:29, haraken wrote:
> > > On 2014/04/02 08:05:50, sof wrote:
> > > > On 2014/04/02 07:49:41, haraken wrote:
> > > > > 
> > > > > Can we implement ASSERT_IS_VALID_MEMBER_POINTER as well?
> > > > 
> > > > Catching out cross-heap references would be useful.
> > > 
> > > Oh, sorry. Cross-heap references are allowed, so we cannot add the ASSERT.
> > > 
> > > If we want to add some ASSERT, we need to check if the pointer is in
either
> of
> > > the thread heaps. However, we cannot check that without stopping all
> threads.
> > > Thus we cannot add the ASSERT.
> > 
> > Interesting, what code relies on creating such cross references?
> 
> I think some classes in webdatabase/ are using cross-thread Members.
> 
> tkent-san: Can you point out any example?

That is correct, we have cross thread pointers in the webdatabase code. When I
added these compile-time assert I initially did have the contains check here but
the check that we really want cannot be done because it needs to check that the
pointer is in *some* oilpan heap - not necessarily for this thread.

[tkent, 2014-04-02 08:26:09]

> I think some classes in webdatabase/ are using cross-thread Members.

Yeah, there are some examples in modules/webdatabase/.
e.g.
  DatabaseThread::m_openDataabases contains Member<DatabaseBackend>. A
DatabaseBackend instance is created in the main thread, it is passed to a
DatabaseThread through CrossThreadPersistent in a task, then is stored into
m_openDatabases as a Member<DatabaseBackend>.

[sof, 2014-04-02 08:32:23]

On 2014/04/02 08:26:09, tkent wrote:
> > I think some classes in webdatabase/ are using cross-thread Members.
> 
> Yeah, there are some examples in modules/webdatabase/.
> e.g.
>   DatabaseThread::m_openDataabases contains Member<DatabaseBackend>. A
> DatabaseBackend instance is created in the main thread, it is passed to a
> DatabaseThread through CrossThreadPersistent in a task, then is stored into
> m_openDatabases as a Member<DatabaseBackend>.

Thanks, I see.

Final Q :) - would it have some merit to use types to record cross-thread
members?

[haraken, 2014-04-02 08:34:10]

On 2014/04/02 08:32:23, sof wrote:
> On 2014/04/02 08:26:09, tkent wrote:
> > > I think some classes in webdatabase/ are using cross-thread Members.
> > 
> > Yeah, there are some examples in modules/webdatabase/.
> > e.g.
> >   DatabaseThread::m_openDataabases contains Member<DatabaseBackend>. A
> > DatabaseBackend instance is created in the main thread, it is passed to a
> > DatabaseThread through CrossThreadPersistent in a task, then is stored into
> > m_openDatabases as a Member<DatabaseBackend>.
> 
> Thanks, I see.
> 
> Final Q :) - would it have some merit to use types to record cross-thread
> members?

IMO, no. Because as I mentioned, the safety of cross-thread Members is already
checked by the "empty-check" ASSERT in ThreadState::detach().

[sof, 2014-04-03 19:39:01]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-04-03 19:39:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/221823002/20001

[sof, 2014-04-03 20:55:17]

The CQ bit was unchecked by sigbjornf@opera.com

[sof, 2014-04-03 20:55:57]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-04-03 20:56:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/221823002/20001

[commit-bot: I haz the power, 2014-04-03 20:57:02]

Change committed as 170792