Remove now-incorrect assert in WebGLContextGroup.

third_party/WebKit/Source/modules/webgl/WebGLContextGroup.cpp

 /*
  * Copyright (C) 2011 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 12 matching lines @@
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "modules/webgl/WebGLContextGroup.h"
 
 namespace blink {
 
 WebGLContextGroup::WebGLContextGroup() : m_numberOfContextLosses(0) {}
 
 gpu::gles2::GLES2Interface* WebGLContextGroup::getAGLInterface() {
-  ASSERT(!m_contexts.isEmpty());
+  // 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. Since the calling code handles
+  // this gracefully, explicitly test for this possibility.
+  if (m_contexts.isEmpty())

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

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]

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]

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]

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.
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?

+    return nullptr;
+
   // Weak processing removes dead entries from the HeapHashSet, so it's
   // guaranteed that this will not return null for the reason that a
   // WeakMember was nulled out.
   return (*m_contexts.begin())->contextGL();
 }
 
 void WebGLContextGroup::addContext(WebGLRenderingContextBase* context) {
   m_contexts.add(context);
 }
 
@@ skipping 12 matching lines @@
 DEFINE_TRACE_WRAPPERS(WebGLContextGroup) {
   // TODO(kbr, mlippautz): need to use the manual write barrier since we
   // need to use weak members to get the desired semantics in Oilpan, but
   // no such TraceWrapperMember (like TraceWrapperWeakMember) exists yet.
   for (auto context : m_contexts) {
     visitor->traceWrappersWithManualWriteBarrier(context);
   }
 }
 
 }  // namespace blink