Oilpan: move WorkerGlobalScope to oilpan's heap.

Oilpan: move WorkerGlobalScope to oilpan's heap.

This moves the WorkerGlobalScope object and the objects derived from it
to the Oilpan heap. This also includes the Console, Location and
Navigator interface objects that are attached to the worker global scope.

R=haraken@chromium.org,ager@chromium.org,abarth@chromium.org
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168830

[sof, 2014-02-24 22:43:07]

Please take a look.

Splitting out *WorkerGlobalScope (+ its Console, Location, Navigator
sub-objects) from https://codereview.chromium.org/177073004/

[sof, 2014-02-24 22:46:24]

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

https://codereview.chromium.org/178663004/diff/1/Source/core/workers/WorkerGl...
Source/core/workers/WorkerGlobalScope.h:60: class WorkerGlobalScope : public
RefCountedWillBeRefCountedGarbageCollected<WorkerGlobalScope>, public
ScriptWrappable, public SecurityContext, public ExecutionContext, public
ExecutionContextClient, public WorkerSupplementable, public
EventTargetWithInlineData {
Just noticed: we now need to address Supplementable/Supplement, I think.

Will attend to tomorrow.

[haraken, 2014-02-25 01:30:49]

Looks good except for WorkerGlobalScope::m_thread. Probably you'll need to move
WorkerThread at the same time.

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

https://codereview.chromium.org/178663004/diff/1/Source/bindings/v8/WorkerScr...
Source/bindings/v8/WorkerScriptController.cpp:164:
V8DOMWrapper::associateObjectWithWrapper<V8WorkerGlobalScope>(PassRefPtrWillBeRawPtr<WorkerGlobalScope>(&m_workerGlobalScope),
contextType, jsWorkerGlobalScope, isolate(), WrapperConfiguration::Dependent);

I'm surprised that we didn't need '&' before :)

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

https://codereview.chromium.org/178663004/diff/1/Source/core/workers/WorkerGl...
Source/core/workers/WorkerGlobalScope.h:60: class WorkerGlobalScope : public
RefCountedWillBeRefCountedGarbageCollected<WorkerGlobalScope>, public
ScriptWrappable, public SecurityContext, public ExecutionContext, public
ExecutionContextClient, public WorkerSupplementable, public
EventTargetWithInlineData {
On 2014/02/24 22:46:24, sof wrote:
> Just noticed: we now need to address Supplementable/Supplement, I think.
> 
> Will attend to tomorrow.

Just to help me understand: Why do we need to address Supplementable/Supplement
now? In my understanding, as long as we're using Persistent handles in
supplementing objects, it won't cause any issue even if we don't support tracing
between supplementable objects and supplementing objects.

Nit: Not related to this CL, we should remove WorkerSupplementable.h and use
Supplementable<WorkerGlobalScope>/Supplement<WorkerGlobalScope>.
WorkerSupplementable/WorkerSupplement looks like an unnecessary typedef
indirection.

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

This raw pointer will cause an issue.

- WorkerThread is off the heap.
- WorkerGlobalScope is on the heap.
- Thus WorkerGlobalScope can outlive WorkerThread.
- ~WorkerGlobalScope() touches m_thread. It can be a stale pointer.

https://codereview.chromium.org/178663004/diff/1/Source/core/workers/WorkerLo...
File Source/core/workers/WorkerLocation.cpp (right):

https://codereview.chromium.org/178663004/diff/1/Source/core/workers/WorkerLo...
Source/core/workers/WorkerLocation.cpp:2: * Copyright (C) 2008 Apple Inc. All
Rights Reserved.

Use the 4-line copyright with 2014.

However, you won't need to add this file once kouhei@ lands a CL that removes
DECLARE_GC_INFO/DEFINE_GC_INFO :)

[Mads Ager (chromium), 2014-02-25 07:34:18]

DBC

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

https://codereview.chromium.org/178663004/diff/1/Source/bindings/v8/WorkerScr...
Source/bindings/v8/WorkerScriptController.cpp:164:
V8DOMWrapper::associateObjectWithWrapper<V8WorkerGlobalScope>(PassRefPtrWillBeRawPtr<WorkerGlobalScope>(&m_workerGlobalScope),
contextType, jsWorkerGlobalScope, isolate(), WrapperConfiguration::Dependent);
On 2014/02/25 01:30:49, haraken wrote:
> 
> I'm surprised that we didn't need '&' before :)

PassRefPtr has a constructor that takes a const reference. We could add one for
RawPtr as well so we can avoid the '&' here. I'm not sure I prefer that though.
I think it makes it clearer what is going on to have the actual '&' here. I
wonder if a decision has been made that the style is to have a constructor from
a const reference and not use the '&' for these reference members?

[haraken, 2014-02-25 07:43:02]

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

https://codereview.chromium.org/178663004/diff/1/Source/bindings/v8/WorkerScr...
Source/bindings/v8/WorkerScriptController.cpp:164:
V8DOMWrapper::associateObjectWithWrapper<V8WorkerGlobalScope>(PassRefPtrWillBeRawPtr<WorkerGlobalScope>(&m_workerGlobalScope),
contextType, jsWorkerGlobalScope, isolate(), WrapperConfiguration::Dependent);
On 2014/02/25 07:34:18, Mads Ager (chromium) wrote:
> On 2014/02/25 01:30:49, haraken wrote:
> > 
> > I'm surprised that we didn't need '&' before :)
> 
> PassRefPtr has a constructor that takes a const reference. We could add one
for
> RawPtr as well so we can avoid the '&' here. I'm not sure I prefer that
though.
> I think it makes it clearer what is going on to have the actual '&' here. I
> wonder if a decision has been made that the style is to have a constructor
from
> a const reference and not use the '&' for these reference members?

I don't know, but I agree with Mads that using explicit '&' is better.

Probably we can try to remove PassRefPtr's constructor that takes a const
reference and see how many places get broken. If there're few places, we can
remove the constructor.

[sof, 2014-02-25 07:46:44]

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

https://codereview.chromium.org/178663004/diff/1/Source/core/workers/WorkerGl...
Source/core/workers/WorkerGlobalScope.h:157: WorkerThread* m_thread;
On 2014/02/25 01:30:49, haraken wrote:
> 
> This raw pointer will cause an issue.
> 
> - WorkerThread is off the heap.
> - WorkerGlobalScope is on the heap.
> - Thus WorkerGlobalScope can outlive WorkerThread.
> - ~WorkerGlobalScope() touches m_thread. It can be a stale pointer.

If you look at WorkerThread::workerThread(), it assumes that the global scope is
promptly deleted upon calling deref() on it.

I think that needs to be adjusted here, which should avoid the need for moving
WorkerThread into the heap in this CL (which would still be problematic given
the lack of finalization order.)

[sof, 2014-02-25 08:56:35]

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

https://codereview.chromium.org/178663004/diff/1/Source/core/workers/WorkerGl...
Source/core/workers/WorkerGlobalScope.h:60: class WorkerGlobalScope : public
RefCountedWillBeRefCountedGarbageCollected<WorkerGlobalScope>, public
ScriptWrappable, public SecurityContext, public ExecutionContext, public
ExecutionContextClient, public WorkerSupplementable, public
EventTargetWithInlineData {
On 2014/02/25 01:30:49, haraken wrote:
> On 2014/02/24 22:46:24, sof wrote:
> > Just noticed: we now need to address Supplementable/Supplement, I think.
> > 
> > Will attend to tomorrow.
> 
> Just to help me understand: Why do we need to address
Supplementable/Supplement
> now? In my understanding, as long as we're using Persistent handles in
> supplementing objects, it won't cause any issue even if we don't support
tracing
> between supplementable objects and supplementing objects.
> 

The problem isn't that such Persistents won't be cleared during the final GC in
ThreadState::cleanup(), but the objects that they point to won't be GCed. And we
(quite reasonably) don't run GCs until a steady state is reached.

Making the Supplements GCed would address this, I think. The alternative is to
register a 'pre-cleanup' task that clears the persistents on a case-by-case
basis. (Unless we add some macros & otherwise to make it more convenient.)

This happens with the WorkerPerformance supplement that hangs off
WorkerGlobalScope.

[Vyacheslav Egorov (Chromium), 2014-02-25 10:41:26]

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

https://codereview.chromium.org/178663004/diff/1/Source/core/workers/WorkerGl...
Source/core/workers/WorkerGlobalScope.h:157: WorkerThread* m_thread;
WorkerGlobalScope can't outlive the thread because it is allocated on the thread
owned heap right now and all objects from the thread owned heap are destroyed on
ThreadState detachment (and we assert that the heap is empty).

[haraken, 2014-02-25 11:16:28]

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

https://codereview.chromium.org/178663004/diff/1/Source/core/workers/WorkerGl...
Source/core/workers/WorkerGlobalScope.h:60: class WorkerGlobalScope : public
RefCountedWillBeRefCountedGarbageCollected<WorkerGlobalScope>, public
ScriptWrappable, public SecurityContext, public ExecutionContext, public
ExecutionContextClient, public WorkerSupplementable, public
EventTargetWithInlineData {
> The problem isn't that such Persistents won't be cleared during the final GC
in
> ThreadState::cleanup(), but the objects that they point to won't be GCed. And
we
> (quite reasonably) don't run GCs until a steady state is reached.
> 
> Making the Supplements GCed would address this, I think. The alternative is to
> register a 'pre-cleanup' task that clears the persistents on a case-by-case
> basis. (Unless we add some macros & otherwise to make it more convenient.)
> 
> This happens with the WorkerPerformance supplement that hangs off
> WorkerGlobalScope.

Let me make sure that I'm understanding correctly:

- Previously WorkerGlobalScope was destructed when WorkerThread::workerThread()
calls 'm_workerGlobalScope = nullptr'. Thus it was guaranteed that the
WorkerGlobalScope does not outlive WorkerThread.

- However, in the new code, someone holds Persistent handles to the
WorkerGlobalScope. Thus 'm_workerGlobalScope = nullptr' won't destruct the
WorkerGlobalScope. Consequently, WorkerGlobalScope will end up outliving
WorkerThread. That's the issue.

Am I understanding correctly?


If my understanding is correct, my opinions are as follows:

- Making the Supplements GCed and removing Persistent handles will solve the
issue, because it will guarantee that 'm_workerGlobalScope = nullptr' destructs
the WorkerGlobalScope.

- However, this is a fragile solution. Once we have a Persistent handle
somewhere, we will hit the same problem again. It's fragile to try to guarantee
that 'm_workerGlobalScope = nullptr' destructs the WorkerGlobalScope.

- Alternately, what we should do here would be using a Member for
WorkerGlobalScope::m_thread, by which we can guarantee that the WorkerThread and
the WorkerGlobalScope die together.

What do you think?

[haraken, 2014-02-25 11:20:47]

Also you might want to remove all RefPtr<WorkerGlobalScope> from the code base
in this CL. For example, RefPtr<WorkerGlobalScope> is used in
WorkerThread::m_workerGlobalScope.

Unfortunately, using RefPtr doesn't cause compile errors because
WorkerGlobalScope is RefCountedGarbageCollected (due to EventTarget). However, I
think we should remove as many RefPtrs as possible and allow RefPtrs only when
someone treats the WorkerGlobalScope as an EventTarget.

[sof, 2014-02-25 11:38:01]

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

https://codereview.chromium.org/178663004/diff/1/Source/core/workers/WorkerGl...
Source/core/workers/WorkerGlobalScope.h:60: class WorkerGlobalScope : public
RefCountedWillBeRefCountedGarbageCollected<WorkerGlobalScope>, public
ScriptWrappable, public SecurityContext, public ExecutionContext, public
ExecutionContextClient, public WorkerSupplementable, public
EventTargetWithInlineData {
On 2014/02/25 11:16:28, haraken wrote:
> > The problem isn't that such Persistents won't be cleared during the final GC
> in
> > ThreadState::cleanup(), but the objects that they point to won't be GCed.
And
> we
> > (quite reasonably) don't run GCs until a steady state is reached.
> > 
> > Making the Supplements GCed would address this, I think. The alternative is
to
> > register a 'pre-cleanup' task that clears the persistents on a case-by-case
> > basis. (Unless we add some macros & otherwise to make it more convenient.)
> > 
> > This happens with the WorkerPerformance supplement that hangs off
> > WorkerGlobalScope.
> 
> Let me make sure that I'm understanding correctly:
> 
> - Previously WorkerGlobalScope was destructed when
WorkerThread::workerThread()
> calls 'm_workerGlobalScope = nullptr'. Thus it was guaranteed that the
> WorkerGlobalScope does not outlive WorkerThread.
> 

Correct.

> - However, in the new code, someone holds Persistent handles to the
> WorkerGlobalScope. Thus 'm_workerGlobalScope = nullptr' won't destruct the
> WorkerGlobalScope. Consequently, WorkerGlobalScope will end up outliving
> WorkerThread. That's the issue.
> 
> Am I understanding correctly?
> 

Essentially. No one is holding onto WorkerGlobalScope via a persistent, but
it has Supplements with Persistents. So when WorkerGlobalScope is finalized,
those Supplements will be destructed & their persistent references let go of.
Those objects won't be GCed.

> 
> If my understanding is correct, my opinions are as follows:
> 
> - Making the Supplements GCed and removing Persistent handles will solve the
> issue, because it will guarantee that 'm_workerGlobalScope = nullptr'
destructs
> the WorkerGlobalScope.
> 

Clearing m_workerGlobalScope won't destruct the object, but become garbage.
But both it and its supplements will be swept out at the next GC (which
strikes during worker thread cleanup just below.)

(I'm currently changing Supplementable to do this; dealing with some template
instantiation issues atm.)

> - However, this is a fragile solution. Once we have a Persistent handle
> somewhere, we will hit the same problem again. It's fragile to try to
guarantee
> that 'm_workerGlobalScope = nullptr' destructs the WorkerGlobalScope.
> 

Yes, registering pre-cleanup tasks can be used to address this, should it arise.

[haraken, 2014-02-25 11:54:57]

> > - However, this is a fragile solution. Once we have a Persistent handle
> > somewhere, we will hit the same problem again. It's fragile to try to
> guarantee
> > that 'm_workerGlobalScope = nullptr' destructs the WorkerGlobalScope.
> > 
> 
> Yes, registering pre-cleanup tasks can be used to address this, should it
arise.

hmm, pre-cleanup tasks sound nasty to me :)

In the end, the goal is to use a Member for WorkerGlobalScope::m_thread. The
current problem is what we should do in a transition period where
WorkerGlobalScope is on the heap but WorkerThread is off the heap. In more
general, the problem is how to handle a raw back pointer from an object X to an
object Y, where X is on the heap and Y is off the heap.

How about the following approach?

- Move the contents of X's destructor to X::detach().
- When Y is destructed, call X::detach() and clear the raw back pointer (from X
to Y).


> Clearing m_workerGlobalScope won't destruct the object, but become garbage.
> But both it and its supplements will be swept out at the next GC (which
> strikes during worker thread cleanup just below.)
> 
> (I'm currently changing Supplementable to do this; dealing with some template
> instantiation issues atm.)

This is a good thing anyway :)

[sof, 2014-02-25 12:25:26]

On 2014/02/25 11:54:57, haraken wrote:
> > > - However, this is a fragile solution. Once we have a Persistent handle
> > > somewhere, we will hit the same problem again. It's fragile to try to
> > guarantee
> > > that 'm_workerGlobalScope = nullptr' destructs the WorkerGlobalScope.
> > > 
> > 
> > Yes, registering pre-cleanup tasks can be used to address this, should it
> arise.
> 
> hmm, pre-cleanup tasks sound nasty to me :)
> 
> In the end, the goal is to use a Member for WorkerGlobalScope::m_thread. The
> current problem is what we should do in a transition period where
> WorkerGlobalScope is on the heap but WorkerThread is off the heap. In more
> general, the problem is how to handle a raw back pointer from an object X to
an
> object Y, where X is on the heap and Y is off the heap.
> 
> How about the following approach?
> 
> - Move the contents of X's destructor to X::detach().
> - When Y is destructed, call X::detach() and clear the raw back pointer (from
X
> to Y).
> 

