Oilpan: Prepare to make ExecutionContext GarbageCollectedMixin.

Oilpan: Prepare to make ExecutionContext GarbageCollectedMixin.

We creates closures with ExecutionContexts in LocalFileSystem. The
ExecutionContext needs to be a CrossThreadPersistent data member of
closure. This CL adds ParamStorageTraits specialized to garbage-collected
classes.

This is a preparation to remove RefCounted from Node.

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

[tkent, 2014-06-06 05:09:37]

Please review this.

[haraken, 2014-06-06 05:54:53]

> The ExecutionContext needs to be a CrossThreadPersistent data member of
> closure. This CL adds ParamStorageTraits specialized to garbage-collected
classes.

Just help me understand:

As I mentioned in other places, we currently don't have a mechanism to create an
object that outlives the thread that allocated the object (a
CrossThreadPersistent is not helpful ). Is it safe to land this CL with the
current situation?

For example, ExecutionContext is passed from one thread to another thread via
ExecutionContextTask which holds a Closure. There is no guarantee that the
thread that created the ExecutionContextTask is alive when the
ExecutionContextTask is consumed by another thread. In fact, it looks like this
is happening in WorkerThreadShutdownStartTask.

https://codereview.chromium.org/318023002/diff/1/Source/core/dom/ExecutionCon...
File Source/core/dom/ExecutionContext.h (right):

https://codereview.chromium.org/318023002/diff/1/Source/core/dom/ExecutionCon...
Source/core/dom/ExecutionContext.h:151: ExecutionContextClient* m_client;

This raw pointer looks safe, but it would be great if you can make
ExecutionContextClient a GC-mixin. Then you can remove
ExecutionContext::setClient() from Document's destructor and
WorkerGlobalScope::dispose(). A follow-up is fine.

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

https://codereview.chromium.org/318023002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:1157: template<typename T, bool isGC> struct
PointerParamStorageTraits;

Nit: isGC => isGarbageCollected

[tkent, 2014-06-06 06:02:05]

On 2014/06/06 05:54:53, haraken wrote:
> > The ExecutionContext needs to be a CrossThreadPersistent data member of
> > closure. This CL adds ParamStorageTraits specialized to garbage-collected
> classes.
> 
> Just help me understand:
> 
> As I mentioned in other places, we currently don't have a mechanism to create
an
> object that outlives the thread that allocated the object (a
> CrossThreadPersistent is not helpful ). Is it safe to land this CL with the
> current situation?

It's safe in the LocalFileSystem case.  Closures created in it are not posted to
other threads, they are just executed later in the same thread.

[haraken, 2014-06-06 06:21:52]

On 2014/06/06 06:02:05, tkent wrote:
> On 2014/06/06 05:54:53, haraken wrote:
> > > The ExecutionContext needs to be a CrossThreadPersistent data member of
> > > closure. This CL adds ParamStorageTraits specialized to garbage-collected
> > classes.
> > 
> > Just help me understand:
> > 
> > As I mentioned in other places, we currently don't have a mechanism to
create
> an
> > object that outlives the thread that allocated the object (a
> > CrossThreadPersistent is not helpful ). Is it safe to land this CL with the
> > current situation?
> 
> It's safe in the LocalFileSystem case.  Closures created in it are not posted
to
> other threads, they are just executed later in the same thread.

Understood, though I'm a bit confused:

- Whether we make ExecutionContext a GC-mixin or not won't matter for the
object-outlive-thread issue. If we hit the object-outlive-thread issue after
landing this CL, we should already be hitting the issue, because whether we make
ExecutionContext a GC-mixin or not doesn't change the timing when a threaded
heap is destroyed. In that sense, if the current code is safe, this CL would be
safe. Therefore, LGTM.

- What I don't fully understand is how the current code is working safely. As
far as I read the code of ExecutionContextTask (e.g.,
WorkerThreadShutdownStartTask), it's possible that the main thread that created
the ExecutionContextTask dies before the worker thread consumes the
ExecuiontContextTask. Then how is it guaranteed that
ExecutionContextTask::performTask(ExecutionContext* context) can access the
context safely?

[tkent, 2014-06-06 06:45:08]

