Remove now-incorrect assert in WebGLContextGroup.

Remove now-incorrect assert in WebGLContextGroup.

BUG=537054, 666061
TBR=zmo@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/f9370828c4ec76881000f95d39f1360e1beeb983
Cr-Commit-Position: refs/heads/master@{#436180}

[Ken Russell (switch to Gerrit), 2016-12-03 22:42:42]

Description was changed from

==========
Remove now-incorrect assert in WebGLContextGroup.

BUG=537054, 666061
TBR=zmo@chromium.org
==========

to

==========
Remove now-incorrect assert in WebGLContextGroup.

BUG=537054, 666061
TBR=zmo@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Ken Russell (switch to Gerrit), 2016-12-03 22:43:27]

FYI. CQ'ing TBR'd.

[Ken Russell (switch to Gerrit), 2016-12-03 22:43:31]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2016-12-03 22:43:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-04 00:18:05]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1480805011877060, "parent_rev":
"847a6cddd6b074323b4a01068117732769060e59", "commit_rev":
"35c42053551c901da3de99dfd428f5b35ced27c8"}

[commit-bot: I haz the power, 2016-12-04 00:18:30]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-12-04 00:22:19]

Description was changed from

==========
Remove now-incorrect assert in WebGLContextGroup.

BUG=537054, 666061
TBR=zmo@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Remove now-incorrect assert in WebGLContextGroup.

