Manage WebGLRenderingContextBase's weak refs manually without Oilpan.

Manage WebGLRenderingContextBase's weak refs manually without Oilpan.

WebGLRenderingContextBase keeps a set of registered on-heap WebGLContextObjects,
ensuring that these are detached if it is finalized first -- it being unsafe to
make any reverse access of the *Base object from those after that point. These
references must however be weak as the *Base object mustn't keep WebGL objects
alive if they're not otherwise used & referenced.

Without Oilpan enabled by default, *Base isn't on the heap, that set of
weak WebGLContextObject references must be managed manually rather than relying
on HeapHashSet<WeakMember<T>>. If we were to do so, a GC might identify a
WebGLContextObject as only being weakly referenced from the *Base object and
clear out the entry for it in the set. Should the *Base object then be finalized
and destructed before that WebGLContextObject is lazily swept, it would attempt
to access an already freed object. With sorry consequences.

To address, we keep an off-heap set of untraced raw WebGLContextObject pointers,
which are effectively weak. Like now/before, it is the responsibility of either
the WebGLContextObject to detach itself when finalized or have the *Base
object do that and detach the whole set.

R=kbr,haraken
BUG=534524
Committed: https://crrev.com/ec0092e045dffb6df9ba3df114724e4dd421ca83
Cr-Commit-Position: refs/heads/master@{#350775}

[sof, 2015-09-24 14:30:49]

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

[sof, 2015-09-24 14:30:50]

please take a look.

The ThreadState changes are included here for testing purposes on the bots. If
approved, will remove those before landing.

[sof, 2015-09-24 14:33:43]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-24 14:34:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363243003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363243003/40001

[haraken, 2015-09-24 15:20:33]

> If we were to do so, a GC might identify a
> WebGLContextObject as only being weakly referenced from the *Base object and
> clear out the entry for it in the set. Should the *Base object then be
finalized
> and destructed before that WebGLContextObject is lazily swept, it would
attempt
> to access an already freed object. With sorry consequences.

Just help me understand: If the Base object is finalized before the
WebGLContextObject, the Base's destructor clears WebGLContextObject::m_context
in WebGLRenderingContextBase::detachAndRemoveAllObjects(). Thus it shouldn't
happen that the WebGLContextObject accesses the already freed Base object,
should it?

[sof, 2015-09-24 15:23:19]

On 2015/09/24 15:20:33, haraken wrote:
> > If we were to do so, a GC might identify a
> > WebGLContextObject as only being weakly referenced from the *Base object and
> > clear out the entry for it in the set. Should the *Base object then be
> finalized
> > and destructed before that WebGLContextObject is lazily swept, it would
> attempt
> > to access an already freed object. With sorry consequences.
> 
> Just help me understand: If the Base object is finalized before the
> WebGLContextObject, the Base's destructor clears WebGLContextObject::m_context
> in WebGLRenderingContextBase::detachAndRemoveAllObjects(). Thus it shouldn't
> happen that the WebGLContextObject accesses the already freed Base object,
> should it?

But if the WebGLContextObject wasn't otherwise referenced during the last GC,
its slot in the hash set as a WeakMember<> would have been cleared at that stage
& detachAndRemvoeAllObjects() wouldn't encompass it.

[haraken, 2015-09-24 15:34:21]

On 2015/09/24 15:23:19, sof wrote:
> On 2015/09/24 15:20:33, haraken wrote:
> > > If we were to do so, a GC might identify a
> > > WebGLContextObject as only being weakly referenced from the *Base object
and
> > > clear out the entry for it in the set. Should the *Base object then be
> > finalized
> > > and destructed before that WebGLContextObject is lazily swept, it would
> > attempt
> > > to access an already freed object. With sorry consequences.
> > 
> > Just help me understand: If the Base object is finalized before the
> > WebGLContextObject, the Base's destructor clears
WebGLContextObject::m_context
> > in WebGLRenderingContextBase::detachAndRemoveAllObjects(). Thus it shouldn't
> > happen that the WebGLContextObject accesses the already freed Base object,
> > should it?
> 
> But if the WebGLContextObject wasn't otherwise referenced during the last GC,
> its slot in the hash set as a WeakMember<> would have been cleared at that
stage
> & detachAndRemvoeAllObjects() wouldn't encompass it.

Thanks, understood!

It seems to me that the underlying problem is that WebGLRenderingContextBase's
destructor is touching (a lot of) other on-heap objects. Making
WebGLRenderingContextBase eagerly-finalized is not enough. I think a better fix
would be to add a pre-finalizer and move the logic currently written in the
destructor to the pre-finalizer. What do you think?

[commit-bot: I haz the power, 2015-09-24 15:59:02]

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

[commit-bot: I haz the power, 2015-09-24 15:59:02]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Ken Russell (switch to Gerrit), 2015-09-24 16:47:35]

