Oilpan: eagerly finalize WebGLRenderingContext objects.

Oilpan: eagerly finalize WebGLRenderingContext objects.

To simplify the destruction of WebGLRenderingContextBase-derived objects
along with their WebGLContextObjects, insist that the rendering context
is finalized first. It may then detach itself from the context objects,
before their finalizers are later run.

Such a finalization ordering avoids the need for the ref-counted
WebGLSharedWebGraphics3D abstraction which supported arbitrary
finalization orders.

R=haraken
BUG=482838
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196437

[sof, 2015-05-21 21:03:18]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-05-21 21:03:18]

please take a look.

[haraken, 2015-05-22 01:09:48]

Thanks for working on this!

It seems I need some more time to understand the issue here. In the bug, you're
saying:

> i.e., WebGLContextGroup is a shared, ref-counted object which associates
WebGLSharedObjects and their WebGLRenderingContextBase(s). Both parties are
responsible for mutually unregistering themselves from each other and the
WebGLContextGroup. Doing so will entail touching to-be-swept objects, but
assuming the manual unregistration protocol is correct, this is safe.

I'm not sure if the manual unregistration protocol is correct with lazy
sweeping.

WebGLContextGroup (which is off-heap) has a set of raw pointers to
WebGLSharedObjects (which is on-heap). The raw pointer is unregistered from the
WebGLContextGroup in WebGLSharedObject's destructor
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
This means that if we enable the lazy sweeping, the WebGLSharedObject's
destructor is delayed and thus the WebGLContextGroup can end up with holding raw
pointers to will-be-swept objects.

This is the same problem as LifecycleObserver and DOMWindowProperty. Maybe we
need to use eager sweeping for WebGLObjects or use a pre-finalizer (if the
nesting is not an issue here)?

[haraken, 2015-05-22 01:10:48]

On 2015/05/22 01:09:48, haraken wrote:
> Thanks for working on this!
> 
> It seems I need some more time to understand the issue here. In the bug,
you're
> saying:
> 
> > i.e., WebGLContextGroup is a shared, ref-counted object which associates
> WebGLSharedObjects and their WebGLRenderingContextBase(s). Both parties are
> responsible for mutually unregistering themselves from each other and the
> WebGLContextGroup. Doing so will entail touching to-be-swept objects, but
> assuming the manual unregistration protocol is correct, this is safe.
> 
> I'm not sure if the manual unregistration protocol is correct with lazy
> sweeping.
> 
> WebGLContextGroup (which is off-heap) has a set of raw pointers to
> WebGLSharedObjects (which is on-heap). The raw pointer is unregistered from
the
> WebGLContextGroup in WebGLSharedObject's destructor
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> This means that if we enable the lazy sweeping, the WebGLSharedObject's
> destructor is delayed and thus the WebGLContextGroup can end up with holding
raw
> pointers to will-be-swept objects.
> 
> This is the same problem as LifecycleObserver and DOMWindowProperty. Maybe we
> need to use eager sweeping for WebGLObjects or use a pre-finalizer (if the
> nesting is not an issue here)?

So my guess is that the error detected by the ASan poisoning is a real error
that needs to be fixed.

[sof, 2015-05-22 05:10:47]

On 2015/05/22 01:10:48, haraken wrote:
> On 2015/05/22 01:09:48, haraken wrote:
> > Thanks for working on this!
> > 
> > It seems I need some more time to understand the issue here. In the bug,
> you're
> > saying:
> > 
> > > i.e., WebGLContextGroup is a shared, ref-counted object which associates
> > WebGLSharedObjects and their WebGLRenderingContextBase(s). Both parties are
> > responsible for mutually unregistering themselves from each other and the
> > WebGLContextGroup. Doing so will entail touching to-be-swept objects, but
> > assuming the manual unregistration protocol is correct, this is safe.
> > 
> > I'm not sure if the manual unregistration protocol is correct with lazy
> > sweeping.
> > 
> > WebGLContextGroup (which is off-heap) has a set of raw pointers to
> > WebGLSharedObjects (which is on-heap). The raw pointer is unregistered from
> the
> > WebGLContextGroup in WebGLSharedObject's destructor
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> > This means that if we enable the lazy sweeping, the WebGLSharedObject's
> > destructor is delayed and thus the WebGLContextGroup can end up with holding
> raw
> > pointers to will-be-swept objects.
> > 
> > This is the same problem as LifecycleObserver and DOMWindowProperty. Maybe
we
> > need to use eager sweeping for WebGLObjects or use a pre-finalizer (if the
> > nesting is not an issue here)?
> 
> So my guess is that the error detected by the ASan poisoning is a real error
> that needs to be fixed.