Yes, that would be a cleaner arrangement. I've moved all code out
of the WorkerGlobalScope destructor.

I've not looked into if it is doable&safe to have the WorkerThread object
GC/destruct itself while it is still on the stack.

Slava correctly (and clarifyingly) points out that without WorkerThread being
GCed, the lifetime of WorkerThread is guaranteed to extend beyond its global
scope.

[haraken, 2014-02-25 12:29:54]

> Yes, that would be a cleaner arrangement. I've moved all code out
> of the WorkerGlobalScope destructor.
> 
> I've not looked into if it is doable&safe to have the WorkerThread object
> GC/destruct itself while it is still on the stack.

Probably we can call WorkerGlobalScope::detach() when WorkerThread calls
'm_workerGlobalScope = nulptr' ? (because it's the exact place where we want to
detach the WorkerGlobalScope)

[sof, 2014-02-25 21:56:43]

Supplementables can now optionally live on the heap. Ideally, that should be
inferrable only by considering if the object type derives from GarbageCollected
or not; see Supplementable.h comment for details.

Please have a look.

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

https://codereview.chromium.org/178663004/diff/1/Source/core/workers/WorkerGl...
Source/core/workers/WorkerGlobalScope.h:60: class WorkerGlobalScope : public
RefCountedWillBeRefCountedGarbageCollected<WorkerGlobalScope>, public
ScriptWrappable, public SecurityContext, public ExecutionContext, public
ExecutionContextClient, public WorkerSupplementable, public
EventTargetWithInlineData {
WorkerGlobalScope is now a GCed Supplementable along with its Supplements.

https://codereview.chromium.org/178663004/diff/1/Source/core/workers/WorkerLo...
File Source/core/workers/WorkerLocation.cpp (right):

https://codereview.chromium.org/178663004/diff/1/Source/core/workers/WorkerLo...
Source/core/workers/WorkerLocation.cpp:2: * Copyright (C) 2008 Apple Inc. All
Rights Reserved.
On 2014/02/25 01:30:49, haraken wrote:
> 
> Use the 4-line copyright with 2014.
> 
> However, you won't need to add this file once kouhei@ lands a CL that removes
> DECLARE_GC_INFO/DEFINE_GC_INFO :)

I've rebased to bring that change into scope; removed this file as a result. A
very welcome improvement.

[haraken, 2014-02-26 02:39:33]

https://codereview.chromium.org/178663004/diff/40001/Source/core/workers/Work...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/178663004/diff/40001/Source/core/workers/Work...
Source/core/workers/WorkerThread.cpp:145: m_workerGlobalScope = nullptr;

Shall we change m_workerGlobalScope to RefPtrWillBePersistent?

We might want to remove all RefPtr<WorkerGlobalScope>s except for a case where
WorkerGlobalScope is treated as EventTargets. "RefCounted"GarbageCollected
should be just for EventTargets.

[sof, 2014-02-26 06:29:40]

https://codereview.chromium.org/178663004/diff/40001/Source/core/workers/Work...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/178663004/diff/40001/Source/core/workers/Work...
Source/core/workers/WorkerThread.cpp:145: m_workerGlobalScope = nullptr;
On 2014/02/26 02:39:34, haraken wrote:
> 
> Shall we change m_workerGlobalScope to RefPtrWillBePersistent?
> 
> We might want to remove all RefPtr<WorkerGlobalScope>s except for a case where
> WorkerGlobalScope is treated as EventTargets. "RefCounted"GarbageCollected
> should be just for EventTargets.

Makes sense to do that (but I didn't want to increase the footprint of this CL
more than needed.) Now done.

[Mads Ager (chromium), 2014-02-26 08:00:36]

https://codereview.chromium.org/178663004/diff/60001/Source/modules/performan...
File Source/modules/performance/WorkerGlobalScopePerformance.cpp (right):

https://codereview.chromium.org/178663004/diff/60001/Source/modules/performan...
Source/modules/performance/WorkerGlobalScopePerformance.cpp:41: #if 0
Remove the code. :)

https://codereview.chromium.org/178663004/diff/60001/Source/platform/Suppleme...
File Source/platform/Supplementable.h (right):

https://codereview.chromium.org/178663004/diff/60001/Source/platform/Suppleme...
Source/platform/Supplementable.h:102: typedef RawPtr<SupplementBase<T, true> >
SupplementArgumentType;
Shouldn't this be PassOwnPtrWillBeRawPtr?

https://codereview.chromium.org/178663004/diff/60001/Source/platform/Suppleme...
Source/platform/Supplementable.h:103: typedef WillBeHeapHashMap<const char*,
RefPtrWillBeMember<SupplementBase<T, true> >, PtrHash<const char*> >
SupplementMap;
Shouldn't this be OwnPtrWillBeMember?

https://codereview.chromium.org/178663004/diff/60001/Source/platform/Suppleme...
Source/platform/Supplementable.h:123: struct SupplementableTraits : public
SupplementableTraitsImpl<T, WTF::IsSubclassOfTemplate<T,
WebCore::GarbageCollected>::value> {
Let's remove SupplementableTraits. As you state it cannot be used and we should
just remove it.

[sof, 2014-02-26 09:11:20]

https://codereview.chromium.org/178663004/diff/60001/Source/modules/performan...
File Source/modules/performance/WorkerGlobalScopePerformance.cpp (right):

https://codereview.chromium.org/178663004/diff/60001/Source/modules/performan...
Source/modules/performance/WorkerGlobalScopePerformance.cpp:41: #if 0
On 2014/02/26 08:00:36, Mads Ager (chromium) wrote:
> Remove the code. :)

Oops; experimental sins removed.

https://codereview.chromium.org/178663004/diff/60001/Source/platform/Suppleme...
File Source/platform/Supplementable.h (right):

https://codereview.chromium.org/178663004/diff/60001/Source/platform/Suppleme...
Source/platform/Supplementable.h:102: typedef RawPtr<SupplementBase<T, true> >
SupplementArgumentType;
On 2014/02/26 08:00:36, Mads Ager (chromium) wrote:
> Shouldn't this be PassOwnPtrWillBeRawPtr?

Yes, that would be tidier.

https://codereview.chromium.org/178663004/diff/60001/Source/platform/Suppleme...
Source/platform/Supplementable.h:103: typedef WillBeHeapHashMap<const char*,
RefPtrWillBeMember<SupplementBase<T, true> >, PtrHash<const char*> >
SupplementMap;
On 2014/02/26 08:00:36, Mads Ager (chromium) wrote:
> Shouldn't this be OwnPtrWillBeMember?

Done.

https://codereview.chromium.org/178663004/diff/60001/Source/platform/Suppleme...
Source/platform/Supplementable.h:123: struct SupplementableTraits : public
SupplementableTraitsImpl<T, WTF::IsSubclassOfTemplate<T,
WebCore::GarbageCollected>::value> {
On 2014/02/26 08:00:36, Mads Ager (chromium) wrote:
> Let's remove SupplementableTraits. As you state it cannot be used and we
should
> just remove it.

yes, i'm currently out of ideas of how to derive a heap-allocated version of
Supplementable just by considering the type.

https://codereview.chromium.org/178663004/diff/80001/Source/platform/Suppleme...
File Source/platform/Supplementable.h (right):

https://codereview.chromium.org/178663004/diff/80001/Source/platform/Suppleme...
Source/platform/Supplementable.h:29: #include "heap/Handle.h"
Adding an #include of heap/ here; is that acceptable or not?

[Mads Ager (chromium), 2014-02-26 10:22:39]

https://codereview.chromium.org/178663004/diff/80001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/178663004/diff/80001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.cpp:187: void WorkerGlobalScope::detach()
I really like this restructuring, thanks! :)

https://codereview.chromium.org/178663004/diff/80001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.cpp:291: void
WorkerGlobalScope::trace(Visitor* visitor)
Don't we need an explicit call to WorkerSupplementable::trace(visitor) in
anything that is WorkerSupplementable (WillBeHeapSupplementable) to get the
tracing of the supplements.

I think we carefully need to step through this code to see that we get the right
visiting behavior here.

https://codereview.chromium.org/178663004/diff/80001/Source/modules/notificat...
File Source/modules/notifications/WorkerGlobalScopeNotifications.h (right):

