Move WorkerThreadable internal classes to Oilpan heap

Move WorkerThreadableLoader internal classes to Oilpan heap

This CL splits WorkerThreadableLoader::MainThreadBridgeBase (and subclasses)
into the following on-heap classes.

 - TaskForwarder and subclasses
   Lives in the main thread and forwards tasks to the worker thread.
 - Bridge
   Lives in the worker thread
 - Peer
   Lives in the worker thread and forwards notifications from
   the loader living in the main thread to ThreadableLoaderClientWrapper
   living in the worker thread with a TaskForwarder.

Pointers to an object living in the other thread are held as
CrossThread[Weak]Persistents (e.g., Bridge::m_peer).

BUG=587663
Committed: https://crrev.com/279f12d293087b7619089ecbe418e83df9d04a76
Cr-Commit-Position: refs/heads/master@{#409727}

[yhirano, 2016-07-15 08:21:51]

Description was changed from

==========
Move WorkerThread internal classes to Oilpan heap

BUG=587663
==========

to

==========
Move WorkerThreadableLoader internal classes to Oilpan heap

This CL splits WorkerThreadableLoader::MainThreadBridgeBase (and subclasses)
into the following on-heap classes.

 - TaskForwarder and subclasses
   Lives in the main thread and forwards tasks to the worke r thread.
 - Bridge
   Lives in the worker thread
 - Peer
   Lives in the worker thread and forwards notifications from
   the loader living in the main thread to ThreadableLoaderClientWrapper
   living in the worker thread with a TaskForwarder.

Pointers to an object living in the other thread are held with
CrossThreadPersistent (e.g., Bridge::m_peer).


BUG=587663
==========

[yhirano, 2016-07-15 08:22:16]

Description was changed from

==========
Move WorkerThreadableLoader internal classes to Oilpan heap

This CL splits WorkerThreadableLoader::MainThreadBridgeBase (and subclasses)
into the following on-heap classes.

 - TaskForwarder and subclasses
   Lives in the main thread and forwards tasks to the worke r thread.
 - Bridge
   Lives in the worker thread
 - Peer
   Lives in the worker thread and forwards notifications from
   the loader living in the main thread to ThreadableLoaderClientWrapper
   living in the worker thread with a TaskForwarder.

Pointers to an object living in the other thread are held with
CrossThreadPersistent (e.g., Bridge::m_peer).


BUG=587663
==========

to

==========
Move WorkerThreadableLoader internal classes to Oilpan heap

This CL splits WorkerThreadableLoader::MainThreadBridgeBase (and subclasses)
into the following on-heap classes.

 - TaskForwarder and subclasses
   Lives in the main thread and forwards tasks to the worker thread.
 - Bridge
   Lives in the worker thread
 - Peer
   Lives in the worker thread and forwards notifications from
   the loader living in the main thread to ThreadableLoaderClientWrapper
   living in the worker thread with a TaskForwarder.

Pointers to an object living in the other thread are held with
CrossThreadPersistent (e.g., Bridge::m_peer).


BUG=587663
==========

[yhirano, 2016-07-15 08:23:07]

Description was changed from

==========
Move WorkerThreadableLoader internal classes to Oilpan heap

This CL splits WorkerThreadableLoader::MainThreadBridgeBase (and subclasses)
into the following on-heap classes.

 - TaskForwarder and subclasses
   Lives in the main thread and forwards tasks to the worker thread.
 - Bridge
   Lives in the worker thread
 - Peer
   Lives in the worker thread and forwards notifications from
   the loader living in the main thread to ThreadableLoaderClientWrapper
   living in the worker thread with a TaskForwarder.

Pointers to an object living in the other thread are held with
CrossThreadPersistent (e.g., Bridge::m_peer).


BUG=587663
==========

to

==========
Move WorkerThreadableLoader internal classes to Oilpan heap

This CL splits WorkerThreadableLoader::MainThreadBridgeBase (and subclasses)
into the following on-heap classes.

 - TaskForwarder and subclasses
   Lives in the main thread and forwards tasks to the worker thread.
 - Bridge
   Lives in the worker thread
 - Peer
   Lives in the worker thread and forwards notifications from
   the loader living in the main thread to ThreadableLoaderClientWrapper
   living in the worker thread with a TaskForwarder.

Pointers to an object living in the other thread are held as
CrossThreadPersistents (e.g., Bridge::m_peer).


BUG=587663
==========

[yhirano, 2016-07-20 02:58:33]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 02:59:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 04:13:32]

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

[commit-bot: I haz the power, 2016-07-20 04:13:33]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-07-21 02:06:40]

Patchset #3 (id:40001) has been deleted

[yhirano, 2016-07-21 02:06:54]

Patchset #2 (id:20001) has been deleted

[yhirano, 2016-07-21 02:07:43]

yhirano@chromium.org changed reviewers:
+ hiroshige@chromium.org, nhiroki@chromium.org, oilpan-reviews@chromium.org

[haraken, 2016-07-21 10:22:28]

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

[haraken, 2016-07-21 10:22:29]

The overall approach looks good.

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp (right):

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:79:
WorkerThreadableLoader::~WorkerThreadableLoader()

Is it guaranteed that WorkerThreadableLoader gets destroyed outside Oilpan's GC?
Otherwise, it would be problematic to touch on-heap objects in
m_bridge->destroy().

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:139:
m_event.wait();

Don't we need to add:

  SafePointScope scope(BlinkGC::HeapPointersOnStack);

?

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:201: }

Can we add DCHECK(!m_peer)?

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:338: {

Don't we need to call:

  m_forwarder->abort();
  m_clientWrapper = nullptr;

in cancel()?

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:349:
CrossThreadPersistent<ThreadableLoaderClientWrapper> clientWrapper =
m_clientWrapper.get();

Shall we use wrapCrossThreadPersistent()?

The same comment for other parts in this file.

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:219:
CrossThreadWeakPersistent<ThreadableLoaderClientWrapper> m_clientWrapper;

Does this need to be a weak pointer?

[yhirano, 2016-07-21 10:56:09]

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp (right):

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:79:
WorkerThreadableLoader::~WorkerThreadableLoader()
On 2016/07/21 10:22:29, haraken wrote:
> 
> Is it guaranteed that WorkerThreadableLoader gets destroyed outside Oilpan's
GC?
> Otherwise, it would be problematic to touch on-heap objects in
> m_bridge->destroy().

No, it's not guaranteed. I thought that calling destroy is safe because m_bridge
is held as Persistent and hence is never unreachable here. Is my understanding
wrong?

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:139:
m_event.wait();
On 2016/07/21 10:22:29, haraken wrote:
> 
> Don't we need to add:
> 
>   SafePointScope scope(BlinkGC::HeapPointersOnStack);
> 
> ?

I placed at the call site. Do you think here is the better place?

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:338: {
On 2016/07/21 10:22:29, haraken wrote:
> 
> Don't we need to call:
> 
>   m_forwarder->abort();
>   m_clientWrapper = nullptr;
> 
> in cancel()?

We don't need to call them when this function is called from Bridge::cancel.
Peer::contextDestroyed also calls this function but it calls them by itself.

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:219:
CrossThreadWeakPersistent<ThreadableLoaderClientWrapper> m_clientWrapper;
On 2016/07/21 10:22:29, haraken wrote:
> 
> Does this need to be a weak pointer?

Having this as a strong persistent may form a reference cycle (if/when we hold
ThreadableLoaderClientWrapper::m_client as a Member).

I made worker->main cross thread persistent pointers strong and main->worker
pointers weak, but I'm not 100% sure it's the right pattern.

[yhirano, 2016-07-21 11:32:43]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 11:33:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-07-21 11:44:55]

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp (right):

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:201: }
On 2016/07/21 10:22:29, haraken wrote:
> 
> Can we add DCHECK(!m_peer)?