It is not a bug, could you go through the WebGL review and consider details of
why we have this mutual unregistration scheme?

I don't have the time any longer to go through & re-explain it here. Things
being the way they are atm.

[haraken, 2015-05-26 05:15:32]

(Will take a look at this within today.)

[haraken, 2015-05-26 11:23:09]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-05-26 11:23:10]

Sorry about delayed review.

Now I understand how the destruction of the WebGL objects are handled :) We
might want to have more comments in the code base though.

https://codereview.chromium.org/1151163002/diff/1/Source/core/html/canvas/Web...
File Source/core/html/canvas/WebGLContextGroup.cpp (right):

https://codereview.chromium.org/1151163002/diff/1/Source/core/html/canvas/Web...
Source/core/html/canvas/WebGLContextGroup.cpp:84:
HashSet<WebGLRenderingContextBase*>::iterator it = m_contexts.begin();

I'd prefer explicitly pass in the WebGLRenderingContextBase to be
poisoned/unpoisoned instead of looking up the first WebGLRenderingContextBase
from the m_contexts.

https://codereview.chromium.org/1151163002/diff/1/Source/core/html/canvas/Web...
Source/core/html/canvas/WebGLContextGroup.cpp:86: if
(!Heap::willObjectBeLazilySwept(context))

Can we replace this with Heap::isHeapObjectAlive()?

poisonContext must not be called for WebGLRenderingContextBases that have been
already finalized, because they should have been already removed from
m_contexts. Thus poisonContext can be called for either of:

(a) a live WebGLRenderingContextBase
(b) a dead WebGLRenderingContextBase that is not yet finalized

For (a), Heap::willObjectBeLazilySwept returns false. For (b),
Heap::willObjectBeLazilySwept returns true. I think you can get the same result
by calling Heap::isHeapObjectAlive(). Given that Heap::willObjectBeLazilySwept
is a super hacky API, I'd prefer using Heap::isHeapObjectAlive().

https://codereview.chromium.org/1151163002/diff/1/Source/core/html/canvas/Web...
Source/core/html/canvas/WebGLContextGroup.cpp:98: if
(!Heap::willObjectBeLazilySwept(object))

Ditto. I'd prefer using Heap::isHeapObjectAlive().

https://codereview.chromium.org/1151163002/diff/1/Source/core/html/canvas/Web...
File Source/core/html/canvas/WebGLSharedObject.cpp (right):

https://codereview.chromium.org/1151163002/diff/1/Source/core/html/canvas/Web...
Source/core/html/canvas/WebGLSharedObject.cpp:51: deleteObject(nullptr);

This deleteObject retrieves a WebGraphicsContext3D of the first
WebGLRenderingContextBase (i.e., m_contexts.begin() in
WebGLContextGroup::getAWebGraphicsContext3D()) and deletes an resource
underlying the WebGLRenderingContextBase. Why is it enough to delete only the
resource of the first WebGLRenderingContextBase? Why don't we need to delete
resources of all other WebGLRenderingContextBases in
WebGLContextGroup::m_contexts?

...while I'm writing this, I found the answer -- at the point where deleteObject
is called, m_context.size() must be 1 and thus it is enough to delete the
resource of the first object.

Maybe we want to have a comment and an ASSERT to check that m_contexts.size() is
1 when we are in WebGLObject::deleteObject().

[sof, 2015-05-26 11:29:47]

https://codereview.chromium.org/1151163002/diff/1/Source/core/html/canvas/Web...
File Source/core/html/canvas/WebGLContextGroup.cpp (right):

