Move WorkerThreadable internal classes to Oilpan heap (issue 2151173003 by yhirano@chromium.org)

[yhi...@chromium.org]

Reviewers: oilpan-reviews, hiroshige, nhiroki (slow)
CL: https://codereview.chromium.org/2151173003/

Description:
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

Base URL: https://chromium.googlesource.com/chromium/src.git@onheap-threadable-loader-client-wrapper

Affected files (+403, -275 lines):
M third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h
M third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp

[har...@chromium.org]

The overall approach looks good.



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

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp#newcode79
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/Source/core/loader/WorkerThreadableLoader.cpp#newcode139
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/Source/core/loader/WorkerThreadableLoader.cpp#newcode201
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/Source/core/loader/WorkerThreadableLoader.cpp#newcode338
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/Source/core/loader/WorkerThreadableLoader.cpp#newcode349
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/Source/core/loader/WorkerThreadableLoader.h
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h
(right):

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

Does this need to be a weak pointer?

https://codereview.chromium.org/2151173003/

[yhi...@chromium.org]

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

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp#newcode79
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/Source/core/loader/WorkerThreadableLoader.cpp#newcode139
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/Source/core/loader/WorkerThreadableLoader.cpp#newcode338
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/Source/core/loader/WorkerThreadableLoader.h
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h
(right):

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h#newcode219
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.

https://codereview.chromium.org/2151173003/

[yhi...@chromium.org]

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

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp#newcode201
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/Source/core/loader/WorkerThreadableLoader.cpp#newcode349
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.

https://codereview.chromium.org/2151173003/

[yhi...@chromium.org]

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

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp#newcode201
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.

https://codereview.chromium.org/2151173003/

[har...@chromium.org]

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/Source/core/loader/WorkerThreadableLoader.cpp
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp
(right):

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp#newcode349
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/Source/core/loader/WorkerThreadableLoader.h
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h
(right):

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h#newcode219
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/Source/core/loader/WorkerThreadableLoader.h
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h
(right):

https://codereview.chromium.org/2151173003/diff/80001/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h#newcode105
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/Source/core/loader/WorkerThreadableLoader.h#newcode144
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

https://codereview.chromium.org/2151173003/

[har...@chromium.org]

[yhi...@chromium.org]

> 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.



https://codereview.chromium.org/2151173003/

[yhi...@chromium.org]

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

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp#newcode349
third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp:349:
CrossThreadPersistent<ThreadableLoaderClientWrapper> clientWrapper =
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/Source/core/loader/WorkerThreadableLoader.h
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h
(right):

https://codereview.chromium.org/2151173003/diff/60001/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h#newcode219
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/Source/core/loader/WorkerThreadableLoader.h
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h
(right):

https://codereview.chromium.org/2151173003/diff/80001/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h#newcode105
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/Source/core/loader/WorkerThreadableLoader.h#newcode144
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.

https://codereview.chromium.org/2151173003/

[har...@chromium.org]

Mostly looks good from the Oilpan perspective.

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

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

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/Source/core/loader/WorkerThreadableLoader.cpp
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp
(right):

https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp#newcode280
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/Source/core/loader/WorkerThreadableLoader.cpp#newcode471
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/Source/core/loader/WorkerThreadableLoader.h
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h
(right):

https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h#newcode59
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

https://codereview.chromium.org/2151173003/

[yhi...@chromium.org]

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

https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp#newcode280
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/Source/core/loader/WorkerThreadableLoader.cpp#newcode471
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/Source/core/loader/WorkerThreadableLoader.h
File third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h
(right):

https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h#newcode59
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.

https://codereview.chromium.org/2151173003/

[yhi...@chromium.org]

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).


https://codereview.chromium.org/2151173003/

[har...@chromium.org]

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

https://codereview.chromium.org/2151173003/diff/160001/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp#newcode471
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.

https://codereview.chromium.org/2151173003/

[yhi...@chromium.org]

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

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

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.

https://codereview.chromium.org/2151173003/

[har...@chromium.org]

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()?



https://codereview.chromium.org/2151173003/

[yhi...@chromium.org]

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.

https://codereview.chromium.org/2151173003/

[har...@chromium.org]

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...



https://codereview.chromium.org/2151173003/

[yhi...@chromium.org]

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...

https://codereview.chromium.org/2151173003/

[har...@chromium.org]

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.



https://codereview.chromium.org/2151173003/

[hiro...@chromium.org]

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

https://codereview.chromium.org/2151173003/diff/240001/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp#newcode136
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.

https://codereview.chromium.org/2151173003/

[yhi...@chromium.org]

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

https://codereview.chromium.org/2151173003/diff/240001/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.cpp#newcode136
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.

https://codereview.chromium.org/2151173003/

[nhi...@chromium.org]

lgtm



https://codereview.chromium.org/2151173003/

[kin...@chromium.org]

DBC (minor)


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

https://codereview.chromium.org/2151173003/diff/260001/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h#newcode94
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.

https://codereview.chromium.org/2151173003/

[yhi...@chromium.org]

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

https://codereview.chromium.org/2151173003/diff/260001/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h#newcode94
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.

https://codereview.chromium.org/2151173003/

[yhi...@chromium.org]

+japhet@

https://codereview.chromium.org/2151173003/

[yhi...@chromium.org]

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

Nate, Hiroshige: Do you have further comments?

https://codereview.chromium.org/2151173003/

[hiro...@chromium.org]

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.

https://codereview.chromium.org/2151173003/

[jap...@chromium.org]

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.

https://codereview.chromium.org/2151173003/

[yhi...@chromium.org]

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!

https://codereview.chromium.org/2151173003/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2151173003/300001


https://codereview.chromium.org/2151173003/

[commit-bot@chromium.org via codereview.chromium.org]

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_asan_rel_ng/builds/203883)

https://codereview.chromium.org/2151173003/

[commit-bot@chromium.org via codereview.chromium.org]

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #13 (id:300001)

https://codereview.chromium.org/2151173003/

[commit-bot@chromium.org via codereview.chromium.org]

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

https://codereview.chromium.org/2151173003/