Done.

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:349:
CrossThreadPersistent<ThreadableLoaderClientWrapper> clientWrapper =
m_clientWrapper.get();
On 2016/07/21 10:22:29, haraken wrote:
> 
> Shall we use wrapCrossThreadPersistent()?
> 
> The same comment for other parts in this file.
> 

Let me confirm: Are you suggesting

(1)  {
    DCHECK(isMainThread());
    CrossThreadPersistent<ThreadableLoaderClientWrapper> clientWrapper =
        wrapCrossThreadPersistent(m_clientWrapper.get());
    if (!clientWrapper)
        return;
    forwardTask(createCrossThreadTask(
        ..., clientWrapper, ...));
}

or 

(2) {
    DCHECK(isMainThread());
    forwardTask(createCrossThreadTask(
        ..., wrapCrossThreadPersistent(m_clientWrapper.get()), ...));
}

?

(1) doens't have a merit, and (2) may bind nullptr for the task as
m_clientWrapper is a weak pointer.

[commit-bot: I haz the power, 2016-07-21 12:38:27]

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

[commit-bot: I haz the power, 2016-07-21 12:38:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yhirano, 2016-07-21 15:06:27]

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp (right):

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:201: }
On 2016/07/21 11:44:55, yhirano wrote:
> On 2016/07/21 10:22:29, haraken wrote:
> > 
> > Can we add DCHECK(!m_peer)?
> 
> Done.

memo: this breaks a test because didStart may be called after destroy. I will
fix this tomorrow.

[haraken, 2016-07-21 15:56:14]

We might want to have a figure (as a comment in code) that illustrates the
lifetime relationship between Peer, Bridge, MainThredableLoader,
ThreadableLoaderClient, ThreadableLoaderClientWrapper and
WorkerThreadableLoader.

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp (right):

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:349:
CrossThreadPersistent<ThreadableLoaderClientWrapper> clientWrapper =
m_clientWrapper.get();
On 2016/07/21 11:44:55, yhirano wrote:
> On 2016/07/21 10:22:29, haraken wrote:
> > 
> > Shall we use wrapCrossThreadPersistent()?
> > 
> > The same comment for other parts in this file.
> > 
> 
> Let me confirm: Are you suggesting
> 
> (1)  {
>     DCHECK(isMainThread());
>     CrossThreadPersistent<ThreadableLoaderClientWrapper> clientWrapper =
>         wrapCrossThreadPersistent(m_clientWrapper.get());
>     if (!clientWrapper)
>         return;
>     forwardTask(createCrossThreadTask(
>         ..., clientWrapper, ...));
> }
> 
> or 
> 
> (2) {
>     DCHECK(isMainThread());
>     forwardTask(createCrossThreadTask(
>         ..., wrapCrossThreadPersistent(m_clientWrapper.get()), ...));
> }
> 
> ?
> 
> (1) doens't have a merit, and (2) may bind nullptr for the task as
> m_clientWrapper is a weak pointer.

I was suggesting 2).

if (!m_clientWrapper)
  return;
createCrossThreadTask(..., wrapCrossThreadPersistent(m_clientWrapper.get())

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:219:
CrossThreadWeakPersistent<ThreadableLoaderClientWrapper> m_clientWrapper;
On 2016/07/21 10:56:09, yhirano wrote:
> On 2016/07/21 10:22:29, haraken wrote:
> > 
> > Does this need to be a weak pointer?
> 
> Having this as a strong persistent may form a reference cycle (if/when we hold
> ThreadableLoaderClientWrapper::m_client as a Member).
> 
> I made worker->main cross thread persistent pointers strong and main->worker
> pointers weak, but I'm not 100% sure it's the right pattern.

I want to confirm two things:

- Even if we use a strong reference and create a cycle, it won't be a problem
because the main->worker strong reference is explicitly broken when
WorkerContextLifecycleObserver::contextDestroyed is called, right?

- Who guarantees that ThreadableLoaderClientWrapper is kept alive while Peer is
holding a weak reference to the ThreadableLoaderClientWrapper?

https://codereview.chromium.org/2151173003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/2151173003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:105: // setting
functions must be called before |signal()| is called.

Can we add asserts about the fact?

https://codereview.chromium.org/2151173003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:144: // A Bridge
instance lives in the worker thread and asks loading tasks the

requests the associated Peer (which lives in the main thread) to process loading
tasks

[haraken, 2016-07-21 15:56:14]

We might want to have a figure (as a comment in code) that illustrates the
lifetime relationship between Peer, Bridge, MainThredableLoader,
ThreadableLoaderClient, ThreadableLoaderClientWrapper and
WorkerThreadableLoader.

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp (right):

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:349:
CrossThreadPersistent<ThreadableLoaderClientWrapper> clientWrapper =
m_clientWrapper.get();
On 2016/07/21 11:44:55, yhirano wrote:
> On 2016/07/21 10:22:29, haraken wrote:
> > 
> > Shall we use wrapCrossThreadPersistent()?
> > 
> > The same comment for other parts in this file.
> > 
> 
> Let me confirm: Are you suggesting
> 
> (1)  {
>     DCHECK(isMainThread());
>     CrossThreadPersistent<ThreadableLoaderClientWrapper> clientWrapper =
>         wrapCrossThreadPersistent(m_clientWrapper.get());
>     if (!clientWrapper)
>         return;
>     forwardTask(createCrossThreadTask(
>         ..., clientWrapper, ...));
> }
> 
> or 
> 
> (2) {
>     DCHECK(isMainThread());
>     forwardTask(createCrossThreadTask(
>         ..., wrapCrossThreadPersistent(m_clientWrapper.get()), ...));
> }
> 
> ?
> 
> (1) doens't have a merit, and (2) may bind nullptr for the task as
> m_clientWrapper is a weak pointer.

I was suggesting 2).

if (!m_clientWrapper)
  return;
