Change ChildProcess::set_main_thread registration to occur inside the RenderThreadImpl::Init method.

Change ChildProcess::set_main_thread registration to occur inside the RenderThreadImpl::Init method.

Background: The Init method is currently called from the RenderThreadImpl constructor. If set_main_thread is not called from Init then ContentRendererClient::RenderThreadStarted, which is also called from Init, will not be able to retrieve the current child thread using ChildThread::current. Retrieving the current child thread from inside RenderThreadStarted is necessary for performing actions that must occur before message processing starts such as adding additional MessageFilters to the child thread's channel.

BUG=112335
TEST=ChildThread::current() returns non-NULL in ContentRendererClient::RenderThreadStarted

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120112

[Marshall, 2012-01-27 19:02:52]

Hi John,

Please review this preliminary patch set and let me know if you agree with the
concept.

Thanks.

[jam, 2012-01-27 19:12:06]

I'm not sure I follow. The callback is from RenderThreadImpl::Init(), so why
doesn't RenderThread::current() work?

[Marshall, 2012-01-27 19:14:14]

On 2012/01/27 19:12:06, John Abd-El-Malek wrote:
> I'm not sure I follow. The callback is from RenderThreadImpl::Init(), so why
> doesn't RenderThread::current() work?

Sorry, that's actually wrong. RenderThread::current() works but the channel has
not yet been created.

[Marshall, 2012-01-27 19:15:31]

On 2012/01/27 19:14:14, Marshall wrote:
> On 2012/01/27 19:12:06, John Abd-El-Malek wrote:
> > I'm not sure I follow. The callback is from RenderThreadImpl::Init(), so why
> > doesn't RenderThread::current() work?
> 
> Sorry, that's actually wrong. RenderThread::current() works but the channel
has
> not yet been created.

Ignore that comment, it doesn't make any sense either :-). Let me look at this
again in the debugger.

[Marshall, 2012-01-27 19:19:45]

On 2012/01/27 19:15:31, Marshall wrote:
> On 2012/01/27 19:14:14, Marshall wrote:
> > On 2012/01/27 19:12:06, John Abd-El-Malek wrote:
> > > I'm not sure I follow. The callback is from RenderThreadImpl::Init(), so
why
> > > doesn't RenderThread::current() work?
> > 
> > Sorry, that's actually wrong. RenderThread::current() works but the channel
> has
> > not yet been created.
> 
> Ignore that comment, it doesn't make any sense either :-). Let me look at this
> again in the debugger.

ChildThread::current() does indeed return NULL, but it might be a symptom of me
running with the "--single-process" flag. In any case, passing the RenderThread
to RenderThreadStarted allows everything to behave correctly for me.

[jam, 2012-01-27 19:22:33]

This doesn't make sense to me, can you dig into this deeper? Are you
calling ChildThread::current() from the renderer thread?
RenderThread::current() is called from all over the place, so it must
return a valid thread even in single process case.

On Fri, Jan 27, 2012 at 11:19 AM, <marshall@chromium.org> wrote:

> On 2012/01/27 19:15:31, Marshall wrote:
>
>> On 2012/01/27 19:14:14, Marshall wrote:
>> > On 2012/01/27 19:12:06, John Abd-El-Malek wrote:
>> > > I'm not sure I follow. The callback is from RenderThreadImpl::Init(),
>> so
>>
> why
>
>> > > doesn't RenderThread::current() work?
>> >
>> > Sorry, that's actually wrong. RenderThread::current() works but the
>> channel
>> has
>> > not yet been created.
>>
>
>  Ignore that comment, it doesn't make any sense either :-). Let me look at
>> this
>> again in the debugger.
>>
>
> ChildThread::current() does indeed return NULL, but it might be a symptom
> of me
> running with the "--single-process" flag. In any case, passing the
> RenderThread
> to RenderThreadStarted allows everything to behave correctly for me.
>
>
https://chromiumcodereview.**appspot.com/9297022/<https://chromiumcodereview....
>

[Marshall, 2012-01-27 19:34:41]

On 2012/01/27 19:22:33, John Abd-El-Malek wrote:
> This doesn't make sense to me, can you dig into this deeper? Are you
> calling ChildThread::current() from the renderer thread?
> RenderThread::current() is called from all over the place, so it must
> return a valid thread even in single process case.

1. RendererMainThread::Init contains the following code:

render_process_->set_main_thread(new RenderThreadImpl(channel_id_));

2. The RenderThreadImpl constructor calls RenderThreadImpl::Init.
3. RenderThreadImpl::Init calls ContentRendererClient::RenderThreadStarted.
4. The RenderThreadImpl constructor completes allowing the set_main_thread call
to proceed.

As a result, calling ChildThread::current() from inside RenderThreadStarted
returns NULL because the RenderThreadImpl object is still being constructed and
has not yet been set as the main thread.

[jam, 2012-01-27 20:20:10]

