Oilpan: move 2D Canvas and WebGL objects to the heap.

Oilpan: move 2D Canvas and WebGL objects to the heap.

This adds Oilpan transition types to the WebGL object hierarchy, along
with some remaining Canvas 2D objects. 

Notes:

- Oilpan does not allow destructors to access other garbage collected
  objects and we want all WebGL script accessible objects to be under
  the control of Oilpan and be garbage collected. To follow and
  fulfill both, we cannot make the objects that the destructors depend
  on for fetching out the graphics context (which they use to finalize
  their platform resources) be garbage collected. For
  WebGLSharedObjects (and WebGLRenderingContextBase) that means that
  WebGLContextGroup cannot be on the heap/garbage-collected, and for
  WebGLContextObjects, the WebGLRenderingContextBase's DrawingBuffer
  cannot be on the heap either.
  
  See details below for how this is done for WebGLContextObject's; the
  changes to WebGLContextGroup and WebGLSharedObject is along similar
  lines.
  
- In terms of file footprint of the changes here, the WebGLExtension-derived
  objects make up a significant portion. Their switch in transition
  types for their create() constructor method is all standard and
  repetitive.

- The release of platform resources when finalizing WebGL objects
  requires some extra care with Oilpan. That release is done using a
  platform graphics context object (blink::WebGraphicsContext3D), which
  is owned by a DrawingBuffer. As finalization order isn't deterministic
  with Oilpan, we have to arrange for that DrawingBuffer's context
  object to remain alive until all objects that need it for platform
  resource release have been finalized. And we cannot rely on fetching
  the context object from another WebGL object on the Oilpan heap, as it
  may or may not have been finalized.

  To address, the (off heap) DrawingBuffer is shared among all objects 
  that need it for finalization, using an off-heap RefCounted abstraction 
  (WebGLSharedWebGraphicsContext3D). When all objects have been finalized, 
  that shared object will be destructed and its underlying DrawingBuffer 
  (and, indirectly, its blink::WebGraphicsContext3D object) is let go of. 

  As a result, each WebGLContextObject adds a RefPtr<> to this shared 
  DrawingBuffer/WebGraphicsContext3D with Oilpan enabled. 

R=haraken@chromium.org,ager@chromium.org,junov@chromium.org,bajones@chromium.org,kbr@chromium.org
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178647

[sof, 2014-07-01 13:54:11]

Please take a look.

As agreed, one CL to handle the changes required to move the WebGL objects over
to use Oilpan transition types.

The bulkiness of the CL in terms of files touched is in most part due to moving
the WebGLExtension-derived objects over. The type of their create() methods
change, but not much more besides.

The changes have been verified to compile with the clang GC plugin, with and
without Oilpan enabled. Trybot runs haven't shown up anything either (but still
waiting for results with the final reviewable bits to come through.)

https://codereview.chromium.org/365653002/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLContextGroup.h (right):

https://codereview.chromium.org/365653002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLContextGroup.h:72:
WillBeHeapHashSet<RawPtrWillBeWeakMember<WebGLRenderingContextBase> >
m_contexts;
Refer to the rendering contexts weakly, so as to not keep them alive when/if the
context group is the only reference.

The weak collecion serves another purpose also: being able to catch when all the
rendering contexts are about to disappear. At that point we have to evict all
the group objects, which we also keep references to weakly. See the declaration
below and clearWeakMembers() method for how that is currently handled.

https://codereview.chromium.org/365653002/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLExtension.h (right):

https://codereview.chromium.org/365653002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLExtension.h:62:
RawPtrWillBeWeakMember<WebGLRenderingContextBase> m_context;
Once the context object disappears, it is cleared from any extension objects
that refer to it. i.e., an extension object shouldn't keep the context alive
(via a reference to it from its wrapper, for instance.)

https://codereview.chromium.org/365653002/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLSharedObject.cpp (right):

https://codereview.chromium.org/365653002/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLSharedObject.cpp:43: #if !ENABLE(OILPAN)
If this object dies at the same time as its context group object (and,
transitively, all the rendering contexts that refer to it), then we do not
delete its underlying resource. The assumption is that the rendering context
will tear down all when finalized & it invoking destroyContext().

In all other cases, we assume that the disappearance of the rendering context
will detach the shared objects (via the context group object.)

[sof, 2014-07-02 05:52:48]

Thinking about the dependencies here some more, I'm considering pulling
WebGLContextGroup off the heap. That might prove more workable overall.

[haraken, 2014-07-02 07:47:39]