On 2015/09/24 15:34:21, haraken wrote:
> On 2015/09/24 15:23:19, sof wrote:
> > On 2015/09/24 15:20:33, haraken wrote:
> > > > If we were to do so, a GC might identify a
> > > > WebGLContextObject as only being weakly referenced from the *Base object
> and
> > > > clear out the entry for it in the set. Should the *Base object then be
> > > finalized
> > > > and destructed before that WebGLContextObject is lazily swept, it would
> > > attempt
> > > > to access an already freed object. With sorry consequences.
> > > 
> > > Just help me understand: If the Base object is finalized before the
> > > WebGLContextObject, the Base's destructor clears
> WebGLContextObject::m_context
> > > in WebGLRenderingContextBase::detachAndRemoveAllObjects(). Thus it
shouldn't
> > > happen that the WebGLContextObject accesses the already freed Base object,
> > > should it?
> > 
> > But if the WebGLContextObject wasn't otherwise referenced during the last
GC,
> > its slot in the hash set as a WeakMember<> would have been cleared at that
> stage
> > & detachAndRemvoeAllObjects() wouldn't encompass it.
> 
> Thanks, understood!
> 
> It seems to me that the underlying problem is that WebGLRenderingContextBase's
> destructor is touching (a lot of) other on-heap objects. Making
> WebGLRenderingContextBase eagerly-finalized is not enough. I think a better
fix
> would be to add a pre-finalizer and move the logic currently written in the
> destructor to the pre-finalizer. What do you think?

I'm really sorry for not having gotten to this yet, but have been planning to
completely get rid of the tracking of the "context objects" and "shared
objects". Since WebGL is now in its own OpenGL context share group, simple
destruction of the WebGraphicsContext3D should be enough to get rid of all of
the OpenGL resources. I need to write a test that verifies that doing this
doesn't cause any resource leaks over time. Should we move in that direction
instead of patching this logic? I'll try to test this later today and if it
looks like it's working will put up a CL.

[Ken Russell (switch to Gerrit), 2015-09-24 16:47:50]

+bajones, zmo as FYI

[sof, 2015-09-24 17:19:51]

On 2015/09/24 15:34:21, haraken wrote:
> On 2015/09/24 15:23:19, sof wrote:
> > On 2015/09/24 15:20:33, haraken wrote:
> > > > If we were to do so, a GC might identify a
> > > > WebGLContextObject as only being weakly referenced from the *Base object
> and
> > > > clear out the entry for it in the set. Should the *Base object then be
> > > finalized
> > > > and destructed before that WebGLContextObject is lazily swept, it would
> > > attempt
> > > > to access an already freed object. With sorry consequences.
> > > 
> > > Just help me understand: If the Base object is finalized before the
> > > WebGLContextObject, the Base's destructor clears
> WebGLContextObject::m_context
> > > in WebGLRenderingContextBase::detachAndRemoveAllObjects(). Thus it
shouldn't
> > > happen that the WebGLContextObject accesses the already freed Base object,
> > > should it?
> > 
> > But if the WebGLContextObject wasn't otherwise referenced during the last
GC,
> > its slot in the hash set as a WeakMember<> would have been cleared at that
> stage
> > & detachAndRemvoeAllObjects() wouldn't encompass it.
> 
> Thanks, understood!
> 
> It seems to me that the underlying problem is that WebGLRenderingContextBase's
> destructor is touching (a lot of) other on-heap objects. Making
> WebGLRenderingContextBase eagerly-finalized is not enough. I think a better
fix
> would be to add a pre-finalizer and move the logic currently written in the
> destructor to the pre-finalizer. What do you think?

This is for _non-Oilpan_ where *Base isn't on the heap. We don't have any known
problems here for ENABLE(OILPAN), except calling clear() in ~WebGLFramebuffer.

I'm seeing some problems coming through in the tryruns for the latest patchset,
so I'm missing some aspect of this - will take a look.

[sof, 2015-09-24 19:47:24]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-24 19:48:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363243003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363243003/60001

[sof, 2015-09-24 20:39:52]