BUG=537054, 666061
TBR=zmo@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/f9370828c4ec76881000f95d39f1360e1beeb983
Cr-Commit-Position: refs/heads/master@{#436180}
==========

[commit-bot: I haz the power, 2016-12-04 00:22:20]

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

[haraken, 2016-12-05 01:31:09]

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

[haraken, 2016-12-05 01:31:10]

https://codereview.chromium.org/2548153002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp (right):

https://codereview.chromium.org/2548153002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp:37: if
(m_contexts.isEmpty())

Hmm, I'm not sure if this works.

If A's destructor runs before B's destructor runs, A's memory is cleared on
Release builds but zapped on Debug builds. So this check will work (by accident)
on Release builds but won't work on Debug builds.

This is exactly why a destructor is not allowed to touch any other on-heap
object. It looks like this CL is violating the rule (and I'm not sure what
problem this CL is going to solve).

[Ken Russell (switch to Gerrit), 2016-12-05 01:38:13]

https://codereview.chromium.org/2548153002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp (right):

https://codereview.chromium.org/2548153002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp:37: if
(m_contexts.isEmpty())
On 2016/12/05 01:31:10, haraken wrote:
> 
> Hmm, I'm not sure if this works.
> 
> If A's destructor runs before B's destructor runs, A's memory is cleared on
> Release builds but zapped on Debug builds. So this check will work (by
accident)
> on Release builds but won't work on Debug builds.
> 
> This is exactly why a destructor is not allowed to touch any other on-heap
> object. It looks like this CL is violating the rule (and I'm not sure what
> problem this CL is going to solve).

m_contexts is a HeapHashSet containing WeakMembers. In earlier discussions (see
the comment just below) we confirmed that these WeakMembers will be removed from
the HeapHashSet early on in the GC cycle, before even the eagerly finalized GC'd
objects destructors are run.

The problem that was found was: a WebGLObject (eagerly finalized) was GC'd, and
it traversed its strong reference to its WebGLContextGroup, but the only
WebGLRenderingContextBase inside that context group had also been GC'd, so the
m_contexts map was empty. I assume that all of the logic for this is correct in
both Release and Debug builds.

The caller of this method, WebGLObject and its subclasses, can deal with this
method returning null.

Again, WebGLObject and its subclasses are specifically eagerly finalized so they
can traverse their strong reference to either their WebGLRenderingContextBase or
WebGLContextGroup.

[haraken, 2016-12-05 01:47:25]

https://codereview.chromium.org/2548153002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp (right):

https://codereview.chromium.org/2548153002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp:37: if
(m_contexts.isEmpty())
On 2016/12/05 01:38:13, Ken Russell wrote:
> On 2016/12/05 01:31:10, haraken wrote:
> > 
> > Hmm, I'm not sure if this works.
> > 
> > If A's destructor runs before B's destructor runs, A's memory is cleared on
> > Release builds but zapped on Debug builds. So this check will work (by
> accident)
> > on Release builds but won't work on Debug builds.
> > 
> > This is exactly why a destructor is not allowed to touch any other on-heap
> > object. It looks like this CL is violating the rule (and I'm not sure what
> > problem this CL is going to solve).
> 
> m_contexts is a HeapHashSet containing WeakMembers. In earlier discussions
(see
> the comment just below) we confirmed that these WeakMembers will be removed
from
> the HeapHashSet early on in the GC cycle, before even the eagerly finalized
GC'd
> objects destructors are run.
> 
> The problem that was found was: a WebGLObject (eagerly finalized) was GC'd,
and
> it traversed its strong reference to its WebGLContextGroup, but the only
> WebGLRenderingContextBase inside that context group had also been GC'd, so the
> m_contexts map was empty. I assume that all of the logic for this is correct
in
> both Release and Debug builds.
> 
> The caller of this method, WebGLObject and its subclasses, can deal with this
> method returning null.
> 
> Again, WebGLObject and its subclasses are specifically eagerly finalized so
they
> can traverse their strong reference to either their WebGLRenderingContextBase
or
> WebGLContextGroup.

Remember that if both WebGLObject and WebGLContextGroup are destructed in the
same GC cycle, the weak processing doesn't run. The weak processing runs only
when WebGLObject is dead but WebGLContextGroup is still alive.

So, m_contexts.isEmpty() means that WebGLObject is dead but WebGLContextGroup is
still alive. Do you really want to return nullptr in this case? As far as I read
the comment, this doesn't seems like what you want to do... ?

[Ken Russell (switch to Gerrit), 2016-12-05 01:59:37]

https://codereview.chromium.org/2548153002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp (right):

https://codereview.chromium.org/2548153002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp:37: if
(m_contexts.isEmpty())
On 2016/12/05 01:47:25, haraken wrote:
> On 2016/12/05 01:38:13, Ken Russell wrote:
> > On 2016/12/05 01:31:10, haraken wrote:
> > > 
> > > Hmm, I'm not sure if this works.
> > > 
> > > If A's destructor runs before B's destructor runs, A's memory is cleared
on
> > > Release builds but zapped on Debug builds. So this check will work (by
> > accident)
> > > on Release builds but won't work on Debug builds.
> > > 
> > > This is exactly why a destructor is not allowed to touch any other on-heap
> > > object. It looks like this CL is violating the rule (and I'm not sure what
> > > problem this CL is going to solve).
> > 
> > m_contexts is a HeapHashSet containing WeakMembers. In earlier discussions
> (see
> > the comment just below) we confirmed that these WeakMembers will be removed
> from
> > the HeapHashSet early on in the GC cycle, before even the eagerly finalized
> GC'd
> > objects destructors are run.
> > 
> > The problem that was found was: a WebGLObject (eagerly finalized) was GC'd,
> and
> > it traversed its strong reference to its WebGLContextGroup, but the only
> > WebGLRenderingContextBase inside that context group had also been GC'd, so
the
> > m_contexts map was empty. I assume that all of the logic for this is correct
> in
> > both Release and Debug builds.
> > 
> > The caller of this method, WebGLObject and its subclasses, can deal with
this
> > method returning null.
> > 
> > Again, WebGLObject and its subclasses are specifically eagerly finalized so
> they
> > can traverse their strong reference to either their
WebGLRenderingContextBase
> or
> > WebGLContextGroup.
> 
> Remember that if both WebGLObject and WebGLContextGroup are destructed in the
> same GC cycle, the weak processing doesn't run. The weak processing runs only
> when WebGLObject is dead but WebGLContextGroup is still alive.

When exactly in Oilpan's GC cycle does weak processing run? I assumed that it
ran before the eagerly-finalized objects' destructors in a GC cycle such as this
one, where both the WebGLObject and its WebGLContextGroup (again, containing
weak references to WebGLRenderingContextBase objects) are reclaimed.

> So, m_contexts.isEmpty() means that WebGLObject is dead but WebGLContextGroup
is
> still alive. Do you really want to return nullptr in this case? As far as I
read
> the comment, this doesn't seems like what you want to do... ?

It's actually fine if this happens. If all the WebGLRenderingContextBases in
this WebGLContextGroup have been GC'd (and currently there's only one per
group), then the underlying graphics resource inside the WebGLObject was already
implicitly deleted, and the WebGLObject should do nothing in its destructor.