https://codereview.chromium.org/1151163002/diff/1/Source/core/html/canvas/Web...
Source/core/html/canvas/WebGLContextGroup.cpp:86: if
(!Heap::willObjectBeLazilySwept(context))
On 2015/05/26 11:23:10, haraken wrote:
> 
> Can we replace this with Heap::isHeapObjectAlive()?
> 
> poisonContext must not be called for WebGLRenderingContextBases that have been
> already finalized, because they should have been already removed from
> m_contexts. Thus poisonContext can be called for either of:
> 
> (a) a live WebGLRenderingContextBase
> (b) a dead WebGLRenderingContextBase that is not yet finalized
> 
> For (a), Heap::willObjectBeLazilySwept returns false. For (b),
> Heap::willObjectBeLazilySwept returns true. I think you can get the same
result
> by calling Heap::isHeapObjectAlive(). Given that Heap::willObjectBeLazilySwept
> is a super hacky API, I'd prefer using Heap::isHeapObjectAlive().

I'm not sure what you refer to..what is Heap::isHeapObjectAlive() ?

[haraken, 2015-05-26 11:36:08]

https://codereview.chromium.org/1151163002/diff/1/Source/core/html/canvas/Web...
File Source/core/html/canvas/WebGLContextGroup.cpp (right):

https://codereview.chromium.org/1151163002/diff/1/Source/core/html/canvas/Web...
Source/core/html/canvas/WebGLContextGroup.cpp:86: if
(!Heap::willObjectBeLazilySwept(context))
On 2015/05/26 11:29:47, sof wrote:
> On 2015/05/26 11:23:10, haraken wrote:
> > 
> > Can we replace this with Heap::isHeapObjectAlive()?
> > 
> > poisonContext must not be called for WebGLRenderingContextBases that have
been
> > already finalized, because they should have been already removed from
> > m_contexts. Thus poisonContext can be called for either of:
> > 
> > (a) a live WebGLRenderingContextBase
> > (b) a dead WebGLRenderingContextBase that is not yet finalized
> > 
> > For (a), Heap::willObjectBeLazilySwept returns false. For (b),
> > Heap::willObjectBeLazilySwept returns true. I think you can get the same
> result
> > by calling Heap::isHeapObjectAlive(). Given that
Heap::willObjectBeLazilySwept
> > is a super hacky API, I'd prefer using Heap::isHeapObjectAlive().
> 
> I'm not sure what you refer to..what is Heap::isHeapObjectAlive() ?

Oh, you're right. isHeapObjectAlive() is available via Visitor... Then it makes
sense to use Heap::willObjectBeLazilySwept().

[haraken, 2015-05-26 11:40:49]

static bool willObjectBeLazilySwept() {
  BasePage* page = pageFromObject(objectPointer);
  if (page->hasBeenSwept())
    return false;
  return !ObjectAliveTrait<T>::isHeapObjectAlive(s_markingVisitor,
const_cast<T*>(objectPointer));
}

BTW, is the 'page->hasBeenSwept()' check needed? Even without having the check,
willObjectBeLazilySwept() will return false for objects in the already-swept
pages, because their marks are already dropped.

[sof, 2015-05-26 11:47:17]

On 2015/05/26 11:40:49, haraken wrote:
> static bool willObjectBeLazilySwept() {
>   BasePage* page = pageFromObject(objectPointer);
>   if (page->hasBeenSwept())
>     return false;
>   return !ObjectAliveTrait<T>::isHeapObjectAlive(s_markingVisitor,
> const_cast<T*>(objectPointer));
> }
> 
> BTW, is the 'page->hasBeenSwept()' check needed? Even without having the
check,
> willObjectBeLazilySwept() will return false for objects in the already-swept
> pages, because their marks are already dropped.

If the (live) object has become unmarked by its page having already been swept,
isHeapObjectAlive() in the above will return false. Making
willObjectBeLazilySwept() return true - that's not what we want, right?

[haraken, 2015-05-26 12:00:31]

On 2015/05/26 11:47:17, sof wrote:
> On 2015/05/26 11:40:49, haraken wrote:
> > static bool willObjectBeLazilySwept() {
> >   BasePage* page = pageFromObject(objectPointer);
> >   if (page->hasBeenSwept())
> >     return false;
> >   return !ObjectAliveTrait<T>::isHeapObjectAlive(s_markingVisitor,
> > const_cast<T*>(objectPointer));
> > }
> > 
> > BTW, is the 'page->hasBeenSwept()' check needed? Even without having the
> check,
> > willObjectBeLazilySwept() will return false for objects in the already-swept
> > pages, because their marks are already dropped.
> 
> If the (live) object has become unmarked by its page having already been
swept,
> isHeapObjectAlive() in the above will return false. Making
> willObjectBeLazilySwept() return true - that's not what we want, right?

