Change WokerThread to use a blink::WebThread.

Change WokerThread to use a blink::WebThread.

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

[nasko, 2014-07-24 10:20:04]

Hey Jochen,
I think this CL is finally in a state good enough for reviewing. Can you take a
look?
Thanks!
Nasko

[jochen (gone - plz use gerrit), 2014-07-24 12:12:33]

how are the WebThread objects deleted?

I'd expect something in the WebThread::cleanup to call the main thread which
then deletes the m_thread (and therefore joins the thread)?

https://codereview.chromium.org/400153002/diff/260001/LayoutTests/SlowTests
File LayoutTests/SlowTests (right):

https://codereview.chromium.org/400153002/diff/260001/LayoutTests/SlowTests#n...
LayoutTests/SlowTests:170: crbug.com/301515
fast/workers/worker-exception-during-navigation.html [ Slow ]
are those really slow or is this just a leftover from debugging?

https://codereview.chromium.org/400153002/diff/260001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/400153002/diff/260001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:78: WorkerSharedTimer(blink::WebThread*
web_thread)
explicit and webThread

https://codereview.chromium.org/400153002/diff/260001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:85: if (m_thread)
when can m_thread be null?

[nasko, 2014-07-24 12:53:18]

WebThread is owned by WorkerThread, so it will be deleted as part of
WorkerThread cleanup. 

WorkerThread is owned by the objects representing different types of workers,
which live on the main thread. The main thread shouldn't exit before those are
cleaned up, which in turn will delete the WebThread, which will do the join
call.

https://codereview.chromium.org/400153002/diff/260001/LayoutTests/SlowTests
File LayoutTests/SlowTests (right):

https://codereview.chromium.org/400153002/diff/260001/LayoutTests/SlowTests#n...
LayoutTests/SlowTests:170: crbug.com/301515
fast/workers/worker-exception-during-navigation.html [ Slow ]
On 2014/07/24 12:12:33, jochen wrote:
> are those really slow or is this just a leftover from debugging?

Those are indeed slow, I've removed all others after debugging.

https://codereview.chromium.org/400153002/diff/260001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/400153002/diff/260001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:78: WorkerSharedTimer(blink::WebThread*
web_thread)
On 2014/07/24 12:12:33, jochen wrote:
> explicit and webThread

Done.

https://codereview.chromium.org/400153002/diff/260001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:85: if (m_thread)
On 2014/07/24 12:12:33, jochen wrote:
> when can m_thread be null?

Shouldn't be null.

[jochen (gone - plz use gerrit), 2014-07-24 13:26:03]

lgtm

[nasko, 2014-07-24 13:30:36]

ager@, can you look over this CL to ensure I'm not messing up oilpan semantics
as I'm moving code around?
Thanks!

[haraken, 2014-07-24 13:32:37]

On 2014/07/24 13:30:36, nasko (in Munich) wrote:
> ager@, can you look over this CL to ensure I'm not messing up oilpan semantics
> as I'm moving code around?
> Thanks!

ager@ is on vacation. Let me take a look tomorrow morning :)

[nasko, 2014-07-24 15:03:18]

Adding aandrey@, who recently fixed instrumentation in workers. Can you ensure I
didn't undo your work?

[aandrey, 2014-07-24 16:07:53]

Instrumentation in workers looks good.

https://codereview.chromium.org/400153002/diff/230018/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/400153002/diff/230018/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:334:
InspectorInstrumentation::didKillAllExecutionContextTasks(m_workerGlobalScope.get());
is it safe to access m_workerGlobalScope here?
if not, just move this line upwards, before calling
m_workerGlobalScope->wasRequestedToTerminate()

[aandrey, 2014-07-24 16:10:52]

also cc-ing @yurys on worker instrumentation

[haraken, 2014-07-25 01:10:20]

LGTM from the oilpan perspective.

[nasko, 2014-07-25 06:30:14]

https://codereview.chromium.org/400153002/diff/230018/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/400153002/diff/230018/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:334:
InspectorInstrumentation::didKillAllExecutionContextTasks(m_workerGlobalScope.get());
On 2014/07/24 16:07:53, aandrey wrote:
> is it safe to access m_workerGlobalScope here?
> if not, just move this line upwards, before calling
> m_workerGlobalScope->wasRequestedToTerminate()

It is safe. It is used further in the WorkerThreadShutdownStartTask,
WorkerThreadShutdownFinishTask, and finally disposed in ::cleanup(). Therefore
the line can be earlier or later, whichever is best. I opted to keep it somewhat
equivalent to where it was called prior to my patch.

[yurys, 2014-07-25 06:41:57]

https://codereview.chromium.org/400153002/diff/230018/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/400153002/diff/230018/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:334:
InspectorInstrumentation::didKillAllExecutionContextTasks(m_workerGlobalScope.get());
It is unsafe to access m_workerGlobalScope here as WorkerThread::stop() is
called on the main thread while m_workerGlobalScope is cleared in
WorkerThread::cleanup which is executed on the worker thread without acquiring a
mutex.

[yurys, 2014-07-25 07:24:46]

On 2014/07/25 06:30:14, nasko (in Munich) wrote:
>
https://codereview.chromium.org/400153002/diff/230018/Source/core/workers/Wor...
> File Source/core/workers/WorkerThread.cpp (right):
> 
>
https://codereview.chromium.org/400153002/diff/230018/Source/core/workers/Wor...
> Source/core/workers/WorkerThread.cpp:334:
>
InspectorInstrumentation::didKillAllExecutionContextTasks(m_workerGlobalScope.get());
> On 2014/07/24 16:07:53, aandrey wrote:
> > is it safe to access m_workerGlobalScope here?
> > if not, just move this line upwards, before calling
> > m_workerGlobalScope->wasRequestedToTerminate()
> 
> It is safe. It is used further in the WorkerThreadShutdownStartTask,
> WorkerThreadShutdownFinishTask, and finally disposed in ::cleanup(). Therefore
> the line can be earlier or later, whichever is best. I opted to keep it
somewhat
> equivalent to where it was called prior to my patch.