On 2015/09/24 17:19:51, sof wrote:
> On 2015/09/24 15:34:21, haraken wrote:
> > On 2015/09/24 15:23:19, sof wrote:
> > > On 2015/09/24 15:20:33, haraken wrote:
> > > > > If we were to do so, a GC might identify a
> > > > > WebGLContextObject as only being weakly referenced from the *Base
object
> > and
> > > > > clear out the entry for it in the set. Should the *Base object then be
> > > > finalized
> > > > > and destructed before that WebGLContextObject is lazily swept, it
would
> > > > attempt
> > > > > to access an already freed object. With sorry consequences.
> > > > 
> > > > Just help me understand: If the Base object is finalized before the
> > > > WebGLContextObject, the Base's destructor clears
> > WebGLContextObject::m_context
> > > > in WebGLRenderingContextBase::detachAndRemoveAllObjects(). Thus it
> shouldn't
> > > > happen that the WebGLContextObject accesses the already freed Base
object,
> > > > should it?
> > > 
> > > But if the WebGLContextObject wasn't otherwise referenced during the last
> GC,
> > > its slot in the hash set as a WeakMember<> would have been cleared at that
> > stage
> > > & detachAndRemvoeAllObjects() wouldn't encompass it.
> > 
> > Thanks, understood!
> > 
> > It seems to me that the underlying problem is that
WebGLRenderingContextBase's
> > destructor is touching (a lot of) other on-heap objects. Making
> > WebGLRenderingContextBase eagerly-finalized is not enough. I think a better
> fix
> > would be to add a pre-finalizer and move the logic currently written in the
> > destructor to the pre-finalizer. What do you think?
> 
> This is for _non-Oilpan_ where *Base isn't on the heap. We don't have any
known
> problems here for ENABLE(OILPAN), except calling clear() in ~WebGLFramebuffer.
> 
> I'm seeing some problems coming through in the tryruns for the latest
patchset,
> so I'm missing some aspect of this - will take a look.

The non-Oilpan WebGLContextObject destructor that
https://codereview.chromium.org/1234883002/ retired had to be reinstated.

[sof, 2015-09-24 20:45:44]

One meta comment re: this bug is that it is due to having to support a hybrid
set of WebGL objects, most on the Oilpan heap, but not all.

(Fully enabling Oilpan needs to happen sooner, this complexity is not tenable
long term imho.)

[commit-bot: I haz the power, 2015-09-24 21:20:10]

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

[commit-bot: I haz the power, 2015-09-24 21:20:10]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2015-09-25 01:55:57]

Thanks for putting this together and so clearly thinking through the issues.

If you're confident in this fix then LGTM and let's proceed with it.

In a separate workspace I'm doing the exercise of removing all of the object
tracking (WebGLRenderingContextBase::m_contextObjects and
WebGLContextGroup::m_groupObjects) and changing the ownership so that those
objects keep their WebGLRenderingContextBase or WebGLContextGroup alive (the
latter was already done). However this looks like it's quite a large change and
I'm not sure if it will work, so if this is an interim fix which definitely
improves stability, let's do it.

[haraken, 2015-09-25 02:25:38]

LGTM 2

[sof, 2015-09-25 05:13:58]

On 2015/09/25 01:55:57, Ken Russell wrote:
> Thanks for putting this together and so clearly thinking through the issues.
> 
> If you're confident in this fix then LGTM and let's proceed with it.
> 
> In a separate workspace I'm doing the exercise of removing all of the object
> tracking (WebGLRenderingContextBase::m_contextObjects and
> WebGLContextGroup::m_groupObjects) and changing the ownership so that those
> objects keep their WebGLRenderingContextBase or WebGLContextGroup alive (the
> latter was already done). However this looks like it's quite a large change
and
> I'm not sure if it will work, so if this is an interim fix which definitely
> improves stability, let's do it.

The WebGLContextObject <-> WebGLRenderingContextBase mutual unregistration for
!ENABLE(OILPAN) is now the same as the one used from 07/2014 until we enabled
Oilpan more fully in August. So I'm relatively confident about stability.

[sof, 2015-09-25 05:14:05]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-09-25 05:14:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363243003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363243003/60001

[commit-bot: I haz the power, 2015-09-25 05:19:05]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-25 05:20:00]

Patchset 4 (id:??) landed as
https://crrev.com/ec0092e045dffb6df9ba3df114724e4dd421ca83
Cr-Commit-Position: refs/heads/master@{#350775}