Agreed. But doesn't it imply that we have a different problem here?

What happens if willObjectBeLazilySwept() is called for an object that meets the
following condition:

- the object is live.
- the object is in the page we're currently sweeping.
- the object is already unmarked.

willObjectBeLazilySwept() will return true for the object -- that's not what we
want...

[sof, 2015-05-26 12:13:41]

On 2015/05/26 12:00:31, haraken wrote:
> On 2015/05/26 11:47:17, sof wrote:
> > On 2015/05/26 11:40:49, haraken wrote:
> > > static bool willObjectBeLazilySwept() {
> > >   BasePage* page = pageFromObject(objectPointer);
> > >   if (page->hasBeenSwept())
> > >     return false;
> > >   return !ObjectAliveTrait<T>::isHeapObjectAlive(s_markingVisitor,
> > > const_cast<T*>(objectPointer));
> > > }
> > > 
> > > BTW, is the 'page->hasBeenSwept()' check needed? Even without having the
> > check,
> > > willObjectBeLazilySwept() will return false for objects in the
already-swept
> > > pages, because their marks are already dropped.
> > 
> > If the (live) object has become unmarked by its page having already been
> swept,
> > isHeapObjectAlive() in the above will return false. Making
> > willObjectBeLazilySwept() return true - that's not what we want, right?
> 
> Agreed. But doesn't it imply that we have a different problem here?
> 
> What happens if willObjectBeLazilySwept() is called for an object that meets
the
> following condition:
> 
> - the object is live.
> - the object is in the page we're currently sweeping.
> - the object is already unmarked.
> 
> willObjectBeLazilySwept() will return true for the object -- that's not what
we
> want...

Yes, there is a window for mischief if on the same page (=> leaving the live
object as poisoned afterwards.)

Hmm, ugly. It's almost as if you want an ASan poison bit flipping operation on
memory regions.

[haraken, 2015-05-26 12:31:21]

On 2015/05/26 12:13:41, sof wrote:
> On 2015/05/26 12:00:31, haraken wrote:
> > On 2015/05/26 11:47:17, sof wrote:
> > > On 2015/05/26 11:40:49, haraken wrote:
> > > > static bool willObjectBeLazilySwept() {
> > > >   BasePage* page = pageFromObject(objectPointer);
> > > >   if (page->hasBeenSwept())
> > > >     return false;
> > > >   return !ObjectAliveTrait<T>::isHeapObjectAlive(s_markingVisitor,
> > > > const_cast<T*>(objectPointer));
> > > > }
> > > > 
> > > > BTW, is the 'page->hasBeenSwept()' check needed? Even without having the
> > > check,
> > > > willObjectBeLazilySwept() will return false for objects in the
> already-swept
> > > > pages, because their marks are already dropped.
> > > 
> > > If the (live) object has become unmarked by its page having already been
> > swept,
> > > isHeapObjectAlive() in the above will return false. Making
> > > willObjectBeLazilySwept() return true - that's not what we want, right?
> > 
> > Agreed. But doesn't it imply that we have a different problem here?
> > 
> > What happens if willObjectBeLazilySwept() is called for an object that meets
> the
> > following condition:
> > 
> > - the object is live.
> > - the object is in the page we're currently sweeping.
> > - the object is already unmarked.
> > 
> > willObjectBeLazilySwept() will return true for the object -- that's not what
> we
> > want...
> 
> Yes, there is a window for mischief if on the same page (=> leaving the live
> object as poisoned afterwards.)
> 
> Hmm, ugly. It's almost as if you want an ASan poison bit flipping operation on
> memory regions.

Yeah ugly...

And I guess this is causing an issue in Timer::fired as well. Timers are not
fired for the live & unmarked object in the page we're currently sweeping. Hmm,
let me think about it a bit more.

[sof, 2015-05-26 12:35:25]