createCrossThreadTask(..., wrapCrossThreadPersistent(m_clientWrapper.get())

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:219:
CrossThreadWeakPersistent<ThreadableLoaderClientWrapper> m_clientWrapper;
On 2016/07/21 10:56:09, yhirano wrote:
> On 2016/07/21 10:22:29, haraken wrote:
> > 
> > Does this need to be a weak pointer?
> 
> Having this as a strong persistent may form a reference cycle (if/when we hold
> ThreadableLoaderClientWrapper::m_client as a Member).
> 
> I made worker->main cross thread persistent pointers strong and main->worker
> pointers weak, but I'm not 100% sure it's the right pattern.

I want to confirm two things:

- Even if we use a strong reference and create a cycle, it won't be a problem
because the main->worker strong reference is explicitly broken when
WorkerContextLifecycleObserver::contextDestroyed is called, right?

- Who guarantees that ThreadableLoaderClientWrapper is kept alive while Peer is
holding a weak reference to the ThreadableLoaderClientWrapper?

https://codereview.chromium.org/2151173003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/2151173003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:105: // setting
functions must be called before |signal()| is called.

Can we add asserts about the fact?

https://codereview.chromium.org/2151173003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:144: // A Bridge
instance lives in the worker thread and asks loading tasks the

requests the associated Peer (which lives in the main thread) to process loading
tasks

[yhirano, 2016-07-22 01:54:06]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 01:54:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-07-22 01:57:36]

> We might want to have a figure (as a comment in code) that illustrates the
> lifetime relationship between Peer, Bridge, MainThredableLoader,
> ThreadableLoaderClient, ThreadableLoaderClientWrapper and
> WorkerThreadableLoader.

That's a good idea, but can you wait until the series of CLs are landed? The
picture will change when I make ThreadableLoader GCed. I added a TODO comment.

[yhirano, 2016-07-22 02:06:36]

Description was changed from

==========
Move WorkerThreadableLoader internal classes to Oilpan heap

This CL splits WorkerThreadableLoader::MainThreadBridgeBase (and subclasses)
into the following on-heap classes.

 - TaskForwarder and subclasses
   Lives in the main thread and forwards tasks to the worker thread.
 - Bridge
   Lives in the worker thread
 - Peer
   Lives in the worker thread and forwards notifications from
   the loader living in the main thread to ThreadableLoaderClientWrapper
   living in the worker thread with a TaskForwarder.

Pointers to an object living in the other thread are held as
CrossThreadPersistents (e.g., Bridge::m_peer).


BUG=587663
==========

to

==========
Move WorkerThreadableLoader internal classes to Oilpan heap

This CL splits WorkerThreadableLoader::MainThreadBridgeBase (and subclasses)
into the following on-heap classes.

 - TaskForwarder and subclasses
   Lives in the main thread and forwards tasks to the worker thread.
 - Bridge
   Lives in the worker thread
 - Peer
   Lives in the worker thread and forwards notifications from
   the loader living in the main thread to ThreadableLoaderClientWrapper
   living in the worker thread with a TaskForwarder.

Pointers to an object living in the other thread are held as
CrossThread[Weak]Persistents (e.g., Bridge::m_peer).


BUG=587663
==========

[commit-bot: I haz the power, 2016-07-22 02:19:07]

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

[commit-bot: I haz the power, 2016-07-22 02:19:07]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yhirano, 2016-07-22 02:26:10]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 02:27:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 02:48:52]

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

[commit-bot: I haz the power, 2016-07-22 02:48:54]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[yhirano, 2016-07-22 03:09:48]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 03:11:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-07-22 03:38:44]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 03:39:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-07-22 04:42:14]

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp (right):

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:349:
CrossThreadPersistent<ThreadableLoaderClientWrapper> clientWrapper =
m_clientWrapper.get();
On 2016/07/21 15:56:14, haraken wrote:
> On 2016/07/21 11:44:55, yhirano wrote:
> > On 2016/07/21 10:22:29, haraken wrote:
> > > 
> > > Shall we use wrapCrossThreadPersistent()?
> > > 
> > > The same comment for other parts in this file.
> > > 
> > 
> > Let me confirm: Are you suggesting
> > 
> > (1)  {
> >     DCHECK(isMainThread());
> >     CrossThreadPersistent<ThreadableLoaderClientWrapper> clientWrapper =
> >         wrapCrossThreadPersistent(m_clientWrapper.get());
> >     if (!clientWrapper)
> >         return;
> >     forwardTask(createCrossThreadTask(
> >         ..., clientWrapper, ...));
> > }
> > 
> > or 
> > 
> > (2) {
> >     DCHECK(isMainThread());
> >     forwardTask(createCrossThreadTask(
> >         ..., wrapCrossThreadPersistent(m_clientWrapper.get()), ...));
> > }
> > 
> > ?
> > 
> > (1) doens't have a merit, and (2) may bind nullptr for the task as
> > m_clientWrapper is a weak pointer.
> 
> I was suggesting 2).
> 
> if (!m_clientWrapper)
>   return;
> createCrossThreadTask(..., wrapCrossThreadPersistent(m_clientWrapper.get())

The pattern is not safe as m_clientWrapper is weak. We need to hold a strong
pointer before checking the nullity.

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:219:
CrossThreadWeakPersistent<ThreadableLoaderClientWrapper> m_clientWrapper;
On 2016/07/21 15:56:14, haraken wrote:
> On 2016/07/21 10:56:09, yhirano wrote:
> > On 2016/07/21 10:22:29, haraken wrote:
> > > 
> > > Does this need to be a weak pointer?
> > 
> > Having this as a strong persistent may form a reference cycle (if/when we
hold
> > ThreadableLoaderClientWrapper::m_client as a Member).
> > 
> > I made worker->main cross thread persistent pointers strong and main->worker
> > pointers weak, but I'm not 100% sure it's the right pattern.
> 
> I want to confirm two things:
> 
> - Even if we use a strong reference and create a cycle, it won't be a problem
> because the main->worker strong reference is explicitly broken when
> WorkerContextLifecycleObserver::contextDestroyed is called, right?
> 

contextDestroyed is called at the worker thread termination, so the loop will
eventually break.


> - Who guarantees that ThreadableLoaderClientWrapper is kept alive while Peer
is
> holding a weak reference to the ThreadableLoaderClientWrapper?
> 

It's WorkerThreadableLoader. A WorkerThreadableLoader has a
ThreadableLoaderClientWrapper as Persistent. I will replace the Persistent with
Member in a later CL.

When ThreadableLoader is moved to Oilpan heap, users will expect there is a
strong member reference or weak reference from a ThreadableLoader to its client
(I'm not sure which is correct right now). Users won't expect there is a strong
persistent reference from a ThreadableLoader to its client, I think.

https://codereview.chromium.org/2151173003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/2151173003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:105: // setting
functions must be called before |signal()| is called.
On 2016/07/21 15:56:14, haraken wrote:
> 
> Can we add asserts about the fact?

Done.

https://codereview.chromium.org/2151173003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:144: // A Bridge
instance lives in the worker thread and asks loading tasks the
On 2016/07/21 15:56:14, haraken wrote:
> 
> requests the associated Peer (which lives in the main thread) to process
loading
> tasks

Done.

[commit-bot: I haz the power, 2016-07-22 06:11:13]

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

[commit-bot: I haz the power, 2016-07-22 06:11:14]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-07-22 08:57:30]

Mostly looks good from the Oilpan perspective.

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:219:
CrossThreadWeakPersistent<ThreadableLoaderClientWrapper> m_clientWrapper;
On 2016/07/22 04:42:14, yhirano wrote:
> On 2016/07/21 15:56:14, haraken wrote:
> > On 2016/07/21 10:56:09, yhirano wrote:
> > > On 2016/07/21 10:22:29, haraken wrote:
> > > > 
> > > > Does this need to be a weak pointer?
> > > 
> > > Having this as a strong persistent may form a reference cycle (if/when we
> hold
> > > ThreadableLoaderClientWrapper::m_client as a Member).
> > > 
> > > I made worker->main cross thread persistent pointers strong and
main->worker
> > > pointers weak, but I'm not 100% sure it's the right pattern.
> > 
> > I want to confirm two things:
> > 
> > - Even if we use a strong reference and create a cycle, it won't be a
problem
> > because the main->worker strong reference is explicitly broken when
> > WorkerContextLifecycleObserver::contextDestroyed is called, right?
> > 
> 
> contextDestroyed is called at the worker thread termination, so the loop will
> eventually break.
> 
> 
> > - Who guarantees that ThreadableLoaderClientWrapper is kept alive while Peer
> is
> > holding a weak reference to the ThreadableLoaderClientWrapper?
> > 
> 
> It's WorkerThreadableLoader. A WorkerThreadableLoader has a
> ThreadableLoaderClientWrapper as Persistent. I will replace the Persistent
with
> Member in a later CL.
> 
> When ThreadableLoader is moved to Oilpan heap, users will expect there is a
> strong member reference or weak reference from a ThreadableLoader to its
client
> (I'm not sure which is correct right now). Users won't expect there is a
strong
> persistent reference from a ThreadableLoader to its client, I think.

Makes sense.

Once we move ThreadableLoader to Oilpan's heap, we can just use strong
references (Member<>s). Cycles between Member<>s is not problematic at all.

In this CL, strong or weak wouldn't matter in practice. I'd slightly prefer
using a strong reference because then we can get rid of if(!clientWrapper) check
from a bunch of methods. We anyway need to get rid of the checks when we change
the reference to Member<> in the next CL. It looks simpler to use a strong
reference from the beginning.

https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp (right):

https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:280: m_peer =
nullptr;

I'd remove line 277 - 280 and instead call destroy() at the end of cancel().

https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:471: void
WorkerThreadableLoader::Peer::contextDestroyed()

Just help me understand: Do we need to observe contextDestroyed()? When the
worker thread is shut down, Bridge::destroy() should have already been called.
Bridge::destroy() calls Peer:cancel(), so we won't need to call cancel() here.

Also the worker thread can abort things himself without relying on the main
thread call m_forwarder->abort().

https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:59: //
TODO(yhirano): Draw a diagram to illustrate the class relationship.

Also I'd suggest renaming classes in a follow-up CL to clarify what is on the
main thread and what is on the worker thread.

e.g.:

Peer => PeerOnMainThread
Bridge => PeerOnWorker
ThreadableLoaderClient => ThreadableLoaderClientOnMainThread
ThreadableLoaderClientWrapper => ThreadableLoaderClientOnWorker

[yhirano, 2016-07-25 02:07:00]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 02:07:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-07-25 02:07:28]

Patchset #8 (id:180001) has been deleted

[commit-bot: I haz the power, 2016-07-25 02:08:22]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yhirano, 2016-07-25 02:10:13]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 02:10:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-07-25 02:20:59]