https://codereview.chromium.org/178663004/diff/80001/Source/modules/notificat...
Source/modules/notifications/WorkerGlobalScopeNotifications.h:41: class
WorkerGlobalScopeNotifications FINAL : public WorkerSupplement {
I don't understand how this works. If this is a WillBeHeapSupplement then the
Supplementable has a HashMap of Members where this can be a member. However,
this object is not in the heap. Don't you need to make this "public
NoBaseWillBeGarbageCollected(Finalized)"?

https://codereview.chromium.org/178663004/diff/80001/Source/platform/Suppleme...
File Source/platform/Supplementable.h (right):

https://codereview.chromium.org/178663004/diff/80001/Source/platform/Suppleme...
Source/platform/Supplementable.h:29: #include "heap/Handle.h"
On 2014/02/26 09:11:21, sof wrote:
> Adding an #include of heap/ here; is that acceptable or not?

I hope so. You are defining WebCore classes here and whenever you do that they
need to interact with the heap infrastructure. We should make sure to ask the
platform owners when we are happy with the change.

[sof, 2014-02-26 12:25:12]

Great feedback, thank you.

https://codereview.chromium.org/178663004/diff/80001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/178663004/diff/80001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.cpp:291: void
WorkerGlobalScope::trace(Visitor* visitor)
On 2014/02/26 10:22:40, Mads Ager (chromium) wrote:
> Don't we need an explicit call to WorkerSupplementable::trace(visitor) in
> anything that is WorkerSupplementable (WillBeHeapSupplementable) to get the
> tracing of the supplements.
> 
> I think we carefully need to step through this code to see that we get the
right
> visiting behavior here.

Yes, thank you. This is a major oversight when adding these GCed
supplementables; it happened to work as the supplements weren't GCed, so they
were kept alive..forever, leaking.

So, all Supplementable objects that are heap allocated must in their trace()
delegate to the Supplementable's trace().

https://codereview.chromium.org/178663004/diff/80001/Source/modules/notificat...
File Source/modules/notifications/WorkerGlobalScopeNotifications.h (right):

https://codereview.chromium.org/178663004/diff/80001/Source/modules/notificat...
Source/modules/notifications/WorkerGlobalScopeNotifications.h:41: class
WorkerGlobalScopeNotifications FINAL : public WorkerSupplement {
On 2014/02/26 10:22:40, Mads Ager (chromium) wrote:
> I don't understand how this works. If this is a WillBeHeapSupplement then the
> Supplementable has a HashMap of Members where this can be a member. However,
> this object is not in the heap. Don't you need to make this "public
> NoBaseWillBeGarbageCollected(Finalized)"?

Done; all heap supplements are now GCed.

[sof, 2014-02-26 12:26:04]

https://codereview.chromium.org/178663004/diff/100001/Source/modules/notifica...
File Source/modules/notifications/WorkerGlobalScopeNotifications.h (right):

https://codereview.chromium.org/178663004/diff/100001/Source/modules/notifica...
Source/modules/notifications/WorkerGlobalScopeNotifications.h:56:
RawPtrWillBeMember<WorkerGlobalScope> m_context;
Note: changed this to be a raw-ptr/member instead of a ref.

[Mads Ager (chromium), 2014-02-26 13:09:08]

https://codereview.chromium.org/178663004/diff/100001/Source/modules/crypto/W...
File Source/modules/crypto/WorkerGlobalScopeCrypto.h (right):

https://codereview.chromium.org/178663004/diff/100001/Source/modules/crypto/W...
Source/modules/crypto/WorkerGlobalScopeCrypto.h:43: class
WorkerGlobalScopeCrypto FINAL : public
NoBaseWillBeGarbageCollectedFinalized<WorkerGlobalScopeCrypto>, public
WorkerSupplement {
As far as I can tell we can get away with not using Finalized here. This is
Final, so there are no subclasses, Supplement doesn't have state, and this class
only has a member which doesn't require the destructor to be called.

https://codereview.chromium.org/178663004/diff/100001/Source/modules/imagebit...
File Source/modules/imagebitmap/ImageBitmapFactories.h (right):

https://codereview.chromium.org/178663004/diff/100001/Source/modules/imagebit...
Source/modules/imagebitmap/ImageBitmapFactories.h:124: class
WorkerGlobalScopeImageBitmapFactories FINAL : public ImageBitmapFactories,
public
NoBaseWillBeGarbageCollectedFinalized<WorkerGlobalScopeImageBitmapFactories>,
public WorkerSupplement {
Please make the garbage collected base class the left-most base class.

This is a little subtle. So this thing is only ever used as a supplement and it
is traced through the supplement map. Whenever an ImageBitmapFactories object is
returned it is returned as a reference to the instance in the supplement map.

Could you add a FIXME: oilpan: here to state that we should collapse the
ImageBitmapFactories into one class once DOMWindow has been moved to the oilpan
heap?

https://codereview.chromium.org/178663004/diff/100001/Source/modules/notifica...
File Source/modules/notifications/WorkerGlobalScopeNotifications.h (right):

https://codereview.chromium.org/178663004/diff/100001/Source/modules/notifica...
Source/modules/notifications/WorkerGlobalScopeNotifications.h:56:
RawPtrWillBeMember<WorkerGlobalScope> m_context;
On 2014/02/26 12:26:05, sof wrote:
> Note: changed this to be a raw-ptr/member instead of a ref.

Yeah, I'm a bit unsure about these. We lose the clear indication that these
cannot be null which is bad. On the other hand, having a member is the right
thing to do for oilpan.

https://codereview.chromium.org/178663004/diff/100001/Source/modules/performa...
File Source/modules/performance/WorkerGlobalScopePerformance.cpp (right):

https://codereview.chromium.org/178663004/diff/100001/Source/modules/performa...
Source/modules/performance/WorkerGlobalScopePerformance.cpp:36: #include
"heap/ThreadState.h"
Why do you need ThreadState here?

https://codereview.chromium.org/178663004/diff/100001/Source/modules/quota/Wo...
File Source/modules/quota/WorkerNavigatorStorageQuota.h (right):

https://codereview.chromium.org/178663004/diff/100001/Source/modules/quota/Wo...
Source/modules/quota/WorkerNavigatorStorageQuota.h:42: class
WorkerNavigatorStorageQuota FINAL : public WillBeHeapSupplement<WorkerNavigator>
{
You need to allocate this in the heap as well.

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

https://codereview.chromium.org/178663004/diff/100001/Source/modules/crypto/W...
File Source/modules/crypto/WorkerGlobalScopeCrypto.h (right):

https://codereview.chromium.org/178663004/diff/100001/Source/modules/crypto/W...
Source/modules/crypto/WorkerGlobalScopeCrypto.h:43: class
WorkerGlobalScopeCrypto FINAL : public
NoBaseWillBeGarbageCollectedFinalized<WorkerGlobalScopeCrypto>, public
WorkerSupplement {
On 2014/02/26 13:09:08, Mads Ager (chromium) wrote:
> As far as I can tell we can get away with not using Finalized here. This is
> Final, so there are no subclasses, Supplement doesn't have state, and this
class
> only has a member which doesn't require the destructor to be called.

Yes, I've verified this (and for other supplements.) Nice.

https://codereview.chromium.org/178663004/diff/100001/Source/modules/imagebit...
File Source/modules/imagebitmap/ImageBitmapFactories.h (right):

https://codereview.chromium.org/178663004/diff/100001/Source/modules/imagebit...
Source/modules/imagebitmap/ImageBitmapFactories.h:124: class
WorkerGlobalScopeImageBitmapFactories FINAL : public ImageBitmapFactories,
public
NoBaseWillBeGarbageCollectedFinalized<WorkerGlobalScopeImageBitmapFactories>,
public WorkerSupplement {
On 2014/02/26 13:09:08, Mads Ager (chromium) wrote:
> Please make the garbage collected base class the left-most base class.
> 
> This is a little subtle. So this thing is only ever used as a supplement and
it
> is traced through the supplement map. Whenever an ImageBitmapFactories object
is
> returned it is returned as a reference to the instance in the supplement map.
> 
> Could you add a FIXME: oilpan: here to state that we should collapse the
> ImageBitmapFactories into one class once DOMWindow has been moved to the
oilpan
> heap?

Done. I think we just have to accept the quirky formulation of this dual
supplement class until both supplements are heap allocated.

https://codereview.chromium.org/178663004/diff/100001/Source/modules/notifica...
File Source/modules/notifications/WorkerGlobalScopeNotifications.h (right):

https://codereview.chromium.org/178663004/diff/100001/Source/modules/notifica...
Source/modules/notifications/WorkerGlobalScopeNotifications.h:56:
RawPtrWillBeMember<WorkerGlobalScope> m_context;
On 2014/02/26 13:09:08, Mads Ager (chromium) wrote:
> On 2014/02/26 12:26:05, sof wrote:
> > Note: changed this to be a raw-ptr/member instead of a ref.
> 
> Yeah, I'm a bit unsure about these. We lose the clear indication that these
> cannot be null which is bad. On the other hand, having a member is the right
> thing to do for oilpan.

Has the ability to declare const-like Members been considered?

https://codereview.chromium.org/178663004/diff/100001/Source/modules/performa...
File Source/modules/performance/WorkerGlobalScopePerformance.cpp (right):

https://codereview.chromium.org/178663004/diff/100001/Source/modules/performa...
Source/modules/performance/WorkerGlobalScopePerformance.cpp:36: #include
"heap/ThreadState.h"
On 2014/02/26 13:09:08, Mads Ager (chromium) wrote:
> Why do you need ThreadState here?

Removed.

https://codereview.chromium.org/178663004/diff/100001/Source/modules/quota/Wo...
File Source/modules/quota/WorkerNavigatorStorageQuota.h (right):

https://codereview.chromium.org/178663004/diff/100001/Source/modules/quota/Wo...
Source/modules/quota/WorkerNavigatorStorageQuota.h:42: class
WorkerNavigatorStorageQuota FINAL : public WillBeHeapSupplement<WorkerNavigator>
{
On 2014/02/26 13:09:08, Mads Ager (chromium) wrote:
> You need to allocate this in the heap as well.

Yes, done.

[haraken, 2014-02-26 14:39:06]

Sorry for the review delay. Let me take a look at this CL tomorrow morning :)
Blink gardening stole my energy...

[Mads Ager (chromium), 2014-02-26 15:10:18]

LGTM, thanks for iterating on this!

It would probably be good to get Haraken's eyes on this tomorrow morning TOK
time before putting it in the CQ. I fear that the two of us have been staring at
this too long to be able to see subtle issues. :)

https://codereview.chromium.org/178663004/diff/100001/Source/modules/notifica...
File Source/modules/notifications/WorkerGlobalScopeNotifications.h (right):

https://codereview.chromium.org/178663004/diff/100001/Source/modules/notifica...
Source/modules/notifications/WorkerGlobalScopeNotifications.h:56:
RawPtrWillBeMember<WorkerGlobalScope> m_context;
On 2014/02/26 14:36:25, sof wrote:
> On 2014/02/26 13:09:08, Mads Ager (chromium) wrote:
> > On 2014/02/26 12:26:05, sof wrote:
> > > Note: changed this to be a raw-ptr/member instead of a ref.
> > 
> > Yeah, I'm a bit unsure about these. We lose the clear indication that these
> > cannot be null which is bad. On the other hand, having a member is the right
> > thing to do for oilpan.
> 
> Has the ability to declare const-like Members been considered?

In principle you should be able to use 'const RefPtrWillBeMember' (if we have
correctly marked the right methods as const on Member and RefPtr). That alone
does not guarantee that it is non-null but then at least we can put an assert in
the constructor that it is not null.

[sof, 2014-02-26 15:29:24]

On 2014/02/26 15:10:18, Mads Ager (chromium) wrote:
> LGTM, thanks for iterating on this!
> 
> It would probably be good to get Haraken's eyes on this tomorrow morning TOK
> time before putting it in the CQ. I fear that the two of us have been staring
at
> this too long to be able to see subtle issues. :)
> 

Thanks ager, great feedback & help.

I won't touch the CL until tomorrow; the Source/platform/ changes need approval
also.

[haraken, 2014-02-27 02:46:00]

Thanks for the iteration! Almost LG with a couple of comments.

https://codereview.chromium.org/178663004/diff/130001/Source/core/workers/Wor...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/178663004/diff/130001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.cpp:59: class CloseWorkerGlobalScopeTask :
public ExecutionContextTask {

Now that we call WorkerGlobalScope::detach() when shutting down the worker
thread, would it be possible to remove CloseWorkerGlobalScopeTask ?

BTW, before your CL, why did we need to clear WorkerGlobalScope asynchronously
using CloseWorkerGlobalScopeTask? Is it OK to replace the asynchronous clean-up
with the synchronous clean-up in your CL? (In your CL, the clean-up is done
synchronously by calling WorkerGlobalScope::detach().)

https://codereview.chromium.org/178663004/diff/130001/Source/core/workers/Wor...
File Source/core/workers/WorkerGlobalScope.h (left):

https://codereview.chromium.org/178663004/diff/130001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.h:159: HashSet<Observer*>
m_workerObservers;

oh, this is unused :)

https://codereview.chromium.org/178663004/diff/130001/Source/core/workers/Wor...
File Source/core/workers/WorkerSupplementable.h (right):

https://codereview.chromium.org/178663004/diff/130001/Source/core/workers/Wor...
Source/core/workers/WorkerSupplementable.h:38: typedef
WillBeHeapSupplement<WorkerGlobalScope> WorkerSupplement;

Let's remove this file and directly use
WillBeHeapSupplementable<WorkerGlobalScope>. These typedefs are not that helpful
and just makes underlying data structure obscure.

https://codereview.chromium.org/178663004/diff/130001/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/178663004/diff/130001/Source/heap/Handle.h#ne...
Source/heap/Handle.h:565: #define WillBeHeapSupplementable
WebCore::HeapSupplementable

Nit: I'd prefer "GarbageCollectedSupplementable" and
"GarbageCollectedSupplement", since they are GarbageCollected classes. I'll
delegate the decision to you.

The underlying issue is that we mix "GarbageCollected" and "Heap" in various
names.

https://codereview.chromium.org/178663004/diff/130001/Source/modules/imagebit...
File Source/modules/imagebitmap/ImageBitmapFactories.h (right):

https://codereview.chromium.org/178663004/diff/130001/Source/modules/imagebit...
Source/modules/imagebitmap/ImageBitmapFactories.h:58: class ImageBitmapFactories
: public Supplement<DOMWindow> {

Keep FINAL.

https://codereview.chromium.org/178663004/diff/130001/Source/modules/notifica...
File Source/modules/notifications/WorkerGlobalScopeNotifications.h (right):

https://codereview.chromium.org/178663004/diff/130001/Source/modules/notifica...
Source/modules/notifications/WorkerGlobalScopeNotifications.h:51: explicit
WorkerGlobalScopeNotifications(RawPtr<WorkerGlobalScope>);

Can we just use a raw pointer?

https://codereview.chromium.org/178663004/diff/130001/Source/modules/quota/Wo...
File Source/modules/quota/WorkerNavigatorStorageQuota.h (right):

https://codereview.chromium.org/178663004/diff/130001/Source/modules/quota/Wo...
Source/modules/quota/WorkerNavigatorStorageQuota.h:42: class
WorkerNavigatorStorageQuota FINAL : public
NoBaseWillBeGarbageCollected<WorkerNavigatorStorageQuota>, public
WillBeHeapSupplement<WorkerNavigator> {

It's a shame that we have to add NoBaseWillBeGarbageCollected to all
supplementing classes.

Would it be possible to remove NoBaseWillBeGarbageCollected by making
HeapSupplement inherit GarbageCollected if T==WorkerNavigator?

https://codereview.chromium.org/178663004/diff/130001/Source/platform/Supplem...
File Source/platform/Supplementable.h (right):

https://codereview.chromium.org/178663004/diff/130001/Source/platform/Supplem...
Source/platform/Supplementable.h:183: }

Let me make sure I'm understanding things correctly:

Given a Supplementable object A and Supplementing objects X and Y, trace() will
be called in the following sequence, right?

(1) A::trace() is called.
(2) A::trace() calls SupplementableBase::trace().
(3) SupplementableBase::trace() calls X::trace().
(4) SupplementableBase::trace() calls Y::trace().

[haraken, 2014-02-27 02:46:51]

https://codereview.chromium.org/178663004/diff/130001/Source/modules/imagebit...
File Source/modules/imagebitmap/ImageBitmapFactories.h (right):

https://codereview.chromium.org/178663004/diff/130001/Source/modules/imagebit...
Source/modules/imagebitmap/ImageBitmapFactories.h:58: class ImageBitmapFactories
: public Supplement<DOMWindow> {
On 2014/02/27 02:46:01, haraken wrote:
> 
> Keep FINAL.

oh, now this is inherited. Ignore this comment.

[Mads Ager (chromium), 2014-02-27 06:41:46]

https://codereview.chromium.org/178663004/diff/130001/Source/modules/quota/Wo...
File Source/modules/quota/WorkerNavigatorStorageQuota.h (right):

https://codereview.chromium.org/178663004/diff/130001/Source/modules/quota/Wo...
Source/modules/quota/WorkerNavigatorStorageQuota.h:42: class
WorkerNavigatorStorageQuota FINAL : public
NoBaseWillBeGarbageCollected<WorkerNavigatorStorageQuota>, public
WillBeHeapSupplement<WorkerNavigator> {
On 2014/02/27 02:46:01, haraken wrote:
> 
> It's a shame that we have to add NoBaseWillBeGarbageCollected to all
> supplementing classes.
> 
> Would it be possible to remove NoBaseWillBeGarbageCollected by making
> HeapSupplement inherit GarbageCollected if T==WorkerNavigator?

In my opinion that would be too magical. However, we should attempt to make it a
compile-time error to put something that is not GarbageCollected into a Member.
I propose to do that as a separate changelist.

[haraken, 2014-02-27 06:57:02]

> > It's a shame that we have to add NoBaseWillBeGarbageCollected to all
> > supplementing classes.
> > 
> > Would it be possible to remove NoBaseWillBeGarbageCollected by making
> > HeapSupplement inherit GarbageCollected if T==WorkerNavigator?
> 
> In my opinion that would be too magical. However, we should attempt to make it
a
> compile-time error to put something that is not GarbageCollected into a
Member.
> I propose to do that as a separate changelist.

Or can we simply make HeapSupplement inherit GarbageCollected?

I don't fully understand why a class X needs to inherit both GarbageCollected
and HeapSupplement, because HeapSupplement already implies that the object is a
supplementing object on the heap.

[Mads Ager (chromium), 2014-02-27 07:00:06]

On 2014/02/27 06:57:02, haraken wrote:
> > > It's a shame that we have to add NoBaseWillBeGarbageCollected to all
> > > supplementing classes.
> > > 
> > > Would it be possible to remove NoBaseWillBeGarbageCollected by making
> > > HeapSupplement inherit GarbageCollected if T==WorkerNavigator?
> > 
> > In my opinion that would be too magical. However, we should attempt to make
it
> a
> > compile-time error to put something that is not GarbageCollected into a
> Member.
> > I propose to do that as a separate changelist.
> 
> Or can we simply make HeapSupplement inherit GarbageCollected?
> 
> I don't fully understand why a class X needs to inherit both GarbageCollected
> and HeapSupplement, because HeapSupplement already implies that the object is
a
> supplementing object on the heap.

I think we need to separate them out. There are different ways of being garbage
collected (do you need finalization, do you need ref counting) which is
independent of whether you are a supplement. So I don't think there is a one
size fits all that you can put on HeapSupplement.

[haraken, 2014-02-27 07:13:36]

> > I don't fully understand why a class X needs to inherit both
GarbageCollected
> > and HeapSupplement, because HeapSupplement already implies that the object
is
> a
> > supplementing object on the heap.
> 
> I think we need to separate them out. There are different ways of being
garbage
> collected (do you need finalization, do you need ref counting) which is
> independent of whether you are a supplement. So I don't think there is a one
> size fits all that you can put on HeapSupplement.

Hmm, supplementing objects shouldn't be refcounted, so the only difference is
whether they want to be finalized or not. I'm not quite sure if it's worth
making the separation just for the finalization. As far as I consider how other
garbage-collected hierarchies are implemented, I think the most straightforward
implementation of HeapSupplement would be like this:

  class HeapSupplement : public GarbageCollectedFinalized<HeapSupplement> {
    virtual trace(Visitor*) = 0;
  };

  class X : public HeapSupplement<X> {
    virtual trace(Visitor*) { }
  };

What do you think?

[Mads Ager (chromium), 2014-02-27 07:24:06]

On 2014/02/27 07:13:36, haraken wrote:
> > > I don't fully understand why a class X needs to inherit both
> GarbageCollected
> > > and HeapSupplement, because HeapSupplement already implies that the object
> is
> > a
> > > supplementing object on the heap.
> > 
> > I think we need to separate them out. There are different ways of being
> garbage
> > collected (do you need finalization, do you need ref counting) which is
> > independent of whether you are a supplement. So I don't think there is a one
> > size fits all that you can put on HeapSupplement.
> 
> Hmm, supplementing objects shouldn't be refcounted, so the only difference is
> whether they want to be finalized or not. I'm not quite sure if it's worth
> making the separation just for the finalization. As far as I consider how
other
> garbage-collected hierarchies are implemented, I think the most
straightforward
> implementation of HeapSupplement would be like this:
> 
>   class HeapSupplement : public GarbageCollectedFinalized<HeapSupplement> {
>     virtual trace(Visitor*) = 0;
>   };
> 
>   class X : public HeapSupplement<X> {
>     virtual trace(Visitor*) { }
>   };
> 
> What do you think?

If the Supplement classes are always simple enough I think that makes sense. I
thought that some of the Supplement objects were complicated with many super
classes. I was wrong. That is the Supplementable objects. :-) If we can make
HeapSupplement imply GarbageCollectedFinalized that would probably be a good
idea. Thanks Haraken.

[sof, 2014-02-27 07:30:18]

On 2014/02/27 07:13:36, haraken wrote:
> > > I don't fully understand why a class X needs to inherit both
> GarbageCollected
> > > and HeapSupplement, because HeapSupplement already implies that the object
> is
> > a
> > > supplementing object on the heap.
> > 
> > I think we need to separate them out. There are different ways of being
> garbage
> > collected (do you need finalization, do you need ref counting) which is
> > independent of whether you are a supplement. So I don't think there is a one
> > size fits all that you can put on HeapSupplement.
> 
> Hmm, supplementing objects shouldn't be refcounted, so the only difference is
> whether they want to be finalized or not. I'm not quite sure if it's worth
> making the separation just for the finalization. As far as I consider how
other
> garbage-collected hierarchies are implemented, I think the most
straightforward
> implementation of HeapSupplement would be like this:
> 
>   class HeapSupplement : public GarbageCollectedFinalized<HeapSupplement> {
>     virtual trace(Visitor*) = 0;
>   };
> 
>   class X : public HeapSupplement<X> {
>     virtual trace(Visitor*) { }
>   };
> 
> What do you think?

(HeapSupplements do have such a trace() pure virtual already; needed to handle
tracing of the supplementable map.)

I considered having HeapSupplement derive from something GCed, but it runs the
risk of conflicting with e.g., supplements that could do something like

  class X : public RefCountedWillBeRefCountedGarbageCollected<X>, public
EventTarget, public HeapSupplement<X> {..};

[Mads Ager (chromium), 2014-02-27 07:32:05]

On 2014/02/27 07:24:06, Mads Ager (chromium) wrote:
> On 2014/02/27 07:13:36, haraken wrote:
> > > > I don't fully understand why a class X needs to inherit both
> > GarbageCollected
> > > > and HeapSupplement, because HeapSupplement already implies that the
object
> > is
> > > a
> > > > supplementing object on the heap.
> > > 
> > > I think we need to separate them out. There are different ways of being
> > garbage
> > > collected (do you need finalization, do you need ref counting) which is
> > > independent of whether you are a supplement. So I don't think there is a
one
> > > size fits all that you can put on HeapSupplement.
> > 
> > Hmm, supplementing objects shouldn't be refcounted, so the only difference
is
> > whether they want to be finalized or not. I'm not quite sure if it's worth
> > making the separation just for the finalization. As far as I consider how
> other
> > garbage-collected hierarchies are implemented, I think the most
> straightforward
> > implementation of HeapSupplement would be like this:
> > 
> >   class HeapSupplement : public GarbageCollectedFinalized<HeapSupplement> {
> >     virtual trace(Visitor*) = 0;
> >   };
> > 
> >   class X : public HeapSupplement<X> {
> >     virtual trace(Visitor*) { }
> >   };
> > 
> > What do you think?
> 
> If the Supplement classes are always simple enough I think that makes sense. I
> thought that some of the Supplement objects were complicated with many super
> classes. I was wrong. That is the Supplementable objects. :-) If we can make
> HeapSupplement imply GarbageCollectedFinalized that would probably be a good
> idea. Thanks Haraken.

Hmm, I don't think that will work. We have objects that are supplements for
multiple classes.

class LocalFileSystem FINAL : public Supplement<Page>, public
Supplement<WorkerClients>

We cannot inherit GarbageCollected twice and the inheritance has to be the
left-most super type. Let's keep them separate.

[haraken, 2014-02-27 07:33:24]

> (HeapSupplements do have such a trace() pure virtual already; needed to handle
> tracing of the supplementable map.)
> 
> I considered having HeapSupplement derive from something GCed, but it runs the
> risk of conflicting with e.g., supplements that could do something like
> 
>   class X : public RefCountedWillBeRefCountedGarbageCollected<X>, public
> EventTarget, public HeapSupplement<X> {..};

I'm not sure if it happens (Is it happening anywhere?). In design, Supplement
objects are just supplementing the Supplementable object (Navigator, DOMWindow,
etc), so I think that it shouldn't happen that Supplement objects want to become
RefCounted. (I might be wrong.)

[haraken, 2014-02-27 07:34:23]

On 2014/02/27 07:32:05, Mads Ager (chromium) wrote:
> On 2014/02/27 07:24:06, Mads Ager (chromium) wrote:
> > On 2014/02/27 07:13:36, haraken wrote:
> > > > > I don't fully understand why a class X needs to inherit both
> > > GarbageCollected
> > > > > and HeapSupplement, because HeapSupplement already implies that the
> object
> > > is
> > > > a
> > > > > supplementing object on the heap.
> > > > 
> > > > I think we need to separate them out. There are different ways of being
> > > garbage
> > > > collected (do you need finalization, do you need ref counting) which is
> > > > independent of whether you are a supplement. So I don't think there is a
> one
> > > > size fits all that you can put on HeapSupplement.
> > > 
> > > Hmm, supplementing objects shouldn't be refcounted, so the only difference
> is
> > > whether they want to be finalized or not. I'm not quite sure if it's worth
> > > making the separation just for the finalization. As far as I consider how
> > other
> > > garbage-collected hierarchies are implemented, I think the most
> > straightforward
> > > implementation of HeapSupplement would be like this:
> > > 
> > >   class HeapSupplement : public GarbageCollectedFinalized<HeapSupplement>
{
> > >     virtual trace(Visitor*) = 0;
> > >   };
> > > 
> > >   class X : public HeapSupplement<X> {
> > >     virtual trace(Visitor*) { }
> > >   };
> > > 
> > > What do you think?
> > 
> > If the Supplement classes are always simple enough I think that makes sense.
I
> > thought that some of the Supplement objects were complicated with many super
> > classes. I was wrong. That is the Supplementable objects. :-) If we can make
> > HeapSupplement imply GarbageCollectedFinalized that would probably be a good
> > idea. Thanks Haraken.
> 
> Hmm, I don't think that will work. We have objects that are supplements for
> multiple classes.
> 
> class LocalFileSystem FINAL : public Supplement<Page>, public
> Supplement<WorkerClients>
> 
> We cannot inherit GarbageCollected twice and the inheritance has to be the
> left-most super type. Let's keep them separate.

ah, this is good point. Agreed with your original idea.

[sof, 2014-02-27 07:35:51]

On 2014/02/27 07:33:24, haraken wrote:
> > (HeapSupplements do have such a trace() pure virtual already; needed to
handle
> > tracing of the supplementable map.)
> > 
> > I considered having HeapSupplement derive from something GCed, but it runs
the
> > risk of conflicting with e.g., supplements that could do something like
> > 
> >   class X : public RefCountedWillBeRefCountedGarbageCollected<X>, public
> > EventTarget, public HeapSupplement<X> {..};
> 
> I'm not sure if it happens (Is it happening anywhere?). In design, Supplement
> objects are just supplementing the Supplementable object (Navigator,
DOMWindow,
> etc), so I think that it shouldn't happen that Supplement objects want to
become
> RefCounted. (I might be wrong.)

But if it does happen you're kind of stuck, so building it in seemed
counterproductive in that regard.

[kouhei (in TOK), 2014-02-27 07:36:57]

On 2014/02/27 07:34:23, haraken wrote:
> On 2014/02/27 07:32:05, Mads Ager (chromium) wrote:
> > On 2014/02/27 07:24:06, Mads Ager (chromium) wrote:
> > > On 2014/02/27 07:13:36, haraken wrote:
> > > > > > I don't fully understand why a class X needs to inherit both
> > > > GarbageCollected
> > > > > > and HeapSupplement, because HeapSupplement already implies that the
> > object
> > > > is
> > > > > a
> > > > > > supplementing object on the heap.
> > > > > 
> > > > > I think we need to separate them out. There are different ways of
being
> > > > garbage
> > > > > collected (do you need finalization, do you need ref counting) which
is
> > > > > independent of whether you are a supplement. So I don't think there is
a
> > one
> > > > > size fits all that you can put on HeapSupplement.
> > > > 
> > > > Hmm, supplementing objects shouldn't be refcounted, so the only
difference
> > is
> > > > whether they want to be finalized or not. I'm not quite sure if it's
worth
> > > > making the separation just for the finalization. As far as I consider
how
> > > other
> > > > garbage-collected hierarchies are implemented, I think the most
> > > straightforward
> > > > implementation of HeapSupplement would be like this:
> > > > 
> > > >   class HeapSupplement : public
GarbageCollectedFinalized<HeapSupplement>
> {
> > > >     virtual trace(Visitor*) = 0;
> > > >   };
> > > > 
> > > >   class X : public HeapSupplement<X> {
> > > >     virtual trace(Visitor*) { }
> > > >   };
> > > > 
> > > > What do you think?
> > > 
> > > If the Supplement classes are always simple enough I think that makes
sense.
> I
> > > thought that some of the Supplement objects were complicated with many
super
> > > classes. I was wrong. That is the Supplementable objects. :-) If we can
make
> > > HeapSupplement imply GarbageCollectedFinalized that would probably be a
good
> > > idea. Thanks Haraken.
> > 
> > Hmm, I don't think that will work. We have objects that are supplements for
> > multiple classes.
> > 
> > class LocalFileSystem FINAL : public Supplement<Page>, public
> > Supplement<WorkerClients>
> > 
> > We cannot inherit GarbageCollected twice and the inheritance has to be the
> > left-most super type. Let's keep them separate.
> 
> ah, this is good point. Agreed with your original idea.

Can we have HeapSupplement inherit GarbageCollectedMixin here? Although its not
yet landed...
It seems like we are trying to accomplish a same thing. Support multiple
inheritance.

...Or we can have class X : public virtual HeapSupplementBase in haraken's
pseudo code.

[Mads Ager (chromium), 2014-02-27 07:46:57]

On 2014/02/27 07:36:57, kouhei wrote:
> On 2014/02/27 07:34:23, haraken wrote:
> > On 2014/02/27 07:32:05, Mads Ager (chromium) wrote:
> > > On 2014/02/27 07:24:06, Mads Ager (chromium) wrote:
> > > > On 2014/02/27 07:13:36, haraken wrote:
> > > > > > > I don't fully understand why a class X needs to inherit both
> > > > > GarbageCollected
> > > > > > > and HeapSupplement, because HeapSupplement already implies that
the
> > > object
> > > > > is
> > > > > > a
> > > > > > > supplementing object on the heap.
> > > > > > 
> > > > > > I think we need to separate them out. There are different ways of
> being
> > > > > garbage
> > > > > > collected (do you need finalization, do you need ref counting) which
> is
> > > > > > independent of whether you are a supplement. So I don't think there
is
> a
> > > one
> > > > > > size fits all that you can put on HeapSupplement.
> > > > > 
> > > > > Hmm, supplementing objects shouldn't be refcounted, so the only
> difference
> > > is
> > > > > whether they want to be finalized or not. I'm not quite sure if it's
> worth
> > > > > making the separation just for the finalization. As far as I consider
> how
> > > > other
> > > > > garbage-collected hierarchies are implemented, I think the most
> > > > straightforward
> > > > > implementation of HeapSupplement would be like this:
> > > > > 
> > > > >   class HeapSupplement : public
> GarbageCollectedFinalized<HeapSupplement>
> > {
> > > > >     virtual trace(Visitor*) = 0;
> > > > >   };
> > > > > 
> > > > >   class X : public HeapSupplement<X> {
> > > > >     virtual trace(Visitor*) { }
> > > > >   };
> > > > > 
> > > > > What do you think?
> > > > 
> > > > If the Supplement classes are always simple enough I think that makes
> sense.
> > I
> > > > thought that some of the Supplement objects were complicated with many
> super
> > > > classes. I was wrong. That is the Supplementable objects. :-) If we can
> make
> > > > HeapSupplement imply GarbageCollectedFinalized that would probably be a
> good
> > > > idea. Thanks Haraken.
> > > 
> > > Hmm, I don't think that will work. We have objects that are supplements
for
> > > multiple classes.
> > > 
> > > class LocalFileSystem FINAL : public Supplement<Page>, public
> > > Supplement<WorkerClients>
> > > 
> > > We cannot inherit GarbageCollected twice and the inheritance has to be the
> > > left-most super type. Let's keep them separate.
> > 
> > ah, this is good point. Agreed with your original idea.
> 
> Can we have HeapSupplement inherit GarbageCollectedMixin here? Although its
not
> yet landed...
> It seems like we are trying to accomplish a same thing. Support multiple
> inheritance.
> 
> ...Or we can have class X : public virtual HeapSupplementBase in haraken's
> pseudo code.

Not only can we inherit from GarbageCollectedMixin, we *must*! Otherwise, we
will not get the right start of object address for marking! We need the adjust
and mark pattern here because we have a pointer into the middle of the object
when we have a HeapSupplement<T>* in a Member.

[haraken, 2014-02-27 08:39:48]

https://codereview.chromium.org/178663004/diff/130001/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/178663004/diff/130001/Source/heap/Handle.h#ne...
Source/heap/Handle.h:565: #define WillBeHeapSupplementable
WebCore::HeapSupplementable
On 2014/02/27 02:46:01, haraken wrote:
> 
> Nit: I'd prefer "GarbageCollectedSupplementable" and
> "GarbageCollectedSupplement", since they are GarbageCollected classes. I'll
> delegate the decision to you.
> 
> The underlying issue is that we mix "GarbageCollected" and "Heap" in various
> names.

Nit: Now I agree with "HeapSupplement" since it doesn't inherit GarbageCollected
:)

[sof, 2014-02-27 08:50:36]

https://codereview.chromium.org/178663004/diff/130001/Source/core/workers/Wor...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/178663004/diff/130001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.cpp:59: class CloseWorkerGlobalScopeTask :
public ExecutionContextTask {
On 2014/02/27 02:46:01, haraken wrote:
> 
> Now that we call WorkerGlobalScope::detach() when shutting down the worker
> thread, would it be possible to remove CloseWorkerGlobalScopeTask ?
> 
> BTW, before your CL, why did we need to clear WorkerGlobalScope asynchronously
> using CloseWorkerGlobalScopeTask? Is it OK to replace the asynchronous
clean-up
> with the synchronous clean-up in your CL? (In your CL, the clean-up is done
> synchronously by calling WorkerGlobalScope::detach().)

This is to handled script-initiated closing (via close()), which isn't an abrupt
operation. I don't see a significant overlap with a final detach operation; not
touching that logic & code here.

https://codereview.chromium.org/178663004/diff/130001/Source/core/workers/Wor...
File Source/core/workers/WorkerGlobalScope.h (left):

https://codereview.chromium.org/178663004/diff/130001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.h:159: HashSet<Observer*>
m_workerObservers;
On 2014/02/27 02:46:01, haraken wrote:
> 
> oh, this is unused :)

Removal handled by tkent, http://codereview.chromium.org/181663004

https://codereview.chromium.org/178663004/diff/130001/Source/core/workers/Wor...
File Source/core/workers/WorkerSupplementable.h (right):

https://codereview.chromium.org/178663004/diff/130001/Source/core/workers/Wor...
Source/core/workers/WorkerSupplementable.h:38: typedef
WillBeHeapSupplement<WorkerGlobalScope> WorkerSupplement;
On 2014/02/27 02:46:01, haraken wrote:
> 
> Let's remove this file and directly use
> WillBeHeapSupplementable<WorkerGlobalScope>. These typedefs are not that
helpful
> and just makes underlying data structure obscure.

Done (but with a faint plea not to extend CLs with tidyups.)

https://codereview.chromium.org/178663004/diff/130001/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/178663004/diff/130001/Source/heap/Handle.h#ne...
Source/heap/Handle.h:565: #define WillBeHeapSupplementable
WebCore::HeapSupplementable
On 2014/02/27 08:39:50, haraken wrote:
> On 2014/02/27 02:46:01, haraken wrote:
> > 
> > Nit: I'd prefer "GarbageCollectedSupplementable" and
> > "GarbageCollectedSupplement", since they are GarbageCollected classes. I'll
> > delegate the decision to you.
> > 
> > The underlying issue is that we mix "GarbageCollected" and "Heap" in various
> > names.
> 
> Nit: Now I agree with "HeapSupplement" since it doesn't inherit
GarbageCollected
> :)

Nooo... I've just gone through the pains of renaming. I'll revert that.

https://codereview.chromium.org/178663004/diff/130001/Source/modules/notifica...
File Source/modules/notifications/WorkerGlobalScopeNotifications.h (right):

https://codereview.chromium.org/178663004/diff/130001/Source/modules/notifica...
Source/modules/notifications/WorkerGlobalScopeNotifications.h:51: explicit
WorkerGlobalScopeNotifications(RawPtr<WorkerGlobalScope>);
On 2014/02/27 02:46:01, haraken wrote:
> 
> Can we just use a raw pointer?

Done.

[haraken, 2014-02-27 09:07:18]

LGTM. I'm sorry that I've confused you a couple of times in this reviewing.

https://codereview.chromium.org/178663004/diff/130001/Source/core/workers/Wor...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/178663004/diff/130001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.cpp:59: class CloseWorkerGlobalScopeTask :
public ExecutionContextTask {
On 2014/02/27 08:50:37, sof wrote:
> On 2014/02/27 02:46:01, haraken wrote:
> > 
> > Now that we call WorkerGlobalScope::detach() when shutting down the worker
> > thread, would it be possible to remove CloseWorkerGlobalScopeTask ?
> > 
> > BTW, before your CL, why did we need to clear WorkerGlobalScope
asynchronously
> > using CloseWorkerGlobalScopeTask? Is it OK to replace the asynchronous
> clean-up
> > with the synchronous clean-up in your CL? (In your CL, the clean-up is done
> > synchronously by calling WorkerGlobalScope::detach().)
> 
> This is to handled script-initiated closing (via close()), which isn't an
abrupt
> operation. I don't see a significant overlap with a final detach operation;
not
> touching that logic & code here.

Thanks for the clarification. Keeping the CloseWorkerGlobalScopeTask  won't
cause any conflict with the detach operation.

https://codereview.chromium.org/178663004/diff/130001/Source/heap/Handle.h
File Source/heap/Handle.h (right):

https://codereview.chromium.org/178663004/diff/130001/Source/heap/Handle.h#ne...
Source/heap/Handle.h:565: #define WillBeHeapSupplementable
WebCore::HeapSupplementable
On 2014/02/27 08:50:37, sof wrote:
> On 2014/02/27 08:39:50, haraken wrote:
> > On 2014/02/27 02:46:01, haraken wrote:
> > > 
> > > Nit: I'd prefer "GarbageCollectedSupplementable" and
> > > "GarbageCollectedSupplement", since they are GarbageCollected classes.
I'll
> > > delegate the decision to you.
> > > 
> > > The underlying issue is that we mix "GarbageCollected" and "Heap" in
various
> > > names.
> > 
> > Nit: Now I agree with "HeapSupplement" since it doesn't inherit
> GarbageCollected
> > :)
> 
> Nooo... I've just gone through the pains of renaming. I'll revert that.

I AM Sorry!

[Mads Ager (chromium), 2014-02-27 09:07:58]

I think the only outstanding issue here is the marking of supplement objects.
Two options:

1. specialize the trace trait for HeapSupplement<T> to call adjustAndMark and
add adjustAndMark to everything that is a HeapSupplement.

2. wait for Kouhei's change and make HeapSupplement<T> inherit from
GarbageCollectedMixin and mark everything that is a HeapSupplement with the
GARBAGE_COLLECTED_MIXIN macro. 

I'm a bit surprised that we haven't seen any issues with this. Are there no
tests that end up tracing Supplement objects?

[haraken, 2014-02-27 09:14:57]

> I'm a bit surprised that we haven't seen any issues with this. Are there no
> tests that end up tracing Supplement objects?

This is the first CL that used Members in Supplement objects. Before this CL, we
were using Persistents to avoid tracing Supplement objects.

[sof, 2014-02-27 15:27:20]

+tkent +jochen.

Source/platform/Supplementable.h has changed a bit here, could platform owners
have a look? thanks.

[jochen (gone - plz use gerrit), 2014-02-27 15:43:57]

+abarth who wrote most of the supplement stuff iirc

[sof, 2014-02-27 15:47:31]

On 2014/02/27 09:07:58, Mads Ager (chromium) wrote:
> I think the only outstanding issue here is the marking of supplement objects.
> Two options:
> 
> 1. specialize the trace trait for HeapSupplement<T> to call adjustAndMark and
> add adjustAndMark to everything that is a HeapSupplement.
> 
> 2. wait for Kouhei's change and make HeapSupplement<T> inherit from
> GarbageCollectedMixin and mark everything that is a HeapSupplement with the
> GARBAGE_COLLECTED_MIXIN macro. 
> 
> I'm a bit surprised that we haven't seen any issues with this. Are there no
> tests that end up tracing Supplement objects?

With ager's guidance, went for 2. Seems to work well (and yes, no tests really
triggering this -- added one now.) Please take a look.