Is there a correctness bug with this code as written in Debug builds?

[haraken, 2016-12-05 02:16:53]

On 2016/12/05 01:59:37, Ken Russell wrote:
>
https://codereview.chromium.org/2548153002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp (right):
> 
>
https://codereview.chromium.org/2548153002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp:37: if
> (m_contexts.isEmpty())
> On 2016/12/05 01:47:25, haraken wrote:
> > On 2016/12/05 01:38:13, Ken Russell wrote:
> > > On 2016/12/05 01:31:10, haraken wrote:
> > > > 
> > > > Hmm, I'm not sure if this works.
> > > > 
> > > > If A's destructor runs before B's destructor runs, A's memory is cleared
> on
> > > > Release builds but zapped on Debug builds. So this check will work (by
> > > accident)
> > > > on Release builds but won't work on Debug builds.
> > > > 
> > > > This is exactly why a destructor is not allowed to touch any other
on-heap
> > > > object. It looks like this CL is violating the rule (and I'm not sure
what
> > > > problem this CL is going to solve).
> > > 
> > > m_contexts is a HeapHashSet containing WeakMembers. In earlier discussions
> > (see
> > > the comment just below) we confirmed that these WeakMembers will be
removed
> > from
> > > the HeapHashSet early on in the GC cycle, before even the eagerly
finalized
> > GC'd
> > > objects destructors are run.
> > > 
> > > The problem that was found was: a WebGLObject (eagerly finalized) was
GC'd,
> > and
> > > it traversed its strong reference to its WebGLContextGroup, but the only
> > > WebGLRenderingContextBase inside that context group had also been GC'd, so
> the
> > > m_contexts map was empty. I assume that all of the logic for this is
correct
> > in
> > > both Release and Debug builds.
> > > 
> > > The caller of this method, WebGLObject and its subclasses, can deal with
> this
> > > method returning null.
> > > 
> > > Again, WebGLObject and its subclasses are specifically eagerly finalized
so
> > they
> > > can traverse their strong reference to either their
> WebGLRenderingContextBase
> > or
> > > WebGLContextGroup.
> > 
> > Remember that if both WebGLObject and WebGLContextGroup are destructed in
the
> > same GC cycle, the weak processing doesn't run. The weak processing runs
only
> > when WebGLObject is dead but WebGLContextGroup is still alive.
> 
> When exactly in Oilpan's GC cycle does weak processing run? I assumed that it
> ran before the eagerly-finalized objects' destructors in a GC cycle such as
this
> one, where both the WebGLObject and its WebGLContextGroup (again, containing
> weak references to WebGLRenderingContextBase objects) are reclaimed.

You're correct about the order:

1) Weak processing
2) Eager destruction
3) Lazy destructions

What I'm saying is that the weak processing does NOT run if both objects die at
the same cycle. The weak processing runs only when A outlives B (where A has a
WeakMember to B).

> 
> > So, m_contexts.isEmpty() means that WebGLObject is dead but
WebGLContextGroup
> is
> > still alive. Do you really want to return nullptr in this case? As far as I
> read
> > the comment, this doesn't seems like what you want to do... ?
> 
> It's actually fine if this happens. If all the WebGLRenderingContextBases in
> this WebGLContextGroup have been GC'd (and currently there's only one per
> group), then the underlying graphics resource inside the WebGLObject was
already
> implicitly deleted, and the WebGLObject should do nothing in its destructor.

But this is different from what the comment is saying. The comment is saying:

// During an Oilpan GC where WebGL objects become unreachable at the same
// time the context does, the m_contexts set can be fully cleared out
// before WebGLObjects' destructors run.