https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp (right):

https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:280: m_peer =
nullptr;
On 2016/07/22 08:57:30, haraken wrote:
> 
> I'd remove line 277 - 280 and instead call destroy() at the end of cancel().

Done. As m_clientWrapper->clearClient should be called differently, I added a
helper function.

https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:471: void
WorkerThreadableLoader::Peer::contextDestroyed()
On 2016/07/22 08:57:30, haraken wrote:
> 
> Just help me understand: Do we need to observe contextDestroyed()? When the
> worker thread is shut down, Bridge::destroy() should have already been called.
> Bridge::destroy() calls Peer:cancel(), so we won't need to call cancel() here.
> 
> Also the worker thread can abort things himself without relying on the main
> thread call m_forwarder->abort().
> 
> 
> 

Peer needs to inherit WorkerThreadableLifecycleObserver from two reasons:


1) It should know if the thread is being terminated in Peer::createAndStart. As
the Peer is not yet associated with a Bridge, I would like to cancel the main
thread loader here.

2) The abort call in contextDestroyed interrupts a worker-side sync loading that
may block the thread termination (Even the worker thread is blocked, the
termination process can be started from the main thread).

https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:59: //
TODO(yhirano): Draw a diagram to illustrate the class relationship.
On 2016/07/22 08:57:30, haraken wrote:
> 
> Also I'd suggest renaming classes in a follow-up CL to clarify what is on the
> main thread and what is on the worker thread.
> 
> e.g.:
> 
> Peer => PeerOnMainThread
> Bridge => PeerOnWorker
> ThreadableLoaderClient => ThreadableLoaderClientOnMainThread
> ThreadableLoaderClientWrapper => ThreadableLoaderClientOnWorker

Added a TODO.

Note that ThreadableLoaderClient is not bound to the main thread. For example,
XHR is a ThreadableLoaderClient and running on both threads.

[yhirano, 2016-07-25 02:29:35]

On 2016/07/22 08:57:30, haraken wrote:
> (snip)
> 
> Makes sense.
> 
> Once we move ThreadableLoader to Oilpan's heap, we can just use strong
> references (Member<>s). Cycles between Member<>s is not problematic at all.
> 
> In this CL, strong or weak wouldn't matter in practice. I'd slightly prefer
> using a strong reference because then we can get rid of if(!clientWrapper)
check
> from a bunch of methods. We anyway need to get rid of the checks when we
change
> the reference to Member<> in the next CL. It looks simpler to use a strong
> reference from the beginning.
> 

Sorry I don't understand. IIUC those checks won't go away even when we replace
WorkerThreadableLoader::m_workerClientWrapper's Persistent with Member. The
ThreadableLoaderClientWrapper is held as Persistent/Member in the worker thread,
but in Peer functions, we are in the main thread, so we cannot rely on that
Persistent/Member (i.e., the protection on the worker thread may go away
completely asynchronously with the per-thread heap mechanism).

[commit-bot: I haz the power, 2016-07-25 03:12:30]

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

[commit-bot: I haz the power, 2016-07-25 03:12:33]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-07-25 07:48:49]

https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp (right):

https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:471: void
WorkerThreadableLoader::Peer::contextDestroyed()
On 2016/07/25 02:20:59, yhirano wrote:
> On 2016/07/22 08:57:30, haraken wrote:
> > 
> > Just help me understand: Do we need to observe contextDestroyed()? When the
> > worker thread is shut down, Bridge::destroy() should have already been
called.
> > Bridge::destroy() calls Peer:cancel(), so we won't need to call cancel()
here.
> > 
> > Also the worker thread can abort things himself without relying on the main
> > thread call m_forwarder->abort().
> > 
> > 
> > 
> 
> Peer needs to inherit WorkerThreadableLifecycleObserver from two reasons:
> 
> 
> 1) It should know if the thread is being terminated in Peer::createAndStart.
As
> the Peer is not yet associated with a Bridge, I would like to cancel the main
> thread loader here.
> 
> 2) The abort call in contextDestroyed interrupts a worker-side sync loading
that
> may block the thread termination (Even the worker thread is blocked, the
> termination process can be started from the main thread).