Please note that since Events are now GCed, another lifetime issue arose when
picking up that change. Upon sweeping out detached WorkerGlobalScopes, Events
dtors may end up destructing their EventTarget, which will cause an execution
context's thread to be accessed (case in point: ~ActiveDOMObject()).
Consequently, the latest patchset leaves the thread set upon detaching the
WorkerGlobalScope. We know that the thread is alive beyond that final GC sweep,
so this ought to be safe. It was the tidiest solution I could come up with, but
perhaps there are better ways?

[haraken, 2014-02-27 23:35:23]

Let me ask a couple of clarifying questions.

> Please note that since Events are now GCed, another lifetime issue arose when
> picking up that change. Upon sweeping out detached WorkerGlobalScopes, Events
> dtors may end up destructing their EventTarget, which will cause an execution
> context's thread to be accessed (case in point: ~ActiveDOMObject()).

In what path does a Event destructor get called when sweeping the
WorkerGlobalScope?

> Consequently, the latest patchset leaves the thread set upon detaching the
> WorkerGlobalScope. We know that the thread is alive beyond that final GC
sweep,
> so this ought to be safe. It was the tidiest solution I could come up with,
but
> perhaps there are better ways?

If I'm understanding correctly, this solution will be OK. I think it's
reasonable to assume that the final GC runs before the thread dies. As long as
the final GC cleans up everything in the thread heap, that's completely fine.