As I mentioned above, m_contexts is not cleared when WebGLObject and
WebGLContextGroup die at the same cycle (because the weak processing does not
run).
 
> Is there a correctness bug with this code as written in Debug builds?

Sorry about the confusion -- ignore my previous comment; I just wanted to say
that touching any other on-heap object is unsafe and if you do that, you will
hit a zapped value and crash.

[Ken Russell (switch to Gerrit), 2016-12-05 03:22:46]

On 2016/12/05 02:16:53, haraken wrote:
> On 2016/12/05 01:59:37, Ken Russell wrote:
> >
>
https://codereview.chromium.org/2548153002/diff/1/third_party/WebKit/Source/m...
> > File third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2548153002/diff/1/third_party/WebKit/Source/m...
> > third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp:37: if
> > (m_contexts.isEmpty())
> > On 2016/12/05 01:47:25, haraken wrote:
> > > On 2016/12/05 01:38:13, Ken Russell wrote:
> > > > On 2016/12/05 01:31:10, haraken wrote:
> > > > > 
> > > > > Hmm, I'm not sure if this works.
> > > > > 
> > > > > If A's destructor runs before B's destructor runs, A's memory is
cleared
> > on
> > > > > Release builds but zapped on Debug builds. So this check will work (by
> > > > accident)
> > > > > on Release builds but won't work on Debug builds.
> > > > > 
> > > > > This is exactly why a destructor is not allowed to touch any other
> on-heap
> > > > > object. It looks like this CL is violating the rule (and I'm not sure
> what
> > > > > problem this CL is going to solve).
> > > > 
> > > > m_contexts is a HeapHashSet containing WeakMembers. In earlier
discussions
> > > (see
> > > > the comment just below) we confirmed that these WeakMembers will be
> removed
> > > from
> > > > the HeapHashSet early on in the GC cycle, before even the eagerly
> finalized
> > > GC'd
> > > > objects destructors are run.
> > > > 
> > > > The problem that was found was: a WebGLObject (eagerly finalized) was
> GC'd,
> > > and
> > > > it traversed its strong reference to its WebGLContextGroup, but the only
> > > > WebGLRenderingContextBase inside that context group had also been GC'd,
so
> > the
> > > > m_contexts map was empty. I assume that all of the logic for this is
> correct
> > > in
> > > > both Release and Debug builds.
> > > > 
> > > > The caller of this method, WebGLObject and its subclasses, can deal with
> > this
> > > > method returning null.
> > > > 
> > > > Again, WebGLObject and its subclasses are specifically eagerly finalized
> so
> > > they
> > > > can traverse their strong reference to either their
> > WebGLRenderingContextBase
> > > or
> > > > WebGLContextGroup.
> > > 
> > > Remember that if both WebGLObject and WebGLContextGroup are destructed in
> the
> > > same GC cycle, the weak processing doesn't run. The weak processing runs
> only
> > > when WebGLObject is dead but WebGLContextGroup is still alive.
> > 
> > When exactly in Oilpan's GC cycle does weak processing run? I assumed that
it
> > ran before the eagerly-finalized objects' destructors in a GC cycle such as
> this
> > one, where both the WebGLObject and its WebGLContextGroup (again, containing
> > weak references to WebGLRenderingContextBase objects) are reclaimed.
> 
> You're correct about the order:
> 
> 1) Weak processing
> 2) Eager destruction
> 3) Lazy destructions
> 
> What I'm saying is that the weak processing does NOT run if both objects die
at
> the same cycle. The weak processing runs only when A outlives B (where A has a
> WeakMember to B).

OK, I think I understand. If the WebGLRenderingContextBase was GC'd in an
earlier cycle, though, then the WebGLContextGroup's weak reference to it would
have been cleared, correct?

 
> > 
> > > So, m_contexts.isEmpty() means that WebGLObject is dead but
> WebGLContextGroup
> > is
> > > still alive. Do you really want to return nullptr in this case? As far as
I
> > read
> > > the comment, this doesn't seems like what you want to do... ?
> > 
> > It's actually fine if this happens. If all the WebGLRenderingContextBases in
> > this WebGLContextGroup have been GC'd (and currently there's only one per
> > group), then the underlying graphics resource inside the WebGLObject was
> already
> > implicitly deleted, and the WebGLObject should do nothing in its destructor.
> 
> But this is different from what the comment is saying. The comment is saying:
> 
> // During an Oilpan GC where WebGL objects become unreachable at the same
> // time the context does, the m_contexts set can be fully cleared out
> // before WebGLObjects' destructors run.
> 
> As I mentioned above, m_contexts is not cleared when WebGLObject and
> WebGLContextGroup die at the same cycle (because the weak processing does not
> run).