Makes sense.

I now understand that Peer needs to observe contextDestroyed(), but do we need
to call Peer::cancel()? I guess Peer::cancel() will anyway be called when
Bridge::destroy() is called (when the worker thread terminates), so we don't
need to call it here.

[yhirano, 2016-07-25 09:29:18]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 09:29:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 10:53:33]

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

[commit-bot: I haz the power, 2016-07-25 10:53:34]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-07-25 11:07:46]

https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp (right):

https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:471: void
WorkerThreadableLoader::Peer::contextDestroyed()
On 2016/07/25 07:48:48, haraken wrote:
> On 2016/07/25 02:20:59, yhirano wrote:
> > On 2016/07/22 08:57:30, haraken wrote:
> > > 
> > > Just help me understand: Do we need to observe contextDestroyed()? When
the
> > > worker thread is shut down, Bridge::destroy() should have already been
> called.
> > > Bridge::destroy() calls Peer:cancel(), so we won't need to call cancel()
> here.
> > > 
> > > Also the worker thread can abort things himself without relying on the
main
> > > thread call m_forwarder->abort().
> > > 
> > > 
> > > 
> > 
> > Peer needs to inherit WorkerThreadableLifecycleObserver from two reasons:
> > 
> > 
> > 1) It should know if the thread is being terminated in Peer::createAndStart.
> As
> > the Peer is not yet associated with a Bridge, I would like to cancel the
main
> > thread loader here.
> > 
> > 2) The abort call in contextDestroyed interrupts a worker-side sync loading
> that
> > may block the thread termination (Even the worker thread is blocked, the
> > termination process can be started from the main thread).
> 
> Makes sense.
> 
> I now understand that Peer needs to observe contextDestroyed(), but do we need
> to call Peer::cancel()? I guess Peer::cancel() will anyway be called when
> Bridge::destroy() is called (when the worker thread terminates), so we don't
> need to call it here.
> 

I'm concerned about the process shutdown scenario. On that case it's possible
that Bridge::destroy runs on the worker thread but the posted Peer::cancel won't
run on the main thread, I think.

What do you think about this scenario? If that's impossible, I think we can
remove this statement.

[haraken, 2016-07-25 11:13:28]

On 2016/07/25 11:07:46, yhirano wrote:
>
https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp (right):
> 
>
https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:471: void
> WorkerThreadableLoader::Peer::contextDestroyed()
> On 2016/07/25 07:48:48, haraken wrote:
> > On 2016/07/25 02:20:59, yhirano wrote:
> > > On 2016/07/22 08:57:30, haraken wrote:
> > > > 
> > > > Just help me understand: Do we need to observe contextDestroyed()? When
> the
> > > > worker thread is shut down, Bridge::destroy() should have already been
> > called.
> > > > Bridge::destroy() calls Peer:cancel(), so we won't need to call cancel()
> > here.
> > > > 
> > > > Also the worker thread can abort things himself without relying on the
> main
> > > > thread call m_forwarder->abort().
> > > > 
> > > > 
> > > > 
> > > 
> > > Peer needs to inherit WorkerThreadableLifecycleObserver from two reasons:
> > > 
> > > 
> > > 1) It should know if the thread is being terminated in
Peer::createAndStart.
> > As
> > > the Peer is not yet associated with a Bridge, I would like to cancel the
> main
> > > thread loader here.
> > > 
> > > 2) The abort call in contextDestroyed interrupts a worker-side sync
loading
> > that
> > > may block the thread termination (Even the worker thread is blocked, the
> > > termination process can be started from the main thread).
> > 
> > Makes sense.
> > 
> > I now understand that Peer needs to observe contextDestroyed(), but do we
need
> > to call Peer::cancel()? I guess Peer::cancel() will anyway be called when
> > Bridge::destroy() is called (when the worker thread terminates), so we don't
> > need to call it here.
> > 
> 
> I'm concerned about the process shutdown scenario. On that case it's possible
> that Bridge::destroy runs on the worker thread but the posted Peer::cancel
won't
> run on the main thread, I think.
> 
> What do you think about this scenario? If that's impossible, I think we can
> remove this statement.

Are you concerned that the main thread shuts down before finishing processing
all tasks posted by the worker thread? Or are you concerned that there may be a
scenario where the worker thread doesn't post a task that triggers
Peer::cancel()?

[yhirano, 2016-07-25 11:25:01]

On 2016/07/25 11:13:28, haraken wrote:
> On 2016/07/25 11:07:46, yhirano wrote:
> >
>
https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:471: void
> > WorkerThreadableLoader::Peer::contextDestroyed()
> > On 2016/07/25 07:48:48, haraken wrote:
> > > On 2016/07/25 02:20:59, yhirano wrote:
> > > > On 2016/07/22 08:57:30, haraken wrote:
> > > > > 
> > > > > Just help me understand: Do we need to observe contextDestroyed()?
When
> > the
> > > > > worker thread is shut down, Bridge::destroy() should have already been
> > > called.
> > > > > Bridge::destroy() calls Peer:cancel(), so we won't need to call
cancel()
> > > here.
> > > > > 
> > > > > Also the worker thread can abort things himself without relying on the
> > main
> > > > > thread call m_forwarder->abort().
> > > > > 
> > > > > 
> > > > > 
> > > > 
> > > > Peer needs to inherit WorkerThreadableLifecycleObserver from two
reasons:
> > > > 
> > > > 
> > > > 1) It should know if the thread is being terminated in
> Peer::createAndStart.
> > > As
> > > > the Peer is not yet associated with a Bridge, I would like to cancel the
> > main
> > > > thread loader here.
> > > > 
> > > > 2) The abort call in contextDestroyed interrupts a worker-side sync
> loading
> > > that
> > > > may block the thread termination (Even the worker thread is blocked, the
> > > > termination process can be started from the main thread).
> > > 
> > > Makes sense.
> > > 
> > > I now understand that Peer needs to observe contextDestroyed(), but do we
> need
> > > to call Peer::cancel()? I guess Peer::cancel() will anyway be called when
> > > Bridge::destroy() is called (when the worker thread terminates), so we
don't
> > > need to call it here.
> > > 
> > 
> > I'm concerned about the process shutdown scenario. On that case it's
possible
> > that Bridge::destroy runs on the worker thread but the posted Peer::cancel
> won't
> > run on the main thread, I think.
> > 
> > What do you think about this scenario? If that's impossible, I think we can
> > remove this statement.
> 
> Are you concerned that the main thread shuts down before finishing processing
> all tasks posted by the worker thread? Or are you concerned that there may be
a
> scenario where the worker thread doesn't post a task that triggers
> Peer::cancel()?

Former.