From your separate email:
> just in case it isn't known, the assert that triggers with current trunk on
multiple tests is addressed by https://codereview.chromium.org/178663004/ ,
where WorkerThread gets a persistent handle to the WorkerGlobalScope instead.

Would you elaborate on how this CL fixes the assertion?

[sof, 2014-02-28 06:17:42]

On 2014/02/27 23:35:23, haraken wrote:
> Let me ask a couple of clarifying questions.
> 
> > Please note that since Events are now GCed, another lifetime issue arose
when
> > picking up that change. Upon sweeping out detached WorkerGlobalScopes,
Events
> > dtors may end up destructing their EventTarget, which will cause an
execution
> > context's thread to be accessed (case in point: ~ActiveDOMObject()).
> 
> In what path does a Event destructor get called when sweeping the
> WorkerGlobalScope?
> 

Here's one stack trace where the WorkerGlobalScope's thread is touched on
sweeping:

#0  WebCore::ActiveDOMObject::~ActiveDOMObject (this=0x31659fe88f10,
__in_chrg=<optimized out>) at
../../third_party/WebKit/Source/core/dom/ActiveDOMObject.cpp:44
#1  0x00000000050ff8e5 in WebCore::MessagePort::~MessagePort
(this=0x31659fe88f10, __in_chrg=<optimized out>) at
../../third_party/WebKit/Source/core/dom/MessagePort.cpp:62
#2  0x00000000050ff930 in WebCore::MessagePort::~MessagePort
(this=0x31659fe88f10, __in_chrg=<optimized out>) at
../../third_party/WebKit/Source/core/dom/MessagePort.cpp:65
#3  0x000000000233ad62 in WTF::RefCounted<WebCore::MessagePort>::deref
(this=0x31659fe88f2c) at ../../third_party/WebKit/Source/wtf/RefCounted.h:181
#4  0x0000000005100dda in WebCore::MessagePort::derefEventTarget
(this=0x31659fe88f10) at
../../third_party/WebKit/Source/core/dom/MessagePort.h:64
#5  0x00000000013a2b1d in WebCore::EventTarget::deref (this=0x31659fe88f40) at
../../third_party/WebKit/Source/core/events/EventTarget.h:101
#6  0x00000000014b020e in WTF::derefIfNotNull<WebCore::EventTarget>
(ptr=0x31659fe88f40) at ../../third_party/WebKit/Source/wtf/PassRefPtr.h:57
#7  0x000000000159bd55 in WTF::RefPtr<WebCore::EventTarget>::~RefPtr
(this=0x7fffdb2a1bf0, __in_chrg=<optimized out>) at
../../third_party/WebKit/Source/wtf/RefPtr.h:54
#8  0x000000000159b524 in WebCore::Event::~Event (this=0x7fffdb2a1bb0,
__in_chrg=<optimized out>) at
../../third_party/WebKit/Source/core/events/Event.cpp:87
#9  0x000000000510a156 in WebCore::MessageEvent::~MessageEvent
(this=0x7fffdb2a1bb0, __in_chrg=<optimized out>) at
../../third_party/WebKit/Source/core/events/MessageEvent.cpp:133
(More stack frames follow...)