On 2012/01/27 19:34:41, Marshall wrote:
> On 2012/01/27 19:22:33, John Abd-El-Malek wrote:
> > This doesn't make sense to me, can you dig into this deeper? Are you
> > calling ChildThread::current() from the renderer thread?
> > RenderThread::current() is called from all over the place, so it must
> > return a valid thread even in single process case.
> 
> 1. RendererMainThread::Init contains the following code:
> 
> render_process_->set_main_thread(new RenderThreadImpl(channel_id_));
> 
> 2. The RenderThreadImpl constructor calls RenderThreadImpl::Init.
> 3. RenderThreadImpl::Init calls ContentRendererClient::RenderThreadStarted.
> 4. The RenderThreadImpl constructor completes allowing the set_main_thread
call
> to proceed.
> 
> As a result, calling ChildThread::current() from inside RenderThreadStarted
> returns NULL because the RenderThreadImpl object is still being constructed
and
> has not yet been set as the main thread.

so maybe the answer is that set_main_thread() should be called inside
ChildThread's constructor?

[Marshall, 2012-01-27 20:44:27]

On 2012/01/27 20:20:10, John Abd-El-Malek wrote:
> On 2012/01/27 19:34:41, Marshall wrote:
> > On 2012/01/27 19:22:33, John Abd-El-Malek wrote:
> > > This doesn't make sense to me, can you dig into this deeper? Are you
> > > calling ChildThread::current() from the renderer thread?
> > > RenderThread::current() is called from all over the place, so it must
> > > return a valid thread even in single process case.
> > 
> > 1. RendererMainThread::Init contains the following code:
> > 
> > render_process_->set_main_thread(new RenderThreadImpl(channel_id_));
> > 
> > 2. The RenderThreadImpl constructor calls RenderThreadImpl::Init.
> > 3. RenderThreadImpl::Init calls ContentRendererClient::RenderThreadStarted.
> > 4. The RenderThreadImpl constructor completes allowing the set_main_thread
> call
> > to proceed.
> > 
> > As a result, calling ChildThread::current() from inside RenderThreadStarted
> > returns NULL because the RenderThreadImpl object is still being constructed
> and
> > has not yet been set as the main thread.
> 
> so maybe the answer is that set_main_thread() should be called inside
> ChildThread's constructor?

Generally speaking, it's probably bad to do so much work in the constructor. If
we called Init() separately we could do something like:

RenderThreadImpl* thread = new RenderThreadImpl(channel_id_)
render_process_->set_main_thread(thread);
thread->Init();

Not calling Init() from the constructor may require a lot of changes elsewhere
in the code, though.

[jam, 2012-01-27 21:12:37]

On 2012/01/27 20:44:27, Marshall wrote:
> On 2012/01/27 20:20:10, John Abd-El-Malek wrote:
> > On 2012/01/27 19:34:41, Marshall wrote:
> > > On 2012/01/27 19:22:33, John Abd-El-Malek wrote:
> > > > This doesn't make sense to me, can you dig into this deeper? Are you
> > > > calling ChildThread::current() from the renderer thread?
> > > > RenderThread::current() is called from all over the place, so it must
> > > > return a valid thread even in single process case.
> > > 
> > > 1. RendererMainThread::Init contains the following code:
> > > 
> > > render_process_->set_main_thread(new RenderThreadImpl(channel_id_));
> > > 
> > > 2. The RenderThreadImpl constructor calls RenderThreadImpl::Init.
> > > 3. RenderThreadImpl::Init calls
ContentRendererClient::RenderThreadStarted.
> > > 4. The RenderThreadImpl constructor completes allowing the set_main_thread
> > call
> > > to proceed.
> > > 
> > > As a result, calling ChildThread::current() from inside
RenderThreadStarted
> > > returns NULL because the RenderThreadImpl object is still being
constructed
> > and
> > > has not yet been set as the main thread.
> > 
> > so maybe the answer is that set_main_thread() should be called inside
> > ChildThread's constructor?
> 
> Generally speaking, it's probably bad to do so much work in the constructor.
If
> we called Init() separately we could do something like:
> 
> RenderThreadImpl* thread = new RenderThreadImpl(channel_id_)
> render_process_->set_main_thread(thread);
> thread->Init();
> 
> Not calling Init() from the constructor may require a lot of changes elsewhere
> in the code, though.

that's why i was suggesting calling set_main_thread in the constructor. it would
be a smaller change with less side effects

[Marshall, 2012-01-30 15:28:42]