Thinking about it more, the situation where weak reference processing would be
skipped would be the one where the WebGLObject, WebGLContextGroup and
WebGLRenderingContextBase all die in the same cycle. The weak reference is
between WebGLContextGroup and WebGLRenderingContextBase. Again, WebGLObject is
eagerly finalized so it should get a first chance to traverse to the other
objects.

I'll put together a CL updating this comment and will appreciate your feedback
on it.


> > Is there a correctness bug with this code as written in Debug builds?
> 
> Sorry about the confusion -- ignore my previous comment; I just wanted to say
> that touching any other on-heap object is unsafe and if you do that, you will
> hit a zapped value and crash.

I thought the whole point of eager finalization was to allow those
eagerly-finalized objects a "first chance" to touch strongly-referenced on-heap
objects in their destructors. It seems to me there are two cases:

1) WebGLRenderingContextBase is GC'd; WebGLObject and WebGLContextGroup are
still alive. WebGLContextGroup::m_contexts is cleared out during weak reference
processing; WebGLObject needs to deal with m_contexts being empty.

2) WebGLContext, WebGLContextGroup and WebGLRenderingContextBase are all GC'd in
the same cycle. No weak reference processing happens. WebGLObject's eager
finalizer allows it to traverse to the (dead) WebGLContextGroup and
WebGLRenderingContextBase to delete itself.

Are these correct and legal?

[Ken Russell (switch to Gerrit), 2016-12-05 05:07:23]