> > Consequently, the latest patchset leaves the thread set upon detaching the
> > WorkerGlobalScope. We know that the thread is alive beyond that final GC
> sweep,
> > so this ought to be safe. It was the tidiest solution I could come up with,
> but
> > perhaps there are better ways?
> 
> If I'm understanding correctly, this solution will be OK. I think it's
> reasonable to assume that the final GC runs before the thread dies. As long as
> the final GC cleans up everything in the thread heap, that's completely fine.
> 
> From your separate email:
> > just in case it isn't known, the assert that triggers with current trunk on
> multiple tests is addressed by https://codereview.chromium.org/178663004/ ,
> where WorkerThread gets a persistent handle to the WorkerGlobalScope instead.
> 
> Would you elaborate on how this CL fixes the assertion?

The WorkerThread now keeps a Persistent to the WorkerGlobalScope, so the assert
that a ref count of 1 holds on shutdown isn't done. Well, it is done, but later
-- the heap has to be empty at the end.

With the RefCountedGarbageCollected changes on EventTarget, trunk now leaves the
ref count at 2 as the persistent inside the RefCountedGarbageCollected wrapper
upped the count by one.

[haraken, 2014-02-28 07:20:18]

On 2014/02/28 06:17:42, sof wrote:
> On 2014/02/27 23:35:23, haraken wrote:
> > Let me ask a couple of clarifying questions.
> > 
> > > Please note that since Events are now GCed, another lifetime issue arose
> when
> > > picking up that change. Upon sweeping out detached WorkerGlobalScopes,
> Events
> > > dtors may end up destructing their EventTarget, which will cause an
> execution
> > > context's thread to be accessed (case in point: ~ActiveDOMObject()).
> > 
> > In what path does a Event destructor get called when sweeping the
> > WorkerGlobalScope?
> > 
> 
> Here's one stack trace where the WorkerGlobalScope's thread is touched on
> sweeping:
> 
> #0  WebCore::ActiveDOMObject::~ActiveDOMObject (this=0x31659fe88f10,
> __in_chrg=<optimized out>) at
> ../../third_party/WebKit/Source/core/dom/ActiveDOMObject.cpp:44
> #1  0x00000000050ff8e5 in WebCore::MessagePort::~MessagePort
> (this=0x31659fe88f10, __in_chrg=<optimized out>) at
> ../../third_party/WebKit/Source/core/dom/MessagePort.cpp:62
> #2  0x00000000050ff930 in WebCore::MessagePort::~MessagePort
> (this=0x31659fe88f10, __in_chrg=<optimized out>) at
> ../../third_party/WebKit/Source/core/dom/MessagePort.cpp:65
> #3  0x000000000233ad62 in WTF::RefCounted<WebCore::MessagePort>::deref
> (this=0x31659fe88f2c) at ../../third_party/WebKit/Source/wtf/RefCounted.h:181
> #4  0x0000000005100dda in WebCore::MessagePort::derefEventTarget
> (this=0x31659fe88f10) at
> ../../third_party/WebKit/Source/core/dom/MessagePort.h:64
> #5  0x00000000013a2b1d in WebCore::EventTarget::deref (this=0x31659fe88f40) at
> ../../third_party/WebKit/Source/core/events/EventTarget.h:101
> #6  0x00000000014b020e in WTF::derefIfNotNull<WebCore::EventTarget>
> (ptr=0x31659fe88f40) at ../../third_party/WebKit/Source/wtf/PassRefPtr.h:57
> #7  0x000000000159bd55 in WTF::RefPtr<WebCore::EventTarget>::~RefPtr
> (this=0x7fffdb2a1bf0, __in_chrg=<optimized out>) at
> ../../third_party/WebKit/Source/wtf/RefPtr.h:54
> #8  0x000000000159b524 in WebCore::Event::~Event (this=0x7fffdb2a1bb0,
> __in_chrg=<optimized out>) at
> ../../third_party/WebKit/Source/core/events/Event.cpp:87
> #9  0x000000000510a156 in WebCore::MessageEvent::~MessageEvent
> (this=0x7fffdb2a1bb0, __in_chrg=<optimized out>) at
> ../../third_party/WebKit/Source/core/events/MessageEvent.cpp:133
> (More stack frames follow...)
> 
> 
> > > Consequently, the latest patchset leaves the thread set upon detaching the
> > > WorkerGlobalScope. We know that the thread is alive beyond that final GC
> > sweep,
> > > so this ought to be safe. It was the tidiest solution I could come up
with,
> > but
> > > perhaps there are better ways?
> > 
> > If I'm understanding correctly, this solution will be OK. I think it's
> > reasonable to assume that the final GC runs before the thread dies. As long
as
> > the final GC cleans up everything in the thread heap, that's completely
fine.

ah, got it. I think this is a problem.

So the problem is:

(1) WorkerThread and WorkerGlobalScope die together?
(2) ~WorkerGlobalScope() calls ~EventTarget() because WorkerGlobalScope is an
EventTarget.
(3) ~EventTarget() touches its execution context, which touches the
WorkerThread.

Am I understanding correctly?


> > From your separate email:
> > > just in case it isn't known, the assert that triggers with current trunk
on
> > multiple tests is addressed by https://codereview.chromium.org/178663004/ ,
> > where WorkerThread gets a persistent handle to the WorkerGlobalScope
instead.
> > 
> > Would you elaborate on how this CL fixes the assertion?
> 
> The WorkerThread now keeps a Persistent to the WorkerGlobalScope, so the
assert
> that a ref count of 1 holds on shutdown isn't done. Well, it is done, but
later
> -- the heap has to be empty at the end.
> 
> With the RefCountedGarbageCollected changes on EventTarget, trunk now leaves
the
> ref count at 2 as the persistent inside the RefCountedGarbageCollected wrapper
> upped the count by one.

I see, thanks.

[sof, 2014-02-28 08:21:58]

On 2014/02/28 07:20:18, haraken wrote:
...
> 
> ah, got it. I think this is a problem.
> 
> So the problem is:
> 
> (1) WorkerThread and WorkerGlobalScope die together?

WorkerThread clears its heap ("cleans up", to use its terminology), which
contains WorkerGlobalScope (and a lot more). It then shuts down.

> (2) ~WorkerGlobalScope() calls ~EventTarget() because WorkerGlobalScope is an
> EventTarget.

When it is finalized, yes. There are no assumed ordering about when finalizers
are run during sweeping.

> (3) ~EventTarget() touches its execution context, which touches the
> WorkerThread.
> 

If an EventTarget ends up being finalized/destructed before the
WorkerGlobalScope is, yes. If not, there will not be an execution context.

[jochen (gone - plz use gerrit), 2014-02-28 08:26:04]

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

https://codereview.chromium.org/178663004/diff/130002/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:141:
ASSERT(m_workerGlobalScope->hasOneRef());
note that this ASSERT() does not hold. The ref count at this point is sometimes
one and sometimes two.

Layout tests just happen to not trigger this assert.

No idea who holds the second ref, I suspect it's a threadable loader.

[sof, 2014-02-28 08:32:51]

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

https://codereview.chromium.org/178663004/diff/130002/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:141:
ASSERT(m_workerGlobalScope->hasOneRef());
On 2014/02/28 08:26:06, jochen (OOO from March 1) wrote:
> note that this ASSERT() does not hold. The ref count at this point is
sometimes
> one and sometimes two.
> 
> Layout tests just happen to not trigger this assert.
> 
> No idea who holds the second ref, I suspect it's a threadable loader.

I see. Has it proven useful to catch out bugs in the past?

[jochen (gone - plz use gerrit), 2014-02-28 08:35:45]

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

https://codereview.chromium.org/178663004/diff/130002/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:141:
ASSERT(m_workerGlobalScope->hasOneRef());
On 2014/02/28 08:32:52, sof wrote:
> On 2014/02/28 08:26:06, jochen (OOO from March 1) wrote:
> > note that this ASSERT() does not hold. The ref count at this point is
> sometimes
> > one and sometimes two.
> > 
> > Layout tests just happen to not trigger this assert.
> > 
> > No idea who holds the second ref, I suspect it's a threadable loader.
> 
> I see. Has it proven useful to catch out bugs in the past?

It clearly indicates that there's a bug. Nobody caught it yet.

[sof, 2014-02-28 08:39:23]

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

https://codereview.chromium.org/178663004/diff/130002/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:141:
ASSERT(m_workerGlobalScope->hasOneRef());
On 2014/02/28 08:35:46, jochen (OOO from March 1) wrote:
> On 2014/02/28 08:32:52, sof wrote:
> > On 2014/02/28 08:26:06, jochen (OOO from March 1) wrote:
> > > note that this ASSERT() does not hold. The ref count at this point is
> > sometimes
> > > one and sometimes two.
> > > 
> > > Layout tests just happen to not trigger this assert.
> > > 
> > > No idea who holds the second ref, I suspect it's a threadable loader.
> > 
> > I see. Has it proven useful to catch out bugs in the past?
> 
> It clearly indicates that there's a bug. Nobody caught it yet.

Is there a bug on it already?

[jochen (gone - plz use gerrit), 2014-02-28 09:33:28]

On 2014/02/28 08:39:23, sof wrote:
>
https://codereview.chromium.org/178663004/diff/130002/Source/core/workers/Wor...
> File Source/core/workers/WorkerThread.cpp (right):
> 
>
https://codereview.chromium.org/178663004/diff/130002/Source/core/workers/Wor...
> Source/core/workers/WorkerThread.cpp:141:
> ASSERT(m_workerGlobalScope->hasOneRef());
> On 2014/02/28 08:35:46, jochen (OOO from March 1) wrote:
> > On 2014/02/28 08:32:52, sof wrote:
> > > On 2014/02/28 08:26:06, jochen (OOO from March 1) wrote:
> > > > note that this ASSERT() does not hold. The ref count at this point is
> > > sometimes
> > > > one and sometimes two.
> > > > 
> > > > Layout tests just happen to not trigger this assert.
> > > > 
> > > > No idea who holds the second ref, I suspect it's a threadable loader.
> > > 
> > > I see. Has it proven useful to catch out bugs in the past?
> > 
> > It clearly indicates that there's a bug. Nobody caught it yet.
> 
> Is there a bug on it already?

nope, let me file one

[jochen (gone - plz use gerrit), 2014-02-28 09:38:09]

filed crbug.com/347928

[sof, 2014-02-28 09:41:33]

On 2014/02/28 09:38:09, jochen (OOO from March 1) wrote:
> filed crbug.com/347928

Thank you, will look into what it hides. Orthogonally to this.

[sof, 2014-02-28 11:33:29]

abarth: Supplementable.h changes - possible to add this to your long ToDo list?
Thanks.

[abarth-chromium, 2014-03-01 07:53:09]

platform/ LGTM