On 2012/01/27 21:12:37, John Abd-El-Malek wrote:
> On 2012/01/27 20:44:27, Marshall wrote:
> > On 2012/01/27 20:20:10, John Abd-El-Malek wrote:
> > > On 2012/01/27 19:34:41, Marshall wrote:
> > > > On 2012/01/27 19:22:33, John Abd-El-Malek wrote:
> > > > > This doesn't make sense to me, can you dig into this deeper? Are you
> > > > > calling ChildThread::current() from the renderer thread?
> > > > > RenderThread::current() is called from all over the place, so it must
> > > > > return a valid thread even in single process case.
> > > > 
> > > > 1. RendererMainThread::Init contains the following code:
> > > > 
> > > > render_process_->set_main_thread(new RenderThreadImpl(channel_id_));
> > > > 
> > > > 2. The RenderThreadImpl constructor calls RenderThreadImpl::Init.
> > > > 3. RenderThreadImpl::Init calls
> ContentRendererClient::RenderThreadStarted.
> > > > 4. The RenderThreadImpl constructor completes allowing the
set_main_thread
> > > call
> > > > to proceed.
> > > > 
> > > > As a result, calling ChildThread::current() from inside
> RenderThreadStarted
> > > > returns NULL because the RenderThreadImpl object is still being
> constructed
> > > and
> > > > has not yet been set as the main thread.
> > > 
> > > so maybe the answer is that set_main_thread() should be called inside
> > > ChildThread's constructor?
> > 
> > Generally speaking, it's probably bad to do so much work in the constructor.
> If
> > we called Init() separately we could do something like:
> > 
> > RenderThreadImpl* thread = new RenderThreadImpl(channel_id_)
> > render_process_->set_main_thread(thread);
> > thread->Init();
> > 
> > Not calling Init() from the constructor may require a lot of changes
elsewhere
> > in the code, though.
> 
> that's why i was suggesting calling set_main_thread in the constructor. it
would
> be a smaller change with less side effects

OK, I'm with you now :-) I've updated the patch set to call set_main_thread from
the RenderThreadImpl::Init() method. Please review.

[jam, 2012-01-30 19:46:12]

http://codereview.chromium.org/9297022/diff/8001/content/renderer/render_thre...
File content/renderer/render_thread_impl.cc (right):

http://codereview.chromium.org/9297022/diff/8001/content/renderer/render_thre...
content/renderer/render_thread_impl.cc:190:
ChildProcess::current()->set_main_thread(this);
nit: can we do this in ChildThread's constructor? we try to make all the
ChildProcess/ChildThread code take the same paths for simplicity

[Marshall, 2012-02-01 15:44:44]

On 2012/01/30 19:46:12, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/9297022/diff/8001/content/renderer/render_thre...
> File content/renderer/render_thread_impl.cc (right):
> 
>
http://codereview.chromium.org/9297022/diff/8001/content/renderer/render_thre...
> content/renderer/render_thread_impl.cc:190:
> ChildProcess::current()->set_main_thread(this);
> nit: can we do this in ChildThread's constructor? we try to make all the
> ChildProcess/ChildThread code take the same paths for simplicity

I don't think we can do this currently. See the following comment in
gpu_process_host.cc near line 136:

// TODO: Currently, ChildProcess supports only a single static instance,
// which is a problem in --single-process mode, where both gpu and renderer
// should be able to create separate instances.

[jam, 2012-02-01 17:09:56]

On 2012/02/01 15:44:44, Marshall wrote:
> On 2012/01/30 19:46:12, John Abd-El-Malek wrote:
> >
>
http://codereview.chromium.org/9297022/diff/8001/content/renderer/render_thre...
> > File content/renderer/render_thread_impl.cc (right):
> > 
> >
>
http://codereview.chromium.org/9297022/diff/8001/content/renderer/render_thre...
> > content/renderer/render_thread_impl.cc:190:
> > ChildProcess::current()->set_main_thread(this);
> > nit: can we do this in ChildThread's constructor? we try to make all the
> > ChildProcess/ChildThread code take the same paths for simplicity
> 
> I don't think we can do this currently. See the following comment in
> gpu_process_host.cc near line 136:

ah yes, lgtm then

nit: the comment that you have duplicated everywhere is unnecessary. if someone
is looking at this code and is trying to figure out where set_main_thread is
called, they can search.
> 
> // TODO: Currently, ChildProcess supports only a single static instance,
> // which is a problem in --single-process mode, where both gpu and renderer
> // should be able to create separate instances.

[Marshall, 2012-02-01 19:45:41]

On 2012/02/01 17:09:56, John Abd-El-Malek wrote:
> On 2012/02/01 15:44:44, Marshall wrote:
> > On 2012/01/30 19:46:12, John Abd-El-Malek wrote:
> > >
> >
>
http://codereview.chromium.org/9297022/diff/8001/content/renderer/render_thre...
> > > File content/renderer/render_thread_impl.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/9297022/diff/8001/content/renderer/render_thre...
> > > content/renderer/render_thread_impl.cc:190:
> > > ChildProcess::current()->set_main_thread(this);
> > > nit: can we do this in ChildThread's constructor? we try to make all the
> > > ChildProcess/ChildThread code take the same paths for simplicity
> > 
> > I don't think we can do this currently. See the following comment in
> > gpu_process_host.cc near line 136:
> 
> ah yes, lgtm then
> 
> nit: the comment that you have duplicated everywhere is unnecessary. if
someone
> is looking at this code and is trying to figure out where set_main_thread is
> called, they can search.

Comments removed.

> > 
> > // TODO: Currently, ChildProcess supports only a single static instance,
> > // which is a problem in --single-process mode, where both gpu and renderer
> > // should be able to create separate instances.

[commit-bot: I haz the power, 2012-02-01 19:49:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marshall@chromium.org/9297022/21003

[commit-bot: I haz the power, 2012-02-01 22:26:18]

Change committed as 120112