Make XHR work in Workers. Creates a 'shadow page' in a worker process to prox...

Make XHR work in Workers. Creates a 'shadow page' in a worker process to proxy the loading requests through.
BUG=4361
TEST=none

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

[Dmitry Titov, 2009-06-12 20:22:28]

This is along the lines of creating 'fake frame' (I called it 'shadow page'
because 'fake' doesn't sound nice :-)

I've added IsWorkerProcess() and also moved ChildThread::current() to 'public'
since it is used now explicitly to get to resource_dispatcher().

I can't make it work in test_shell since it would need quite a lot of additional
changes, either it has to be a separate change or we just need to test xhr in
workers somehow else (ui_test), I'd like to think about it and perhaps do a
separate CL, so no test for this one.

[Dmitry Titov, 2009-06-12 21:46:06]

Ah, this seems to be incomplete. I'll soon upload another patch with couple of
fixes...

[Dmitry Titov, 2009-06-12 23:04:45]

Now it's better. Instead of adding IsWorkerProcess similar to IsPluginProcess
(this made some code from renderer to include files from worker and other weird
stuff) I've added 3 virtuals to ChildThread (IsRenderThread() etc). this allows
dynamic checks and keeps implementation simple.

[Dmitry Titov, 2009-06-12 23:05:15]

ready for review.

[jam, 2009-06-12 23:27:41]

http://codereview.chromium.org/126070/diff/1/4
File chrome/common/child_thread.h (right):

http://codereview.chromium.org/126070/diff/1/4#newcode38
Line 38: static ChildThread* current();
I prefer keeping this the way it used to be.  I had specifically kept
ChildThread::current private to force people to call RenderThread/PluginThread.

For the one use that you need, can you just call
ChildProces::current()->child_thread() instead?

http://codereview.chromium.org/126070/diff/1/9
File chrome/renderer/DEPS (right):

http://codereview.chromium.org/126070/diff/1/9#newcode5
Line 5: "+chrome/worker", # to pull IsWorkerProcess()
We really try to avoid having any of these directories depend on each other (the
current ones are bad, we shouldn't add more).

I think this patch can be done without having to add this, I'll commet in the
places where it's used.

http://codereview.chromium.org/126070/diff/1014/16
File chrome/common/child_thread.h (right):

http://codereview.chromium.org/126070/diff/1014/16#newcode41
Line 41: virtual bool IsWorkerThread() { return false; }
I've tried to specifically avoid this situation where ChildProcess/ChildThread
needs to have some function for all the different types (i.e. there's already
UtilityProcess).

What we had before, but lost in a bunch of rewritings, is that
RenderThread::current() will use TLS so that it only returns something if it's
called on the correct thread in the correct process (i.e. currently it can be
called on the wrong thread, which is a bug since it's not thread safe).  The
benefit is that to check to see which process you're in, you can call
RenderThread::current() to see if it's NULL or not, and it would still work in
--single-process-mode, which using the CommandLine to look for the process type
would fail in.

http://codereview.chromium.org/126070/diff/1014/14
File webkit/glue/webworker_impl.cc (right):

http://codereview.chromium.org/126070/diff/1014/14#newcode49
Line 49: virtual void Release() { }
just for brevity, can you remove all the blank lines?

http://codereview.chromium.org/126070/diff/1014/14#newcode110
Line 110: static WorkerWebViewDelegate worker_delegate;
Why not just have this as a member of WebWorkerImpl?

http://codereview.chromium.org/126070/diff/1014/14#newcode168
Line 168: ASSERT(!web_view_);
we use DCHECKs in the Chrome code, not ASSERT.

[Dmitry Titov, 2009-06-13 01:29:52]

Updated patch uploaded.

http://codereview.chromium.org/126070/diff/1/4
File chrome/common/child_thread.h (right):

http://codereview.chromium.org/126070/diff/1/4#newcode38
Line 38: static ChildThread* current();
On 2009/06/12 23:27:41, John Abd-El-Malek wrote:
> I prefer keeping this the way it used to be.  I had specifically kept
> ChildThread::current private to force people to call
RenderThread/PluginThread.
> 
> For the one use that you need, can you just call
> ChildProces::current()->child_thread() instead?

I'm not seeing what is a benefit of keeping this private. The ChildThread has a
set of public methods that are implemented on it, not on derived classes. So if
one needs to call the method of the ChildThread, I'm not sure how only exposing
access to RenderThread or PluginThread helps? There is code that is reused in
more then one type of process, it should be able to access common functionality,
otherwise it will need to attempt to cast to various derived types until it
'finds the right one'. Using ChildProcess::current()->child_thread() looks
inconsistent at least.

http://codereview.chromium.org/126070/diff/1/9
File chrome/renderer/DEPS (right):

http://codereview.chromium.org/126070/diff/1/9#newcode5
Line 5: "+chrome/worker", # to pull IsWorkerProcess()
On 2009/06/12 23:27:41, John Abd-El-Malek wrote:
> We really try to avoid having any of these directories depend on each other
(the
> current ones are bad, we shouldn't add more).
> 
> I think this patch can be done without having to add this, I'll commet in the
> places where it's used.

This file is no longer a part of the change, since I don't have IsWorkerProcess
anymore.

http://codereview.chromium.org/126070/diff/1014/16
File chrome/common/child_thread.h (right):

http://codereview.chromium.org/126070/diff/1014/16#newcode41
Line 41: virtual bool IsWorkerThread() { return false; }
On 2009/06/12 23:27:41, John Abd-El-Malek wrote:
> I've tried to specifically avoid this situation where ChildProcess/ChildThread
> needs to have some function for all the different types (i.e. there's already
> UtilityProcess).
> 
> What we had before, but lost in a bunch of rewritings, is that
> RenderThread::current() will use TLS so that it only returns something if it's
> called on the correct thread in the correct process (i.e. currently it can be
> called on the wrong thread, which is a bug since it's not thread safe).  The
> benefit is that to check to see which process you're in, you can call
> RenderThread::current() to see if it's NULL or not, and it would still work in
> --single-process-mode, which using the CommandLine to look for the process
type
> would fail in.

I like the idea that specific accessors would return NULL if the call is made on
a wrong thread or the ChildThread subclass is present but of wrong type. In fact
some code already depends on it (unit tests) so I went ahead and changed the
accessors to return NULL if the ChildThread is of wrong type.

So there are actually 2 checks: that the thread object is of proper type *and*
that the call to 'current()' is made on an actual thread that was initialized
with that object.

So lets say we don't want to create several TLS slots (one per type, it'd be a
waste) and we have just one. We can put a pointer to base type into this slot.
So when we get it back, and we need to cast it - how do we know this is actually
an instance of the required type? Don't we need some virtual check like this?

I feel having TLS or, even better - not having a TLS but instead a thread id
stored in the ChildThread instance so one could compare with current thread id
and return NULL if mismatch, is a good idea in this context. It is perhaps a
change deserving a separate CL.. Would TODO suffice for now?

http://codereview.chromium.org/126070/diff/1014/14
File webkit/glue/webworker_impl.cc (right):

http://codereview.chromium.org/126070/diff/1014/14#newcode49
Line 49: virtual void Release() { }
On 2009/06/12 23:27:41, John Abd-El-Malek wrote:
> just for brevity, can you remove all the blank lines?

Done.

http://codereview.chromium.org/126070/diff/1014/14#newcode110
Line 110: static WorkerWebViewDelegate worker_delegate;
On 2009/06/12 23:27:41, John Abd-El-Malek wrote:
> Why not just have this as a member of WebWorkerImpl?

As a static member? Done.

http://codereview.chromium.org/126070/diff/1014/14#newcode168
Line 168: ASSERT(!web_view_);
On 2009/06/12 23:27:41, John Abd-El-Malek wrote:
> we use DCHECKs in the Chrome code, not ASSERT.

Done.

[jam, 2009-06-13 02:15:06]

http://codereview.chromium.org/126070/diff/1/4
File chrome/common/child_thread.h (right):

http://codereview.chromium.org/126070/diff/1/4#newcode38
Line 38: static ChildThread* current();
On 2009/06/13 01:29:54, Dmitry Titov wrote:
> On 2009/06/12 23:27:41, John Abd-El-Malek wrote:
> > I prefer keeping this the way it used to be.  I had specifically kept
> > ChildThread::current private to force people to call
> RenderThread/PluginThread.
> > 
> > For the one use that you need, can you just call
> > ChildProces::current()->child_thread() instead?
> 
> I'm not seeing what is a benefit of keeping this private. The ChildThread has
a
> set of public methods that are implemented on it, not on derived classes. So
if
> one needs to call the method of the ChildThread, I'm not sure how only
exposing
> access to RenderThread or PluginThread helps? There is code that is reused in
> more then one type of process, it should be able to access common
functionality,
> otherwise it will need to attempt to cast to various derived types until it
> 'finds the right one'. Using ChildProcess::current()->child_thread() looks
> inconsistent at least. 

The reason is that ChildThread shouldn't be called directly by renderer/plugin
code.  They should know which process they're in, and as a result call
RenderThread/PluginThread etc directly.  That was one of the design decisions
when that code was written..  But I see your point in that giving access using
ChildProcess and not through ChildThread doesn't improve the situation.  so I'm
fine with this.

http://codereview.chromium.org/126070/diff/1014/16#newcode41
Line 41: friend class ChildProcess;
On 2009/06/13 01:29:54, Dmitry Titov wrote:
> On 2009/06/12 23:27:41, John Abd-El-Malek wrote:
> > I've tried to specifically avoid this situation where
ChildProcess/ChildThread
> > needs to have some function for all the different types (i.e. there's
already
> > UtilityProcess).
> > 
> > What we had before, but lost in a bunch of rewritings, is that
> > RenderThread::current() will use TLS so that it only returns something if
it's
> > called on the correct thread in the correct process (i.e. currently it can
be
> > called on the wrong thread, which is a bug since it's not thread safe).  The
> > benefit is that to check to see which process you're in, you can call
> > RenderThread::current() to see if it's NULL or not, and it would still work
in
> > --single-process-mode, which using the CommandLine to look for the process
> type
> > would fail in.
> 
> I like the idea that specific accessors would return NULL if the call is made
on
> a wrong thread or the ChildThread subclass is present but of wrong type. In
fact
> some code already depends on it (unit tests) so I went ahead and changed the
> accessors to return NULL if the ChildThread is of wrong type.
> 
> So there are actually 2 checks: that the thread object is of proper type *and*
> that the call to 'current()' is made on an actual thread that was initialized
> with that object.
> 
> So lets say we don't want to create several TLS slots (one per type, it'd be a
> waste) and we have just one.

We have a way of doing this without wasting TLS slots, see LazyInstance.

> We can put a pointer to base type into this slot.
> So when we get it back, and we need to cast it - how do we know this is
actually
> an instance of the required type? Don't we need some virtual check like this?
> 
> I feel having TLS or, even better - not having a TLS but instead a thread id
> stored in the ChildThread instance so one could compare with current thread id
> and return NULL if mismatch, is a good idea in this context. It is perhaps a
> change deserving a separate CL.. Would TODO suffice for now?
> 

Your change motivated me to add TLS slots back to the code.  I think it'll make
things simpler, and it also avoids bugs.  I figured I'd try it out to see if'll
expose bugs in which case it'll slow you down since they're unrelated, and
during that course I found a bug in the worker code!  I just sent you the
change.

http://codereview.chromium.org/126070/diff/1014/14
File webkit/glue/webworker_impl.cc (right):

http://codereview.chromium.org/126070/diff/1014/14#newcode110
Line 110: static WorkerWebViewDelegate worker_delegate;
On 2009/06/13 01:29:54, Dmitry Titov wrote:
> On 2009/06/12 23:27:41, John Abd-El-Malek wrote:
> > Why not just have this as a member of WebWorkerImpl?
> 
> As a static member? Done.

I meant why does this need to be a static member even?  Why can't it be a member
variable of WebWorkerImpl?

[Dmitry Titov, 2009-06-13 06:13:19]

On 2009/06/13 02:15:06, John Abd-El-Malek wrote:
> http://codereview.chromium.org/126070/diff/1/4
> File chrome/common/child_thread.h (right):
> 
> http://codereview.chromium.org/126070/diff/1/4#newcode38
> Line 38: static ChildThread* current();
> On 2009/06/13 01:29:54, Dmitry Titov wrote:
> > On 2009/06/12 23:27:41, John Abd-El-Malek wrote:
> > > I prefer keeping this the way it used to be.  I had specifically kept
> > > ChildThread::current private to force people to call
> > RenderThread/PluginThread.
> > > 
> > > For the one use that you need, can you just call
> > > ChildProces::current()->child_thread() instead?
> > 
> > I'm not seeing what is a benefit of keeping this private. The ChildThread
has
> a
> > set of public methods that are implemented on it, not on derived classes. So
> if
> > one needs to call the method of the ChildThread, I'm not sure how only
> exposing
> > access to RenderThread or PluginThread helps? There is code that is reused
in
> > more then one type of process, it should be able to access common
> functionality,
> > otherwise it will need to attempt to cast to various derived types until it
> > 'finds the right one'. Using ChildProcess::current()->child_thread() looks
> > inconsistent at least. 
> 
> The reason is that ChildThread shouldn't be called directly by renderer/plugin
> code.  They should know which process they're in, and as a result call
> RenderThread/PluginThread etc directly.  That was one of the design decisions
> when that code was written..  But I see your point in that giving access using
> ChildProcess and not through ChildThread doesn't improve the situation.  so
I'm
> fine with this.
> 
> http://codereview.chromium.org/126070/diff/1014/16#newcode41
> Line 41: friend class ChildProcess;
> On 2009/06/13 01:29:54, Dmitry Titov wrote:
> > On 2009/06/12 23:27:41, John Abd-El-Malek wrote:
> > > I've tried to specifically avoid this situation where
> ChildProcess/ChildThread
> > > needs to have some function for all the different types (i.e. there's
> already
> > > UtilityProcess).
> > > 
> > > What we had before, but lost in a bunch of rewritings, is that
> > > RenderThread::current() will use TLS so that it only returns something if
> it's
> > > called on the correct thread in the correct process (i.e. currently it can
> be
> > > called on the wrong thread, which is a bug since it's not thread safe). 
The
> > > benefit is that to check to see which process you're in, you can call
> > > RenderThread::current() to see if it's NULL or not, and it would still
work
> in
> > > --single-process-mode, which using the CommandLine to look for the process
> > type
> > > would fail in.
> > 
> > I like the idea that specific accessors would return NULL if the call is
made
> on
> > a wrong thread or the ChildThread subclass is present but of wrong type. In
> fact
> > some code already depends on it (unit tests) so I went ahead and changed the
> > accessors to return NULL if the ChildThread is of wrong type.
> > 
> > So there are actually 2 checks: that the thread object is of proper type
*and*
> > that the call to 'current()' is made on an actual thread that was
initialized
> > with that object.
> > 
> > So lets say we don't want to create several TLS slots (one per type, it'd be
a
> > waste) and we have just one.
> 
> We have a way of doing this without wasting TLS slots, see LazyInstance.
> 
> > We can put a pointer to base type into this slot.
> > So when we get it back, and we need to cast it - how do we know this is
> actually
> > an instance of the required type? Don't we need some virtual check like
this?
> > 
> > I feel having TLS or, even better - not having a TLS but instead a thread id
> > stored in the ChildThread instance so one could compare with current thread
id
> > and return NULL if mismatch, is a good idea in this context. It is perhaps a
> > change deserving a separate CL.. Would TODO suffice for now?
> > 
> 
> Your change motivated me to add TLS slots back to the code.  I think it'll
make
> things simpler, and it also avoids bugs.  I figured I'd try it out to see
if'll
> expose bugs in which case it'll slow you down since they're unrelated, and
> during that course I found a bug in the worker code!  I just sent you the
> change.
> 
> http://codereview.chromium.org/126070/diff/1014/14
> File webkit/glue/webworker_impl.cc (right):
> 
> http://codereview.chromium.org/126070/diff/1014/14#newcode110
> Line 110: static WorkerWebViewDelegate worker_delegate;
> On 2009/06/13 01:29:54, Dmitry Titov wrote:
> > On 2009/06/12 23:27:41, John Abd-El-Malek wrote:
> > > Why not just have this as a member of WebWorkerImpl?
> > 
> > As a static member? Done.
> 
> I meant why does this need to be a static member even?  Why can't it be a
member
> variable of WebWorkerImpl?

Cool, I'll update patch over weekend!

[michaeln, 2009-06-15 19:14:18]

http://codereview.chromium.org/126070/diff/1/2
File webkit/glue/webworker_impl.cc (right):

http://codereview.chromium.org/126070/diff/1/2#newcode186
Line 186: loading_document_ = web_frame->frame()->document();
Nice!

Have you considered introducing this in webcore directly instead of in chromium
specific code? The less chromium specific bits the better.

I think we'll want something like this for 'shared' workers in webcore and
chromium alike. The same techinique works equally well for dedicated workers in
webcore and chromium alike.

[Dmitry Titov, 2009-06-15 19:23:40]

On 2009/06/13 02:15:06, John Abd-El-Malek wrote:
> > > Why not just have this as a member of WebWorkerImpl?
> > 
> > As a static member? Done.
> 
> I meant why does this need to be a static member even?  Why can't it be a
member variable of WebWorkerImpl?

It's a static class, no members. I only need 1 instance per process. If I wanted
to allocate one, I'd need to keep it in some RefPtr structure since Delegate has
AddRef/Release that would need to be implmented then too... Doesn't it look
complex?

[Dmitry Titov, 2009-06-15 19:26:59]

On 2009/06/15 19:14:18, michaeln wrote:
> http://codereview.chromium.org/126070/diff/1/2
> File webkit/glue/webworker_impl.cc (right):
> 
> http://codereview.chromium.org/126070/diff/1/2#newcode186
> Line 186: loading_document_ = web_frame->frame()->document();
> Nice!
> 
> Have you considered introducing this in webcore directly instead of in
chromium
> specific code? The less chromium specific bits the better.
> 
> I think we'll want something like this for 'shared' workers in webcore and
> chromium alike. The same techinique works equally well for dedicated workers
in
> webcore and chromium alike.

That's a good idea. I think I'll chat with Drew about it, see how it can be
applied to shared workers...

[Dmitry Titov, 2009-06-15 20:13:34]

John, how's the last patch looking? Uploaded and some comments above.

[jam, 2009-06-15 21:47:10]

On 2009/06/15 19:23:40, Dmitry Titov wrote:
> On 2009/06/13 02:15:06, John Abd-El-Malek wrote:
> > > > Why not just have this as a member of WebWorkerImpl?
> > > 
> > > As a static member? Done.
> > 
> > I meant why does this need to be a static member even?  Why can't it be a
> member variable of WebWorkerImpl?
> 
> It's a static class, no members. I only need 1 instance per process. If I
wanted
> to allocate one, I'd need to keep it in some RefPtr structure since Delegate
has
> AddRef/Release that would need to be implmented then too... Doesn't it look
> complex?

We go to great lengths to avoid static objects in the codebase.  This seems like
an object which doesn't need to be one.  You can either change the global object
to be a static pointer, or just have one per WebWorkerImpl (I don't see a big
harm in this, are you worried about expanding the memory usage by < 100 bytes?).

[jam, 2009-06-15 21:50:41]

btw, when I said static objects, I meant static objects with constructors.  You
can search for the chrome-team thread with title "Static initializers" for more
info.

[jam, 2009-06-15 23:11:37]

lgtm