On 2015/05/26 12:31:21, haraken wrote:
> On 2015/05/26 12:13:41, sof wrote:
> > On 2015/05/26 12:00:31, haraken wrote:
> > > On 2015/05/26 11:47:17, sof wrote:
> > > > On 2015/05/26 11:40:49, haraken wrote:
> > > > > static bool willObjectBeLazilySwept() {
> > > > >   BasePage* page = pageFromObject(objectPointer);
> > > > >   if (page->hasBeenSwept())
> > > > >     return false;
> > > > >   return !ObjectAliveTrait<T>::isHeapObjectAlive(s_markingVisitor,
> > > > > const_cast<T*>(objectPointer));
> > > > > }
> > > > > 
> > > > > BTW, is the 'page->hasBeenSwept()' check needed? Even without having
the
> > > > check,
> > > > > willObjectBeLazilySwept() will return false for objects in the
> > already-swept
> > > > > pages, because their marks are already dropped.
> > > > 
> > > > If the (live) object has become unmarked by its page having already been
> > > swept,
> > > > isHeapObjectAlive() in the above will return false. Making
> > > > willObjectBeLazilySwept() return true - that's not what we want, right?
> > > 
> > > Agreed. But doesn't it imply that we have a different problem here?
> > > 
> > > What happens if willObjectBeLazilySwept() is called for an object that
meets
> > the
> > > following condition:
> > > 
> > > - the object is live.
> > > - the object is in the page we're currently sweeping.
> > > - the object is already unmarked.
> > > 
> > > willObjectBeLazilySwept() will return true for the object -- that's not
what
> > we
> > > want...
> > 
> > Yes, there is a window for mischief if on the same page (=> leaving the live
> > object as poisoned afterwards.)
> > 
> > Hmm, ugly. It's almost as if you want an ASan poison bit flipping operation
on
> > memory regions.
> 
> Yeah ugly...
> 
> And I guess this is causing an issue in Timer::fired as well. Timers are not
> fired for the live & unmarked object in the page we're currently sweeping.
Hmm,
> let me think about it a bit more.

ok. It sounds as if we need some same-page logic in that liveness predicate,
like it or not.

[haraken, 2015-05-26 12:50:12]

https://codereview.chromium.org/1151163002/diff/1/Source/core/html/canvas/Web...
File Source/core/html/canvas/WebGLSharedObject.cpp (right):

https://codereview.chromium.org/1151163002/diff/1/Source/core/html/canvas/Web...
Source/core/html/canvas/WebGLSharedObject.cpp:51: deleteObject(nullptr);
On 2015/05/26 11:23:10, haraken wrote:
> 
> This deleteObject retrieves a WebGraphicsContext3D of the first
> WebGLRenderingContextBase (i.e., m_contexts.begin() in
> WebGLContextGroup::getAWebGraphicsContext3D()) and deletes an resource
> underlying the WebGLRenderingContextBase. Why is it enough to delete only the
> resource of the first WebGLRenderingContextBase? Why don't we need to delete
> resources of all other WebGLRenderingContextBases in
> WebGLContextGroup::m_contexts?
> 
> ...while I'm writing this, I found the answer -- at the point where
deleteObject
> is called, m_context.size() must be 1 and thus it is enough to delete the
> resource of the first object.
> 
> Maybe we want to have a comment and an ASSERT to check that m_contexts.size()
is
> 1 when we are in WebGLObject::deleteObject().

BTW, doesn't this fact imply that the getAWebGraphicsContext3D() that needs
unpoisoning can be called only for the WebGLRenderingContextBase that is being
destructed just right now?

If that is the case:

- We won't need to unpoison the WebGLRenderingContextBase. It should be already
unpoisoned (to call the destructor).

- Maybe we can just mark WebGLRenderingContextBase EAGERLY_FINALIZE? Then it can
just touch WebGLSharedObjects without unpoisoning them.