1. A dedicated worker is running, a WorkerThreadableLoader is running on the
thread, and a DocumentThreadableLoader associated with it is running on the main
thread.
2. blink::shutdown is called.
3. WorkerThread::terminateAndWaitForAllWorkers() is called.
4. WorkerThreadLifecycleNotifier::notifyContextDestroyed() is called.
5. WorkerGlobalScope::dispose is called [on the worker thread]
6. WorkerThreadableLoader::destroy is called and Peer::cancel is posted on the
main thread [on the worker thread].
7. (end of WorkerThread::terminateAndWaitForAllWorkers())
8. Platform::shutdown is called.

As everything in blink::shutdown is executed synchronously, I think there is no
chance to run the Peer::cancel task posted in 6.

[haraken, 2016-07-25 11:29:41]

On 2016/07/25 11:25:01, yhirano wrote:
> On 2016/07/25 11:13:28, haraken wrote:
> > On 2016/07/25 11:07:46, yhirano wrote:
> > >
> >
>
https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:471: void
> > > WorkerThreadableLoader::Peer::contextDestroyed()
> > > On 2016/07/25 07:48:48, haraken wrote:
> > > > On 2016/07/25 02:20:59, yhirano wrote:
> > > > > On 2016/07/22 08:57:30, haraken wrote:
> > > > > > 
> > > > > > Just help me understand: Do we need to observe contextDestroyed()?
> When
> > > the
> > > > > > worker thread is shut down, Bridge::destroy() should have already
been
> > > > called.
> > > > > > Bridge::destroy() calls Peer:cancel(), so we won't need to call
> cancel()
> > > > here.
> > > > > > 
> > > > > > Also the worker thread can abort things himself without relying on
the
> > > main
> > > > > > thread call m_forwarder->abort().
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > 
> > > > > Peer needs to inherit WorkerThreadableLifecycleObserver from two
> reasons:
> > > > > 
> > > > > 
> > > > > 1) It should know if the thread is being terminated in
> > Peer::createAndStart.
> > > > As
> > > > > the Peer is not yet associated with a Bridge, I would like to cancel
the
> > > main
> > > > > thread loader here.
> > > > > 
> > > > > 2) The abort call in contextDestroyed interrupts a worker-side sync
> > loading
> > > > that
> > > > > may block the thread termination (Even the worker thread is blocked,
the
> > > > > termination process can be started from the main thread).
> > > > 
> > > > Makes sense.
> > > > 
> > > > I now understand that Peer needs to observe contextDestroyed(), but do
we
> > need
> > > > to call Peer::cancel()? I guess Peer::cancel() will anyway be called
when
> > > > Bridge::destroy() is called (when the worker thread terminates), so we
> don't
> > > > need to call it here.
> > > > 
> > > 
> > > I'm concerned about the process shutdown scenario. On that case it's
> possible
> > > that Bridge::destroy runs on the worker thread but the posted Peer::cancel
> > won't
> > > run on the main thread, I think.
> > > 
> > > What do you think about this scenario? If that's impossible, I think we
can
> > > remove this statement.
> > 
> > Are you concerned that the main thread shuts down before finishing
processing
> > all tasks posted by the worker thread? Or are you concerned that there may
be
> a
> > scenario where the worker thread doesn't post a task that triggers
> > Peer::cancel()?
> 
> Former.
> 
> 1. A dedicated worker is running, a WorkerThreadableLoader is running on the
> thread, and a DocumentThreadableLoader associated with it is running on the
main
> thread.
> 2. blink::shutdown is called.
> 3. WorkerThread::terminateAndWaitForAllWorkers() is called.
> 4. WorkerThreadLifecycleNotifier::notifyContextDestroyed() is called.
> 5. WorkerGlobalScope::dispose is called [on the worker thread]
> 6. WorkerThreadableLoader::destroy is called and Peer::cancel is posted on the
> main thread [on the worker thread].
> 7. (end of WorkerThread::terminateAndWaitForAllWorkers())
> 8. Platform::shutdown is called.
> 
> As everything in blink::shutdown is executed synchronously, I think there is
no
> chance to run the Peer::cancel task posted in 6.

Thanks for the details.

Should we  probably call messageloop.RunUntilIdle() before 8? (tzik@ was trying
something like this.) I think it should be guaranteed that all tasks posted to
the main thread are processed before the main thread shuts down. Otherwise, it
will cause similar threading issues in other places...

[yhirano, 2016-07-25 12:10:04]

On 2016/07/25 11:29:41, haraken wrote:
> On 2016/07/25 11:25:01, yhirano wrote:
> > On 2016/07/25 11:13:28, haraken wrote:
> > > On 2016/07/25 11:07:46, yhirano wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
> > > > File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp
> > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
> > > > third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:471:
void
> > > > WorkerThreadableLoader::Peer::contextDestroyed()
> > > > On 2016/07/25 07:48:48, haraken wrote:
> > > > > On 2016/07/25 02:20:59, yhirano wrote:
> > > > > > On 2016/07/22 08:57:30, haraken wrote:
> > > > > > > 
> > > > > > > Just help me understand: Do we need to observe contextDestroyed()?
> > When
> > > > the
> > > > > > > worker thread is shut down, Bridge::destroy() should have already
> been
> > > > > called.
> > > > > > > Bridge::destroy() calls Peer:cancel(), so we won't need to call
> > cancel()
> > > > > here.
> > > > > > > 
> > > > > > > Also the worker thread can abort things himself without relying on
> the
> > > > main
> > > > > > > thread call m_forwarder->abort().
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > 
> > > > > > Peer needs to inherit WorkerThreadableLifecycleObserver from two
> > reasons:
> > > > > > 
> > > > > > 
> > > > > > 1) It should know if the thread is being terminated in
> > > Peer::createAndStart.
> > > > > As
> > > > > > the Peer is not yet associated with a Bridge, I would like to cancel
> the
> > > > main
> > > > > > thread loader here.
> > > > > > 
> > > > > > 2) The abort call in contextDestroyed interrupts a worker-side sync
> > > loading
> > > > > that
> > > > > > may block the thread termination (Even the worker thread is blocked,
> the
> > > > > > termination process can be started from the main thread).
> > > > > 
> > > > > Makes sense.
> > > > > 
> > > > > I now understand that Peer needs to observe contextDestroyed(), but do
> we
> > > need
> > > > > to call Peer::cancel()? I guess Peer::cancel() will anyway be called
> when
> > > > > Bridge::destroy() is called (when the worker thread terminates), so we
> > don't
> > > > > need to call it here.
> > > > > 
> > > > 
> > > > I'm concerned about the process shutdown scenario. On that case it's
> > possible
> > > > that Bridge::destroy runs on the worker thread but the posted
Peer::cancel
> > > won't
> > > > run on the main thread, I think.
> > > > 
> > > > What do you think about this scenario? If that's impossible, I think we
> can
> > > > remove this statement.
> > > 
> > > Are you concerned that the main thread shuts down before finishing
> processing
> > > all tasks posted by the worker thread? Or are you concerned that there may
> be
> > a
> > > scenario where the worker thread doesn't post a task that triggers
> > > Peer::cancel()?
> > 
> > Former.
> > 
> > 1. A dedicated worker is running, a WorkerThreadableLoader is running on the
> > thread, and a DocumentThreadableLoader associated with it is running on the
> main
> > thread.
> > 2. blink::shutdown is called.
> > 3. WorkerThread::terminateAndWaitForAllWorkers() is called.
> > 4. WorkerThreadLifecycleNotifier::notifyContextDestroyed() is called.
> > 5. WorkerGlobalScope::dispose is called [on the worker thread]
> > 6. WorkerThreadableLoader::destroy is called and Peer::cancel is posted on
the
> > main thread [on the worker thread].
> > 7. (end of WorkerThread::terminateAndWaitForAllWorkers())
> > 8. Platform::shutdown is called.
> > 
> > As everything in blink::shutdown is executed synchronously, I think there is
> no
> > chance to run the Peer::cancel task posted in 6.
> 
> Thanks for the details.
> 
> Should we  probably call messageloop.RunUntilIdle() before 8? (tzik@ was
trying
> something like this.) I think it should be guaranteed that all tasks posted to
> the main thread are processed before the main thread shuts down. Otherwise, it
> will cause similar threading issues in other places...