Here is a first round of comments!

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLContextGroup.cpp (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLContextGroup.cpp:37: return contextGroup.release();

return adoptRefWillBeNoop(new WebGLContextGroup());

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLContextGroup.cpp:58: if (m_savedContext)

Just help me understand: This branch hits only when WebGLContextGroup is
double-traced, right?

However, basically we want to consider a way to avoid introducing m_savedContext
(but I have no idea at the moment).

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLContextGroup.cpp:84: // Oilpan: if removal happens
during weak callback processing, ignore.

Specifically, when can the removal happen during weak processing?

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLContextGroup.cpp:135: m_savedContext =
getAWebGraphicsContext3D();

It looks dangerous to do something non-trivial in trace(), since trace() can be
called while the object is not fully constructed.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLContextGroup.h (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLContextGroup.h:53: void
removeContext(WebGLRenderingContextBase*);

Can we add #if !ENABLE(OILPAN) to this method?

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLExtension.h (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLExtension.h:52: bool isLost()

I'm not quite sure if isLost() works as expected in oilpan builds.

In non-oilpan builds, isLost() starts returning true promptly after lose()
succeeds in losing the context. On the other hand, in oilpan builds, isLost()
doesn't return true until the context is collected by a GC. Wouldn't the timing
difference cause issues?

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLFramebuffer.cpp:287: deleteObject(0);

Are you sure that this doesn't touch on-heap objects? deleteObject() can call
deleteObjectImpl(), which is overridden in many subclasses.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLProgram.cpp (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLProgram.cpp:51: deleteObject(0);

Ditto. I'm not sure if deleteObject() doesn't touch on-heap objects.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderbuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderbuffer.cpp:41: deleteObject(0);

Ditto.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContext.cpp (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContext.cpp:120: 

Nit: Unnecessary empty line.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:97:
DEFINE_STATIC_LOCAL(Vector<WebGLRenderingContextBase*>, activeContexts, ());

Can we make this a PersistentHeapVector<WeakMember<WebGLRenderingContextBase>> ?

Let's make sure there are not bare pointers to on-heap objects :)

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:103:
DEFINE_STATIC_LOCAL(Vector<WebGLRenderingContextBase*>, forciblyEvictedContexts,
());

Ditto. This should be a
PersistentHeapVector<WeakMember<WebGLRenderingContextBase>>.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:191: if
(!evictedContext->m_restoreAllowed) {

willDestroyContext() is called in WebGLRenderingContextBase's destructor. Is it
safe to touch evicted contexts that can be destructed in the same GC cycle?

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:220: class
ScopedDrawingBufferBinder {

ScopedDrawingBufferBinder can be stack-allocated. Add STACK_ALLOCATED.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:240: WebGLFramebuffer*
m_framebufferBinding;

WebGLFramebuffer is on-heap, so this should be a Member.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:490:
WebGLRenderingContextBase* m_context;

Would you add a comment about why we can't make it a Persistent?

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:507:
WebGLRenderingContextBase* m_context;

Ditto.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:693:
m_textureUnits[i].m_textureCubeMapBinding = nullptr;

Nit: Wrong indentation.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:699:
detachAndRemoveAllObjects();

Is it OK to not call detachContext() on detached context objects? (I understand
it's not that easy to call detachContext() since the context objects are
on-heap, but I want to understand if it's OK to not call detachContext().)

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:714: destroyContext();

It looks safe to call destroyContext() in the destructor since it just touches
the DrawingBuffer.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:716:
willDestroyContext(this);

Not related to this CL, it looks strange we call willDestroyContext() after
destroyContext(). I'd rename destroyContext() to destroyDrawingBuffer().

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:1499: return o;

This should be o.release(). The same comment for other parts in this file.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:2090:
RefPtrWillBeMember<WebGLContextAttributes> attributes =
m_requestedAttributes->clone();

This should be RefPtrWillBeRawPtr.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:4304: while
(m_contextObjects.size() > 0) {

This loop will cause an issue in oilpan builds. In oilpan builds,
removeContextObject() won't be called until a GC is triggered, and thus
m_contextObjects.begin() will return the same object again and again.

You'll need to rewrite the loop using a for loop.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.h:646: RefPtrWillBeMember<T>&
m_extensionField;

It looks weird to use RefPtrWillBeMember<T>&...

Even if we need to use the reference, I guess we won't need to trace it, since a
real owner of the RefPtrWillBeMember should be tracing it.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.h:653:
WillBeHeapVector<RawPtrWillBeMember<ExtensionTracker> > m_extensions;

Not related to this CL, we should use OwnPtr<ExtensionTracker> in oilpan builds.
WebGLRenderingContextBase should own those extensions.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.h:932:
WTF_ALLOW_MOVE_INIT_AND_COMPARE_WITH_MEM_FUNCTIONS(WebCore::WebGLRenderingContextBase::TextureUnitState);

Is it safe to move TextureUnitState with memcpy even though TextureUnitState
contains RefPtrs?

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLTexture.cpp (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLTexture.cpp:59: #if !ENABLE(OILPAN)

I wonder why you remove only this deleteObject().

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLVertexArrayObjectOES.h (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLVertexArrayObjectOES.h:115: static const bool
canCopyWithMemcpy = true;

Is it to safe to move the memory with memcpy even though VertexAttribState has a
RefPtr?

[sof, 2014-07-02 07:53:43]

On 2014/07/02 07:47:39, haraken wrote:
> Here is a first round of comments!
> 

Thanks, will go through them next. I've now switched WebGLContextGroup back to
being a ref-counted off-heap object; this might invalidate/address some of the
lifetime issues raised.

[haraken, 2014-07-02 08:25:20]

On 2014/07/02 07:53:43, sof wrote:
> On 2014/07/02 07:47:39, haraken wrote:
> > Here is a first round of comments!
> > 
> 
> Thanks, will go through them next. I've now switched WebGLContextGroup back to
> being a ref-counted off-heap object; this might invalidate/address some of the
> lifetime issues raised.

That sounds like a good idea. Let's go conservatively :)

[sof, 2014-07-02 11:43:54]

Thanks again for the careful review; followups below.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLContextGroup.cpp (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLContextGroup.cpp:37: return contextGroup.release();
On 2014/07/02 07:47:37, haraken wrote:
> 
> return adoptRefWillBeNoop(new WebGLContextGroup());

I've switched this back to returning PassRefPtr<> & left the method as-is (i.e.,
with an explicit release()).

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLContextGroup.cpp:58: if (m_savedContext)
On 2014/07/02 07:47:38, haraken wrote:
> 
> Just help me understand: This branch hits only when WebGLContextGroup is
> double-traced, right?
> 
> However, basically we want to consider a way to avoid introducing
m_savedContext
> (but I have no idea at the moment).

Yes (to the last point), that's what I ended up with also -- having the context
group be garbage collected when all other objects depend on it for their
finalization of their resources, is either going to be hacky (like here) or not
work out at all.

Consequently, I decided to go back to having it be off-heap. Which means caching
a final 'saved context' like here to make detachment of the group objects work
out, is now no longer done.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLContextGroup.cpp:84: // Oilpan: if removal happens
during weak callback processing, ignore.
On 2014/07/02 07:47:38, haraken wrote:
> 
> Specifically, when can the removal happen during weak processing?

It happens when tracing has determined that no contexts survived the GC, and we
flush the group objects using detachAndRemoveAllObjects().

(This is no longer done.)

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLContextGroup.h (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLContextGroup.h:53: void
removeContext(WebGLRenderingContextBase*);
On 2014/07/02 07:47:38, haraken wrote:
> 
> Can we add #if !ENABLE(OILPAN) to this method?

I don't think that's workable overall as contexts will have to be explicitly
managed by the WebGLContextGroup, so as to reliably be able to release its
'group objects' (and in turn, have them reliably delete their underlying
resource objects.)

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLExtension.h (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLExtension.h:52: bool isLost()
On 2014/07/02 07:47:38, haraken wrote:
> 
> I'm not quite sure if isLost() works as expected in oilpan builds.
> 
> In non-oilpan builds, isLost() starts returning true promptly after lose()
> succeeds in losing the context. On the other hand, in oilpan builds, isLost()
> doesn't return true until the context is collected by a GC. Wouldn't the
timing
> difference cause issues?

It will be observably different, but the timing of losing context isn't fixed
(just that it may happen.)

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLFramebuffer.cpp:287: deleteObject(0);
On 2014/07/02 07:47:38, haraken wrote:
> 
> Are you sure that this doesn't touch on-heap objects? deleteObject() can call
> deleteObjectImpl(), which is overridden in many subclasses.

Commented on at
https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLProgram.cpp (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLProgram.cpp:51: deleteObject(0);
On 2014/07/02 07:47:38, haraken wrote:
> 
> Ditto. I'm not sure if deleteObject() doesn't touch on-heap objects.

It makes the assumption that the context group is alive and that its context
rendering objects are valid. To make that work, WebGLContextGroup is back to
being RefCounted + it has the special detach-all-objects behavior when the last
rendering context object is removed.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContext.cpp (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContext.cpp:120: 
On 2014/07/02 07:47:38, haraken wrote:
> 
> Nit: Unnecessary empty line.

Gone.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:97:
DEFINE_STATIC_LOCAL(Vector<WebGLRenderingContextBase*>, activeContexts, ());
On 2014/07/02 07:47:38, haraken wrote:
> 
> Can we make this a PersistentHeapVector<WeakMember<WebGLRenderingContextBase>>
?
> 
> Let's make sure there are not bare pointers to on-heap objects :)

Weaks aren't supported in heap vectors.

These are safe as-is due to their explicit lifetime handling, in the dtor for
WebGLRenderingContextBase (and methods for removing contexts.) Adding a
Persistent would create a leak & cause them never to be released, so I've kept
it (but added a comment.)

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:103:
DEFINE_STATIC_LOCAL(Vector<WebGLRenderingContextBase*>, forciblyEvictedContexts,
());
On 2014/07/02 07:47:38, haraken wrote:
> 
> Ditto. This should be a
> PersistentHeapVector<WeakMember<WebGLRenderingContextBase>>.

See above.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:191: if
(!evictedContext->m_restoreAllowed) {
On 2014/07/02 07:47:39, haraken wrote:
> 
> willDestroyContext() is called in WebGLRenderingContextBase's destructor. Is
it
> safe to touch evicted contexts that can be destructed in the same GC cycle?

I think it is safe, as long as each context takes care of removing itself from
either when finalized.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:220: class
ScopedDrawingBufferBinder {
On 2014/07/02 07:47:38, haraken wrote:
> 
> ScopedDrawingBufferBinder can be stack-allocated. Add STACK_ALLOCATED.

Thanks, done.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:240: WebGLFramebuffer*
m_framebufferBinding;
On 2014/07/02 07:47:38, haraken wrote:
> 
> WebGLFramebuffer is on-heap, so this should be a Member.

Added transition type.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:490:
WebGLRenderingContextBase* m_context;
On 2014/07/02 07:47:38, haraken wrote:
> 
> Would you add a comment about why we can't make it a Persistent?

That creates a leak of WebGLRenderingContextBase objects (and everything that
they refer to.)

It is ynusual to have the callback objects be on the heap, but I've now moved
them there. This is safe as we explicitly unregister them when finalizing.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:693:
m_textureUnits[i].m_textureCubeMapBinding = nullptr;
On 2014/07/02 07:47:38, haraken wrote:
> 
> Nit: Wrong indentation.

Tidied.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:699:
detachAndRemoveAllObjects();
On 2014/07/02 07:47:38, haraken wrote:
> 
> Is it OK to not call detachContext() on detached context objects? (I
understand
> it's not that easy to call detachContext() since the context objects are
> on-heap, but I want to understand if it's OK to not call detachContext().)

I don't think it is acceptable not to do so, i.e., finalize without releasing
the underlying platform resource.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:716:
willDestroyContext(this);
On 2014/07/02 07:47:39, haraken wrote:
> 
> Not related to this CL, it looks strange we call willDestroyContext() after
> destroyContext(). I'd rename destroyContext() to destroyDrawingBuffer().

Sort of agree, but it does a bit more.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:1499: return o;
On 2014/07/02 07:47:38, haraken wrote:
> 
> This should be o.release(). The same comment for other parts in this file.

Alright, added.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:2090:
RefPtrWillBeMember<WebGLContextAttributes> attributes =
m_requestedAttributes->clone();
On 2014/07/02 07:47:38, haraken wrote:
> 
> This should be RefPtrWillBeRawPtr.

Oops, thanks - well spotted.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:4304: while
(m_contextObjects.size() > 0) {
On 2014/07/02 07:47:38, haraken wrote:
> 
> This loop will cause an issue in oilpan builds. In oilpan builds,
> removeContextObject() won't be called until a GC is triggered, and thus
> m_contextObjects.begin() will return the same object again and again.
> 
> You'll need to rewrite the loop using a for loop.

It's fine as it is; WebGLContextObject::detachContext() calls removeContext().

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.h:646: RefPtrWillBeMember<T>&
m_extensionField;
On 2014/07/02 07:47:39, haraken wrote:
> 
> It looks weird to use RefPtrWillBeMember<T>&...
> 
> Even if we need to use the reference, I guess we won't need to trace it, since
a
> real owner of the RefPtrWillBeMember should be tracing it.

Dropped tracing off it; I don't see a ready way around "forwarding" the
extension to the underlying field once it is created.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.h:653:
WillBeHeapVector<RawPtrWillBeMember<ExtensionTracker> > m_extensions;
On 2014/07/02 07:47:39, haraken wrote:
> 
> Not related to this CL, we should use OwnPtr<ExtensionTracker> in oilpan
builds.
> WebGLRenderingContextBase should own those extensions.

That would be tidier (and not have to keep them on the heap); switched. That is,
an off-heap Vector of OwnPtrs (which we do trace.)

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.h:932:
WTF_ALLOW_MOVE_INIT_AND_COMPARE_WITH_MEM_FUNCTIONS(WebCore::WebGLRenderingContextBase::TextureUnitState);
On 2014/07/02 07:47:39, haraken wrote:
> 
> Is it safe to move TextureUnitState with memcpy even though TextureUnitState
> contains RefPtrs?

Yes; under what conditions could a bit-preserving move be observable? I can't
think of any.

(The question suggests that there are some, however. :) )

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLTexture.cpp (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLTexture.cpp:59: #if !ENABLE(OILPAN)
On 2014/07/02 07:47:39, haraken wrote:
> 
> I wonder why you remove only this deleteObject().

Re-instated; we have to reliably let go of the underlying objects/resources.

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLVertexArrayObjectOES.h (right):

https://codereview.chromium.org/365653002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLVertexArrayObjectOES.h:115: static const bool
canCopyWithMemcpy = true;
On 2014/07/02 07:47:39, haraken wrote:
> 
> Is it to safe to move the memory with memcpy even though VertexAttribState has
a
> RefPtr?

I don't think it is relied upon, but what's your concern?

[haraken, 2014-07-03 04:40:30]

Mostly looks good! Here is the final round of comments.

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/TextMet...
File Source/core/html/TextMetrics.h (right):

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/TextMet...
Source/core/html/TextMetrics.h:36: class TextMetrics FINAL : public
RefCountedWillBeGarbageCollectedFinalized<TextMetrics>, public ScriptWrappable {

Now that ScriptWrappable doesn't need a destructor, you can drop Finalized.

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasContextAttributes.h (right):

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasContextAttributes.h:38: class
CanvasContextAttributes : public
RefCountedWillBeGarbageCollectedFinalized<CanvasContextAttributes> {

Can we probably drop Finalized?

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/Path2D.h (right):

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/Path2D.h:42: class Path2D FINAL : public
RefCountedWillBeGarbageCollectedFinalized<Path2D>, public CanvasPathMethods,
public ScriptWrappable {

Can we probably drop Finalized?

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLContextGroup.h (right):

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLContextGroup.h:68: // Oilpan: this object is not on
the heap, but keeps bare pointers

FIXME: Oilpan: ?

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLContextObject.cpp (right):

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLContextObject.cpp:44:
m_context->removeContextObject(this);

Not related to this CL, I wonder why this destructor doesn't call
deleteObject(m_context->webContext()).

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLFramebuffer.cpp:287: deleteObject(0);

Just to confirm: As long as we pass 0 to deleteObject(), the below
deleteObjectImpl() shouldn't be called, and thus deleteObject() shouldn't touch
on-heap objects. Right?

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:98: // the references were
traced as per usual.

An alternative would be to use
PersistentHeapHashSet<WeakMember<WebGLRenderingContextBase>>, but I'm OK with
the Vector<WebGLRenderingContextBase*> with the FIXME.

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:194:
WebGLRenderingContextBase* evictedContext = forciblyEvictedContexts().first();

Sorry for asking again, but how is it guaranteed that the |evictedContext| is
not dying in the same GC cycle of the |context|? If they can die together, it's
not safe to call willDestroyContext() from the destructor of the |context|.

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:494: virtual
~WebGLRenderingContextLostCallback() {}

Nit: One space needed between { and }.

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:728:
detachAndRemoveAllObjects();

Again, is it OK not to call detachAndRemoveAllObjects() in oilpan builds? It
looks not OK according to your previous comment on this.

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.h:650: RefPtrWillBeMember<T>&
m_extensionField;

Not directly related to this CL, we might want to refactor code and remove the
reference in a follow-up.

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.h:654: RefPtrWillBeMember<T>
m_extension;

Shouldn't this be RefPtrWillBePersistent, since ExtensionTracker is still
off-heap? If using a persistent causes a cycle, we should move the
ExtentionTracker to the heap.

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLShaderPrecisionFormat.h (right):

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLShaderPrecisionFormat.h:38: class
WebGLShaderPrecisionFormat FINAL : public
RefCountedWillBeGarbageCollectedFinalized<WebGLShaderPrecisionFormat>, public
ScriptWrappable {

Can we probably drop Finalized?

[sof, 2014-07-03 07:25:49]

Looks like we could have hit a brickwall or two here.

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/TextMet...
File Source/core/html/TextMetrics.h (right):

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/TextMet...
Source/core/html/TextMetrics.h:36: class TextMetrics FINAL : public
RefCountedWillBeGarbageCollectedFinalized<TextMetrics>, public ScriptWrappable {
On 2014/07/03 04:40:29, haraken wrote:
> 
> Now that ScriptWrappable doesn't need a destructor, you can drop Finalized.

Gone.

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasContextAttributes.h (right):

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasContextAttributes.h:38: class
CanvasContextAttributes : public
RefCountedWillBeGarbageCollectedFinalized<CanvasContextAttributes> {
On 2014/07/03 04:40:29, haraken wrote:
> 
> Can we probably drop Finalized?

Yes, the classes that derive from it (WebGLContextAttributes and
Canvas2DContextAttributes) do not need finalization.

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/Path2D.h (right):

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/Path2D.h:42: class Path2D FINAL : public
RefCountedWillBeGarbageCollectedFinalized<Path2D>, public CanvasPathMethods,
public ScriptWrappable {
On 2014/07/03 04:40:29, haraken wrote:
> 
> Can we probably drop Finalized?

SkPath (via Path) has a destructor.

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLContextGroup.h (right):

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLContextGroup.h:68: // Oilpan: this object is not on
the heap, but keeps bare pointers
On 2014/07/03 04:40:29, haraken wrote:
> 
> FIXME: Oilpan: ?

Prefixed.

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLContextObject.cpp (right):

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLContextObject.cpp:44:
m_context->removeContextObject(this);
On 2014/07/03 04:40:29, haraken wrote:
> 
> Not related to this CL, I wonder why this destructor doesn't call
> deleteObject(m_context->webContext()).

Looks like its derived classes does it instead.

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLFramebuffer.cpp:287: deleteObject(0);
On 2014/07/03 04:40:29, haraken wrote:
> 
> Just to confirm: As long as we pass 0 to deleteObject(), the below
> deleteObjectImpl() shouldn't be called, and thus deleteObject() shouldn't
touch
> on-heap objects. Right?

No, it will. Sigh.

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:98: // the references were
traced as per usual.
On 2014/07/03 04:40:29, haraken wrote:
> 
> An alternative would be to use
> PersistentHeapHashSet<WeakMember<WebGLRenderingContextBase>>, but I'm OK with
> the Vector<WebGLRenderingContextBase*> with the FIXME.

Who knows if a hash set preserves any assumed ordering.

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:194:
WebGLRenderingContextBase* evictedContext = forciblyEvictedContexts().first();
On 2014/07/03 04:40:29, haraken wrote:
> 
> Sorry for asking again, but how is it guaranteed that the |evictedContext| is
> not dying in the same GC cycle of the |context|? If they can die together,
it's
> not safe to call willDestroyContext() from the destructor of the |context|.

_Each_ WebGLContextRenderingBase calls willDestroyContext(). It removes itself
from both activeContexted and evictedContexts, so by the time any other such
object runs this code, we _know_ that the objects on that list haven't been
finalized yet.

If that brings about moving a context to activeContexts which is about to be
finalized later, that isn't a problem. Right?

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:494: virtual
~WebGLRenderingContextLostCallback() {}
On 2014/07/03 04:40:29, haraken wrote:
> 
> Nit: One space needed between { and }.

Added.

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:728:
detachAndRemoveAllObjects();
On 2014/07/03 04:40:29, haraken wrote:
> 
> Again, is it OK not to call detachAndRemoveAllObjects() in oilpan builds? It
> looks not OK according to your previous comment on this.

It touches heap objects, so it cannot happen here. Hmm, I really don't like this
object setup - will have to rethink.

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLRenderingContextBase.h:654: RefPtrWillBeMember<T>
m_extension;
On 2014/07/03 04:40:29, haraken wrote:
> 
> Shouldn't this be RefPtrWillBePersistent, since ExtensionTracker is still
> off-heap? If using a persistent causes a cycle, we should move the
> ExtentionTracker to the heap.

It is off-heap, but traced. See above FIXME.

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/WebGLShaderPrecisionFormat.h (right):

https://codereview.chromium.org/365653002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/WebGLShaderPrecisionFormat.h:38: class
WebGLShaderPrecisionFormat FINAL : public
RefCountedWillBeGarbageCollectedFinalized<WebGLShaderPrecisionFormat>, public
ScriptWrappable {
On 2014/07/03 04:40:29, haraken wrote:
> 
> Can we probably drop Finalized?

Done.

[Ken Russell (switch to Gerrit), 2014-07-08 19:07:11]

Thanks for working on this. This is a pretty large review and it'll take some
time to go through it. Do you have a timeline for when you wanted to land this?

How has this been tested in non-Oilpan configurations? Have you sent this to the
GPU try bots (win_gpu, mac_gpu, linux_gpu)? Since this probably affects GC
behavior it'd be good to gain some confidence with repeated runs.

[Ken Russell (switch to Gerrit), 2014-07-08 19:07:43]

Also +junov and +senorblanco for the 2D canvas changes.

[sof, 2014-07-08 19:47:20]

On 2014/07/08 19:07:11, Ken Russell wrote:
> Thanks for working on this. This is a pretty large review and it'll take some
> time to go through it. Do you have a timeline for when you wanted to land
this?
> 
> How has this been tested in non-Oilpan configurations? Have you sent this to
the
> GPU try bots (win_gpu, mac_gpu, linux_gpu)? Since this probably affects GC
> behavior it'd be good to gain some confidence with repeated runs.

Thanks kbr, but please don't spend time on it in its current state.

I've run into some tricky issues on how to recast WebGLFramebuffer's detachment
notifications + same for WebGLContextObject detachment when finalizing these
objects in Oilpan. i.e., need to work out how to preserve the timely deletion of
underlying platform resources before I can consider this complete. Started
looking at it again this afternoon, so I hope to have something ready fairly
soon.

[Stephen White, 2014-07-08 19:50:21]

On 2014/07/08 19:07:43, Ken Russell wrote:
> Also +junov and +senorblanco for the 2D canvas changes.

Canvas 2D changes look reasonable to me (up to my understanding of oilpan), but
I'll leave for Justin to take a look.

[Justin Novosad, 2014-07-08 20:25:55]

On 2014/07/08 19:50:21, Stephen White wrote:
> On 2014/07/08 19:07:43, Ken Russell wrote:
> > Also +junov and +senorblanco for the 2D canvas changes.
> 
> Canvas 2D changes look reasonable to me (up to my understanding of oilpan),
but
> I'll leave for Justin to take a look.

2D Canvas parts lgtm

[sof, 2014-07-09 11:50:09]

The problem we're faced with here is that platform resource finalization is done
with respect to a blink::WebGraphicsContext3D object, which is either passed
along as an argument or fetched out via a back pointer to a
WebGLRenderingContextBase. These deleteObject() finalization operations are
initiated from the destructors or when handling onDetached() notifications.

If WebGL objects are on the Oilpan heap, which we do want, objects attached to a
WebGLRenderingContextBase cannot safely access the blink::WebGraphicsContext3D
of the WebGLRenderingContextBase parent/owner, nor can we explicit detach the
attached objects from a WebGLRenderingContextBase either when finalizing it, as
Oilpan's sweeping phase has no ordering wrt finalization.

The only workable solution I can think of is to remove the need for an external
blink::WebGraphicsContext3D object when deleting platform resources. i.e., with
Oilpan the objects will each keep a copy to use when finalizing. The latest
patchset (#7) does this.

This might add unacceptable heft to WebGLObject-derived objects, I suppose.

[sof, 2014-07-10 18:01:34]

It'd be good if we could get this ready Oilpan-wise before considering asking
others to have a look.

That's a long-winded way of saying.. ping ;-)

[haraken, 2014-07-11 01:55:02]

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLFramebuffer.cpp:287: detach();

Just help me understand: Why was non-oilpan code working without having this
detach()? Or, why do we need to introduce this detach() now?

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLObject.cpp (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLObject.cpp:37: , m_context3d(base->webContext())

I basically like the idea of caching the webContext into WebGLObjects.

How is it guaranteed that the webContext cached here is still valid when
WebGLObject::deleteObject() gets called?

For example, WebGLContextObject is expecting to return 0 for
WebGLContextObject::getAWebGraphicsContext3D() after
WebGLContextObject::detachContext() is called. See
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[sof, 2014-07-11 04:54:10]

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLFramebuffer.cpp:287: detach();
On 2014/07/11 01:55:02, haraken wrote:
> 
> Just help me understand: Why was non-oilpan code working without having this
> detach()? Or, why do we need to introduce this detach() now?

detach() enables the final deleteObject() to thoroughly do its work and call
upon deleteObjectImpl(). In non-Oilpan, the WebGLObjects will be explicitly
detach()ed from above before finalizing, but that doesn't happen in an Oilpan
setting.

The call is cheap, so I elected not to #if-protect. No problem adding such, if
preferable.

[haraken, 2014-07-11 05:27:54]

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLFramebuffer.cpp:287: detach();
On 2014/07/11 04:54:10, sof wrote:
> On 2014/07/11 01:55:02, haraken wrote:
> > 
> > Just help me understand: Why was non-oilpan code working without having this
> > detach()? Or, why do we need to introduce this detach() now?
> 
> detach() enables the final deleteObject() to thoroughly do its work and call
> upon deleteObjectImpl(). In non-Oilpan, the WebGLObjects will be explicitly
> detach()ed from above before finalizing, but that doesn't happen in an Oilpan
> setting.

Would you help me understand where the WebGLObjects are explicitly detached in
non-oilpan?

[sof, 2014-07-11 05:40:09]

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLFramebuffer.cpp:287: detach();
On 2014/07/11 05:27:54, haraken wrote:
> On 2014/07/11 04:54:10, sof wrote:
> > On 2014/07/11 01:55:02, haraken wrote:
> > > 
> > > Just help me understand: Why was non-oilpan code working without having
this
> > > detach()? Or, why do we need to introduce this detach() now?
> > 
> > detach() enables the final deleteObject() to thoroughly do its work and call
> > upon deleteObjectImpl(). In non-Oilpan, the WebGLObjects will be explicitly
> > detach()ed from above before finalizing, but that doesn't happen in an
Oilpan
> > setting.
> 
> Would you help me understand where the WebGLObjects are explicitly detached in
> non-oilpan?

WebGLRenderingContextBase::detachAndRemoveAllObjects() and the calls to
detachContextGroup() from WebGLContextGroup::removeContext() when the group is
all but drained of context objects.

[haraken, 2014-07-11 05:41:34]

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLFramebuffer.cpp:287: detach();
On 2014/07/11 05:40:09, sof wrote:
> On 2014/07/11 05:27:54, haraken wrote:
> > On 2014/07/11 04:54:10, sof wrote:
> > > On 2014/07/11 01:55:02, haraken wrote:
> > > > 
> > > > Just help me understand: Why was non-oilpan code working without having
> this
> > > > detach()? Or, why do we need to introduce this detach() now?
> > > 
> > > detach() enables the final deleteObject() to thoroughly do its work and
call
> > > upon deleteObjectImpl(). In non-Oilpan, the WebGLObjects will be
explicitly
> > > detach()ed from above before finalizing, but that doesn't happen in an
> Oilpan
> > > setting.
> > 
> > Would you help me understand where the WebGLObjects are explicitly detached
in
> > non-oilpan?
> 
> WebGLRenderingContextBase::detachAndRemoveAllObjects() and the calls to
> detachContextGroup() from WebGLContextGroup::removeContext() when the group is
> all but drained of context objects.

ah, makes sense. Thanks.

[Mads Ager (chromium), 2014-07-11 06:47:23]

I'm not all the way through yet but here are a couple of comments. I'll look at
the rest in an hour or so. :)

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLContextGroup.h (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLContextGroup.h:68: // FIXME: Oilpan: this object is
not on the heap, but keeps bare
Could you update the comment with the reason why we are not using oilpan pointer
types here? Lead to leaks through RefPtr/Persistent chains? When this object is
moved to the oilpan heap should these just be normal HeapHashSet<Member<T> >?

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.h:598:
m_extension->lose(true);
I don't see how this is safe? This should be in #if ENABLE(OILPAN) right? It
seems like you have made the pointer that lose is clearing weak already, right?

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.h:646: // FIXME: Oilpan:
off-heap (vector) collections with OwnPtr<>s
We are working hard on getting rid of tracing of off-heap collections and
OwnPtrs. We cannot support them because we will need to be able to
conservatively scan objects which means that all pointers that have to be traced
have to be in the object directly. Can we change these tracker objects to be
heap allocated and use OwnPtrWillBeMember? As far as I can tell we shouldn't be
using the destructors of these tracker objects anyway with oilpan.

[sof, 2014-07-11 06:51:35]

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLObject.cpp (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLObject.cpp:37: , m_context3d(base->webContext())
On 2014/07/11 01:55:02, haraken wrote:
> 
> I basically like the idea of caching the webContext into WebGLObjects.
> 
> How is it guaranteed that the webContext cached here is still valid when
> WebGLObject::deleteObject() gets called?
> 
> For example, WebGLContextObject is expecting to return 0 for
> WebGLContextObject::getAWebGraphicsContext3D() after
> WebGLContextObject::detachContext() is called. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

It's a very good question.

All remaining WebGLSharedObjects attached to a WebGLContextGroup will be
explicitly detached (=> delete their resources) when the final
WebGLRenderingContextBase removes itself. So no problem there, the resources
will be deleted and when those WebGLSharedObjects instances are finalized, the
object deletion will be a no-op.

But that leaves the WebGLContextObjects (the other WebGLObject-derived class)
attached to WebGLRenderingContextBase. When the WebGLRenderingContextBase
finalizer runs, it will in the end destruct the DrawingBuffer and its 3d context
at the end of that. But there might be WebGLContextObject finalizers running
after that, which will attempt to use the 3d context..

Hmm, I wonder if we can salvage that situation by having
WebGLRenderingContextBase keep an off-heap hash set of WebGLContextObjects.

[haraken, 2014-07-11 07:01:34]

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLObject.cpp (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLObject.cpp:37: , m_context3d(base->webContext())
On 2014/07/11 06:51:35, sof wrote:
> On 2014/07/11 01:55:02, haraken wrote:
> > 
> > I basically like the idea of caching the webContext into WebGLObjects.
> > 
> > How is it guaranteed that the webContext cached here is still valid when
> > WebGLObject::deleteObject() gets called?
> > 
> > For example, WebGLContextObject is expecting to return 0 for
> > WebGLContextObject::getAWebGraphicsContext3D() after
> > WebGLContextObject::detachContext() is called. See
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> It's a very good question.
> 
> All remaining WebGLSharedObjects attached to a WebGLContextGroup will be
> explicitly detached (=> delete their resources) when the final
> WebGLRenderingContextBase removes itself. So no problem there, the resources
> will be deleted and when those WebGLSharedObjects instances are finalized, the
> object deletion will be a no-op.
> 
> But that leaves the WebGLContextObjects (the other WebGLObject-derived class)
> attached to WebGLRenderingContextBase. When the WebGLRenderingContextBase
> finalizer runs, it will in the end destruct the DrawingBuffer and its 3d
context
> at the end of that. But there might be WebGLContextObject finalizers running
> after that, which will attempt to use the 3d context..
> 
> Hmm, I wonder if we can salvage that situation by having
> WebGLRenderingContextBase keep an off-heap hash set of WebGLContextObjects.

Or probably can we create Context3DObserver or something like that, and let the
observer clear m_context3d when the context3d is destructed?

[Mads Ager (chromium), 2014-07-11 09:15:01]

I few more comments.

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2385:
RefPtrWillBeMember<Path2D> path2d;
RefPtrWillBeRawPtr<Path2D> path2d = nullptr;

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLBuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLBuffer.cpp:49: detach();
Since the call to detach is not needed in the non-oilpan build we should
probably add a comment here stating why the detach call is here? We might also
want to have the #ifdef ENABLE(OILPAN) to make it clear that this code is only
here for the oilpan build. I know the #ifdef is ugly, but it does document what
is only here for oilpan.

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLContextGroup.h (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLContextGroup.h:74:
HashSet<WebGLRenderingContextBase*> m_contexts;
I think I might be confused, but based on my searching in the code I cannot
figure out how there can ever be more than one context in m_contexts. The only
place that I can find that adds anything to m_contexts is one call right after
the context group has been created:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

It looks like this could just be a pointer that is set on creation instead of a
set? Am I missing something?

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLObject.cpp (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLObject.cpp:37: , m_context3d(base->webContext())
On 2014/07/11 06:51:35, sof wrote:
> On 2014/07/11 01:55:02, haraken wrote:
> > 
> > I basically like the idea of caching the webContext into WebGLObjects.
> > 
> > How is it guaranteed that the webContext cached here is still valid when
> > WebGLObject::deleteObject() gets called?
> > 
> > For example, WebGLContextObject is expecting to return 0 for
> > WebGLContextObject::getAWebGraphicsContext3D() after
> > WebGLContextObject::detachContext() is called. See
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> It's a very good question.
> 
> All remaining WebGLSharedObjects attached to a WebGLContextGroup will be
> explicitly detached (=> delete their resources) when the final
> WebGLRenderingContextBase removes itself. So no problem there, the resources
> will be deleted and when those WebGLSharedObjects instances are finalized, the
> object deletion will be a no-op.
> 
> But that leaves the WebGLContextObjects (the other WebGLObject-derived class)
> attached to WebGLRenderingContextBase. When the WebGLRenderingContextBase
> finalizer runs, it will in the end destruct the DrawingBuffer and its 3d
context
> at the end of that. But there might be WebGLContextObject finalizers running
> after that, which will attempt to use the 3d context..
> 
> Hmm, I wonder if we can salvage that situation by having
> WebGLRenderingContextBase keep an off-heap hash set of WebGLContextObjects.

We should attempt to avoid that. We are attempting to get rid of all tracing of
off-heap objects. In order to make GCs completely safe while we are in the
middle of object creation (nested allocations in constructors) we need to be
able to conservatively scan objects. We can't do that if a correct trace relies
on the trace method being called to trace object that are not on the oilpan
heap.

As haraken suggests, maybe we can use LifecycleObserver and make WebGLObjects
Context3DLifecycleObservers? That would add a set of observers to context3d but
then this pointer would be cleared in a safe way if the context3d dies before
the WebGLObject.

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLProgram.cpp (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLProgram.cpp:52: #if ENABLE(OILPAN)
Instead of clearing the pointers here could we #ifdef the part of
deleteObjectImpl that we don't want to run? Or is it important that onDetached
is called on other paths that also lead to deleteObjectImpl?

[sof, 2014-07-11 09:42:03]

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLContextGroup.h (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLContextGroup.h:68: // FIXME: Oilpan: this object is
not on the heap, but keeps bare
On 2014/07/11 06:47:23, Mads Ager (chromium) wrote:
> Could you update the comment with the reason why we are not using oilpan
pointer
> types here? Lead to leaks through RefPtr/Persistent chains? When this object
is
> moved to the oilpan heap should these just be normal HeapHashSet<Member<T> >?

It's covered in the review why this collection (and owning object) is really
quite tricky to keep on the heap, but will certainly try to.

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLObject.cpp (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLObject.cpp:37: , m_context3d(base->webContext())
On 2014/07/11 09:15:01, Mads Ager (chromium) wrote:
> On 2014/07/11 06:51:35, sof wrote:
> > On 2014/07/11 01:55:02, haraken wrote:
> > > 
> > > I basically like the idea of caching the webContext into WebGLObjects.
> > > 
> > > How is it guaranteed that the webContext cached here is still valid when
> > > WebGLObject::deleteObject() gets called?
> > > 
> > > For example, WebGLContextObject is expecting to return 0 for
> > > WebGLContextObject::getAWebGraphicsContext3D() after
> > > WebGLContextObject::detachContext() is called. See
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > It's a very good question.
> > 
> > All remaining WebGLSharedObjects attached to a WebGLContextGroup will be
> > explicitly detached (=> delete their resources) when the final
> > WebGLRenderingContextBase removes itself. So no problem there, the resources
> > will be deleted and when those WebGLSharedObjects instances are finalized,
the
> > object deletion will be a no-op.
> > 
> > But that leaves the WebGLContextObjects (the other WebGLObject-derived
class)
> > attached to WebGLRenderingContextBase. When the WebGLRenderingContextBase
> > finalizer runs, it will in the end destruct the DrawingBuffer and its 3d
> context
> > at the end of that. But there might be WebGLContextObject finalizers running
> > after that, which will attempt to use the 3d context..
> > 
> > Hmm, I wonder if we can salvage that situation by having
> > WebGLRenderingContextBase keep an off-heap hash set of WebGLContextObjects.
> 
> We should attempt to avoid that. We are attempting to get rid of all tracing
of
> off-heap objects. In order to make GCs completely safe while we are in the
> middle of object creation (nested allocations in constructors) we need to be
> able to conservatively scan objects. We can't do that if a correct trace
relies
> on the trace method being called to trace object that are not on the oilpan
> heap.
> 
> As haraken suggests, maybe we can use LifecycleObserver and make WebGLObjects
> Context3DLifecycleObservers? That would add a set of observers to context3d
but
> then this pointer would be cleared in a safe way if the context3d dies before
> the WebGLObject.

That would be a tidy way to handle registration and lifetime mechanics, but it
would still require keeping the observers in a collection, I think. Which cannot
be on the heap.

To address this issue, I've done the following:

 - WebGLContextObjects (i.e., WebGLFramebuffer and WebGLVertexArrayObjectOES
only atm) now share a RefPtr<>'ed DrawingBuffer with their
WebGLRenderingContextBase.
 - when all those objects and the 'parent' WebGLRenderingContextBase object have
been finalized, the DrawingBuffer and its platform context object
(WebGraphicsContext3D) is destructed.
 - WebGLSharedObject-derived objects get their WebGraphicsContext3D from their
context object instead of keeping a separate object. As they're explicitly
detached when the WebGLRenderingContextBase removes itself from the
WebGLContextGroup, this is fine.
 - WebGLObject no longer keeps a copy of a WebGraphicsContext3D object.

Have a look; the concern with WebGLObject growing an extra field falls away with
this approach.

[Mads Ager (chromium), 2014-07-11 10:18:53]

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLContextGroup.h (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLContextGroup.h:68: // FIXME: Oilpan: this object is
not on the heap, but keeps bare
On 2014/07/11 09:42:02, sof wrote:
> On 2014/07/11 06:47:23, Mads Ager (chromium) wrote:
> > Could you update the comment with the reason why we are not using oilpan
> pointer
> > types here? Lead to leaks through RefPtr/Persistent chains? When this object
> is
> > moved to the oilpan heap should these just be normal HeapHashSet<Member<T>
>?
> 
> It's covered in the review why this collection (and owning object) is really
> quite tricky to keep on the heap, but will certainly try to.

I guess my question boils down to whether or not these could be:

WillBePersistentHeapHashSet<RawPtrWillBeMember<WebGLRenderingContextBase> >

That might lead to leaks, not sure. Off-heap collections of raw pointers is
something we should avoid if we possibly can. :)

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLObject.cpp (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLObject.cpp:37: , m_context3d(base->webContext())
On 2014/07/11 09:42:02, sof wrote:
> On 2014/07/11 09:15:01, Mads Ager (chromium) wrote:
> > On 2014/07/11 06:51:35, sof wrote:
> > > On 2014/07/11 01:55:02, haraken wrote:
> > > > 
> > > > I basically like the idea of caching the webContext into WebGLObjects.
> > > > 
> > > > How is it guaranteed that the webContext cached here is still valid when
> > > > WebGLObject::deleteObject() gets called?
> > > > 
> > > > For example, WebGLContextObject is expecting to return 0 for
> > > > WebGLContextObject::getAWebGraphicsContext3D() after
> > > > WebGLContextObject::detachContext() is called. See
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > 
> > > It's a very good question.
> > > 
> > > All remaining WebGLSharedObjects attached to a WebGLContextGroup will be
> > > explicitly detached (=> delete their resources) when the final
> > > WebGLRenderingContextBase removes itself. So no problem there, the
resources
> > > will be deleted and when those WebGLSharedObjects instances are finalized,
> the
> > > object deletion will be a no-op.
> > > 
> > > But that leaves the WebGLContextObjects (the other WebGLObject-derived
> class)
> > > attached to WebGLRenderingContextBase. When the WebGLRenderingContextBase
> > > finalizer runs, it will in the end destruct the DrawingBuffer and its 3d
> > context
> > > at the end of that. But there might be WebGLContextObject finalizers
running
> > > after that, which will attempt to use the 3d context..
> > > 
> > > Hmm, I wonder if we can salvage that situation by having
> > > WebGLRenderingContextBase keep an off-heap hash set of
WebGLContextObjects.
> > 
> > We should attempt to avoid that. We are attempting to get rid of all tracing
> of
> > off-heap objects. In order to make GCs completely safe while we are in the
> > middle of object creation (nested allocations in constructors) we need to be
> > able to conservatively scan objects. We can't do that if a correct trace
> relies
> > on the trace method being called to trace object that are not on the oilpan
> > heap.
> > 
> > As haraken suggests, maybe we can use LifecycleObserver and make
WebGLObjects
> > Context3DLifecycleObservers? That would add a set of observers to context3d
> but
> > then this pointer would be cleared in a safe way if the context3d dies
before
> > the WebGLObject.
> 
> That would be a tidy way to handle registration and lifetime mechanics, but it
> would still require keeping the observers in a collection, I think. Which
cannot
> be on the heap.

I guess we could use PersistentHeapHashSet<Member<Observer> >? But it seems to
me that your new solution is cleaner. :)

> To address this issue, I've done the following:
> 
>  - WebGLContextObjects (i.e., WebGLFramebuffer and WebGLVertexArrayObjectOES
> only atm) now share a RefPtr<>'ed DrawingBuffer with their
> WebGLRenderingContextBase.
>  - when all those objects and the 'parent' WebGLRenderingContextBase object
have
> been finalized, the DrawingBuffer and its platform context object
> (WebGraphicsContext3D) is destructed.
>  - WebGLSharedObject-derived objects get their WebGraphicsContext3D from their
> context object instead of keeping a separate object. As they're explicitly
> detached when the WebGLRenderingContextBase removes itself from the
> WebGLContextGroup, this is fine.
>  - WebGLObject no longer keeps a copy of a WebGraphicsContext3D object.
> 
> Have a look; the concern with WebGLObject growing an extra field falls away
with
> this approach.

This looks cleaner. It is more explicit and easier to wrap your head around.
What do you think Haraken?

[sof, 2014-07-11 14:00:08]

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.h:646: // FIXME: Oilpan:
off-heap (vector) collections with OwnPtr<>s
On 2014/07/11 06:47:23, Mads Ager (chromium) wrote:
> We are working hard on getting rid of tracing of off-heap collections and
> OwnPtrs. We cannot support them because we will need to be able to
> conservatively scan objects which means that all pointers that have to be
traced
> have to be in the object directly. Can we change these tracker objects to be
> heap allocated and use OwnPtrWillBeMember? As far as I can tell we shouldn't
be
> using the destructors of these tracker objects anyway with oilpan.

I've been trying to recount all reasons for moving these ExtensionTrackers off
the heap earlier in this review (they were on the heap initially), but with the
weak member from WebGLExtension to its context, I can't see the problem wrt
safety or being observably different than in non-Oilpan.

So, let's try again. Moved it back, but been spending quite a while trying to
make sense of a crash that I'm running into with that in place --
http://pastebin.com/7pSz1vha

i.e., entering an empty vtbl entry?(!) when tracing some hash table. It
definitely is triggered by that heap vector and the ExtensionTracker
objects...bizarre.

Anyone else having run into something like this?

[sof, 2014-07-12 07:22:09]

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.h:646: // FIXME: Oilpan:
off-heap (vector) collections with OwnPtr<>s
On 2014/07/11 14:00:08, sof wrote:
> On 2014/07/11 06:47:23, Mads Ager (chromium) wrote:
> > We are working hard on getting rid of tracing of off-heap collections and
> > OwnPtrs. We cannot support them because we will need to be able to
> > conservatively scan objects which means that all pointers that have to be
> traced
> > have to be in the object directly. Can we change these tracker objects to be
> > heap allocated and use OwnPtrWillBeMember? As far as I can tell we shouldn't
> be
> > using the destructors of these tracker objects anyway with oilpan.
> 
> I've been trying to recount all reasons for moving these ExtensionTrackers off
> the heap earlier in this review (they were on the heap initially), but with
the
> weak member from WebGLExtension to its context, I can't see the problem wrt
> safety or being observably different than in non-Oilpan.
> 
> So, let's try again. Moved it back, but been spending quite a while trying to
> make sense of a crash that I'm running into with that in place --
> http://pastebin.com/7pSz1vha
> 
> i.e., entering an empty vtbl entry?(!) when tracing some hash table. It
> definitely is triggered by that heap vector and the ExtensionTracker
> objects...bizarre.
> 
> Anyone else having run into something like this?

If this seemingly unrelated bug ends up requiring more work and investigation,
I'm tempted to revert back to having these extension trackers off the heap here.
Keeping a large CL like this alive across ToT changes isn't ideal.

https://codereview.chromium.org/365653002/diff/180001/Source/bindings/core/v8...
File Source/bindings/core/v8/Nullable.h (right):

https://codereview.chromium.org/365653002/diff/180001/Source/bindings/core/v8...
Source/bindings/core/v8/Nullable.h:48: // FIXME: candidate for EnableIf<> and
NeedsTracing<>, somehow ?
When rebasing past r177935's addition of Nullable<> as a return type for
getAttachedShaders(), I noticed that Nullable doesn't handle traceable values.

We need to include a trace() method depending on the nullable's type; the
workaround here just provides it always (and doesn't do much tracing, just
keeping the GC plugin checks quiet.)

I also annotated the class with DISALLOW_ALLOCATION().

[sof, 2014-07-13 20:42:18]

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.h:646: // FIXME: Oilpan:
off-heap (vector) collections with OwnPtr<>s
On 2014/07/12 07:22:09, sof wrote:
> On 2014/07/11 14:00:08, sof wrote:
> > On 2014/07/11 06:47:23, Mads Ager (chromium) wrote:
> > > We are working hard on getting rid of tracing of off-heap collections and
> > > OwnPtrs. We cannot support them because we will need to be able to
> > > conservatively scan objects which means that all pointers that have to be
> > traced
> > > have to be in the object directly. Can we change these tracker objects to
be
> > > heap allocated and use OwnPtrWillBeMember? As far as I can tell we
shouldn't
> > be
> > > using the destructors of these tracker objects anyway with oilpan.
> > 
> > I've been trying to recount all reasons for moving these ExtensionTrackers
off
> > the heap earlier in this review (they were on the heap initially), but with
> the
> > weak member from WebGLExtension to its context, I can't see the problem wrt
> > safety or being observably different than in non-Oilpan.
> > 
> > So, let's try again. Moved it back, but been spending quite a while trying
to
> > make sense of a crash that I'm running into with that in place --
> > http://pastebin.com/7pSz1vha
> > 
> > i.e., entering an empty vtbl entry?(!) when tracing some hash table. It
> > definitely is triggered by that heap vector and the ExtensionTracker
> > objects...bizarre.
> > 
> > Anyone else having run into something like this?
> 
> If this seemingly unrelated bug ends up requiring more work and investigation,
> I'm tempted to revert back to having these extension trackers off the heap
here.
> Keeping a large CL like this alive across ToT changes isn't ideal.

Some more info/background:
 
- The crash happens when a GC is triggered when needing to expand

    HeapHashSet<WeakMember<DocumentVisibilityObserver>>

- DocumentVisibilityObserver is a Mixin, and HTMLCanvasElement derives from it.

- Such weak-membered mixins aren't common, I'd allege.

- The crash happens when tracing hashtable backing store, the marking of an
element fails.

- It is clearly unsafe to assume that the backing store being traced is that of
the hashtable being expanded (I've been unable to trap the crash in gdb), but
the two does seem connected..
- I've gone over the details to check if NeedsAdjustAndMark<> is somehow  not
returning the expected value for the class types involved here, but haven't
found anything.

Might the recent ephemeron changes not treat Mixins correctly, somehow?

[sof, 2014-07-14 09:20:16]

This is an unrelated bug, I'd now claim.

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.h:646: // FIXME: Oilpan:
off-heap (vector) collections with OwnPtr<>s
On 2014/07/13 20:42:18, sof wrote:
> On 2014/07/12 07:22:09, sof wrote:
> > On 2014/07/11 14:00:08, sof wrote:
> > > On 2014/07/11 06:47:23, Mads Ager (chromium) wrote:
> > > > We are working hard on getting rid of tracing of off-heap collections
and
> > > > OwnPtrs. We cannot support them because we will need to be able to
> > > > conservatively scan objects which means that all pointers that have to
be
> > > traced
> > > > have to be in the object directly. Can we change these tracker objects
to
> be
> > > > heap allocated and use OwnPtrWillBeMember? As far as I can tell we
> shouldn't
> > > be
> > > > using the destructors of these tracker objects anyway with oilpan.
> > > 
> > > I've been trying to recount all reasons for moving these ExtensionTrackers
> off
> > > the heap earlier in this review (they were on the heap initially), but
with
> > the
> > > weak member from WebGLExtension to its context, I can't see the problem
wrt
> > > safety or being observably different than in non-Oilpan.
> > > 
> > > So, let's try again. Moved it back, but been spending quite a while trying
> to
> > > make sense of a crash that I'm running into with that in place --
> > > http://pastebin.com/7pSz1vha
> > > 
> > > i.e., entering an empty vtbl entry?(!) when tracing some hash table. It
> > > definitely is triggered by that heap vector and the ExtensionTracker
> > > objects...bizarre.
> > > 
> > > Anyone else having run into something like this?
> > 
> > If this seemingly unrelated bug ends up requiring more work and
investigation,
> > I'm tempted to revert back to having these extension trackers off the heap
> here.
> > Keeping a large CL like this alive across ToT changes isn't ideal.
> 
> Some more info/background:
>  
> - The crash happens when a GC is triggered when needing to expand
> 
>     HeapHashSet<WeakMember<DocumentVisibilityObserver>>
> 
> - DocumentVisibilityObserver is a Mixin, and HTMLCanvasElement derives from
it.
> 
> - Such weak-membered mixins aren't common, I'd allege.
> 
> - The crash happens when tracing hashtable backing store, the marking of an
> element fails.
> 
> - It is clearly unsafe to assume that the backing store being traced is that
of
> the hashtable being expanded (I've been unable to trap the crash in gdb), but
> the two does seem connected..
> - I've gone over the details to check if NeedsAdjustAndMark<> is somehow  not
> returning the expected value for the class types involved here, but haven't
> found anything.
> 
> Might the recent ephemeron changes not treat Mixins correctly, somehow?

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

has some other reported instances of the same crash stack. The linux-dbg crash
stack there shows that it is indeed crashing when tracing the HeapHashSet<> in
question here. It is invoking the Mixin mark() version, but the non-null object
reference is invalid.

I've verified that zeroedBackingMalloc() doesn't let through non-zero'ed blocks
in debug builds. (But is that guaranteed to happen in release builds?)

[Mads Ager (chromium), 2014-07-14 09:40:06]

On 2014/07/11 14:00:08, sof wrote:
>
https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
> File Source/core/html/canvas/WebGLRenderingContextBase.h (right):
> 
>
https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
> Source/core/html/canvas/WebGLRenderingContextBase.h:646: // FIXME: Oilpan:
> off-heap (vector) collections with OwnPtr<>s
> On 2014/07/11 06:47:23, Mads Ager (chromium) wrote:
> > We are working hard on getting rid of tracing of off-heap collections and
> > OwnPtrs. We cannot support them because we will need to be able to
> > conservatively scan objects which means that all pointers that have to be
> traced
> > have to be in the object directly. Can we change these tracker objects to be
> > heap allocated and use OwnPtrWillBeMember? As far as I can tell we shouldn't
> be
> > using the destructors of these tracker objects anyway with oilpan.
> 
> I've been trying to recount all reasons for moving these ExtensionTrackers off
> the heap earlier in this review (they were on the heap initially), but with
the
> weak member from WebGLExtension to its context, I can't see the problem wrt
> safety or being observably different than in non-Oilpan.
> 
> So, let's try again. Moved it back, but been spending quite a while trying to
> make sense of a crash that I'm running into with that in place --
> http://pastebin.com/7pSz1vha
> 
> i.e., entering an empty vtbl entry?(!) when tracing some hash table. It
> definitely is triggered by that heap vector and the ExtensionTracker
> objects...bizarre.
> 
> Anyone else having run into something like this?

Yes, this is exactly the bug that we are trying to fix. :-\ I should have been
more explicit in my explanation of why we are forcing the rule that there be no
tracing of off-heap structures at all.

The problem is that we have invalid assumptions about the vtable pointer if we
get a garbage collection during the construction of an object. When an object is
being constructed and we get a GC during the construction we will find a pointer
to it from the stack. We then invoke its virtual trace method but the vtable is
not yet initialized. Therefore, when we find objects with vtables on the stack
we need to conservatively scan the objects instead of calling their virtual
trace methods. This is also the reason why we cannot allow tracing of off-heap
collections or tracing of OwnPtrs. That would *require* us to call the trace
method and conservative scanning of objects would not be guaranteed to be
enough.

This crash is happening elsewhere as well and is not related to your change
here. Erik is working on the last bits of making the tracing use conservative
scanning for these cases to avoid this crash.

[sof, 2014-07-14 10:03:00]

On 2014/07/14 09:40:06, Mads Ager (chromium) wrote:
> On 2014/07/11 14:00:08, sof wrote:
> >
>
https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
> > File Source/core/html/canvas/WebGLRenderingContextBase.h (right):
> > 
> >
>
https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
> > Source/core/html/canvas/WebGLRenderingContextBase.h:646: // FIXME: Oilpan:
> > off-heap (vector) collections with OwnPtr<>s
> > On 2014/07/11 06:47:23, Mads Ager (chromium) wrote:
> > > We are working hard on getting rid of tracing of off-heap collections and
> > > OwnPtrs. We cannot support them because we will need to be able to
> > > conservatively scan objects which means that all pointers that have to be
> > traced
> > > have to be in the object directly. Can we change these tracker objects to
be
> > > heap allocated and use OwnPtrWillBeMember? As far as I can tell we
shouldn't
> > be
> > > using the destructors of these tracker objects anyway with oilpan.
> > 
> > I've been trying to recount all reasons for moving these ExtensionTrackers
off
> > the heap earlier in this review (they were on the heap initially), but with
> the
> > weak member from WebGLExtension to its context, I can't see the problem wrt
> > safety or being observably different than in non-Oilpan.
> > 
> > So, let's try again. Moved it back, but been spending quite a while trying
to
> > make sense of a crash that I'm running into with that in place --
> > http://pastebin.com/7pSz1vha
> > 
> > i.e., entering an empty vtbl entry?(!) when tracing some hash table. It
> > definitely is triggered by that heap vector and the ExtensionTracker
> > objects...bizarre.
> > 
> > Anyone else having run into something like this?
> 
> Yes, this is exactly the bug that we are trying to fix. :-\ I should have been
> more explicit in my explanation of why we are forcing the rule that there be
no
> tracing of off-heap structures at all.
> 
> The problem is that we have invalid assumptions about the vtable pointer if we
> get a garbage collection during the construction of an object. When an object
is
> being constructed and we get a GC during the construction we will find a
pointer
> to it from the stack. We then invoke its virtual trace method but the vtable
is
> not yet initialized. Therefore, when we find objects with vtables on the stack
> we need to conservatively scan the objects instead of calling their virtual
> trace methods. This is also the reason why we cannot allow tracing of off-heap
> collections or tracing of OwnPtrs. That would *require* us to call the trace
> method and conservative scanning of objects would not be guaranteed to be
> enough.
> 
> This crash is happening elsewhere as well and is not related to your change
> here. Erik is working on the last bits of making the tracing use conservative
> scanning for these cases to avoid this crash.

Thanks, now I finally get the underlying reasons for why this rule needs to be
imposed.

It's curious that this crash struck when making ExtensionTracker follow that
rule :)

[sof, 2014-07-14 11:47:53]

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLContextGroup.h (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLContextGroup.h:68: // FIXME: Oilpan: this object is
not on the heap, but keeps bare
On 2014/07/11 10:18:52, Mads Ager (chromium) wrote:
> On 2014/07/11 09:42:02, sof wrote:
> > On 2014/07/11 06:47:23, Mads Ager (chromium) wrote:
> > > Could you update the comment with the reason why we are not using oilpan
> > pointer
> > > types here? Lead to leaks through RefPtr/Persistent chains? When this
object
> > is
> > > moved to the oilpan heap should these just be normal HeapHashSet<Member<T>
> >?
> > 
> > It's covered in the review why this collection (and owning object) is really
> > quite tricky to keep on the heap, but will certainly try to.
> 
> I guess my question boils down to whether or not these could be:
> 
> WillBePersistentHeapHashSet<RawPtrWillBeMember<WebGLRenderingContextBase> >
> 
> That might lead to leaks, not sure. Off-heap collections of raw pointers is
> something we should avoid if we possibly can. :)

That persistent reference would keep the rendering context(s) alive, so wouldn't
be workable.

Hmm, looking closer at how WebGLContextGroup is currently used.. the above set
only ever has one entry, which is the rendering context object creating it (and
keeping a RefPtr.) Is that because WebGLContextGroup will be generalized to hold
multiple rendering contexts in the future, or that it previously did? If the
former, then I believe we can get rid of this abstraction and make
WebGLSharedObjects be quite similar to WebGLContextObjects in terms of resource
finalization.

WebGL folks: could you comment on that, please?

[sof, 2014-07-14 13:58:45]

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLContextGroup.h (right):

https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
Source/core/html/canvas/WebGLContextGroup.h:68: // FIXME: Oilpan: this object is
not on the heap, but keeps bare
On 2014/07/14 11:47:53, sof wrote:
> On 2014/07/11 10:18:52, Mads Ager (chromium) wrote:
> > On 2014/07/11 09:42:02, sof wrote:
> > > On 2014/07/11 06:47:23, Mads Ager (chromium) wrote:
> > > > Could you update the comment with the reason why we are not using oilpan
> > > pointer
> > > > types here? Lead to leaks through RefPtr/Persistent chains? When this
> object
> > > is
> > > > moved to the oilpan heap should these just be normal
HeapHashSet<Member<T>
> > >?
> > > 
> > > It's covered in the review why this collection (and owning object) is
really
> > > quite tricky to keep on the heap, but will certainly try to.
> > 
> > I guess my question boils down to whether or not these could be:
> > 
> > WillBePersistentHeapHashSet<RawPtrWillBeMember<WebGLRenderingContextBase> >
> > 
> > That might lead to leaks, not sure. Off-heap collections of raw pointers is
> > something we should avoid if we possibly can. :)
> 
> That persistent reference would keep the rendering context(s) alive, so
wouldn't
> be workable.
> 
> Hmm, looking closer at how WebGLContextGroup is currently used.. the above set
> only ever has one entry, which is the rendering context object creating it
(and
> keeping a RefPtr.) Is that because WebGLContextGroup will be generalized to
hold
> multiple rendering contexts in the future, or that it previously did? If the
> former, then I believe we can get rid of this abstraction and make
> WebGLSharedObjects be quite similar to WebGLContextObjects in terms of
resource
> finalization.
> 
> WebGL folks: could you comment on that, please?

Tried this locally & it works out.

The single-rendering-context-WebGLContextGroup use is something that ager also
discovered; so, does WebGLContextGroup still have a role to play?

[sof, 2014-07-15 07:51:31]

haraken: is the use of WebGLSharedWebGraphicsContext3D acceptable to you?

[haraken, 2014-07-15 09:05:08]

> haraken: is the use of WebGLSharedWebGraphicsContext3D acceptable to you?

I haven't looked at the details, but I think it's reasonable to add an
indirection off-heap class to chain the destructors in an expected order.

I'm happy to take a closer look if the CL is ready. I'm a bit behind, so I'd be
happy if you could quickly explain how the destruction issue is going to be
handled (probably in the CL description) :)

[sof, 2014-07-15 10:05:13]

On 2014/07/15 09:05:08, haraken wrote:
> > haraken: is the use of WebGLSharedWebGraphicsContext3D acceptable to you?
> 
> I haven't looked at the details, but I think it's reasonable to add an
> indirection off-heap class to chain the destructors in an expected order.
> 
> I'm happy to take a closer look if the CL is ready. I'm a bit behind, so I'd
be
> happy if you could quickly explain how the destruction issue is going to be
> handled (probably in the CL description) :)

Thanks, tried to summarize the issue&approach in the description.

Other than that, the only other substantial issue remaining, I think, is the
representation of WebGLContextGroup and its collections.

[sof, 2014-07-15 14:35:36]

On 2014/07/15 10:05:13, sof wrote:
> On 2014/07/15 09:05:08, haraken wrote:
> > > haraken: is the use of WebGLSharedWebGraphicsContext3D acceptable to you?
> > 
> > I haven't looked at the details, but I think it's reasonable to add an
> > indirection off-heap class to chain the destructors in an expected order.
> > 
> > I'm happy to take a closer look if the CL is ready. I'm a bit behind, so I'd
> be
> > happy if you could quickly explain how the destruction issue is going to be
> > handled (probably in the CL description) :)
> 
> Thanks, tried to summarize the issue&approach in the description.
> 
> Other than that, the only other substantial issue remaining, I think, is the
> representation of WebGLContextGroup and its collections.

Looked into the provenance of WebGLContextGroup a bit. It comes from

  https://bugs.webkit.org/show_bug.cgi?id=75906

which was added (afaict) in order to prepare for WEBGL_shared_resources support
(e.g., https://codereview.chromium.org/17230006/ ).

Does WEBGL_shared_resources have a future?

[haraken, 2014-07-15 15:04:56]

Mostly looks good!

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLContextObject.cpp (right):

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLContextObject.cpp:44:
WebGLContextObject::~WebGLContextObject()

Just to confirm: This destructor can drop the last ref to
m_sharedWebGraphicsContext3D. There is no problem in calling DrawingBuffer's
destructor in WebGLContextObject's destructor, right?

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLFramebuffer.cpp:288: deleteObject(0);

Can we add a helper function that does detach() and deleteObject(0)? For
example, WebGLContextObject::destroyObject() or something like that.

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLFramebuffer.cpp:561:
it->value->onDetached(context3d);

Would you elaborate on why we don't need to call onDetached on the
m_attachments? I guess it's not guaranteed that the m_attachments die together
with this WebGLFramebuffer object.

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderbuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderbuffer.cpp:42:
m_emulatedStencilBuffer.clear();

I'm curious why this is needed.

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:780:
m_sharedWebGraphicsContext3D.clear();

Just to confirm: This can drop the last ref to the DrawingBuffer. There is no
problem to call DrawingBuffer's destructor in WebGLRenderingContextBase's
destructor, right?

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:5530:
m_drawingBuffer.clear();

It's a pain that we have to manage both m_sharedWebGraphicsContext3D and
m_drawingBuffer. Also we don't want to have different logic between oilpan and
non-oilpan as much as possible.

Can we remove m_drawingBuffer from WebGLRenderingContextBase and have the
WebGLRenderingContextBase hold m_sharedWebGraphicsContext3D in both oilpan and
non-oilpan? We can call methods of the DrawingBuffer through the
m_sharedWebGraphicsContext3D. (Probably we can add a helper method like
WebGLRenderingContextBase::drawingBuffer(), which returns
m_sharedWebGraphicsContext3D->drawingBuffer()).

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.h:622: m_extension =
T::create(context);

Just to confirm: It's guaranteed that T holds a strong reference to the
WebGLRenderingContextBase, and thus it's guaranteed that the
WebGLRenderingContextBase, the Extensions and the ExtensionTrackers die
together. Right?

[sof, 2014-07-15 16:23:00]

Thanks; comments below - will follow up with an updated patchset later that
tidies.

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLContextObject.cpp (right):

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLContextObject.cpp:44:
WebGLContextObject::~WebGLContextObject()
On 2014/07/15 15:04:56, haraken wrote:
> 
> Just to confirm: This destructor can drop the last ref to
> m_sharedWebGraphicsContext3D. There is no problem in calling DrawingBuffer's
> destructor in WebGLContextObject's destructor, right?

None known - it will happen during the same sweep as that of the
WebGLRenderingContextBase that handles it destruction non-Oilpan.

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLFramebuffer.cpp:288: deleteObject(0);
On 2014/07/15 15:04:56, haraken wrote:
> 
> Can we add a helper function that does detach() and deleteObject(0)? For
> example, WebGLContextObject::destroyObject() or something like that.

Will do.

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLFramebuffer.cpp:561:
it->value->onDetached(context3d);
On 2014/07/15 15:04:56, haraken wrote:
> 
> Would you elaborate on why we don't need to call onDetached on the
> m_attachments? I guess it's not guaranteed that the m_attachments die together
> with this WebGLFramebuffer object.

Attachment objects are WebGLSharedObjects. These will be separately (and
explicitly) detached and destroyed via the WebGLContextGroup, hence the
onDetached aren't needed from this dtor. And not possible, as it would touch
other heap objects.

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderbuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderbuffer.cpp:42:
m_emulatedStencilBuffer.clear();
On 2014/07/15 15:04:56, haraken wrote:
> 
> I'm curious why this is needed.

m_emulatedStencilBuffer is on the heap. If you look at deleteObjectImpl() below,
it tries to force the deletion of its object, which it cannot do.
m_emulatedStencilBuffer (another WebGLRenderbuffer) clears out its own resource
when finalized.

Promptly deleting the stencil buffer is needed if the object is otherwise
deleted/detached, so removing the call to deleteEmulatedStencilBuffer() entirely
is not on for Oilpan.

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:780:
m_sharedWebGraphicsContext3D.clear();
On 2014/07/15 15:04:56, haraken wrote:
> 
> Just to confirm: This can drop the last ref to the DrawingBuffer. There is no
> problem to call DrawingBuffer's destructor in WebGLRenderingContextBase's
> destructor, right?

None; this is where it happens non-Oilpan (right below.)

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:5530:
m_drawingBuffer.clear();
On 2014/07/15 15:04:56, haraken wrote:
> 
> It's a pain that we have to manage both m_sharedWebGraphicsContext3D and
> m_drawingBuffer. Also we don't want to have different logic between oilpan and
> non-oilpan as much as possible.
> 
> Can we remove m_drawingBuffer from WebGLRenderingContextBase and have the
> WebGLRenderingContextBase hold m_sharedWebGraphicsContext3D in both oilpan and
> non-oilpan? We can call methods of the DrawingBuffer through the
> m_sharedWebGraphicsContext3D. (Probably we can add a helper method like
> WebGLRenderingContextBase::drawingBuffer(), which returns
> m_sharedWebGraphicsContext3D->drawingBuffer()).
> 

It's not that bad, is it? We access the m_shared* separately when tearing down a
drawingbuffer in two spots here.

I can rephrase the code to access the DrawingBuffer via an accessor instead, and
not keep a separate m_drawingBuffer with Oilpan enabled. Unless there are other
reasons for exposing WebGLSharedWebGraphicsContext3D object in non-Oilpan, I'd
be quite hesitant to impose it.

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.h:622: m_extension =
T::create(context);
On 2014/07/15 15:04:56, haraken wrote:
> 
> Just to confirm: It's guaranteed that T holds a strong reference to the
> WebGLRenderingContextBase, and thus it's guaranteed that the
> WebGLRenderingContextBase, the Extensions and the ExtensionTrackers die
> together. Right?

No, the extension (a WebGLExtension) holds a weak reference. Its lifetime may
extend beyond the WebGLRenderingContextBase object it was created for, so that
reference must be cleared out when the latter goes away.

[sof, 2014-07-15 21:49:07]

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLFramebuffer.cpp:288: deleteObject(0);
On 2014/07/15 16:23:00, sof wrote:
> On 2014/07/15 15:04:56, haraken wrote:
> > 
> > Can we add a helper function that does detach() and deleteObject(0)? For
> > example, WebGLContextObject::destroyObject() or something like that.
> 
> Will do.

Done.

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:5530:
m_drawingBuffer.clear();
On 2014/07/15 16:23:00, sof wrote:
> On 2014/07/15 15:04:56, haraken wrote:
> > 
> > It's a pain that we have to manage both m_sharedWebGraphicsContext3D and
> > m_drawingBuffer. Also we don't want to have different logic between oilpan
and
> > non-oilpan as much as possible.
> > 
> > Can we remove m_drawingBuffer from WebGLRenderingContextBase and have the
> > WebGLRenderingContextBase hold m_sharedWebGraphicsContext3D in both oilpan
and
> > non-oilpan? We can call methods of the DrawingBuffer through the
> > m_sharedWebGraphicsContext3D. (Probably we can add a helper method like
> > WebGLRenderingContextBase::drawingBuffer(), which returns
> > m_sharedWebGraphicsContext3D->drawingBuffer()).
> > 
> 
> It's not that bad, is it? We access the m_shared* separately when tearing down
a
> drawingbuffer in two spots here.
> 
> I can rephrase the code to access the DrawingBuffer via an accessor instead,
and
> not keep a separate m_drawingBuffer with Oilpan enabled. Unless there are
other
> reasons for exposing WebGLSharedWebGraphicsContext3D object in non-Oilpan, I'd
> be quite hesitant to impose it.
> 

Added drawingBuffer() accessor.

[Ken Russell (switch to Gerrit), 2014-07-16 01:40:13]

On 2014/07/15 14:35:36, sof wrote:
> Does WEBGL_shared_resources have a future?

First -- I'm very sorry for not having gotten to this review yet. It's on the
top of my list, and I will review (or start reviewing?) tomorrow.

We need to assume that WEBGL_shared_resources does in fact have a future.

[haraken, 2014-07-16 02:08:20]

From the perspective of oilpan, looks good. I'll take another close look after
kbr@'s review.

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.h:622: m_extension =
T::create(context);
On 2014/07/15 16:23:00, sof wrote:
> On 2014/07/15 15:04:56, haraken wrote:
> > 
> > Just to confirm: It's guaranteed that T holds a strong reference to the
> > WebGLRenderingContextBase, and thus it's guaranteed that the
> > WebGLRenderingContextBase, the Extensions and the ExtensionTrackers die
> > together. Right?
> 
> No, the extension (a WebGLExtension) holds a weak reference. Its lifetime may
> extend beyond the WebGLRenderingContextBase object it was created for, so that
> reference must be cleared out when the latter goes away.

Then why don't we need to call m_extension->lose() in ~TypedExtensionTracker()?

[sof, 2014-07-16 05:02:48]

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.h:622: m_extension =
T::create(context);
On 2014/07/16 02:08:19, haraken wrote:
> On 2014/07/15 16:23:00, sof wrote:
> > On 2014/07/15 15:04:56, haraken wrote:
> > > 
> > > Just to confirm: It's guaranteed that T holds a strong reference to the
> > > WebGLRenderingContextBase, and thus it's guaranteed that the
> > > WebGLRenderingContextBase, the Extensions and the ExtensionTrackers die
> > > together. Right?
> > 
> > No, the extension (a WebGLExtension) holds a weak reference. Its lifetime
may
> > extend beyond the WebGLRenderingContextBase object it was created for, so
that
> > reference must be cleared out when the latter goes away.
> 
> Then why don't we need to call m_extension->lose() in
~TypedExtensionTracker()?

If the extension tracker dies, then WebGLRenderingContextBase dies. If the
WebGLExtension doesn't die at the same time, its weak reference will be cleared.
Which is what lose() does.

If WebGLExtension dies also at the same time, no need to call lose() either as
it wouldn't be observable.

[haraken, 2014-07-16 05:04:23]

On 2014/07/16 05:02:48, sof wrote:
>
https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
> File Source/core/html/canvas/WebGLRenderingContextBase.h (right):
> 
>
https://codereview.chromium.org/365653002/diff/220001/Source/core/html/canvas...
> Source/core/html/canvas/WebGLRenderingContextBase.h:622: m_extension =
> T::create(context);
> On 2014/07/16 02:08:19, haraken wrote:
> > On 2014/07/15 16:23:00, sof wrote:
> > > On 2014/07/15 15:04:56, haraken wrote:
> > > > 
> > > > Just to confirm: It's guaranteed that T holds a strong reference to the
> > > > WebGLRenderingContextBase, and thus it's guaranteed that the
> > > > WebGLRenderingContextBase, the Extensions and the ExtensionTrackers die
> > > > together. Right?
> > > 
> > > No, the extension (a WebGLExtension) holds a weak reference. Its lifetime
> may
> > > extend beyond the WebGLRenderingContextBase object it was created for, so
> that
> > > reference must be cleared out when the latter goes away.
> > 
> > Then why don't we need to call m_extension->lose() in
> ~TypedExtensionTracker()?
> 
> If the extension tracker dies, then WebGLRenderingContextBase dies. If the
> WebGLExtension doesn't die at the same time, its weak reference will be
cleared.
> Which is what lose() does.
> 
> If WebGLExtension dies also at the same time, no need to call lose() either as
> it wouldn't be observable.

Makes sense, thanks for the clarification!

[sof, 2014-07-16 12:22:35]

https://codereview.chromium.org/365653002/diff/260001/Source/bindings/core/v8...
File Source/bindings/core/v8/Nullable.h (right):

https://codereview.chromium.org/365653002/diff/260001/Source/bindings/core/v8...
Source/bindings/core/v8/Nullable.h:48: TraceIfNeeded<T>::trace(visitor,
&m_value);
I rephrased Nullable to use TraceIfNeeded<> instead (see Handle.h addition.)

[sof, 2014-07-16 13:12:33]

On 2014/07/16 01:40:13, Ken Russell wrote:
> On 2014/07/15 14:35:36, sof wrote:
> > Does WEBGL_shared_resources have a future?
> 
> First -- I'm very sorry for not having gotten to this review yet. It's on the
> top of my list, and I will review (or start reviewing?) tomorrow.
> 

It looks quite daunting I'm afraid, but adding these types piecemeal isn't
readily feasible. A lot of it is repetitive and doesn't bring any change in
functionality though (well, besides putting the objects on the Oilpan heap
instead upon construction.) No (intentional) changes in behavior made.

I've updated and expanded the description some.

> We need to assume that WEBGL_shared_resources does in fact have a future.

Thanks for confirming; the code support for it is unchanged here.

[Ken Russell (switch to Gerrit), 2014-07-17 01:18:30]

On 2014/07/16 13:12:33, sof wrote:
> It looks quite daunting I'm afraid, but adding these types piecemeal isn't
> readily feasible. A lot of it is repetitive and doesn't bring any change in
> functionality though (well, besides putting the objects on the Oilpan heap
> instead upon construction.) No (intentional) changes in behavior made.

It looks very very good overall. Still working through it. Please bear with me
for another day or so.

[bajones, 2014-07-18 20:31:29]

Non-owner LGTM on the WebGL code. A couple of questions/nits.

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLActiveInfo.h (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLActiveInfo.h:37: class WebGLActiveInfo FINAL :
public RefCountedWillBeGarbageCollectedFinalized<WebGLActiveInfo>, public
ScriptWrappable {
This doesn't need to be Finalized, does it?

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLUniformLocation.h (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLUniformLocation.h:37: class WebGLUniformLocation
FINAL : public RefCountedWillBeGarbageCollectedFinalized<WebGLUniformLocation>,
public ScriptWrappable {
This also looks like it doesn't need Finalization

[sof, 2014-07-18 21:39:30]

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLActiveInfo.h (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLActiveInfo.h:37: class WebGLActiveInfo FINAL :
public RefCountedWillBeGarbageCollectedFinalized<WebGLActiveInfo>, public
ScriptWrappable {
On 2014/07/18 20:31:29, bajones wrote:
> This doesn't need to be Finalized, does it?

String gets in the way of that; it having a RefPtr<> to its Impl that needs to
be finalized.

(Classes derived from the non-finalized GarbageCollected<> are checked by the
Oilpan clang plugin, and will flag an error if you tried to use it here.)

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLUniformLocation.h (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLUniformLocation.h:37: class WebGLUniformLocation
FINAL : public RefCountedWillBeGarbageCollectedFinalized<WebGLUniformLocation>,
public ScriptWrappable {
On 2014/07/18 20:31:29, bajones wrote:
> This also looks like it doesn't need Finalization

Indeed so, it doesn't need finalization. Thanks, will upload a new patchset
tomorrow that derives from RefCountedWillBeGarbageCollected<> instead.

[Ken Russell (switch to Gerrit), 2014-07-19 00:27:16]

First, I'm sorry again for taking to long to review this, and thank you for
rediscovering and working through all of the tricky inter-object references in
WebGL's object graph.

WebGL parts LGTM. A few questions and comments.

While reviewing this CL I realized that we can probably get rid of the global
WebGLSharedObject and WebGLContextObject tables at this point. Now that WebGL's
in its own client-side share group I am 99% sure that the command buffer will
handle correctly deleting the resources when the underlying WebGraphicsContext3D
goes away. I think that would simplify things fairly significantly.

How can we avoid breaking the Oilpan build if we make this change after this CL
lands? How stable is the Oilpan build now, and with your changes here, does it
run the WebGL conformance tests successfully?

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLBuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLBuffer.cpp:49: detachAndDeleteObject();
It's not obvious why we need to forcibly call detach. I vaguely recall from the
discussion that it's because there's no guarantee about destruction order in
sub-graphs, so the previous unchaining behavior of container (like
WebGLFramebuffer) to pointed-to object (like WebGLRenderbuffer) no longer works.
Please add a comment.

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLFramebuffer.cpp:287: detachAndDeleteObject();
Please add a comment about the need to manually call detach here.

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLFramebuffer.cpp:558: #if !ENABLE(OILPAN)
Please add a comment about why we can't iterate the attachment map here when
Oilpan's enabled. I remember it was covered in the review comments, but the
information needs to be made persistent.

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLObject.cpp (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLObject.cpp:30: #include
"core/html/canvas/WebGLRenderingContextBase.h"
This #include looks unnecessary.

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLProgram.cpp (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLProgram.cpp:56: detachAndDeleteObject();
Please add a comment about the need to manually call detach here.

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderbuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderbuffer.cpp:45: detachAndDeleteObject();
Please document the need to manually call detach().

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.h:666: // that it is not
deleted before this object's destructor is called
Given that the ExtensionTracker will be garbage-collected too, is this comment
obsolete in Oilpan?

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLShader.cpp (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLShader.cpp:50: detachAndDeleteObject();
Please add a comment about the need to manually call detach here.

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLTexture.cpp (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLTexture.cpp:59: detachAndDeleteObject();
Same here.

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLVertexArrayObjectOES.cpp (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLVertexArrayObjectOES.cpp:63:
detachAndDeleteObject();
Please add a comment about the need to manually call detach here.

[sof, 2014-07-19 17:09:56]

On 2014/07/14 10:03:00, sof wrote:
> On 2014/07/14 09:40:06, Mads Ager (chromium) wrote:
> > On 2014/07/11 14:00:08, sof wrote:
> > >
> >
>
https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
> > > File Source/core/html/canvas/WebGLRenderingContextBase.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...
> > > Source/core/html/canvas/WebGLRenderingContextBase.h:646: // FIXME: Oilpan:
> > > off-heap (vector) collections with OwnPtr<>s
> > > On 2014/07/11 06:47:23, Mads Ager (chromium) wrote:
> > > > We are working hard on getting rid of tracing of off-heap collections
and
> > > > OwnPtrs. We cannot support them because we will need to be able to
> > > > conservatively scan objects which means that all pointers that have to
be
> > > traced
> > > > have to be in the object directly. Can we change these tracker objects
to
> be
> > > > heap allocated and use OwnPtrWillBeMember? As far as I can tell we
> shouldn't
> > > be
> > > > using the destructors of these tracker objects anyway with oilpan.
> > > 
> > > I've been trying to recount all reasons for moving these ExtensionTrackers
> off
> > > the heap earlier in this review (they were on the heap initially), but
with
> > the
> > > weak member from WebGLExtension to its context, I can't see the problem
wrt
> > > safety or being observably different than in non-Oilpan.
> > > 
> > > So, let's try again. Moved it back, but been spending quite a while trying
> to
> > > make sense of a crash that I'm running into with that in place --
> > > http://pastebin.com/7pSz1vha
> > > 
> > > i.e., entering an empty vtbl entry?(!) when tracing some hash table. It
> > > definitely is triggered by that heap vector and the ExtensionTracker
> > > objects...bizarre.
> > > 
> > > Anyone else having run into something like this?
> > 
> > Yes, this is exactly the bug that we are trying to fix. :-\ I should have
been
> > more explicit in my explanation of why we are forcing the rule that there be
> no
> > tracing of off-heap structures at all.
> > 
> > The problem is that we have invalid assumptions about the vtable pointer if
we
> > get a garbage collection during the construction of an object. When an
object
> is
> > being constructed and we get a GC during the construction we will find a
> pointer
> > to it from the stack. We then invoke its virtual trace method but the vtable
> is
> > not yet initialized. Therefore, when we find objects with vtables on the
stack
> > we need to conservatively scan the objects instead of calling their virtual
> > trace methods. This is also the reason why we cannot allow tracing of
off-heap
> > collections or tracing of OwnPtrs. That would *require* us to call the trace
> > method and conservative scanning of objects would not be guaranteed to be
> > enough.
> > 
> > This crash is happening elsewhere as well and is not related to your change
> > here. Erik is working on the last bits of making the tracing use
conservative
> > scanning for these cases to avoid this crash.
> 
> Thanks, now I finally get the underlying reasons for why this rule needs to be
> imposed.
> 
> It's curious that this crash struck when making ExtensionTracker follow that
> rule :)

Can confirm that https://codereview.chromium.org/406523002 fixes this one :)

[sof, 2014-07-19 19:55:58]

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLBuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLBuffer.cpp:49: detachAndDeleteObject();
On 2014/07/19 00:27:15, Ken Russell wrote:
> It's not obvious why we need to forcibly call detach. I vaguely recall from
the
> discussion that it's because there's no guarantee about destruction order in
> sub-graphs, so the previous unchaining behavior of container (like
> WebGLFramebuffer) to pointed-to object (like WebGLRenderbuffer) no longer
works.
> Please add a comment.

Yes, it definitely merits a comment; added a longer one that tries to explain.

For this particular one, the WebGLContextGroup isn't guaranteed to be finalized
before all the WebGLSharedObjects it tracks, hence the need.

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLFramebuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLFramebuffer.cpp:287: detachAndDeleteObject();
On 2014/07/19 00:27:15, Ken Russell wrote:
> Please add a comment about the need to manually call detach here.

Added a comment that explains why this WebGLContextObject-derived object needs
it with Oilpan (and why always do it.)

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLFramebuffer.cpp:558: #if !ENABLE(OILPAN)
On 2014/07/19 00:27:15, Ken Russell wrote:
> Please add a comment about why we can't iterate the attachment map here when
> Oilpan's enabled. I remember it was covered in the review comments, but the
> information needs to be made persistent.

Yes, added a comment that tries to explains the two issues here (touching other
heap objects, and them now being self-contained wrt detachment.)

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLObject.cpp (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLObject.cpp:30: #include
"core/html/canvas/WebGLRenderingContextBase.h"
On 2014/07/19 00:27:15, Ken Russell wrote:
> This #include looks unnecessary.

Thanks, removed.

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLProgram.cpp (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLProgram.cpp:56: detachAndDeleteObject();
On 2014/07/19 00:27:15, Ken Russell wrote:
> Please add a comment about the need to manually call detach here.

Added; also explained why the above clearing is done with Oilpan enabled.

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderbuffer.cpp (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderbuffer.cpp:45: detachAndDeleteObject();
On 2014/07/19 00:27:15, Ken Russell wrote:
> Please document the need to manually call detach().

Added

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.h (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.h:666: // that it is not
deleted before this object's destructor is called
On 2014/07/19 00:27:16, Ken Russell wrote:
> Given that the ExtensionTracker will be garbage-collected too, is this comment
> obsolete in Oilpan?

The comment could be rephrased later in more Oilpan-like terms, but it still
holds. We still have to trace it and keep it alive from here.

The reference to a RefPtrWillBeMember<> that these trackers is uncommon, but
it's wrong trace that also. I've removed that from the above trace().

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLShader.cpp (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLShader.cpp:50: detachAndDeleteObject();
On 2014/07/19 00:27:16, Ken Russell wrote:
> Please add a comment about the need to manually call detach here.

Thanks, added.

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLTexture.cpp (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLTexture.cpp:59: detachAndDeleteObject();
On 2014/07/19 00:27:16, Ken Russell wrote:
> Same here.

Added; I also added a comment to WebGLObject::detachAndDeleteObject() itself.

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLUniformLocation.h (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLUniformLocation.h:37: class WebGLUniformLocation
FINAL : public RefCountedWillBeGarbageCollectedFinalized<WebGLUniformLocation>,
public ScriptWrappable {
On 2014/07/18 21:39:29, sof wrote:
> On 2014/07/18 20:31:29, bajones wrote:
> > This also looks like it doesn't need Finalization
> 
> Indeed so, it doesn't need finalization. Thanks, will upload a new patchset
> tomorrow that derives from RefCountedWillBeGarbageCollected<> instead.

Done.

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLVertexArrayObjectOES.cpp (right):

https://codereview.chromium.org/365653002/diff/260001/Source/core/html/canvas...
Source/core/html/canvas/WebGLVertexArrayObjectOES.cpp:63:
detachAndDeleteObject();
On 2014/07/19 00:27:16, Ken Russell wrote:
> Please add a comment about the need to manually call detach here.

Added

[sof, 2014-07-19 20:20:05]

On 2014/07/19 00:27:16, Ken Russell wrote:
> First, I'm sorry again for taking to long to review this, and thank you for
> rediscovering and working through all of the tricky inter-object references in
> WebGL's object graph.
> 
> WebGL parts LGTM. A few questions and comments.
> 
> While reviewing this CL I realized that we can probably get rid of the global
> WebGLSharedObject and WebGLContextObject tables at this point. Now that
WebGL's
> in its own client-side share group I am 99% sure that the command buffer will
> handle correctly deleting the resources when the underlying
WebGraphicsContext3D
> goes away. I think that would simplify things fairly significantly.
> 

If that does it, that would be a major simplification. Cheaper teardowns too.

> How can we avoid breaking the Oilpan build if we make this change after this
CL
> lands? How stable is the Oilpan build now, and with your changes here, does it
> run the WebGL conformance tests successfully?
> 

When run locally, the WebGL conformance tests results come out the same for ToT
(no Oilpan) and this CL (with and without Oilpan.)

I'll happily pitch in and take care of any Oilpan adjustments. Along with
removing the hash sets that holds WebGLContextObjects and WebGLSharedObjects,
I'm guessing most destructors will turn empty. If so, not much Oilpan-specific
code would remain. There could be some follow-on simplifications and adjustments
needed on the Oilpan side with that in place, but shouldn't be a problem.

If it lands first, then it is probably easier to restart adding Oilpan
transition types to WebGL.

[sof, 2014-07-20 05:04:16]

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLContextGroup.h (right):

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/WebGLContextGroup.h:68: // FIXME: Oilpan: this object is
not on the heap, but keeps bare
The representation of these object references from this RefCounted<> object is
what remains to be settled, I believe -- see
https://codereview.chromium.org/365653002/diff/120001/Source/core/html/canvas...

I could expand the comment some more on why heap-based alternatives are
problematic, but the current representation and handling is acceptable and safe
to me.

[haraken, 2014-07-21 14:10:30]

Let me take a final look at this CL on Tuesday morning :)

[sof, 2014-07-21 14:25:43]

On 2014/07/21 14:10:30, haraken wrote:
> Let me take a final look at this CL on Tuesday morning :)

That'd be great (but whenever there is time) :)

Went over it this morning to check that I hadn't overlooked anything; hope that
holds true.

[Ken Russell (switch to Gerrit), 2014-07-21 17:52:13]

On 2014/07/19 20:20:05, sof wrote:
> I'll happily pitch in and take care of any Oilpan adjustments. Along with
> removing the hash sets that holds WebGLContextObjects and WebGLSharedObjects,
> I'm guessing most destructors will turn empty. If so, not much Oilpan-specific
> code would remain. There could be some follow-on simplifications and
adjustments
> needed on the Oilpan side with that in place, but shouldn't be a problem.
> 
> If it lands first, then it is probably easier to restart adding Oilpan
> transition types to WebGL.

We'll wait for this CL to land before making any major surgery to WebGL's object
graph.

[haraken, 2014-07-22 03:46:11]

LGTM with comments.

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2377:
RefPtrWillBeMember<Path2D> path2d;

RefPtrWillBeRawPtr<Path2D> path2d = nullptr;

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLContextGroup.h (right):

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/WebGLContextGroup.h:75: HashSet<WebGLSharedObject*>
m_groupObjects;

Probably we discussed this before, but why can't we use
PersistentHeapHashSet<WeakMember<WebGLRenderingContextBase>> ?

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLContextObject.cpp (right):

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/WebGLContextObject.cpp:68: return
m_sharedWebGraphicsContext3D ? m_sharedWebGraphicsContext3D->webContext() : 0;

Just help me understand: When does it happen that m_sharedWebGraphicsContext3D
is not 0 but m_sharedWebGraphicsContext3D->webContext() returns 0? I wonder why
we need to 0 checks:

- The first check in WebGLContextObject::getAWebGraphicsContext3D().
- The second check in WebGLSharedWebGraphicsContext3D::webContext().

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLObject.cpp (right):

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/WebGLObject.cpp:32:
WebGLObject::WebGLObject(WebGLRenderingContextBase* base)

|base| is not needed.

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/WebGLObject.cpp:41: #if ENABLE(OILPAN)

Do we need #if ENABLE(OILPAN)? I guess ASSERT(m_deleted) should hold in
non-oilpan builds as well.

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/WebGLObject.cpp:101: if (m_deleted)

Probably not related to this CL, but why isn't this 'if(!m_deleted)' ?

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:5556:
m_sharedWebGraphicsContext3D =
WebGLSharedWebGraphicsContext3D::create(buffer.release());

Probably we can eagerly create m_sharedWebGraphicsContext3D when
WebGLRenderingContextBase is constructed.

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLSharedObject.cpp (right):

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/WebGLSharedObject.cpp:41:
WebGLSharedObject::~WebGLSharedObject()

Just help me understand: Why don't we need to call detachAndDeleteObject(0) in
~WebGLSharedObject() ?

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLSharedWebGraphicsContext3D.h (right):

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/WebGLSharedWebGraphicsContext3D.h:47: 

Unnecessary empty lines.

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLVertexArrayObjectOES.h (right):

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/WebGLVertexArrayObjectOES.h:112: static const bool
needsDestruction = false;

Doesn't RefPtr need destruction?

[sof, 2014-07-22 06:15:21]

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2377:
RefPtrWillBeMember<Path2D> path2d;
On 2014/07/22 03:46:10, haraken wrote:
> 
> RefPtrWillBeRawPtr<Path2D> path2d = nullptr;

Don't think so, getWithUndefinedOrNullCheck() takes a RefPtrWillBeMember<>
reference.

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLContextGroup.h (right):

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/WebGLContextGroup.h:75: HashSet<WebGLSharedObject*>
m_groupObjects;
On 2014/07/22 03:46:10, haraken wrote:
> 
> Probably we discussed this before, but why can't we use
> PersistentHeapHashSet<WeakMember<WebGLRenderingContextBase>> ?

Doesn't the above comment referring to (one of) the previous thread shed some
light on this? It will leak, I think, and prevent correct
finalization/detachment of the WebGLSharedObjects from here.

(This object will be simplified a fair bit if the graphics context will be able
to release all underlying resources, btw.)

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLContextObject.cpp (right):

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/WebGLContextObject.cpp:68: return
m_sharedWebGraphicsContext3D ? m_sharedWebGraphicsContext3D->webContext() : 0;
On 2014/07/22 03:46:10, haraken wrote:
> 
> Just help me understand: When does it happen that m_sharedWebGraphicsContext3D
> is not 0 but m_sharedWebGraphicsContext3D->webContext() returns 0? I wonder
why
> we need to 0 checks:
> 
> - The first check in WebGLContextObject::getAWebGraphicsContext3D().
> - The second check in WebGLSharedWebGraphicsContext3D::webContext().

This seems like a trifling matter. You can dispose() of a
WebGLSharedWebGraphicsContext3D to clear out the buffer, which is why it makes
sense to explicitly null check in webContext(). Similarly, you can detach a
WebGLContextObject and clear out the m_sharedWebGraphicsContext3D reference.

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLObject.cpp (right):

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/WebGLObject.cpp:32:
WebGLObject::WebGLObject(WebGLRenderingContextBase* base)
On 2014/07/22 03:46:10, haraken wrote:
> 
> |base| is not needed.

Removed.

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/WebGLObject.cpp:41: #if ENABLE(OILPAN)
On 2014/07/22 03:46:10, haraken wrote:
> 
> Do we need #if ENABLE(OILPAN)? I guess ASSERT(m_deleted) should hold in
> non-oilpan builds as well.

I made it condition to avoid needlessly upsetting non-Oilpan. I can remove.

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLRenderingContextBase.cpp (right):

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/WebGLRenderingContextBase.cpp:5556:
m_sharedWebGraphicsContext3D =
WebGLSharedWebGraphicsContext3D::create(buffer.release());
On 2014/07/22 03:46:10, haraken wrote:
> 
> Probably we can eagerly create m_sharedWebGraphicsContext3D when
> WebGLRenderingContextBase is constructed.

It is. The null check is needed if the context has been explicitly destroyed.

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLSharedObject.cpp (right):

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/WebGLSharedObject.cpp:41:
WebGLSharedObject::~WebGLSharedObject()
On 2014/07/22 03:46:10, haraken wrote:
> 
> Just help me understand: Why don't we need to call detachAndDeleteObject(0) in
> ~WebGLSharedObject() ?

Because WebGLContextGroup will explicitly detach them.

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLSharedWebGraphicsContext3D.h (right):

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/WebGLSharedWebGraphicsContext3D.h:47: 
On 2014/07/22 03:46:10, haraken wrote:
> 
> Unnecessary empty lines.

Removed.

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLVertexArrayObjectOES.h (right):

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/WebGLVertexArrayObjectOES.h:112: static const bool
needsDestruction = false;
On 2014/07/22 03:46:10, haraken wrote:
> 
> Doesn't RefPtr need destruction?

Wrapped in ENABLE(OILPAN).

[sof, 2014-07-22 08:39:57]

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
File Source/core/html/canvas/WebGLSharedObject.cpp (right):

https://codereview.chromium.org/365653002/diff/270001/Source/core/html/canvas...
Source/core/html/canvas/WebGLSharedObject.cpp:41:
WebGLSharedObject::~WebGLSharedObject()
On 2014/07/22 06:15:21, sof wrote:
> On 2014/07/22 03:46:10, haraken wrote:
> > 
> > Just help me understand: Why don't we need to call detachAndDeleteObject(0)
in
> > ~WebGLSharedObject() ?
> 
> Because WebGLContextGroup will explicitly detach them.

This is an incomplete response, I realized. WebGLSharedObject doesn't have a
graphics object on its own -- the derived classes' dtors will take care of
deleting their resources.

[haraken, 2014-07-22 08:53:48]

Thanks, LGTM to land :)

[sof, 2014-07-22 12:31:46]

On 2014/07/22 08:53:48, haraken wrote:
> Thanks, LGTM to land :)

Let's try, thanks everyone.

[sof, 2014-07-22 12:32:05]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-07-22 12:33:12]

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

[commit-bot: I haz the power, 2014-07-22 13:14:59]

Change committed as 178647

[zerny-chromium, 2014-08-01 08:05:15]

https://codereview.chromium.org/365653002/diff/290001/Source/platform/heap/Ha...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/365653002/diff/290001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:700: struct TraceIfNeeded : public
TraceIfEnabled<T, WebCore::IsGarbageCollectedType<typename
RemoveHeapPointerWrapperTypes<typename
WTF::RemovePointer<T>::Type>::Type>::value> { };
I don't think this trait does what we would like it to do. It is used in
Nullable (and ScriptPromiseProperty) to trace a (nullable) value object. So
first of all it is more lax in that it is also defined for non-values (ie, all
GC derived types) when it is only used for part-object allocated collections.
More importantly it is too narrow since it will not identify that a value object
with a trace method should be traced (ie, any object using DISALLOW_ALLOCATION
but with traced fields).

In short, I think we should use the existing NeedsTracing trait here. It is
default defined to be true if the type has a trace method and specialized for
the various other types (eg, Member) already. I'll upload a CL shortly.

[sof, 2014-08-06 17:40:54]

On 2014/07/21 17:52:13, Ken Russell wrote:
> On 2014/07/19 20:20:05, sof wrote:
> > I'll happily pitch in and take care of any Oilpan adjustments. Along with
> > removing the hash sets that holds WebGLContextObjects and
WebGLSharedObjects,
> > I'm guessing most destructors will turn empty. If so, not much
Oilpan-specific
> > code would remain. There could be some follow-on simplifications and
> adjustments
> > needed on the Oilpan side with that in place, but shouldn't be a problem.
> > 
> > If it lands first, then it is probably easier to restart adding Oilpan
> > transition types to WebGL.
> 
> We'll wait for this CL to land before making any major surgery to WebGL's
object
> graph.

Is there a bug on this improvement? (Just want to make it sure it isn't
forgotten about.)

[sof, 2014-08-06 17:40:54]

On 2014/07/21 17:52:13, Ken Russell wrote:
> On 2014/07/19 20:20:05, sof wrote:
> > I'll happily pitch in and take care of any Oilpan adjustments. Along with
> > removing the hash sets that holds WebGLContextObjects and
WebGLSharedObjects,
> > I'm guessing most destructors will turn empty. If so, not much
Oilpan-specific
> > code would remain. There could be some follow-on simplifications and
> adjustments
> > needed on the Oilpan side with that in place, but shouldn't be a problem.
> > 
> > If it lands first, then it is probably easier to restart adding Oilpan
> > transition types to WebGL.
> 
> We'll wait for this CL to land before making any major surgery to WebGL's
object
> graph.

Is there a bug on this improvement? (Just want to make it sure it isn't
forgotten about.)

[Ken Russell (switch to Gerrit), 2014-08-09 01:43:03]

On 2014/08/06 17:40:54, sof wrote:
> On 2014/07/21 17:52:13, Ken Russell wrote:
> > On 2014/07/19 20:20:05, sof wrote:
> > > I'll happily pitch in and take care of any Oilpan adjustments. Along with
> > > removing the hash sets that holds WebGLContextObjects and
> WebGLSharedObjects,
> > > I'm guessing most destructors will turn empty. If so, not much
> Oilpan-specific
> > > code would remain. There could be some follow-on simplifications and
> > adjustments
> > > needed on the Oilpan side with that in place, but shouldn't be a problem.
> > > 
> > > If it lands first, then it is probably easier to restart adding Oilpan
> > > transition types to WebGL.
> > 
> > We'll wait for this CL to land before making any major surgery to WebGL's
> object
> > graph.
> 
> Is there a bug on this improvement? (Just want to make it sure it isn't
> forgotten about.)

Thanks for reminding me. Filed as Issue 402206.