On 2014/06/06 06:21:52, haraken wrote:
> - Whether we make ExecutionContext a GC-mixin or not won't matter for the
> object-outlive-thread issue. If we hit the object-outlive-thread issue after
> landing this CL, we should already be hitting the issue, because whether we
make
> ExecutionContext a GC-mixin or not doesn't change the timing when a threaded
> heap is destroyed. In that sense, if the current code is safe, this CL would
be
> safe. Therefore, LGTM.

Right.

> - What I don't fully understand is how the current code is working safely. As
> far as I read the code of ExecutionContextTask (e.g.,
> WorkerThreadShutdownStartTask), it's possible that the main thread that
created
> the ExecutionContextTask dies before the worker thread consumes the
> ExecuiontContextTask. Then how is it guaranteed that
> ExecutionContextTask::performTask(ExecutionContext* context) can access the
> context safely?

ExecutionContextTask is not related to this CL at all.  This CL is unrelated to
the outlive issue you are facing.

[tkent, 2014-06-06 06:52:48]

https://codereview.chromium.org/318023002/diff/1/Source/core/dom/ExecutionCon...
File Source/core/dom/ExecutionContext.h (right):

https://codereview.chromium.org/318023002/diff/1/Source/core/dom/ExecutionCon...
Source/core/dom/ExecutionContext.h:151: ExecutionContextClient* m_client;
On 2014/06/06 05:54:53, haraken wrote:
> 
> This raw pointer looks safe, but it would be great if you can make
> ExecutionContextClient a GC-mixin. Then you can remove
> ExecutionContext::setClient() from Document's destructor and
> WorkerGlobalScope::dispose(). A follow-up is fine.

At this moment, ExecutionContextClient is always ExecutionContext itself.  So
tracing ExecutionContextClient is not attractive.

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

https://codereview.chromium.org/318023002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:1157: template<typename T, bool isGC> struct
PointerParamStorageTraits;
On 2014/06/06 05:54:53, haraken wrote:
> 
> Nit: isGC => isGarbageCollected

Done.

[tkent, 2014-06-06 06:52:55]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-06-06 06:53:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/318023002/20001

[commit-bot: I haz the power, 2014-06-06 07:58:12]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7110)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10540)

[commit-bot: I haz the power, 2014-06-06 07:59:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-06 07:59:34]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[tkent, 2014-06-06 08:00:45]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-06-06 08:01:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/318023002/20001

[commit-bot: I haz the power, 2014-06-06 08:39:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[commit-bot: I haz the power, 2014-06-06 08:40:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-06 08:40:32]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[tkent, 2014-06-06 08:44:55]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-06-06 08:45:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/318023002/20001

[commit-bot: I haz the power, 2014-06-06 08:47:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[commit-bot: I haz the power, 2014-06-06 08:49:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-06 08:49:16]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[tkent, 2014-06-06 09:06:16]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-06-06 09:06:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/318023002/20001

[commit-bot: I haz the power, 2014-06-06 09:09:30]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[commit-bot: I haz the power, 2014-06-06 09:10:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-06 09:10:45]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[tkent, 2014-06-06 09:14:57]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-06-06 09:15:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/318023002/20001

[commit-bot: I haz the power, 2014-06-06 09:28:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[commit-bot: I haz the power, 2014-06-06 09:31:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-06 09:31:51]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[tkent, 2014-06-06 10:54:24]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-06-06 10:55:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/318023002/20001

[commit-bot: I haz the power, 2014-06-06 11:00:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[commit-bot: I haz the power, 2014-06-06 11:02:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-06 11:02:17]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[tkent, 2014-06-06 12:28:48]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-06-06 12:29:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/318023002/20001

[commit-bot: I haz the power, 2014-06-06 12:32:26]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[commit-bot: I haz the power, 2014-06-06 12:34:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-06 12:34:55]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[tkent, 2014-06-07 04:27:35]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-06-07 04:28:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/318023002/20001

[commit-bot: I haz the power, 2014-06-07 04:29:17]

Change committed as 175734

[dcheng, 2014-06-07 08:11:21]

A revert of this CL has been created in
https://codereview.chromium.org/319333002/ by dcheng@chromium.org.

The reason for reverting is: plugins/document-open.html is crashing on debug
layout bots.