Oilpan: move core/workers to oilpan's heap.

Oilpan: move core/workers to oilpan's heap.

This moves most of the web worker related objects to Oilpan using
transition types, including MessagePort and MessageChannel.

R=
BUG=340522

[sof, 2014-02-24 12:22:23]

When you get a chance, please take a look.

Note:
 - serviceworker/ and websockets/ code had to be touched to carry through the
changes here. We've been asked to hold off with changing these two modules, but
the (type) changes are incidental, I'd argue.
 - a very simple WorkerLocation.cpp file is added here. I used the copyright
noticed of WorkerLocation.h on it. I don't know what's right/wrong practice
here.
 - with presubmit currently being run (and failing) for trybot runs, I'm
currently unable to fully test wrt potential regressions.

[sof, 2014-02-24 12:53:29]

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Worker.cpp
File Source/core/workers/Worker.cpp (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Worker.c...
Source/core/workers/Worker.cpp:106: // NOTE: oilpan: if the Worker fails to
fully construct (e.g., invalid URL),
I'll remove this comment before finalizing; just wanted to give context to a
seemingly random change.

[Mads Ager (chromium), 2014-02-24 13:27:53]

https://codereview.chromium.org/177073004/diff/1/Source/bindings/v8/WorkerScr...
File Source/bindings/v8/WorkerScriptController.cpp (right):

https://codereview.chromium.org/177073004/diff/1/Source/bindings/v8/WorkerScr...
Source/bindings/v8/WorkerScriptController.cpp:164:
V8DOMWrapper::associateObjectWithWrapper<V8WorkerGlobalScope>(PassRefPtrWillBeRawPtr<WorkerGlobalScope>(&m_workerGlobalScope),
contextType, jsWorkerGlobalScope, isolate(), WrapperConfiguration::Dependent);
It seems like a lot of stuff has moved to references instead of pointers in the
code base. If PassRefPtr has a constructor from "const U&" we probably should
have that for RawPtr as well so we don't have to explicitly write the & here.

The lifetime is a little bit nasty here. When there is a reference to an object
somewhere (like the reference to a WorkerGlobalScope from
WorkerScriptController) there is the assumption that it is kept alive by
something else. I would be more comfortable if we always traced these pointers
to guarantee that there are no use-after-free bugs hiding. However, that would
get us back where it is not clear whether or not something can be NULL. Another
option would be to teach our visitor more about references and always trace
references if the are referencing something that is garbage collected.
Currently, if you trace a reference that does not mark the object itself, it
only calls the trace method.

This is fine for now, but we need to consider how to best handle these
references.

https://codereview.chromium.org/177073004/diff/1/Source/core/dom/MessagePort.h
File Source/core/dom/MessagePort.h (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/dom/MessagePort....
Source/core/dom/MessagePort.h:117: OwnPtr<blink::WebMessagePortChannel>
m_entangledChannel;
This doesn't actually point back to a WebCore MessageChannel, right?

https://codereview.chromium.org/177073004/diff/1/Source/core/dom/MessagePort....
Source/core/dom/MessagePort.h:122: WeakPtrFactory<MessagePort> m_weakFactory;
Down the line we should use real weak pointers provided by the oilpan system for
this. I think this will work for now, but we really should get rid of it and
just hand out raw pointers that are put in WeakMembers. That requires the
holders of those pointers to be in the heap as well so we probably cannot do it
yet. These places are luckily easy to find when they have been explicitly
implemented with the WeakPtrFactory, so that is really nice.

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Abstract...
File Source/core/workers/AbstractWorker.h (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Abstract...
Source/core/workers/AbstractWorker.h:63: void trace(Visitor*) { }
Please make this one virtual and implement it in all subclasses as well.

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Dedicate...
File Source/core/workers/DedicatedWorkerGlobalScope.h (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Dedicate...
Source/core/workers/DedicatedWorkerGlobalScope.h:44: class
DedicatedWorkerGlobalScope FINAL : public WorkerGlobalScope {
Please implement the virtual trace method for this and explicitly dispatch to
the parent:

virtual void trace(Visitor* visitor) { WorkerGlobalScope::trace(visitor); }

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Dedicate...
File Source/core/workers/DedicatedWorkerThread.h (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Dedicate...
Source/core/workers/DedicatedWorkerThread.h:42: class DedicatedWorkerThread
FINAL : public WorkerThread {
Please implement the virtual trace method for this and explicitly dispatch to
WorkerThread::trace. That should help us avoid bugs where the trace chain is
incorrectly set up.

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Worker.h
File Source/core/workers/Worker.h (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Worker.h...
Source/core/workers/Worker.h:65: void trace(Visitor*);
This will not work if you have a pointer with type AbstractWorker.

Make the trace method virtual and make sure that it dispatches explicitly to the
super class:

virtual void trace(Visitor* visitor)
{
    AbstractWorker::trace(visitor);
    ...
}

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/WorkerGl...
File Source/core/workers/WorkerGlobalScope.h (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/WorkerGl...
Source/core/workers/WorkerGlobalScope.h:127: void trace(Visitor*);
Please make the trace method virtual and implement it in the subclasses so that
we get full tracing dependent on the actual class.

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/WorkerTh...
File Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/WorkerTh...
Source/core/workers/WorkerThread.h:78: void trace(Visitor*);
Please make this virtual and make sure to implement it in subclasses with
explicit dispatching to the superclass trace method.

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/WorkerTh...
File Source/core/workers/WorkerThreadStartupData.h (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/WorkerTh...
Source/core/workers/WorkerThreadStartupData.h:46: class WorkerThreadStartupData
: NoBaseWillBeGarbageCollected<WorkerThreadStartupData> {
This needs to be GarbageCollectedFinalized. String and OwnPtr members needs the
destructor to be called.

[haraken, 2014-02-24 13:28:36]

not LGTM.

I reviewed half of the CL but the CL is too big to verify correctness. It would
be great if you could split the CL into pieces.

https://codereview.chromium.org/177073004/diff/1/Source/bindings/tests/result...
File Source/bindings/tests/results/V8TestObject.cpp (right):

https://codereview.chromium.org/177073004/diff/1/Source/bindings/tests/result...
Source/bindings/tests/results/V8TestObject.cpp:2236:
V8TRYCATCH_VOID(Vector<RefPtrWillBeRawPtr<MessagePort> >, cppValue,
(toRefPtrNativeArray<MessagePort, V8MessagePort>(jsValue, 0,
info.GetIsolate())));

Why do we observe a change to bindings/tests/results/, even though you are not
making a change to code_generator_v8.pm ?

https://codereview.chromium.org/177073004/diff/1/Source/core/dom/MessagePort.h
File Source/core/dom/MessagePort.h (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/dom/MessagePort....
Source/core/dom/MessagePort.h:122: WeakPtrFactory<MessagePort> m_weakFactory;

Shouldn't this become a WeakMember<MessagePort> ?

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Abstract...
File Source/core/workers/AbstractWorker.h (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Abstract...
Source/core/workers/AbstractWorker.h:63: void trace(Visitor*) { }

This should be: virtual void trace(Visitor*) = 0.

Also, please override trace methods() in the subclasses of AbstractWorker in
this CL.

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Dedicate...
File Source/core/workers/DedicatedWorkerGlobalScope.idl (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Dedicate...
Source/core/workers/DedicatedWorkerGlobalScope.idl:33: WillBeGarbageCollected

You don't need to write [WillBeGarbageCollected] in a subclass.
[WillBeGarbageCollected] is inherited.

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/SharedWo...
File Source/core/workers/SharedWorkerGlobalScope.idl (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/SharedWo...
Source/core/workers/SharedWorkerGlobalScope.idl:33: WillBeGarbageCollected

You don't need to write [WillBeGarbageCollected] in a subclass. It's inherited.

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/SharedWo...
File Source/core/workers/SharedWorkerThread.h (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/SharedWo...
Source/core/workers/SharedWorkerThread.h:40: class SharedWorkerThread : public
WorkerThread {

Let's override trace() methods in all subclasses of a garbage-collected class.
The same comment for other places in this CL.

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Worker.cpp
File Source/core/workers/Worker.cpp (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Worker.c...
Source/core/workers/Worker.cpp:111:
m_contextProxy->terminateWorkerGlobalScope();

Don't you need to clear m_contextProxy in terminate()? Otherwise, m_contextProxy
will become a stale pointer when Worker outlives the context.

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Worker.h
File Source/core/workers/Worker.h (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Worker.h...
Source/core/workers/Worker.h:75: WorkerGlobalScopeProxy* m_contextProxy; // The
proxy outlives the worker to perform thread shutdown.

It's not obvious to me that this raw pointer is safe. In particular, it looks
dangerous that Worker's destructor is touching the raw pointer.

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/WorkerCo...
File Source/core/workers/WorkerConsole.h (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/WorkerCo...
Source/core/workers/WorkerConsole.h:50: using
RefCountedWillBeRefCountedGarbageCollected<WorkerConsole>::ref;

Probably can you use:

#if ENABLE(OILPAN)
  using RefCounted<WorkerConsole>::ref;
  using RefCounted<WorkerConsole>::deref;
#endif

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/WorkerCo...
Source/core/workers/WorkerConsole.h:71: WorkerGlobalScope* m_scope;

You need to change this to a Member.

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/WorkerGl...
File Source/core/workers/WorkerGlobalScope.h (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/WorkerGl...
Source/core/workers/WorkerGlobalScope.h:157: WorkerThread* m_thread;

You need to change this to a Member.

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/WorkerGl...
Source/core/workers/WorkerGlobalScope.h:162: HashSet<Observer*>
m_workerObservers;

Are these raw pointers safe?

[sof, 2014-02-24 13:36:26]

On 2014/02/24 13:28:36, haraken wrote:
> not LGTM.
> 
> I reviewed half of the CL but the CL is too big to verify correctness. It
would
> be great if you could split the CL into pieces.
> 

Let me think about how. The CL is arguably fairly focused, but once you change a
type, dependent code will have to be updated.

[haraken, 2014-02-24 13:59:47]

On 2014/02/24 13:36:26, sof wrote:
> On 2014/02/24 13:28:36, haraken wrote:
> > not LGTM.
> > 
> > I reviewed half of the CL but the CL is too big to verify correctness. It
> would
> > be great if you could split the CL into pieces.
> > 
> 
> Let me think about how. The CL is arguably fairly focused, but once you change
a
> type, dependent code will have to be updated.

Yeah, probably I've been saying something inconsistent. I'm sorry about that.

- At first, I was thinking that as we move things more incrementally, we will
end up producing more leaks in the transition period. Thus I was thinking that
we want to move things in one go as much as possible.

- However, in the process of actual reviewing, I noticed the following facts:

--- If we move too many things in one go, it's hard to verify correctness. 
--- Regarding memory leaks, we just need to be careful when changing a raw
pointer to a Member. Changing a RefPtr to a Persistent won't produce leaks.
--- Even if we produce leaks, the leaks will be just temporary ones in the
transition period. The leaks will be fixed as a natural consequence of moving
the entire hierarchy to the heap. In other words, the leaks will be fixed by the
time we eliminate the oilpan flag for the hierarchy. Thus producing leaks behind
the oilpan flag won't be a big issue.

- Given the above, now I'm thinking that it would be better to land CLs
incrementally with a reviewable size. Mechanical changes are fine, but a lot of
non-trivial changes should be split into pieces.

Please correct me :) Opinions are welcome.

[Mads Ager (chromium), 2014-02-24 14:03:36]

On 2014/02/24 13:59:47, haraken wrote:
> On 2014/02/24 13:36:26, sof wrote:
> > On 2014/02/24 13:28:36, haraken wrote:
> > > not LGTM.
> > > 
> > > I reviewed half of the CL but the CL is too big to verify correctness. It
> > would
> > > be great if you could split the CL into pieces.
> > > 
> > 
> > Let me think about how. The CL is arguably fairly focused, but once you
change
> a
> > type, dependent code will have to be updated.
> 
> Yeah, probably I've been saying something inconsistent. I'm sorry about that.
> 
> - At first, I was thinking that as we move things more incrementally, we will
> end up producing more leaks in the transition period. Thus I was thinking that
> we want to move things in one go as much as possible.
> 
> - However, in the process of actual reviewing, I noticed the following facts:
> 
> --- If we move too many things in one go, it's hard to verify correctness. 
> --- Regarding memory leaks, we just need to be careful when changing a raw
> pointer to a Member. Changing a RefPtr to a Persistent won't produce leaks.
> --- Even if we produce leaks, the leaks will be just temporary ones in the
> transition period. The leaks will be fixed as a natural consequence of moving
> the entire hierarchy to the heap. In other words, the leaks will be fixed by
the
> time we eliminate the oilpan flag for the hierarchy. Thus producing leaks
behind
> the oilpan flag won't be a big issue.
> 
> - Given the above, now I'm thinking that it would be better to land CLs
> incrementally with a reviewable size. Mechanical changes are fine, but a lot
of
> non-trivial changes should be split into pieces.

For my own sanity I'm splitting up the work on StyleSheets in multiple smaller
chunks as well. So I agree with wanting to land things in small chunks. I do
think that we need to avoid leaks under the flag though. If we introduce one we
need to immediately work to get it under control. Otherwise we will be building
up non-trivial issues that we pretend will fix themselves. :-)

I think we can manage this particular change. There are issues with the change
as noted in my comments, but I will be happy to review this with its current
size.

[sof, 2014-02-24 14:10:20]

On 2014/02/24 14:03:36, Mads Ager (chromium) wrote:
> On 2014/02/24 13:59:47, haraken wrote:
> > On 2014/02/24 13:36:26, sof wrote:
> > > On 2014/02/24 13:28:36, haraken wrote:
> > > > not LGTM.
> > > > 
> > > > I reviewed half of the CL but the CL is too big to verify correctness.
It
> > > would
> > > > be great if you could split the CL into pieces.
> > > > 
> > > 
> > > Let me think about how. The CL is arguably fairly focused, but once you
> change
> > a
> > > type, dependent code will have to be updated.
> > 
> > Yeah, probably I've been saying something inconsistent. I'm sorry about
that.
> > 
> > - At first, I was thinking that as we move things more incrementally, we
will
> > end up producing more leaks in the transition period. Thus I was thinking
that
> > we want to move things in one go as much as possible.
> > 
> > - However, in the process of actual reviewing, I noticed the following
facts:
> > 
> > --- If we move too many things in one go, it's hard to verify correctness. 
> > --- Regarding memory leaks, we just need to be careful when changing a raw
> > pointer to a Member. Changing a RefPtr to a Persistent won't produce leaks.
> > --- Even if we produce leaks, the leaks will be just temporary ones in the
> > transition period. The leaks will be fixed as a natural consequence of
moving
> > the entire hierarchy to the heap. In other words, the leaks will be fixed by
> the
> > time we eliminate the oilpan flag for the hierarchy. Thus producing leaks
> behind
> > the oilpan flag won't be a big issue.
> > 
> > - Given the above, now I'm thinking that it would be better to land CLs
> > incrementally with a reviewable size. Mechanical changes are fine, but a lot
> of
> > non-trivial changes should be split into pieces.
> 
> For my own sanity I'm splitting up the work on StyleSheets in multiple smaller
> chunks as well. So I agree with wanting to land things in small chunks. I do
> think that we need to avoid leaks under the flag though. If we introduce one
we
> need to immediately work to get it under control. Otherwise we will be
building
> up non-trivial issues that we pretend will fix themselves. :-)
> 
> I think we can manage this particular change. There are issues with the change
> as noted in my comments, but I will be happy to review this with its current
> size.

Thanks for the (meta) feedback, agree with it all.

Separating out the WorkerThread changes into a separate CL is doable I reckon,
and the startup data conversion from an OwnPtr.

[haraken, 2014-02-24 14:14:16]

> Thanks for the (meta) feedback, agree with it all.
> 
> Separating out the WorkerThread changes into a separate CL is doable I reckon,
> and the startup data conversion from an OwnPtr.

That sounds great!

I'm sorry if my original comment was too harsh :)

[sof, 2014-02-24 22:44:26]

See https://codereview.chromium.org/178663004/ for the updated version
addressing the feedback below.

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Dedicate...
File Source/core/workers/DedicatedWorkerGlobalScope.h (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Dedicate...
Source/core/workers/DedicatedWorkerGlobalScope.h:44: class
DedicatedWorkerGlobalScope FINAL : public WorkerGlobalScope {
On 2014/02/24 13:27:53, Mads Ager (chromium) wrote:
> Please implement the virtual trace method for this and explicitly dispatch to
> the parent:
> 
> virtual void trace(Visitor* visitor) { WorkerGlobalScope::trace(visitor); }

Done.

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Dedicate...
File Source/core/workers/DedicatedWorkerGlobalScope.idl (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/Dedicate...
Source/core/workers/DedicatedWorkerGlobalScope.idl:33: WillBeGarbageCollected
On 2014/02/24 13:28:36, haraken wrote:
> 
> You don't need to write [WillBeGarbageCollected] in a subclass.
> [WillBeGarbageCollected] is inherited.

Done.

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/SharedWo...
File Source/core/workers/SharedWorkerGlobalScope.idl (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/SharedWo...
Source/core/workers/SharedWorkerGlobalScope.idl:33: WillBeGarbageCollected
On 2014/02/24 13:28:36, haraken wrote:
> 
> You don't need to write [WillBeGarbageCollected] in a subclass. It's
inherited.

Done.

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/WorkerCo...
File Source/core/workers/WorkerConsole.h (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/WorkerCo...
Source/core/workers/WorkerConsole.h:50: using
RefCountedWillBeRefCountedGarbageCollected<WorkerConsole>::ref;
On 2014/02/24 13:28:36, haraken wrote:
> 
> Probably can you use:
> 
> #if ENABLE(OILPAN)
>   using RefCounted<WorkerConsole>::ref;
>   using RefCounted<WorkerConsole>::deref;
> #endif

If I understand you correctly, not until we can remove the 'ref forwarding' that
ConsoleBase uses (and make this object be *GarbageCollectedFinalized rather than
*RefCountedGarbageCollected.)

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/WorkerCo...
Source/core/workers/WorkerConsole.h:71: WorkerGlobalScope* m_scope;
On 2014/02/24 13:28:36, haraken wrote:
> 
> You need to change this to a Member.

Done.

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/WorkerGl...
File Source/core/workers/WorkerGlobalScope.h (right):

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/WorkerGl...
Source/core/workers/WorkerGlobalScope.h:127: void trace(Visitor*);
On 2014/02/24 13:27:53, Mads Ager (chromium) wrote:
> Please make the trace method virtual and implement it in the subclasses so
that
> we get full tracing dependent on the actual class.

Done.

https://codereview.chromium.org/177073004/diff/1/Source/core/workers/WorkerGl...
Source/core/workers/WorkerGlobalScope.h:162: HashSet<Observer*>
m_workerObservers;
On 2014/02/24 13:28:36, haraken wrote:
> 
> Are these raw pointers safe?

Good question :) It's another unused field; removed.

[sof, 2014-03-12 07:39:47]

Before closing this CL, it was split up as:

 * WorkerGlobalScope + Worker{Navigator,Console,Location} supplements:
      https://codereview.chromium.org/178663004/
 * Worker objects:
      https://codereview.chromium.org/180743028/
 * WorkerThreadStartupData:
      https://codereview.chromium.org/191003010/

MessagePort+Channel have not been tackled separately yet; WorkerScriptLoader
could be moved, but no pressing need.