On 2016/12/05 03:22:46, Ken Russell wrote:
> On 2016/12/05 02:16:53, haraken wrote:
> > On 2016/12/05 01:59:37, Ken Russell wrote:
> > >
> >
>
https://codereview.chromium.org/2548153002/diff/1/third_party/WebKit/Source/m...
> > > File third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2548153002/diff/1/third_party/WebKit/Source/m...
> > > third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp:37: if
> > > (m_contexts.isEmpty())
> > > On 2016/12/05 01:47:25, haraken wrote:
> > > > On 2016/12/05 01:38:13, Ken Russell wrote:
> > > > > On 2016/12/05 01:31:10, haraken wrote:
> > > > > > 
> > > > > > Hmm, I'm not sure if this works.
> > > > > > 
> > > > > > If A's destructor runs before B's destructor runs, A's memory is
> cleared
> > > on
> > > > > > Release builds but zapped on Debug builds. So this check will work
(by
> > > > > accident)
> > > > > > on Release builds but won't work on Debug builds.
> > > > > > 
> > > > > > This is exactly why a destructor is not allowed to touch any other
> > on-heap
> > > > > > object. It looks like this CL is violating the rule (and I'm not
sure
> > what
> > > > > > problem this CL is going to solve).
> > > > > 
> > > > > m_contexts is a HeapHashSet containing WeakMembers. In earlier
> discussions
> > > > (see
> > > > > the comment just below) we confirmed that these WeakMembers will be
> > removed
> > > > from
> > > > > the HeapHashSet early on in the GC cycle, before even the eagerly
> > finalized
> > > > GC'd
> > > > > objects destructors are run.
> > > > > 
> > > > > The problem that was found was: a WebGLObject (eagerly finalized) was
> > GC'd,
> > > > and
> > > > > it traversed its strong reference to its WebGLContextGroup, but the
only
> > > > > WebGLRenderingContextBase inside that context group had also been
GC'd,
> so
> > > the
> > > > > m_contexts map was empty. I assume that all of the logic for this is
> > correct
> > > > in
> > > > > both Release and Debug builds.
> > > > > 
> > > > > The caller of this method, WebGLObject and its subclasses, can deal
with
> > > this
> > > > > method returning null.
> > > > > 
> > > > > Again, WebGLObject and its subclasses are specifically eagerly
finalized
> > so
> > > > they
> > > > > can traverse their strong reference to either their
> > > WebGLRenderingContextBase
> > > > or
> > > > > WebGLContextGroup.
> > > > 
> > > > Remember that if both WebGLObject and WebGLContextGroup are destructed
in
> > the
> > > > same GC cycle, the weak processing doesn't run. The weak processing runs
> > only
> > > > when WebGLObject is dead but WebGLContextGroup is still alive.
> > > 
> > > When exactly in Oilpan's GC cycle does weak processing run? I assumed that
> it
> > > ran before the eagerly-finalized objects' destructors in a GC cycle such
as
> > this
> > > one, where both the WebGLObject and its WebGLContextGroup (again,
containing
> > > weak references to WebGLRenderingContextBase objects) are reclaimed.
> > 
> > You're correct about the order:
> > 
> > 1) Weak processing
> > 2) Eager destruction
> > 3) Lazy destructions
> > 
> > What I'm saying is that the weak processing does NOT run if both objects die
> at
> > the same cycle. The weak processing runs only when A outlives B (where A has
a
> > WeakMember to B).
> 
> OK, I think I understand. If the WebGLRenderingContextBase was GC'd in an
> earlier cycle, though, then the WebGLContextGroup's weak reference to it would
> have been cleared, correct?
> 
>  
> > > 
> > > > So, m_contexts.isEmpty() means that WebGLObject is dead but
> > WebGLContextGroup
> > > is
> > > > still alive. Do you really want to return nullptr in this case? As far
as
> I
> > > read
> > > > the comment, this doesn't seems like what you want to do... ?
> > > 
> > > It's actually fine if this happens. If all the WebGLRenderingContextBases
in
> > > this WebGLContextGroup have been GC'd (and currently there's only one per
> > > group), then the underlying graphics resource inside the WebGLObject was
> > already
> > > implicitly deleted, and the WebGLObject should do nothing in its
destructor.
> > 
> > But this is different from what the comment is saying. The comment is
saying:
> > 
> > // During an Oilpan GC where WebGL objects become unreachable at the same
> > // time the context does, the m_contexts set can be fully cleared out
> > // before WebGLObjects' destructors run.
> > 
> > As I mentioned above, m_contexts is not cleared when WebGLObject and
> > WebGLContextGroup die at the same cycle (because the weak processing does
not
> > run).
> 
> Thinking about it more, the situation where weak reference processing would be
> skipped would be the one where the WebGLObject, WebGLContextGroup and
> WebGLRenderingContextBase all die in the same cycle. The weak reference is
> between WebGLContextGroup and WebGLRenderingContextBase. Again, WebGLObject is
> eagerly finalized so it should get a first chance to traverse to the other
> objects.
> 
> I'll put together a CL updating this comment and will appreciate your feedback
> on it.
> 
> 
> > > Is there a correctness bug with this code as written in Debug builds?
> > 
> > Sorry about the confusion -- ignore my previous comment; I just wanted to
say
> > that touching any other on-heap object is unsafe and if you do that, you
will
> > hit a zapped value and crash.
> 
> I thought the whole point of eager finalization was to allow those
> eagerly-finalized objects a "first chance" to touch strongly-referenced
on-heap
> objects in their destructors. It seems to me there are two cases:
> 
> 1) WebGLRenderingContextBase is GC'd; WebGLObject and WebGLContextGroup are
> still alive. WebGLContextGroup::m_contexts is cleared out during weak
reference
> processing; WebGLObject needs to deal with m_contexts being empty.
> 
> 2) WebGLContext, WebGLContextGroup and WebGLRenderingContextBase are all GC'd
in
> the same cycle. No weak reference processing happens. WebGLObject's eager
> finalizer allows it to traverse to the (dead) WebGLContextGroup and
> WebGLRenderingContextBase to delete itself.
> 
> Are these correct and legal?

For the record: https://codereview.chromium.org/2547393002/ updates this
comment.