Maybe, but I'm not sure. For this class, it's enough to cancel the loader at the
step 4 (i.e., Peer::contextDestroyed). It's not a so bad solution, I believe.
Personally I hope other classes can do the same thing, but I'm don't know if
it's possible right now...

[haraken, 2016-07-25 12:15:44]

On 2016/07/25 12:10:04, yhirano wrote:
> On 2016/07/25 11:29:41, haraken wrote:
> > On 2016/07/25 11:25:01, yhirano wrote:
> > > On 2016/07/25 11:13:28, haraken wrote:
> > > > On 2016/07/25 11:07:46, yhirano wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
> > > > > File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp
> > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Sou...
> > > > > third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:471:
> void
> > > > > WorkerThreadableLoader::Peer::contextDestroyed()
> > > > > On 2016/07/25 07:48:48, haraken wrote:
> > > > > > On 2016/07/25 02:20:59, yhirano wrote:
> > > > > > > On 2016/07/22 08:57:30, haraken wrote:
> > > > > > > > 
> > > > > > > > Just help me understand: Do we need to observe
contextDestroyed()?
> > > When
> > > > > the
> > > > > > > > worker thread is shut down, Bridge::destroy() should have
already
> > been
> > > > > > called.
> > > > > > > > Bridge::destroy() calls Peer:cancel(), so we won't need to call
> > > cancel()
> > > > > > here.
> > > > > > > > 
> > > > > > > > Also the worker thread can abort things himself without relying
on
> > the
> > > > > main
> > > > > > > > thread call m_forwarder->abort().
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > 
> > > > > > > Peer needs to inherit WorkerThreadableLifecycleObserver from two
> > > reasons:
> > > > > > > 
> > > > > > > 
> > > > > > > 1) It should know if the thread is being terminated in
> > > > Peer::createAndStart.
> > > > > > As
> > > > > > > the Peer is not yet associated with a Bridge, I would like to
cancel
> > the
> > > > > main
> > > > > > > thread loader here.
> > > > > > > 
> > > > > > > 2) The abort call in contextDestroyed interrupts a worker-side
sync
> > > > loading
> > > > > > that
> > > > > > > may block the thread termination (Even the worker thread is
blocked,
> > the
> > > > > > > termination process can be started from the main thread).
> > > > > > 
> > > > > > Makes sense.
> > > > > > 
> > > > > > I now understand that Peer needs to observe contextDestroyed(), but
do
> > we
> > > > need
> > > > > > to call Peer::cancel()? I guess Peer::cancel() will anyway be called
> > when
> > > > > > Bridge::destroy() is called (when the worker thread terminates), so
we
> > > don't
> > > > > > need to call it here.
> > > > > > 
> > > > > 
> > > > > I'm concerned about the process shutdown scenario. On that case it's
> > > possible
> > > > > that Bridge::destroy runs on the worker thread but the posted
> Peer::cancel
> > > > won't
> > > > > run on the main thread, I think.
> > > > > 
> > > > > What do you think about this scenario? If that's impossible, I think
we
> > can
> > > > > remove this statement.
> > > > 
> > > > Are you concerned that the main thread shuts down before finishing
> > processing
> > > > all tasks posted by the worker thread? Or are you concerned that there
may
> > be
> > > a
> > > > scenario where the worker thread doesn't post a task that triggers
> > > > Peer::cancel()?
> > > 
> > > Former.
> > > 
> > > 1. A dedicated worker is running, a WorkerThreadableLoader is running on
the
> > > thread, and a DocumentThreadableLoader associated with it is running on
the
> > main
> > > thread.
> > > 2. blink::shutdown is called.
> > > 3. WorkerThread::terminateAndWaitForAllWorkers() is called.
> > > 4. WorkerThreadLifecycleNotifier::notifyContextDestroyed() is called.
> > > 5. WorkerGlobalScope::dispose is called [on the worker thread]
> > > 6. WorkerThreadableLoader::destroy is called and Peer::cancel is posted on
> the
> > > main thread [on the worker thread].
> > > 7. (end of WorkerThread::terminateAndWaitForAllWorkers())
> > > 8. Platform::shutdown is called.
> > > 
> > > As everything in blink::shutdown is executed synchronously, I think there
is
> > no
> > > chance to run the Peer::cancel task posted in 6.
> > 
> > Thanks for the details.
> > 
> > Should we  probably call messageloop.RunUntilIdle() before 8? (tzik@ was
> trying
> > something like this.) I think it should be guaranteed that all tasks posted
to
> > the main thread are processed before the main thread shuts down. Otherwise,
it
> > will cause similar threading issues in other places...
> 
> Maybe, but I'm not sure. For this class, it's enough to cancel the loader at
the
> step 4 (i.e., Peer::contextDestroyed). It's not a so bad solution, I believe.
> Personally I hope other classes can do the same thing, but I'm don't know if
> it's possible right now...

Anyway I now understand the situation well. I think it should be fixed somehow
(=guarantee that the main thread processes all tasks before shutting down) but
it won't need to block this CL.

LGTM on my side.

[yhirano, 2016-07-26 04:40:22]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 04:40:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 05:55:20]

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

[commit-bot: I haz the power, 2016-07-26 05:55:21]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-07-26 07:32:52]

https://codereview.chromium.org/2151173003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp (right):

https://codereview.chromium.org/2151173003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:136:
DCHECK(!m_isSignalCalled);
Violations to DCHECKs on |m_isSignalCalled| and |m_isWaitDone| cause race
conditions and use-after-free, so it might be better to use CHECK() for them.

[yhirano, 2016-07-26 09:55:13]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 09:55:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-07-26 09:57:34]

https://codereview.chromium.org/2151173003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp (right):

https://codereview.chromium.org/2151173003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:136:
DCHECK(!m_isSignalCalled);
On 2016/07/26 07:32:52, hiroshige wrote:
> Violations to DCHECKs on |m_isSignalCalled| and |m_isWaitDone| cause race
> conditions and use-after-free, so it might be better to use CHECK() for them.