That code is pretty ugly.  :(

https://codereview.chromium.org/178663004/diff/200001/Source/platform/DEPS
File Source/platform/DEPS (right):

https://codereview.chromium.org/178663004/diff/200001/Source/platform/DEPS#ne...
Source/platform/DEPS:9: "+heap",
Sigh.  I had hoped to avoid this dep but its ok.

[sof, 2014-03-01 08:13:37]

On 2014/03/01 07:53:09, abarth wrote:
> platform/ LGTM
> 
> That code is pretty ugly.  :(
> 

Thank you; should the codebase reach the point where all Supplement(able)s are
heap allocated, we can drop the transition templates added.

(I want to make progress, but will wait until Monday lest there is breakage.)

[abarth-chromium, 2014-03-01 08:28:25]

Yeah, I understand that the transition is a big ugly.

[abarth-chromium, 2014-03-01 08:28:46]

On 2014/03/01 08:28:25, abarth wrote:
> Yeah, I understand that the transition is a big ugly.

s/big/bit/

[haraken, 2014-03-03 01:49:32]

sof: Do we still have any on-going issue we should discuss in this CL? Or is the
CL pretty ready for landing? (Then I'll take a final look.)

[sof, 2014-03-03 06:08:41]

On 2014/03/03 01:49:32, haraken wrote:
> sof: Do we still have any on-going issue we should discuss in this CL? Or is
the
> CL pretty ready for landing? (Then I'll take a final look.)

I'm not aware of any other issues, so that'd be great. CL passes local tests +
has been built with the Blink GC plugin enabled.

[haraken, 2014-03-03 06:51:10]

On 2014/03/03 06:08:41, sof wrote:
> On 2014/03/03 01:49:32, haraken wrote:
> > sof: Do we still have any on-going issue we should discuss in this CL? Or is
> the
> > CL pretty ready for landing? (Then I'll take a final look.)
> 
> I'm not aware of any other issues, so that'd be great. CL passes local tests +
> has been built with the Blink GC plugin enabled.

Sounds great.

One concern is that this CL will hide an existing bug about
m_workerGlobalScope->hasOneRef(). When I moved Events to the heap, I hit a lot
of flaky assertions about m_workerGlobalScope->hasOneRef().
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...

As far as I investigate, I noticed that WorkerGlobalScope's wrapper is retaining
the second reference. I don't yet know who is retaining the WorkerGlobalScope's
wrapper, but the point is that we have a bug that WorkerGlobalScope's wrapper is
still alive at the point where WorkerThread is going to be detached (This bug is
already happening in non-oilpan builds:
https://code.google.com/p/chromium/issues/detail?id=347928).

[sof, 2014-03-03 07:03:55]

On 2014/03/03 06:51:10, haraken wrote:
> On 2014/03/03 06:08:41, sof wrote:
> > On 2014/03/03 01:49:32, haraken wrote:
> > > sof: Do we still have any on-going issue we should discuss in this CL? Or
is
> > the
> > > CL pretty ready for landing? (Then I'll take a final look.)
> > 
> > I'm not aware of any other issues, so that'd be great. CL passes local tests
+
> > has been built with the Blink GC plugin enabled.
> 
> Sounds great.
> 
> One concern is that this CL will hide an existing bug about
> m_workerGlobalScope->hasOneRef(). When I moved Events to the heap, I hit a lot
> of flaky assertions about m_workerGlobalScope->hasOneRef().
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%2520Linux%2520Oil...
> 

I don't think they were flaky per se, but due to the delayed sweeping of Events
this will happen most times.

> As far as I investigate, I noticed that WorkerGlobalScope's wrapper is
retaining
> the second reference. I don't yet know who is retaining the
WorkerGlobalScope's
> wrapper, but the point is that we have a bug that WorkerGlobalScope's wrapper
is
> still alive at the point where WorkerThread is going to be detached (This bug
is
> already happening in non-oilpan builds:
> https://code.google.com/p/chromium/issues/detail?id=347928).

As reported by jochen above.

In any case, this isn't (or won't be) an Oilpan-specific issue.

[Mads Ager (chromium), 2014-03-03 07:15:23]

The Supplement changes now look solid. Thanks for adding the test.

LGTM

[sof, 2014-03-03 12:41:00]

haraken: once revert https://codereview.chromium.org/185393006/ lands, I'd like
to rebase & commit this one, so as to make progress. Would that be ok?

[haraken, 2014-03-03 12:55:49]

Double-checked. LGTM.

https://codereview.chromium.org/178663004/diff/220001/Source/core/workers/Wor...
File Source/core/workers/WorkerGlobalScope.h (right):

https://codereview.chromium.org/178663004/diff/220001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.h:82: void detach();

(I'm sorry I suggested "detach" but) we normally call this "dispose".
"attach"/"detach" is used for attaching/detaching renderers to/from Nodes. So
would you rename to "dispose"?

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

https://codereview.chromium.org/178663004/diff/220001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:151: ThreadState::current()->cleanup();

One question:

In the non-oilpan world, it was guaranteed that the worker thread is the last
guy who de-references m_workerGlobalScope. In other words, it was guaranteed
that 'm_workerGlobalScope = nullptr' destroys WorkerGlobalScope. Thus it was OK
to clean up the worker thread after calling 'm_workerGlobalScope = nullptr'.

On the other hand, in the oilpan world, it is not guaranteed that
'm_workerGlobalScope = nullptr' destroys WorkerGlobalScope. It's possible that
WorkerGlobalScope's wrapper is still alive at this point. I don't know how it
happens but it actually happened when I moved Events to the heap. Do you think
this is an expected behavior, or a bug we should pursue?

https://codereview.chromium.org/178663004/diff/220001/Source/heap/ThreadState.h
File Source/heap/ThreadState.h (right):

https://codereview.chromium.org/178663004/diff/220001/Source/heap/ThreadState...
Source/heap/ThreadState.h:554:
USED_FROM_MULTIPLE_THREADS(DedicatedWorkerGlobalScope);

Now you can remove USED_FROM_MULTIPLE_THREADS. AnyThread is made the default for
non-Node objects.

https://codereview.chromium.org/178663004/diff/220001/Source/modules/imagebit...
File Source/modules/imagebitmap/ImageBitmapFactories.h (right):

https://codereview.chromium.org/178663004/diff/220001/Source/modules/imagebit...
Source/modules/imagebitmap/ImageBitmapFactories.h:129: virtual void
trace(Visitor*);

Add OVERRIDE.

https://codereview.chromium.org/178663004/diff/220001/Source/modules/indexedd...
File Source/modules/indexeddb/WorkerGlobalScopeIndexedDatabase.cpp (right):

https://codereview.chromium.org/178663004/diff/220001/Source/modules/indexedd...
Source/modules/indexeddb/WorkerGlobalScopeIndexedDatabase.cpp:75: void
WorkerGlobalScopeIndexedDatabase::trace(Visitor* visitor)

Drop |visitor|.

[haraken, 2014-03-03 12:58:08]

On 2014/03/03 12:41:00, sof wrote:
> haraken: once revert https://codereview.chromium.org/185393006/ lands, I'd
like
> to rebase & commit this one, so as to make progress. Would that be ok?

https://codereview.chromium.org/185393006/ was landed, but I need to revert
https://codereview.chromium.org/181153003/ too. I'm making the revert CL, so
would you wait for a while? I'll hurry.

[sof, 2014-03-03 13:13:57]

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

https://codereview.chromium.org/178663004/diff/220001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:151: ThreadState::current()->cleanup();
On 2014/03/03 12:55:50, haraken wrote:
> 
> One question:
> 
> In the non-oilpan world, it was guaranteed that the worker thread is the last
> guy who de-references m_workerGlobalScope. In other words, it was guaranteed
> that 'm_workerGlobalScope = nullptr' destroys WorkerGlobalScope. Thus it was
OK
> to clean up the worker thread after calling 'm_workerGlobalScope = nullptr'.
> 
> On the other hand, in the oilpan world, it is not guaranteed that
> 'm_workerGlobalScope = nullptr' destroys WorkerGlobalScope. It's possible that
> WorkerGlobalScope's wrapper is still alive at this point. I don't know how it
> happens but it actually happened when I moved Events to the heap. Do you think
> this is an expected behavior, or a bug we should pursue?

We do assert for an empty heap at the end of shutting down, so if whoever
referred to the wrapper wasn't in the heap, it'll trigger. Does that address
your concern a bit re: making a potential bug harder to detect?

[haraken, 2014-03-03 13:18:20]

> > One question:
> > 
> > In the non-oilpan world, it was guaranteed that the worker thread is the
last
> > guy who de-references m_workerGlobalScope. In other words, it was guaranteed
> > that 'm_workerGlobalScope = nullptr' destroys WorkerGlobalScope. Thus it was
> OK
> > to clean up the worker thread after calling 'm_workerGlobalScope = nullptr'.
> > 
> > On the other hand, in the oilpan world, it is not guaranteed that
> > 'm_workerGlobalScope = nullptr' destroys WorkerGlobalScope. It's possible
that
> > WorkerGlobalScope's wrapper is still alive at this point. I don't know how
it
> > happens but it actually happened when I moved Events to the heap. Do you
think
> > this is an expected behavior, or a bug we should pursue?
> 
> We do assert for an empty heap at the end of shutting down, so if whoever
> referred to the wrapper wasn't in the heap, it'll trigger. Does that address
> your concern a bit re: making a potential bug harder to detect?

Good point, thanks for the clarification. Yes, the empty assert will hit if
WorkerGlobalScope's wrapper is kept alive.

LGTM.

[haraken, 2014-03-03 13:19:15]

Feel free to reland your CL once https://codereview.chromium.org/185463005/
lands. It will move Event back to the peaceful, RefCounted world.

[sof, 2014-03-04 08:17:59]

https://codereview.chromium.org/178663004/diff/220001/Source/core/workers/Wor...
File Source/core/workers/WorkerGlobalScope.h (right):

https://codereview.chromium.org/178663004/diff/220001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.h:82: void detach();
On 2014/03/03 12:55:50, haraken wrote:
> 
> (I'm sorry I suggested "detach" but) we normally call this "dispose".
> "attach"/"detach" is used for attaching/detaching renderers to/from Nodes. So
> would you rename to "dispose"?

dispose() it is.

https://codereview.chromium.org/178663004/diff/220001/Source/modules/imagebit...
File Source/modules/imagebitmap/ImageBitmapFactories.h (right):

https://codereview.chromium.org/178663004/diff/220001/Source/modules/imagebit...
Source/modules/imagebitmap/ImageBitmapFactories.h:129: virtual void
trace(Visitor*);
On 2014/03/03 12:55:50, haraken wrote:
> 
> Add OVERRIDE.

That wouldn't be quite right, this is a/the base GC class. Kept as is.

https://codereview.chromium.org/178663004/diff/220001/Source/modules/indexedd...
File Source/modules/indexeddb/WorkerGlobalScopeIndexedDatabase.cpp (right):

https://codereview.chromium.org/178663004/diff/220001/Source/modules/indexedd...
Source/modules/indexeddb/WorkerGlobalScopeIndexedDatabase.cpp:75: void
WorkerGlobalScopeIndexedDatabase::trace(Visitor* visitor)
On 2014/03/03 12:55:50, haraken wrote:
> 
> Drop |visitor|.

Done.

[sof, 2014-03-04 08:22:49]

On 2014/03/03 13:19:15, haraken wrote:
> Feel free to reland your CL once https://codereview.chromium.org/185463005/
> lands. It will move Event back to the peaceful, RefCounted world.

Thanks for getting that landed; we're now ready here.

[sof, 2014-03-04 08:23:06]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-03-04 08:25:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/178663004/240001

[commit-bot: I haz the power, 2014-03-04 12:54:33]

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

[commit-bot: I haz the power, 2014-03-04 12:54:34]

Retried try job too often on linux_blink for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[sof, 2014-03-04 12:55:57]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-03-04 12:56:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/178663004/240001

[commit-bot: I haz the power, 2014-03-04 16:42:31]

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

[commit-bot: I haz the power, 2014-03-04 16:42:32]

Retried try job too often on linux_blink for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[sof, 2014-03-04 16:46:05]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-03-04 16:46:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/178663004/240001

[commit-bot: I haz the power, 2014-03-05 08:59:34]

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

[commit-bot: I haz the power, 2014-03-05 08:59:36]

Retried try job too often on linux_blink for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[sof, 2014-03-05 09:42:33]

On 2014/03/05 08:59:36, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_blink for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

This is showing up an occasional WorkerGlobalScope detach problem in one worker
test (with enable_oilpan=0 only) that had gone undetected; addressing.

[sof, 2014-03-05 12:06:23]

On 2014/03/05 09:42:33, sof wrote:
> On 2014/03/05 08:59:36, I haz the power (commit-bot) wrote:
> > Retried try job too often on linux_blink for step(s) webkit_tests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
> 
> This is showing up an occasional WorkerGlobalScope detach problem in one
worker
> test (with enable_oilpan=0 only) that had gone undetected; addressing.

The two-stage dispose + deref&delete in non-Oilpan settings of WorkerGlobalScope
from the worker thread were causing problems on explicit terminate() calls on
the Worker; timing dependent, but a worker global scope and its sub objects
could end up being double-freed.

I haven't debugged fully the conditions that are at play, but it seems like a
side issue to what this CL is about. Hence, dropped back to previous arrangement
of sync'ly deleting the WorkerGlobalScope. No changes with Oilpan enabled.

Please take a look at WorkerThread.cpp & WorkerGlobalScope.cpp

[haraken, 2014-03-05 14:41:06]

https://codereview.chromium.org/178663004/diff/280001/Source/core/workers/Wor...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/178663004/diff/280001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.cpp:201: // thread is accessed by other
GCed objects being finalized/destructed.

hmm, I think this is a bit hacky. Probably we've discussed this before but let
me confirm one more time:

What is the "other GCed object" that touches WorkerGlobalScope::m_thread? It
sounds wrong to allow destructors of GCed objects to touch
WorkerGlobalScope::m_thread, because both the GCed objects and the
WorkerGlobalScope are on the heap. (i.e., GCed objects should not be allowed to
touch other GCed objects in their destructors.)

[sof, 2014-03-05 14:43:16]

On 2014/03/05 14:41:06, haraken wrote:
>
https://codereview.chromium.org/178663004/diff/280001/Source/core/workers/Wor...
> File Source/core/workers/WorkerGlobalScope.cpp (right):
> 
>
https://codereview.chromium.org/178663004/diff/280001/Source/core/workers/Wor...
> Source/core/workers/WorkerGlobalScope.cpp:201: // thread is accessed by other
> GCed objects being finalized/destructed.
> 
> hmm, I think this is a bit hacky. Probably we've discussed this before but let
> me confirm one more time:
> 
> What is the "other GCed object" that touches WorkerGlobalScope::m_thread? It
> sounds wrong to allow destructors of GCed objects to touch
> WorkerGlobalScope::m_thread, because both the GCed objects and the
> WorkerGlobalScope are on the heap. (i.e., GCed objects should not be allowed
to
> touch other GCed objects in their destructors.)

See https://codereview.chromium.org/178663004/#msg51 for one stack.

[sof, 2014-03-05 14:43:22]

On 2014/03/05 14:41:06, haraken wrote:
>
https://codereview.chromium.org/178663004/diff/280001/Source/core/workers/Wor...
> File Source/core/workers/WorkerGlobalScope.cpp (right):
> 
>
https://codereview.chromium.org/178663004/diff/280001/Source/core/workers/Wor...
> Source/core/workers/WorkerGlobalScope.cpp:201: // thread is accessed by other
> GCed objects being finalized/destructed.
> 
> hmm, I think this is a bit hacky. Probably we've discussed this before but let
> me confirm one more time:
> 
> What is the "other GCed object" that touches WorkerGlobalScope::m_thread? It
> sounds wrong to allow destructors of GCed objects to touch
> WorkerGlobalScope::m_thread, because both the GCed objects and the
> WorkerGlobalScope are on the heap. (i.e., GCed objects should not be allowed
to
> touch other GCed objects in their destructors.)

See https://codereview.chromium.org/178663004/#msg51 for one stack.

[haraken, 2014-03-05 14:59:15]

On 2014/03/05 14:43:22, sof wrote:
> On 2014/03/05 14:41:06, haraken wrote:
> >
>
https://codereview.chromium.org/178663004/diff/280001/Source/core/workers/Wor...
> > File Source/core/workers/WorkerGlobalScope.cpp (right):
> > 
> >
>
https://codereview.chromium.org/178663004/diff/280001/Source/core/workers/Wor...
> > Source/core/workers/WorkerGlobalScope.cpp:201: // thread is accessed by
other
> > GCed objects being finalized/destructed.
> > 
> > hmm, I think this is a bit hacky. Probably we've discussed this before but
let
> > me confirm one more time:
> > 
> > What is the "other GCed object" that touches WorkerGlobalScope::m_thread? It
> > sounds wrong to allow destructors of GCed objects to touch
> > WorkerGlobalScope::m_thread, because both the GCed objects and the
> > WorkerGlobalScope are on the heap. (i.e., GCed objects should not be allowed
> to
> > touch other GCed objects in their destructors.)
> 
> See https://codereview.chromium.org/178663004/#msg51 for one stack.

Thanks!

#0  WebCore::ActiveDOMObject::~ActiveDOMObject (this=0x31659fe88f10,
__in_chrg=<optimized out>) at
../../third_party/WebKit/Source/core/dom/ActiveDOMObject.cpp:44
#1  0x00000000050ff8e5 in WebCore::MessagePort::~MessagePort
(this=0x31659fe88f10, __in_chrg=<optimized out>) at
../../third_party/WebKit/Source/core/dom/MessagePort.cpp:62
#2  0x00000000050ff930 in WebCore::MessagePort::~MessagePort
(this=0x31659fe88f10, __in_chrg=<optimized out>) at
../../third_party/WebKit/Source/core/dom/MessagePort.cpp:65
#3  0x000000000233ad62 in WTF::RefCounted<WebCore::MessagePort>::deref
(this=0x31659fe88f2c) at ../../third_party/WebKit/Source/wtf/RefCounted.h:181
#4  0x0000000005100dda in WebCore::MessagePort::derefEventTarget
(this=0x31659fe88f10) at
../../third_party/WebKit/Source/core/dom/MessagePort.h:64
#5  0x00000000013a2b1d in WebCore::EventTarget::deref (this=0x31659fe88f40) at
../../third_party/WebKit/Source/core/events/EventTarget.h:101
#6  0x00000000014b020e in WTF::derefIfNotNull<WebCore::EventTarget>
(ptr=0x31659fe88f40) at ../../third_party/WebKit/Source/wtf/PassRefPtr.h:57
#7  0x000000000159bd55 in WTF::RefPtr<WebCore::EventTarget>::~RefPtr
(this=0x7fffdb2a1bf0, __in_chrg=<optimized out>) at
../../third_party/WebKit/Source/wtf/RefPtr.h:54
#8  0x000000000159b524 in WebCore::Event::~Event (this=0x7fffdb2a1bb0,
__in_chrg=<optimized out>) at
../../third_party/WebKit/Source/core/events/Event.cpp:87
#9  0x000000000510a156 in WebCore::MessageEvent::~MessageEvent
(this=0x7fffdb2a1bb0, __in_chrg=<optimized out>) at
../../third_party/WebKit/Source/core/events/MessageEvent.cpp:133
(More stack frames follow...)

Sorry but would you give me the "more stack frames"? I'm curious whether this
stack trace is inside a destructor of some on-heap object or not.

[sof, 2014-03-05 15:13:54]

On 2014/03/05 14:59:15, haraken wrote:
> On 2014/03/05 14:43:22, sof wrote:
> > On 2014/03/05 14:41:06, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/178663004/diff/280001/Source/core/workers/Wor...
> > > File Source/core/workers/WorkerGlobalScope.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/178663004/diff/280001/Source/core/workers/Wor...
> > > Source/core/workers/WorkerGlobalScope.cpp:201: // thread is accessed by
> other
> > > GCed objects being finalized/destructed.
> > > 
> > > hmm, I think this is a bit hacky. Probably we've discussed this before but
> let
> > > me confirm one more time:
> > > 
> > > What is the "other GCed object" that touches WorkerGlobalScope::m_thread?
It
> > > sounds wrong to allow destructors of GCed objects to touch
> > > WorkerGlobalScope::m_thread, because both the GCed objects and the
> > > WorkerGlobalScope are on the heap. (i.e., GCed objects should not be
allowed
> > to
> > > touch other GCed objects in their destructors.)
> > 
> > See https://codereview.chromium.org/178663004/#msg51 for one stack.
> 
> Thanks!
> 
> #0  WebCore::ActiveDOMObject::~ActiveDOMObject (this=0x31659fe88f10,
> __in_chrg=<optimized out>) at
> ../../third_party/WebKit/Source/core/dom/ActiveDOMObject.cpp:44
> #1  0x00000000050ff8e5 in WebCore::MessagePort::~MessagePort
> (this=0x31659fe88f10, __in_chrg=<optimized out>) at
> ../../third_party/WebKit/Source/core/dom/MessagePort.cpp:62
> #2  0x00000000050ff930 in WebCore::MessagePort::~MessagePort
> (this=0x31659fe88f10, __in_chrg=<optimized out>) at
> ../../third_party/WebKit/Source/core/dom/MessagePort.cpp:65
> #3  0x000000000233ad62 in WTF::RefCounted<WebCore::MessagePort>::deref
> (this=0x31659fe88f2c) at ../../third_party/WebKit/Source/wtf/RefCounted.h:181
> #4  0x0000000005100dda in WebCore::MessagePort::derefEventTarget
> (this=0x31659fe88f10) at
> ../../third_party/WebKit/Source/core/dom/MessagePort.h:64
> #5  0x00000000013a2b1d in WebCore::EventTarget::deref (this=0x31659fe88f40) at
> ../../third_party/WebKit/Source/core/events/EventTarget.h:101
> #6  0x00000000014b020e in WTF::derefIfNotNull<WebCore::EventTarget>
> (ptr=0x31659fe88f40) at ../../third_party/WebKit/Source/wtf/PassRefPtr.h:57
> #7  0x000000000159bd55 in WTF::RefPtr<WebCore::EventTarget>::~RefPtr
> (this=0x7fffdb2a1bf0, __in_chrg=<optimized out>) at
> ../../third_party/WebKit/Source/wtf/RefPtr.h:54
> #8  0x000000000159b524 in WebCore::Event::~Event (this=0x7fffdb2a1bb0,
> __in_chrg=<optimized out>) at
> ../../third_party/WebKit/Source/core/events/Event.cpp:87
> #9  0x000000000510a156 in WebCore::MessageEvent::~MessageEvent
> (this=0x7fffdb2a1bb0, __in_chrg=<optimized out>) at
> ../../third_party/WebKit/Source/core/events/MessageEvent.cpp:133
> (More stack frames follow...)
> 
> Sorry but would you give me the "more stack frames"? I'm curious whether this
> stack trace is inside a destructor of some on-heap object or not.

I don't have that gdb session available any longer, I'm afraid. But it was
a MessageEvent being finalized via its wrapper, I'm quite sure.

Events aren't, of course, presently on the heap, but that doesn't change the
issue. Fundamentally you cannot control finalization order of GCed
objects -- if an object happens to be finalized before the WorkerGlobalScope
is and the context on the lifecycle observer object is a WorkerGlobalScope,
it will be able to access it & its thread. If the WorkerGlobalScope is
finalized, it will be removed as a lifecycle context -- objects finalized
later will clearly not touch the dead object.

And the reason why we're disposing explicitly is covered some here

  https://codereview.chromium.org/178663004/#msg6
  https://codereview.chromium.org/178663004/#msg13

(and a few more; we've been around this issue a bit already! :) )

[sof, 2014-03-05 16:11:19]

With the modules/notifications/ removal (r168471) having been
reverted/reinstated (r168486), the rebase here will need to re-add the changes
made there.

Or wait until that removal sticks.

[haraken, 2014-03-06 00:53:49]

> Events aren't, of course, presently on the heap, but that doesn't change the
> issue. Fundamentally you cannot control finalization order of GCed
> objects -- if an object happens to be finalized before the WorkerGlobalScope
> is and the context on the lifecycle observer object is a WorkerGlobalScope,
> it will be able to access it & its thread. If the WorkerGlobalScope is
> finalized, it will be removed as a lifecycle context -- objects finalized
> later will clearly not touch the dead object.
> 
> And the reason why we're disposing explicitly is covered some here
> 
>   https://codereview.chromium.org/178663004/#msg6
>   https://codereview.chromium.org/178663004/#msg13
> 
> (and a few more; we've been around this issue a bit already! :) )

Thanks for the clarification!

Does that mean that we should delay finalizing WorkerGlobalScope just like we
did for SVGImage's destructor?
https://codereview.chromium.org/185613003/

[sof, 2014-03-06 06:25:28]

On 2014/03/06 00:53:49, haraken wrote:
> > Events aren't, of course, presently on the heap, but that doesn't change the
> > issue. Fundamentally you cannot control finalization order of GCed
> > objects -- if an object happens to be finalized before the WorkerGlobalScope
> > is and the context on the lifecycle observer object is a WorkerGlobalScope,
> > it will be able to access it & its thread. If the WorkerGlobalScope is
> > finalized, it will be removed as a lifecycle context -- objects finalized
> > later will clearly not touch the dead object.
> > 
> > And the reason why we're disposing explicitly is covered some here
> > 
> >   https://codereview.chromium.org/178663004/#msg6
> >   https://codereview.chromium.org/178663004/#msg13
> > 
> > (and a few more; we've been around this issue a bit already! :) )
> 
> Thanks for the clarification!
> 
> Does that mean that we should delay finalizing WorkerGlobalScope just like we
> did for SVGImage's destructor?
> https://codereview.chromium.org/185613003/

i.e., delayed because that destructor performs heap allocation.

You could perhaps make that work here by having WorkerGlobalScope essentially
live outside the heap, as it (and all it refers to) has to be disposed of and
finalized here. Would that lead somewhere simpler/better?

But, again, I'm not altering the termination steps for Oilpan in this latest
change. Just reverting the non-Oilpan steps back to previous.

[sof, 2014-03-07 10:34:22]

Rebased to pick up http://crrev.com/187483002 (feel free to Cc: or ping me on
reviews that touch closely related code :) )

Also, reinstated the modules/notifications/ changes.

Are we good to go?

[haraken, 2014-03-10 01:27:11]

> > Does that mean that we should delay finalizing WorkerGlobalScope just like
we
> > did for SVGImage's destructor?
> > https://codereview.chromium.org/185613003/
> 
> i.e., delayed because that destructor performs heap allocation.

I'm a bit confused. In your latest CL, how is the WorkerGlobalScope's destructor
delayed?

> You could perhaps make that work here by having WorkerGlobalScope essentially
> live outside the heap, as it (and all it refers to) has to be disposed of and
> finalized here. Would that lead somewhere simpler/better?

I don't fully understand your idea, but I think we somehow need to (1) put
WorkerGlobalScope on the heap and (2) make sure that the WorkerGlobalScope is
destructed at the very end of the worker thread.

> Rebased to pick up http://crrev.com/187483002 (feel free to Cc: or ping me on
> reviews that touch closely related code :) )

oh, sorry. Could you subscribe oilpan-reviews@chromium.org? :)

[sof, 2014-03-10 06:38:22]

On 2014/03/10 01:27:11, haraken wrote:
> > > Does that mean that we should delay finalizing WorkerGlobalScope just like
> we
> > > did for SVGImage's destructor?
> > > https://codereview.chromium.org/185613003/
> > 
> > i.e., delayed because that destructor performs heap allocation.
> 
> I'm a bit confused. In your latest CL, how is the WorkerGlobalScope's
destructor
> delayed?
> 

I'm a bit confused as well :) The above just restated what the CL you pointed to
was doing. I've not taken on that suggestion, sorry if inefficiency was created
& it lead you think otherwise. I don't think it is suited or needed here.

> > You could perhaps make that work here by having WorkerGlobalScope
essentially
> > live outside the heap, as it (and all it refers to) has to be disposed of
and
> > finalized here. Would that lead somewhere simpler/better?
> 
> I don't fully understand your idea, but I think we somehow need to (1) put
> WorkerGlobalScope on the heap and (2) make sure that the WorkerGlobalScope is
> destructed at the very end of the worker thread.
>

2 is at odds with 1, unless there is a way to exempt an object (and all it
points to) from being swept. (I'm assuming not doing another GC.)

I would argue what's currently there goes far enough in providing the above, we
explicit dispose() the WorkerGlobalScope, releasing most resources, letting the
object be later on finalized and destructed during GC. And so on, I won't rehash
what has been said already above.

[haraken, 2014-03-10 07:10:01]

https://codereview.chromium.org/178663004/diff/300001/Source/core/workers/Wor...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/178663004/diff/300001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.cpp:201: // thread is accessed by other
GCed objects being finalized/destructed.

Thanks sof for the clarification! Now I think this CL is almost ready to go.

Let me ask a final question: Why is it OK to not clear m_thread in
WorkerGlobalScope::dispose()? It looks strange that WorkerThread is disposing
WorkerGlobalScope but WorkerGlobalScope does not (intentionally) clear the raw
pointer to the WorkerThread. I do understand why you cannot clear m_thread, but
it looks strange that you cannot clear m_thread.

[Mads Ager (chromium), 2014-03-10 08:51:11]

https://codereview.chromium.org/178663004/diff/300001/Source/core/workers/Wor...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/178663004/diff/300001/Source/core/workers/Wor...
Source/core/workers/WorkerGlobalScope.cpp:201: // thread is accessed by other
GCed objects being finalized/destructed.
On 2014/03/10 07:10:03, haraken wrote:
> 
> Thanks sof for the clarification! Now I think this CL is almost ready to go.
> 
> Let me ask a final question: Why is it OK to not clear m_thread in
> WorkerGlobalScope::dispose()? It looks strange that WorkerThread is disposing
> WorkerGlobalScope but WorkerGlobalScope does not (intentionally) clear the raw
> pointer to the WorkerThread. I do understand why you cannot clear m_thread,
but
> it looks strange that you cannot clear m_thread.

Is the issue here that lifecycle observers will access this object until the
destructor has been called? If that is the only issue this is working as it
should and the comment should be updated to state that m_thread can be accessed
by context life cycle observers until the WorkerGlobalScope is destructed.

The other option, which might be clearer, would be to make it possible to notify
the observers that the context is gone at this point by explicitly clearing the
observers in the lifecycle notifier. Long term, the context observers should
directly use our weak pointers instead, but we need to deal with this manually
for now until we get Document into the heap as well.

I think it would be fine to land this with the comment updated if the lifecycle
observers are the only ones accessing m_thread.

[haraken, 2014-03-10 09:03:30]

Discussed offline, LGTM. Let's add a FIXME and then land this CL.

[haraken, 2014-03-10 09:06:09]

On 2014/03/10 09:03:30, haraken wrote:
> Discussed offline, LGTM. Let's add a FIXME and then land this CL.

Forgot to mention that thanks for being persistent on this super-complicated
stuff :)

[sof, 2014-03-10 10:33:49]

On 2014/03/10 09:03:30, haraken wrote:
> Discussed offline, LGTM. Let's add a FIXME and then land this CL.

Done; thanks everyone, let's give this one a go.

[sof, 2014-03-10 10:34:02]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-03-10 10:34:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/178663004/310001

[commit-bot: I haz the power, 2014-03-10 11:10:26]

Change committed as 168830