Both WorkerThreadShutdownStartTask and WorkerThreadShutdownFinishTask are
executed on the worker thread and so does WorkerThread::cleanup. But as long as
all these tasks are initiated by WorkerThread::stop it should be enough to check
m_terminated to ensure global scope is not being destroyed. LGTM.

[darin (slow to review), 2014-07-25 16:30:31]

https://codereview.chromium.org/400153002/diff/290001/public/platform/WebThre...
File public/platform/WebThread.h (right):

https://codereview.chromium.org/400153002/diff/290001/public/platform/WebThre...
public/platform/WebThread.h:73: virtual void
setSharedTimerFiredFunction(SharedTimerFunction) = 0;
Why was this added to WebThread when you can implement it on top of
postDelayedTask just as src/content/ does?

[kinuko, 2014-07-26 03:49:39]

https://codereview.chromium.org/400153002/diff/290001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/400153002/diff/290001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:345: return;
nit: no need to return here (the end of method), or probably we can just exit
earlier on line 339 if !m_workerGlobalScope

https://codereview.chromium.org/400153002/diff/290001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:357: return;
Is it safe to check this here on worker thread without lock?  Isn't releasing
the weak factory enough to stop this task to run?

[jochen (gone - plz use gerrit), 2014-07-28 08:54:23]

On 2014/07/25 at 16:30:31, darin wrote:
>
https://codereview.chromium.org/400153002/diff/290001/public/platform/WebThre...
> File public/platform/WebThread.h (right):
> 
>
https://codereview.chromium.org/400153002/diff/290001/public/platform/WebThre...
> public/platform/WebThread.h:73: virtual void
setSharedTimerFiredFunction(SharedTimerFunction) = 0;
> Why was this added to WebThread when you can implement it on top of
postDelayedTask just as src/content/ does?

src/content uses a timer, and it uses the same API for the main thread (via
blink::Platform)

[nasko, 2014-07-28 09:52:59]

https://codereview.chromium.org/400153002/diff/290001/public/platform/WebThre...
File public/platform/WebThread.h (right):

https://codereview.chromium.org/400153002/diff/290001/public/platform/WebThre...
public/platform/WebThread.h:73: virtual void
setSharedTimerFiredFunction(SharedTimerFunction) = 0;
On 2014/07/25 16:30:31, darin wrote:
> Why was this added to WebThread when you can implement it on top of
> postDelayedTask just as src/content/ does?

I intended to keep this similarly to how the main thread works. It seemed to me
that re-implementing the base::Timer functionality is unnecessary code
duplication.

[nasko, 2014-07-28 12:57:13]

Fixes based on kinuko's review.

https://codereview.chromium.org/400153002/diff/290001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/400153002/diff/290001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:345: return;
On 2014/07/26 03:49:39, kinuko (ooo) wrote:
> nit: no need to return here (the end of method), or probably we can just exit
> earlier on line 339 if !m_workerGlobalScope

Done.

https://codereview.chromium.org/400153002/diff/290001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:357: return;
On 2014/07/26 03:49:39, kinuko (ooo) wrote:
> Is it safe to check this here on worker thread without lock?  Isn't releasing
> the weak factory enough to stop this task to run?

Done.

[jochen (gone - plz use gerrit), 2014-07-29 08:20:14]

postDelayedTask version lgtm

[nasko, 2014-07-29 08:51:32]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-07-29 08:51:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nasko@chromium.org/400153002/370001

[commit-bot: I haz the power, 2014-07-29 09:26:18]

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

[commit-bot: I haz the power, 2014-07-29 09:55:44]

Change committed as 179117

[alancutter (OOO until 2018), 2014-07-29 11:22:31]

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

The reason for reverting is: Failed compile on Linux Tests and Linux Tests (dbg)
builders.
http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/38284
http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests%20%28dbg%2...

../../mojo/services/html_viewer/blink_platform_impl.cc:183:14:error: allocating
an object of abstract class type 'mojo::WebThreadImpl'
  return new WebThreadImpl(name);
             ^
../../third_party/WebKit/public/platform/WebThread.h:73:18: note: unimplemented
pure virtual method 'setSharedTimerFiredFunction' in 'WebThreadImpl'
    virtual void setSharedTimerFiredFunction(SharedTimerFunction) = 0;
                 ^.

[darin (slow to review), 2014-07-29 19:28:21]

https://codereview.chromium.org/400153002/diff/370001/public/platform/WebThre...
File public/platform/WebThread.h (right):

https://codereview.chromium.org/400153002/diff/370001/public/platform/WebThre...
public/platform/WebThread.h:71: // Delayed work is driven by a shared timer.
The intent was to remove these functions, right?

[nasko, 2014-07-29 22:12:58]

On 2014/07/29 19:28:21, darin wrote:
>
https://codereview.chromium.org/400153002/diff/370001/public/platform/WebThre...
> File public/platform/WebThread.h (right):
> 
>
https://codereview.chromium.org/400153002/diff/370001/public/platform/WebThre...
> public/platform/WebThread.h:71: // Delayed work is driven by a shared timer.
> The intent was to remove these functions, right?

Yes, it was. I converted to using postDelayedTask and completely forgot to
remove them : (. I will put up an identical CL and remove those first thing in
the morning.

[haraken, 2014-08-01 20:41:04]

This thread caused timeout in a bunch of worker tests in oilpan builds. I'm
investigating. (You don't need to revert since oilpan builds are not a tree
closer.)

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...