Done.

[nhiroki, 2016-07-26 10:26:24]

lgtm

[commit-bot: I haz the power, 2016-07-26 12:17:28]

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

[commit-bot: I haz the power, 2016-07-26 12:17:29]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-07-27 02:17:49]

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

[kinuko, 2016-07-27 02:17:50]

DBC (minor)

https://codereview.chromium.org/2151173003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/2151173003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:94: class
AsyncTaskForwarder final : public TaskForwarder {
nit: declaration of TaskForwarder subclasses and WaitableEventWithTasks could be
possibly moved into .cpp?  Having so many inner class declarations in a header
slightly hurts readability.

[kinuko, 2016-07-27 02:18:03]

kinuko@chromium.org changed reviewers:
- kinuko@chromium.org

[yhirano, 2016-07-28 06:28:59]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 06:29:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-07-28 07:25:12]

https://codereview.chromium.org/2151173003/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h (right):

https://codereview.chromium.org/2151173003/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h:94: class
AsyncTaskForwarder final : public TaskForwarder {
On 2016/07/27 02:17:49, kinuko wrote:
> nit: declaration of TaskForwarder subclasses and WaitableEventWithTasks could
be
> possibly moved into .cpp?  Having so many inner class declarations in a header
> slightly hurts readability.

Done.

[yhirano, 2016-07-28 07:25:47]

yhirano@chromium.org changed reviewers:
+ japhet@chromium.org

[yhirano, 2016-07-28 07:25:48]

+japhet@

[commit-bot: I haz the power, 2016-07-28 08:12:08]

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

[commit-bot: I haz the power, 2016-07-28 08:12:10]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-08-01 05:28:48]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 05:28:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-08-01 07:57:43]

On 2016/07/28 07:25:48, yhirano wrote:
> +japhet@

Nate, Hiroshige: Do you have further comments?

[hiroshige, 2016-08-01 08:16:26]

On 2016/08/01 07:57:43, yhirano wrote:
> On 2016/07/28 07:25:48, yhirano wrote:
> > +japhet@
> 
> Nate, Hiroshige: Do you have further comments?

lgtm.

[commit-bot: I haz the power, 2016-08-01 08:45:01]

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

[commit-bot: I haz the power, 2016-08-01 08:45:02]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2016-08-03 19:23:10]

On 2016/08/01 07:57:43, yhirano wrote:
> On 2016/07/28 07:25:48, yhirano wrote:
> > +japhet@
> 
> Nate, Hiroshige: Do you have further comments?

I don't know WorkerThreadableLoader well enough to feel comfortable giving a
real approval, but this seems reasonable to me.

[yhirano, 2016-08-04 03:06:01]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-08-04 03:06:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/2151173003/#ps300001
(title: "rebase")

[yhirano, 2016-08-04 03:06:01]

The CQ bit was unchecked by yhirano@chromium.org

[yhirano, 2016-08-04 03:06:08]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 03:06:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-08-04 03:06:27]

On 2016/08/03 19:23:10, Nate Chapin wrote:
> On 2016/08/01 07:57:43, yhirano wrote:
> > On 2016/07/28 07:25:48, yhirano wrote:
> > > +japhet@
> > 
> > Nate, Hiroshige: Do you have further comments?
> 
> I don't know WorkerThreadableLoader well enough to feel comfortable giving a
> real approval, but this seems reasonable to me.

Thank you!

[yhirano, 2016-08-04 03:55:10]

The CQ bit was unchecked by yhirano@chromium.org

[yhirano, 2016-08-04 03:55:22]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-08-04 03:55:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/2151173003/#ps300001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-04 03:55:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 05:47:20]

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

[commit-bot: I haz the power, 2016-08-04 05:47:22]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yhirano, 2016-08-04 05:49:48]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2016-08-04 05:50:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 06:24:01]

Description was changed from

==========
Move WorkerThreadableLoader internal classes to Oilpan heap

This CL splits WorkerThreadableLoader::MainThreadBridgeBase (and subclasses)
into the following on-heap classes.

 - TaskForwarder and subclasses
   Lives in the main thread and forwards tasks to the worker thread.
 - Bridge
   Lives in the worker thread
 - Peer
   Lives in the worker thread and forwards notifications from
   the loader living in the main thread to ThreadableLoaderClientWrapper
   living in the worker thread with a TaskForwarder.

Pointers to an object living in the other thread are held as
CrossThread[Weak]Persistents (e.g., Bridge::m_peer).


BUG=587663
==========

to

==========
Move WorkerThreadableLoader internal classes to Oilpan heap

This CL splits WorkerThreadableLoader::MainThreadBridgeBase (and subclasses)
into the following on-heap classes.

 - TaskForwarder and subclasses
   Lives in the main thread and forwards tasks to the worker thread.
 - Bridge
   Lives in the worker thread
 - Peer
   Lives in the worker thread and forwards notifications from
   the loader living in the main thread to ThreadableLoaderClientWrapper
   living in the worker thread with a TaskForwarder.

Pointers to an object living in the other thread are held as
CrossThread[Weak]Persistents (e.g., Bridge::m_peer).


BUG=587663
==========

[commit-bot: I haz the power, 2016-08-04 06:24:04]

Committed patchset #13 (id:300001)

[commit-bot: I haz the power, 2016-08-04 06:25:36]

Description was changed from

==========
Move WorkerThreadableLoader internal classes to Oilpan heap

This CL splits WorkerThreadableLoader::MainThreadBridgeBase (and subclasses)
into the following on-heap classes.

 - TaskForwarder and subclasses
   Lives in the main thread and forwards tasks to the worker thread.
 - Bridge
   Lives in the worker thread
 - Peer
   Lives in the worker thread and forwards notifications from
   the loader living in the main thread to ThreadableLoaderClientWrapper
   living in the worker thread with a TaskForwarder.

Pointers to an object living in the other thread are held as
CrossThread[Weak]Persistents (e.g., Bridge::m_peer).


BUG=587663
==========

to

==========
Move WorkerThreadableLoader internal classes to Oilpan heap

This CL splits WorkerThreadableLoader::MainThreadBridgeBase (and subclasses)
into the following on-heap classes.

 - TaskForwarder and subclasses
   Lives in the main thread and forwards tasks to the worker thread.
 - Bridge
   Lives in the worker thread
 - Peer
   Lives in the worker thread and forwards notifications from
   the loader living in the main thread to ThreadableLoaderClientWrapper
   living in the worker thread with a TaskForwarder.

Pointers to an object living in the other thread are held as
CrossThread[Weak]Persistents (e.g., Bridge::m_peer).

BUG=587663

Committed: https://crrev.com/279f12d293087b7619089ecbe418e83df9d04a76
Cr-Commit-Position: refs/heads/master@{#409727}
==========

[commit-bot: I haz the power, 2016-08-04 06:25:38]

Patchset 13 (id:??) landed as
https://crrev.com/279f12d293087b7619089ecbe418e83df9d04a76
Cr-Commit-Position: refs/heads/master@{#409727}