(I'm not sure.)

[haraken, 2015-05-26 14:57:47]

Also I noticed that Timers won't hit the problem (by accident) because
Timer::fired() should not be dispatched while sweeping one page.

[haraken, 2015-05-26 15:00:26]

On 2015/05/26 14:57:47, haraken wrote:
> Also I noticed that Timers won't hit the problem (by accident) because
> Timer::fired() should not be dispatched while sweeping one page.

Overall, willObjectBeLazilySwept() is a hacky API and we should consider a way
to avoid using it as much as possible. In the particular case of WebGL objects,
if we can avoid the problem by making WebGLRenderingContextBase
EAGERLY_FINALIZE, it sounds better to me. In the particular case of Timers, we
might want to introduce HeapTimer after the timer heap is removed.

[sof, 2015-05-26 18:30:54]

On 2015/05/26 12:50:12, haraken wrote:
>
https://codereview.chromium.org/1151163002/diff/1/Source/core/html/canvas/Web...
> File Source/core/html/canvas/WebGLSharedObject.cpp (right):
> 
>
https://codereview.chromium.org/1151163002/diff/1/Source/core/html/canvas/Web...
> Source/core/html/canvas/WebGLSharedObject.cpp:51: deleteObject(nullptr);
> On 2015/05/26 11:23:10, haraken wrote:
> > 
> > This deleteObject retrieves a WebGraphicsContext3D of the first
> > WebGLRenderingContextBase (i.e., m_contexts.begin() in
> > WebGLContextGroup::getAWebGraphicsContext3D()) and deletes an resource
> > underlying the WebGLRenderingContextBase. Why is it enough to delete only
the
> > resource of the first WebGLRenderingContextBase? Why don't we need to delete
> > resources of all other WebGLRenderingContextBases in
> > WebGLContextGroup::m_contexts?
> > 
> > ...while I'm writing this, I found the answer -- at the point where
> deleteObject
> > is called, m_context.size() must be 1 and thus it is enough to delete the
> > resource of the first object.
> > 
> > Maybe we want to have a comment and an ASSERT to check that
m_contexts.size()
> is
> > 1 when we are in WebGLObject::deleteObject().
> 
> BTW, doesn't this fact imply that the getAWebGraphicsContext3D() that needs
> unpoisoning can be called only for the WebGLRenderingContextBase that is being
> destructed just right now?
> 
> If that is the case:
> 
> - We won't need to unpoison the WebGLRenderingContextBase. It should be
already
> unpoisoned (to call the destructor).
> 
> - Maybe we can just mark WebGLRenderingContextBase EAGERLY_FINALIZE? Then it
can
> just touch WebGLSharedObjects without unpoisoning them.
> 
> (I'm not sure.)

Imposing a finalization order like that would work, once eager finalization
support is available on trunk.

[sof, 2015-06-01 18:43:51]

On 2015/05/26 18:30:54, sof wrote:
> On 2015/05/26 12:50:12, haraken wrote:
> >
>
https://codereview.chromium.org/1151163002/diff/1/Source/core/html/canvas/Web...
> > File Source/core/html/canvas/WebGLSharedObject.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1151163002/diff/1/Source/core/html/canvas/Web...
> > Source/core/html/canvas/WebGLSharedObject.cpp:51: deleteObject(nullptr);
> > On 2015/05/26 11:23:10, haraken wrote:
> > > 
> > > This deleteObject retrieves a WebGraphicsContext3D of the first
> > > WebGLRenderingContextBase (i.e., m_contexts.begin() in
> > > WebGLContextGroup::getAWebGraphicsContext3D()) and deletes an resource
> > > underlying the WebGLRenderingContextBase. Why is it enough to delete only
> the
> > > resource of the first WebGLRenderingContextBase? Why don't we need to
delete
> > > resources of all other WebGLRenderingContextBases in
> > > WebGLContextGroup::m_contexts?
> > > 
> > > ...while I'm writing this, I found the answer -- at the point where
> > deleteObject
> > > is called, m_context.size() must be 1 and thus it is enough to delete the
> > > resource of the first object.
> > > 
> > > Maybe we want to have a comment and an ASSERT to check that
> m_contexts.size()
> > is
> > > 1 when we are in WebGLObject::deleteObject().
> > 
> > BTW, doesn't this fact imply that the getAWebGraphicsContext3D() that needs
> > unpoisoning can be called only for the WebGLRenderingContextBase that is
being
> > destructed just right now?
> > 
> > If that is the case:
> > 
> > - We won't need to unpoison the WebGLRenderingContextBase. It should be
> already
> > unpoisoned (to call the destructor).
> > 
> > - Maybe we can just mark WebGLRenderingContextBase EAGERLY_FINALIZE? Then it
> can
> > just touch WebGLSharedObjects without unpoisoning them.
> > 
> > (I'm not sure.)
> 
> Imposing a finalization order like that would work, once eager finalization
> support is available on trunk.

The relative finalization ordering that eager finalization provides is
sufficient to allow for the removal of the RefPtr-based
WebGLSharedWebGraphicsContext3D object that we had to introduce when
Oilpan-ifying WebGL. Which was needed as we had no control over finalization
order; we do now to some extent.
 
Need to land the GC plugin support for eager finalization first though (
https://codereview.chromium.org/1158623002/ )

[sof, 2015-06-03 14:27:01]

On 2015/06/01 18:43:51, sof wrote:
> On 2015/05/26 18:30:54, sof wrote:
> > On 2015/05/26 12:50:12, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/1151163002/diff/1/Source/core/html/canvas/Web...
> > > File Source/core/html/canvas/WebGLSharedObject.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1151163002/diff/1/Source/core/html/canvas/Web...
> > > Source/core/html/canvas/WebGLSharedObject.cpp:51: deleteObject(nullptr);
> > > On 2015/05/26 11:23:10, haraken wrote:
> > > > 
> > > > This deleteObject retrieves a WebGraphicsContext3D of the first
> > > > WebGLRenderingContextBase (i.e., m_contexts.begin() in
> > > > WebGLContextGroup::getAWebGraphicsContext3D()) and deletes an resource
> > > > underlying the WebGLRenderingContextBase. Why is it enough to delete
only
> > the
> > > > resource of the first WebGLRenderingContextBase? Why don't we need to
> delete
> > > > resources of all other WebGLRenderingContextBases in
> > > > WebGLContextGroup::m_contexts?
> > > > 
> > > > ...while I'm writing this, I found the answer -- at the point where
> > > deleteObject
> > > > is called, m_context.size() must be 1 and thus it is enough to delete
the
> > > > resource of the first object.
> > > > 
> > > > Maybe we want to have a comment and an ASSERT to check that
> > m_contexts.size()
> > > is
> > > > 1 when we are in WebGLObject::deleteObject().
> > > 
> > > BTW, doesn't this fact imply that the getAWebGraphicsContext3D() that
needs
> > > unpoisoning can be called only for the WebGLRenderingContextBase that is
> being
> > > destructed just right now?
> > > 
> > > If that is the case:
> > > 
> > > - We won't need to unpoison the WebGLRenderingContextBase. It should be
> > already
> > > unpoisoned (to call the destructor).
> > > 
> > > - Maybe we can just mark WebGLRenderingContextBase EAGERLY_FINALIZE? Then
it
> > can
> > > just touch WebGLSharedObjects without unpoisoning them.
> > > 
> > > (I'm not sure.)
> > 
> > Imposing a finalization order like that would work, once eager finalization
> > support is available on trunk.
> 
> The relative finalization ordering that eager finalization provides is
> sufficient to allow for the removal of the RefPtr-based
> WebGLSharedWebGraphicsContext3D object that we had to introduce when
> Oilpan-ifying WebGL. Which was needed as we had no control over finalization
> order; we do now to some extent.
>  
> Need to land the GC plugin support for eager finalization first though (
> https://codereview.chromium.org/1158623002/ )

Blocked on that, but ps#2 has been updated to use eager finalization.

[haraken, 2015-06-03 14:31:29]

LGTM

It would be helpful to add a brief comment about why it needs to be
EAGERLY_FINALIZE()ed.

[sof, 2015-06-03 20:48:38]

On 2015/06/03 14:31:29, haraken wrote:
> LGTM
> 
> It would be helpful to add a brief comment about why it needs to be
> EAGERLY_FINALIZE()ed.

done; let's try to adopt that practice for eagerly finalized objects?

[sof, 2015-06-03 20:48:47]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-06-03 20:48:48]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1151163002/#ps40001
(title: "add explanatory comment")

[commit-bot: I haz the power, 2015-06-03 20:50:13]

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

[commit-bot: I haz the power, 2015-06-03 22:23:54]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196437