Implement WebThreadSupportingGC, which wraps a WebThread attached to Oilpan's GC

Implement WebThreadSupportingGC, which wraps a WebThread attached to Oilpan's GC

In order to attach/detach a WebThread to/from Oilpan's GC,
we need a bunch of setups about PendingGCRunner, MessageLoopInterruptor etc.
To avoid duplicating the setup code in all WebThread customers,
this CL adds a thin wrapper class (WebThreadSupportingGC) for a WebThread
attached to Oilpan's GC.

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

[haraken, 2014-08-13 04:04:02]

PTAL.

(At the moment, I introduced WebThreadRunner as a proxy class for a WebThread.
I'm not sure if this is a great approach. Any feedback is welcome.)

[Mads Ager (chromium), 2014-08-13 07:26:20]

LGTM

Since the WebThreads are supplied by the embedder and we therefore cannot use
inheritance; it seems like wrapping and delegating is the right way to go.

https://codereview.chromium.org/469683002/diff/20001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserThread.h (right):

https://codereview.chromium.org/469683002/diff/20001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.h:35: #include
"platform/heap/glue/MessageLoopInterruptor.h"
You can probably remove these two platform/heap includes now.

https://codereview.chromium.org/469683002/diff/20001/Source/core/workers/Work...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/469683002/diff/20001/Source/core/workers/Work...
Source/core/workers/WorkerThread.h:35: #include
"platform/heap/glue/MessageLoopInterruptor.h"
Remove the heap glue includes?

https://codereview.chromium.org/469683002/diff/20001/Source/platform/WebThrea...
File Source/platform/WebThreadRunner.h (right):

https://codereview.chromium.org/469683002/diff/20001/Source/platform/WebThrea...
Source/platform/WebThreadRunner.h:18: class WebThreadRunner {
WebThreadSupportingGC?

Should we add a comment along the lines of:

"WebThreadSupportingGC wraps a WebThread and adds support for attaching to and
detaching from the Blink GC infrastructure. The attachGC method must be called
during initialization on the WebThread and before the thread allocates any
objects managed by the Blink GC. The detach GC method must be called on the
WebThread during shutdown when the thread no longer needs to access objects
managed by the Blink GC."

?

[haraken, 2014-08-13 08:03:09]

jochen@ or tkent-san@: would you take another look?

https://codereview.chromium.org/469683002/diff/20001/Source/core/html/parser/...
File Source/core/html/parser/HTMLParserThread.h (right):

https://codereview.chromium.org/469683002/diff/20001/Source/core/html/parser/...
Source/core/html/parser/HTMLParserThread.h:35: #include
"platform/heap/glue/MessageLoopInterruptor.h"
On 2014/08/13 07:26:20, Mads Ager (chromium) wrote:
> You can probably remove these two platform/heap includes now.

Done.

https://codereview.chromium.org/469683002/diff/20001/Source/core/workers/Work...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/469683002/diff/20001/Source/core/workers/Work...
Source/core/workers/WorkerThread.h:35: #include
"platform/heap/glue/MessageLoopInterruptor.h"
On 2014/08/13 07:26:20, Mads Ager (chromium) wrote:
> Remove the heap glue includes?

Done.

https://codereview.chromium.org/469683002/diff/20001/Source/platform/WebThrea...
File Source/platform/WebThreadRunner.h (right):

https://codereview.chromium.org/469683002/diff/20001/Source/platform/WebThrea...
Source/platform/WebThreadRunner.h:18: class WebThreadRunner {
On 2014/08/13 07:26:20, Mads Ager (chromium) wrote:
> WebThreadSupportingGC?
> 
> Should we add a comment along the lines of:
> 
> "WebThreadSupportingGC wraps a WebThread and adds support for attaching to and
> detaching from the Blink GC infrastructure. The attachGC method must be called
> during initialization on the WebThread and before the thread allocates any
> objects managed by the Blink GC. The detach GC method must be called on the
> WebThread during shutdown when the thread no longer needs to access objects
> managed by the Blink GC."
> 
> ?

Renamed to WebThreadSupportingGC & added the comment.

[jochen (gone - plz use gerrit), 2014-08-13 12:35:42]

https://codereview.chromium.org/469683002/diff/60001/Source/platform/WebThrea...
File Source/platform/WebThreadSupportingGC.h (right):

https://codereview.chromium.org/469683002/diff/60001/Source/platform/WebThrea...
Source/platform/WebThreadSupportingGC.h:31: m_thread->postTask(task);
this is not safe. on a worker thread, all posted tasks must be wrapped in some
wrapper class that checks the worker scope's state before execution. there's
another interface we use that has a postTask method that does the right
wrapping, something like WebWorkerRunLoop. can you piggyback on that?

[Mads Ager (chromium), 2014-08-13 12:40:37]

https://codereview.chromium.org/469683002/diff/60001/Source/platform/WebThrea...
File Source/platform/WebThreadSupportingGC.h (right):

https://codereview.chromium.org/469683002/diff/60001/Source/platform/WebThrea...
Source/platform/WebThreadSupportingGC.h:31: m_thread->postTask(task);
On 2014/08/13 12:35:42, jochen (slow - soon OOO) wrote:
> this is not safe. on a worker thread, all posted tasks must be wrapped in some
> wrapper class that checks the worker scope's state before execution. there's
> another interface we use that has a postTask method that does the right
> wrapping, something like WebWorkerRunLoop. can you piggyback on that?

I'm not sure I understand. We are only replacing blink:WebThread in the code
base with WebThreadSupportingGC. So, where we call this postTask method we would
directly call blink::WebThread::postTask before. So, we will end up calling
exactly the same as we did before. Where does this become unsafe? I very well
might be missing something, but I don't see it.

[haraken, 2014-08-13 12:44:17]

On 2014/08/13 12:35:42, jochen (slow - soon OOO) wrote:
>
https://codereview.chromium.org/469683002/diff/60001/Source/platform/WebThrea...
> File Source/platform/WebThreadSupportingGC.h (right):
> 
>
https://codereview.chromium.org/469683002/diff/60001/Source/platform/WebThrea...
> Source/platform/WebThreadSupportingGC.h:31: m_thread->postTask(task);
> this is not safe. on a worker thread, all posted tasks must be wrapped in some
> wrapper class that checks the worker scope's state before execution.

As Mads mentioned, WebThreadSupportingGC is just a very thin wrapper class of
blink::WebThread.

It's a responsibility of the caller side of WebThreadSupportingGC to check
WorkerGlobalScope's state. In case of worker threads, it's already taken care of
in WorkerThreadShutdownStartTask::performTask and
orkerThreadShutdownFinishTask::performTask.

[jochen (gone - plz use gerrit), 2014-08-13 13:07:16]

On 2014/08/13 at 12:44:17, haraken wrote:
> On 2014/08/13 12:35:42, jochen (slow - soon OOO) wrote:
> >
https://codereview.chromium.org/469683002/diff/60001/Source/platform/WebThrea...
> > File Source/platform/WebThreadSupportingGC.h (right):
> > 
> >
https://codereview.chromium.org/469683002/diff/60001/Source/platform/WebThrea...
> > Source/platform/WebThreadSupportingGC.h:31: m_thread->postTask(task);
> > this is not safe. on a worker thread, all posted tasks must be wrapped in
some
> > wrapper class that checks the worker scope's state before execution.
> 
> As Mads mentioned, WebThreadSupportingGC is just a very thin wrapper class of
blink::WebThread.
> 
> It's a responsibility of the caller side of WebThreadSupportingGC to check
WorkerGlobalScope's state. In case of worker threads, it's already taken care of
in WorkerThreadShutdownStartTask::performTask and
orkerThreadShutdownFinishTask::performTask.

ah, I thought you use this new API to post tasks from the outside.

then this lgtm

[haraken, 2014-08-13 13:07:42]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-08-13 13:09:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/469683002/60001

[commit-bot: I haz the power, 2014-08-13 13:20:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/19646)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22543)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/41107)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/46325)

[commit-bot: I haz the power, 2014-08-13 13:22:45]

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

[commit-bot: I haz the power, 2014-08-13 13:22:47]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/19647)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/41109)

[haraken, 2014-09-12 07:58:14]

The CQ bit was checked by haraken@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/469683002/100001

[commit-bot: I haz the power, 2014-09-12 08:08:26]

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

[commit-bot: I haz the power, 2014-09-12 08:08:27]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/55142)

[haraken, 2014-09-22 02:32:57]

The CQ bit was checked by haraken@chromium.org

[haraken, 2014-09-22 02:33:20]

Landing this CL. Sorry for soooo late on this.

[commit-bot: I haz the power, 2014-09-22 02:33:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/469683002/120001

[commit-bot: I haz the power, 2014-09-22 02:46:55]

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

[commit-bot: I haz the power, 2014-09-22 02:46:57]

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

[haraken, 2014-09-22 03:17:01]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-09-22 03:17:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/469683002/120001

[commit-bot: I haz the power, 2014-09-22 03:27:59]

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

[commit-bot: I haz the power, 2014-09-22 03:28:01]

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

[haraken, 2014-09-22 03:32:19]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-09-22 03:33:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/469683002/140001

[commit-bot: I haz the power, 2014-09-22 04:37:42]

Committed patchset #8 (id:140001) as 182385