Bug 214765

<rdar://problem/66401953>

Adding Sam and Chris who might have advice here.

I see expando-loss passing. expando-loss-2 is failing though. Direct link: https://www.khronos.org/registry/webgl/conformance-suites/2.0.0/conformance2/misc/expando-loss-2.html

Using Strong is usually not the right solution for GC problems. Rather, the bindings layer likely needs custom marking to inform the GC about the relationships. In most cases, this is done by using the JSCustomMarkFunction extended attribute in the IDL file, and then implementing a custom visitAdditionalChildren for the wrapper.

(In reply to Sam Weinig from comment #4) > Using Strong is usually not the right solution for GC problems. Rather, the > bindings layer likely needs custom marking to inform the GC about the > relationships. > > In most cases, this is done by using the JSCustomMarkFunction extended > attribute in the IDL file, and then implementing a custom > visitAdditionalChildren for the wrapper. Alternatively, if the objects that are getting collected have references to their parents, you could fix this on their side, by marking them reachable from the WebGLRenderingContextBase. It work was done towards this for WebGL1 maybe, see uses of GenerateIsReachable=ImplWebGLRenderingContext.

(In reply to Sam Weinig from comment #5) > (In reply to Sam Weinig from comment #4) > > Using Strong is usually not the right solution for GC problems. Rather, the > > bindings layer likely needs custom marking to inform the GC about the > > relationships. > > > > In most cases, this is done by using the JSCustomMarkFunction extended > > attribute in the IDL file, and then implementing a custom > > visitAdditionalChildren for the wrapper. > > Alternatively, if the objects that are getting collected have references to > their parents, you could fix this on their side, by marking them reachable > from the WebGLRenderingContextBase. It work was done towards this for WebGL1 > maybe, see uses of GenerateIsReachable=ImplWebGLRenderingContext. Hm, no, it looks like GenerateIsReachable won't work for at least WebGLTexture, as it inherits from WebGLSharedObject, and therefore can belong to multiple contexts it seems. So the contexts will each have to mark the textures they currently own.

Thanks very much for the tips Sam. I was going down the wrong track trying to use Strong for this purpose. It looks like extending JSWebGLRenderingContext's and JSWebGL2RenderingContext's existing visitAdditionalChildren callbacks will be both simpler and more correct.

Created attachment 406340 [details] Patch

This patch implements what I understood as being the needed tracing, but it doesn't work - the results produced by: ./Tools/Scripts/run-webkit-tests --debug webgl/2.0.0/conformance/misc/expando-loss.html ./Tools/Scripts/run-webkit-tests --debug webgl/2.0.0/conformance2/misc/expando-loss-2.html are identical to what they are currently - which are failing results. Would appreciate any advice. Thanks.

Note also - would appreciate help with the unresolved symbol JSC::SlotVisitor::addOpaqueRoot(void*) on the tv, tv-sim, watch-sim platforms. Maybe I hooked this up incorrectly, but ideally the tracing code will live inside the WebGL classes rather than the bindings.

Looks like this patch only works in Mac Debug builds - which is the flavor I was testing.

ysuzuki@ helped me figure out what was wrong with the earlier patch - have a version that's working now. Obsoleting the old one.

Created attachment 406359 [details] Patch

Comment on attachment 406359 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406359&action=review > Source/WebCore/html/canvas/WebGLFramebuffer.h:38 > +} // namespace JSC Normally we reserve this kind of comment for larger namespace blocks.

Thanks Darin for your review and Yusuke for your help on this patch. Removing the extraneous namespace comments and CQ'ing.

Created attachment 406387 [details] Patch

Committed r265502: <https://trac.webkit.org/changeset/265502> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406387 [details].

Comment on attachment 406387 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406387&action=review Commented about locking & FIXME :) > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:3041 > + WebGLRenderingContextBase::visitReferencedJSWrappers(visitor); > + > + visitor.addOpaqueRoot(m_readFramebufferBinding.get()); > + if (m_readFramebufferBinding) > + m_readFramebufferBinding->visitReferencedJSWrappers(visitor); > + > + visitor.addOpaqueRoot(m_boundTransformFeedback.get()); > + if (m_boundTransformFeedback) > + m_boundTransformFeedback->visitReferencedJSWrappers(visitor); > + > + visitor.addOpaqueRoot(m_boundCopyReadBuffer.get()); > + visitor.addOpaqueRoot(m_boundCopyWriteBuffer.get()); > + visitor.addOpaqueRoot(m_boundPixelPackBuffer.get()); > + visitor.addOpaqueRoot(m_boundPixelUnpackBuffer.get()); > + visitor.addOpaqueRoot(m_boundTransformFeedbackBuffer.get()); > + visitor.addOpaqueRoot(m_boundUniformBuffer.get()); > + > + for (auto& buffer : m_boundIndexedUniformBuffers) > + visitor.addOpaqueRoot(buffer.get()); > + > + for (auto& entry : m_activeQueries) > + visitor.addOpaqueRoot(entry.value.get()); > + > + for (auto& entry : m_boundSamplers) > + visitor.addOpaqueRoot(entry.get()); GC marking can run concurrently to the main thread. So we should have a lock when touching these fields. Can you add that? > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:3042 > +} Can you also add FIXME to change this model to querying connected WebGLContext instead? i.e. we should not register a lot of opaque roots here, and instead, each object should have a reference to WebGLContext and say "This is live if WebGLContext is live". This model is not good in terms of performance, and we should fix it. > Source/WebCore/html/canvas/WebGLBuffer.idl:28 > + GenerateIsReachable=Impl Can you also add FIXME to change this model to querying connected WebGLContext instead? i.e. we should not register a lot of opaque roots here, and instead, each object should have a reference to WebGLContext and say "This is live if WebGLContext is live". This model is not good in terms of performance, and we should fix it. > Source/WebCore/html/canvas/WebGLFramebuffer.cpp:145 > + visitor.addOpaqueRoot(m_renderbuffer.get()); Can you also add FIXME to change this model to querying connected WebGLContext instead? i.e. we should not register a lot of opaque roots here, and instead, each object should have a reference to WebGLContext and say "This is live if WebGLContext is live". This model is not good in terms of performance, and we should fix it. > Source/WebCore/html/canvas/WebGLFramebuffer.cpp:267 > + visitor.addOpaqueRoot(m_texture.get()); Can you also add FIXME to change this model to querying connected WebGLContext instead? i.e. we should not register a lot of opaque roots here, and instead, each object should have a reference to WebGLContext and say "This is live if WebGLContext is live". This model is not good in terms of performance, and we should fix it. > Source/WebCore/html/canvas/WebGLFramebuffer.cpp:708 > + for (auto& entry : m_attachments) Can you also add FIXME to change this model to querying connected WebGLContext instead? i.e. we should not register a lot of opaque roots here, and instead, each object should have a reference to WebGLContext and say "This is live if WebGLContext is live". This model is not good in terms of performance, and we should fix it. > Source/WebCore/html/canvas/WebGLFramebuffer.idl:28 > + GenerateIsReachable=Impl Can you also add FIXME to change this model to querying connected WebGLContext instead? i.e. we should not register a lot of opaque roots here, and instead, each object should have a reference to WebGLContext and say "This is live if WebGLContext is live". This model is not good in terms of performance, and we should fix it. > Source/WebCore/html/canvas/WebGLProgram.cpp:204 > + visitor.addOpaqueRoot(m_fragmentShader.get()); Can you also add FIXME to change this model to querying connected WebGLContext instead? i.e. we should not register a lot of opaque roots here, and instead, each object should have a reference to WebGLContext and say "This is live if WebGLContext is live". This model is not good in terms of performance, and we should fix it. > Source/WebCore/html/canvas/WebGLProgram.idl:28 > + GenerateIsReachable=Impl Can you also add FIXME to change this model to querying connected WebGLContext instead? i.e. we should not register a lot of opaque roots here, and instead, each object should have a reference to WebGLContext and say "This is live if WebGLContext is live". This model is not good in terms of performance, and we should fix it. > Source/WebCore/html/canvas/WebGLQuery.idl:28 > + GenerateIsReachable=Impl Can you also add FIXME to change this model to querying connected WebGLContext instead? i.e. we should not register a lot of opaque roots here, and instead, each object should have a reference to WebGLContext and say "This is live if WebGLContext is live". This model is not good in terms of performance, and we should fix it. > Source/WebCore/html/canvas/WebGLRenderbuffer.idl:28 > + GenerateIsReachable=Impl Can you also add FIXME to change this model to querying connected WebGLContext instead? i.e. we should not register a lot of opaque roots here, and instead, each object should have a reference to WebGLContext and say "This is live if WebGLContext is live". This model is not good in terms of performance, and we should fix it. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:7798 > + visitor.addOpaqueRoot(m_boundArrayBuffer.get()); > + > + visitor.addOpaqueRoot(m_boundVertexArrayObject.get()); > + if (m_boundVertexArrayObject) > + m_boundVertexArrayObject->visitReferencedJSWrappers(visitor); > + > + visitor.addOpaqueRoot(m_currentProgram.get()); > + if (m_currentProgram) > + m_currentProgram->visitReferencedJSWrappers(visitor); > + > + visitor.addOpaqueRoot(m_framebufferBinding.get()); > + if (m_framebufferBinding) > + m_framebufferBinding->visitReferencedJSWrappers(visitor); > + > + visitor.addOpaqueRoot(m_renderbufferBinding.get()); > + > + for (auto& unit : m_textureUnits) { > + visitor.addOpaqueRoot(unit.texture2DBinding.get()); > + visitor.addOpaqueRoot(unit.textureCubeMapBinding.get()); > + visitor.addOpaqueRoot(unit.texture3DBinding.get()); > + visitor.addOpaqueRoot(unit.texture2DArrayBinding.get()); > + } > + > + // Extensions' IDL files use GenerateIsReachable=ImplWebGLRenderingContext, > + // which checks to see whether the context is in the opaque root set (it is; > + // it's added in JSWebGLRenderingContext / JSWebGL2RenderingContext's custom > + // bindings code). For this reason it's unnecessary to explicitly add opaque > + // roots for extensions. We should have a lock for each access to these fields due to the reason described above. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:7799 > +} Can you also add FIXME to change this model to querying connected WebGLContext instead? i.e. we should not register a lot of opaque roots here, and instead, each object should have a reference to WebGLContext and say "This is live if WebGLContext is live". This model is not good in terms of performance, and we should fix it. > Source/WebCore/html/canvas/WebGLSampler.idl:28 > + GenerateIsReachable=Impl Can you also add FIXME to change this model to querying connected WebGLContext instead? i.e. we should not register a lot of opaque roots here, and instead, each object should have a reference to WebGLContext and say "This is live if WebGLContext is live". This model is not good in terms of performance, and we should fix it. > Source/WebCore/html/canvas/WebGLShader.idl:28 > + GenerateIsReachable=Impl Can you also add FIXME to change this model to querying connected WebGLContext instead? i.e. we should not register a lot of opaque roots here, and instead, each object should have a reference to WebGLContext and say "This is live if WebGLContext is live". This model is not good in terms of performance, and we should fix it. > Source/WebCore/html/canvas/WebGLTexture.idl:28 > + GenerateIsReachable=Impl Can you also add FIXME to change this model to querying connected WebGLContext instead? i.e. we should not register a lot of opaque roots here, and instead, each object should have a reference to WebGLContext and say "This is live if WebGLContext is live". This model is not good in terms of performance, and we should fix it. > Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:96 > + for (auto& buffer : m_boundIndexedTransformFeedbackBuffers) > + visitor.addOpaqueRoot(buffer.get()); > + > + visitor.addOpaqueRoot(m_program.get()); We should have a lock for each access of these fields due to the above reason. > Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:98 > + Can you also add FIXME to change this model to querying connected WebGLContext instead? i.e. we should not register a lot of opaque roots here, and instead, each object should have a reference to WebGLContext and say "This is live if WebGLContext is live". This model is not good in terms of performance, and we should fix it. > Source/WebCore/html/canvas/WebGLTransformFeedback.idl:29 > + GenerateIsReachable=Impl Can you also add FIXME to change this model to querying connected WebGLContext instead? i.e. we should not register a lot of opaque roots here, and instead, each object should have a reference to WebGLContext and say "This is live if WebGLContext is live". This model is not good in terms of performance, and we should fix it. > Source/WebCore/html/canvas/WebGLVertexArrayObject.idl:29 > + GenerateIsReachable=Impl Can you also add FIXME to change this model to querying connected WebGLContext instead? i.e. we should not register a lot of opaque roots here, and instead, each object should have a reference to WebGLContext and say "This is live if WebGLContext is live". This model is not good in terms of performance, and we should fix it. > Source/WebCore/html/canvas/WebGLVertexArrayObjectBase.cpp:114 > + visitor.addOpaqueRoot(m_boundElementArrayBuffer.get()); > + for (auto& state : m_vertexAttribState) > + visitor.addOpaqueRoot(state.bufferBinding.get()); Ditto. Need locking. > Source/WebCore/html/canvas/WebGLVertexArrayObjectBase.cpp:115 > +} Can you also add FIXME to change this model to querying connected WebGLContext instead? i.e. we should not register a lot of opaque roots here, and instead, each object should have a reference to WebGLContext and say "This is live if WebGLContext is live". This model is not good in terms of performance, and we should fix it. > Source/WebCore/html/canvas/WebGLVertexArrayObjectOES.idl:29 > + GenerateIsReachable=Impl Can you also add FIXME to change this model to querying connected WebGLContext instead? i.e. we should not register a lot of opaque roots here, and instead, each object should have a reference to WebGLContext and say "This is live if WebGLContext is live". This model is not good in terms of performance, and we should fix it.

Oops, guess I should not have done review+. Concerned about the threading issue!

ysuzuki@ thanks for your review. I'll file a follow-on bug about addressing your feedback, specifically the locking issue. However - I disagree with the idea of adding a FIXME to change "GenerateIsReachable=Impl" to querying the WebGL context. It would be *very* expensive to answer the question for any WebGLTexture, WebGLBuffer, etc. "am I latched into the WebGL context state"? That would basically mean searching through all of the latched objects that this patch currently enumerates, looking to see if it's referenced - and that would be done for *every* reachable WebGL object during GC.

Filed Bug 215394 about addressing ysuzuki@'s review feedback.

Re-opened since this is blocked by bug 215396

After discussion with ysuzuki@ on Slack, reverting the above patch in Bug 215396 because designing the locking mechanism is going to take some time.

Revert landed in r265523. Will re-attempt.

Created attachment 406462 [details] Patch

ysuzuki@ could you please review this new patch? The new locking covers all of the data structures like HashMaps and Vectors which are traversed by visitAdditionalChildren and their callees, as well as mutations of RefPtrs whose contents are tested via if-tests before descending into them. Thanks for explaining the concurrent marking to me!

Comment on attachment 406462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406462&action=review > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:698 > + LockHolder locker(m_objectGraphLock); Use auto locker = holdLock(m_objectGraphLock) instead. > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1847 > + LockHolder locker(objectGraphLock()); Ditto. > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1878 > + LockHolder locker(objectGraphLock()); Ditto. > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1903 > + LockHolder locker(objectGraphLock()); Ditto. > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1976 > + LockHolder locker(objectGraphLock()); Ditto. > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2002 > + LockHolder locker(objectGraphLock()); Ditto. > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2191 > + LockHolder locker(objectGraphLock()); Ditto. > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2261 > + LockHolder locker(objectGraphLock()); Ditto. > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:3032 > +void WebGL2RenderingContext::visitReferencedJSWrappers(JSC::SlotVisitor& visitor) I think we should rename this to, something like `addMembersToOpaqueRoots` because this function is not actually visiting as GC does. > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:3033 > +{ This function is not grabbing a lock. > Source/WebCore/html/canvas/WebGLFramebuffer.cpp:144 > + void WebGLRenderbufferAttachment::visitReferencedJSWrappers(JSC::SlotVisitor& visitor) Ditto. > Source/WebCore/html/canvas/WebGLFramebuffer.cpp:266 > + void WebGLTextureAttachment::visitReferencedJSWrappers(JSC::SlotVisitor& visitor) Ditto. > Source/WebCore/html/canvas/WebGLFramebuffer.cpp:713 > +void WebGLFramebuffer::visitReferencedJSWrappers(JSC::SlotVisitor& visitor) Ditto. > Source/WebCore/html/canvas/WebGLProgram.cpp:202 > +void WebGLProgram::visitReferencedJSWrappers(JSC::SlotVisitor& visitor) Ditto. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1324 > + LockHolder locker(m_objectGraphLock); Ditto. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1429 > + LockHolder locker(m_objectGraphLock); Ditto. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1442 > + LockHolder locker(m_objectGraphLock); Ditto. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1457 > + LockHolder locker(m_objectGraphLock); Ditto. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1473 > + LockHolder locker(m_objectGraphLock); Ditto. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:2017 > + LockHolder locker(objectGraphLock()); Ditto. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:2026 > + LockHolder locker(m_objectGraphLock); Ditto. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:2042 > + LockHolder locker(objectGraphLock()); Ditto. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:2143 > + LockHolder locker(m_objectGraphLock); Ditto. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5922 > + LockHolder locker(m_objectGraphLock); Ditto. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5989 > + LockHolder locker(m_objectGraphLock); Ditto. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:7795 > +void WebGLRenderingContextBase::visitReferencedJSWrappers(JSC::SlotVisitor& visitor) Ditto. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:7797 > + LockHolder lock(m_objectGraphLock); Ditto. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:7832 > +Lock* WebGLRenderingContextBase::objectGraphLock() > +{ > + return &m_objectGraphLock; > +} Let's just return Lock&. > Source/WebCore/html/canvas/WebGLSharedObject.cpp:65 > + return m_contextGroup ? m_contextGroup->objectGraphLockForAContext() : nullptr; When it is returning nullptr, shouldn't we need to take a lock? > Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:93 > +void WebGLTransformFeedback::visitReferencedJSWrappers(JSC::SlotVisitor& visitor) Ditto. > Source/WebCore/html/canvas/WebGLVertexArrayObjectBase.cpp:112 > +void WebGLVertexArrayObjectBase::visitReferencedJSWrappers(JSC::SlotVisitor& visitor) Ditto.

Comment on attachment 406462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406462&action=review New patch incoming which addresses this review feedback and also fixes the timing-out layout tests. >> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:698 >> + LockHolder locker(m_objectGraphLock); > > Use > > auto locker = holdLock(m_objectGraphLock) instead. Done. >> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1847 >> + LockHolder locker(objectGraphLock()); > > Ditto. Done. >> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1878 >> + LockHolder locker(objectGraphLock()); > > Ditto. Done. >> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1903 >> + LockHolder locker(objectGraphLock()); > > Ditto. Done. >> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1976 >> + LockHolder locker(objectGraphLock()); > > Ditto. Done. >> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2002 >> + LockHolder locker(objectGraphLock()); > > Ditto. Done. >> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2191 >> + LockHolder locker(objectGraphLock()); > > Ditto. Done. >> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2261 >> + LockHolder locker(objectGraphLock()); > > Ditto. Done. >> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:3032 >> +void WebGL2RenderingContext::visitReferencedJSWrappers(JSC::SlotVisitor& visitor) > > I think we should rename this to, something like `addMembersToOpaqueRoots` because this function is not actually visiting as GC does. Renamed this and the related methods. >> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:3033 >> +{ > > This function is not grabbing a lock. Thanks for catching that, I'll fix. WebGLRenderingContextBase::visitReferencedJSWrappers does. It wasn't clear to me whether Lock supports recursive locking or not. >> Source/WebCore/html/canvas/WebGLFramebuffer.cpp:144 >> + void WebGLRenderbufferAttachment::visitReferencedJSWrappers(JSC::SlotVisitor& visitor) > > Ditto. Renamed. >> Source/WebCore/html/canvas/WebGLFramebuffer.cpp:266 >> + void WebGLTextureAttachment::visitReferencedJSWrappers(JSC::SlotVisitor& visitor) > > Ditto. Renamed. >> Source/WebCore/html/canvas/WebGLFramebuffer.cpp:713 >> +void WebGLFramebuffer::visitReferencedJSWrappers(JSC::SlotVisitor& visitor) > > Ditto. Renamed. >> Source/WebCore/html/canvas/WebGLProgram.cpp:202 >> +void WebGLProgram::visitReferencedJSWrappers(JSC::SlotVisitor& visitor) > > Ditto. Renamed. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1324 >> + LockHolder locker(m_objectGraphLock); > > Ditto. Changed. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1429 >> + LockHolder locker(m_objectGraphLock); > > Ditto. Changed. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1442 >> + LockHolder locker(m_objectGraphLock); > > Ditto. Changed. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1457 >> + LockHolder locker(m_objectGraphLock); > > Ditto. Changed. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1473 >> + LockHolder locker(m_objectGraphLock); > > Ditto. Changed. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:2017 >> + LockHolder locker(objectGraphLock()); > > Ditto. Changed. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:2026 >> + LockHolder locker(m_objectGraphLock); > > Ditto. Changed. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:2042 >> + LockHolder locker(objectGraphLock()); > > Ditto. Changed. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:2143 >> + LockHolder locker(m_objectGraphLock); > > Ditto. Changed. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5922 >> + LockHolder locker(m_objectGraphLock); > > Ditto. Changed. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5989 >> + LockHolder locker(m_objectGraphLock); > > Ditto. Changed. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:7795 >> +void WebGLRenderingContextBase::visitReferencedJSWrappers(JSC::SlotVisitor& visitor) > > Ditto. Renamed. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:7797 >> + LockHolder lock(m_objectGraphLock); > > Ditto. Changed. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:7832 >> +} > > Let's just return Lock&. I thought this was going to be a problem, but after making everything compile using holdLock and Lock& elsewhere, there were only a few places that needed to check whether the context had been destroyed and return early rather than trying to grab the lock. Changed to Lock&. >> Source/WebCore/html/canvas/WebGLSharedObject.cpp:65 >> + return m_contextGroup ? m_contextGroup->objectGraphLockForAContext() : nullptr; > > When it is returning nullptr, shouldn't we need to take a lock? The few callers of this can check explicitly whether the context has been destroyed, so changed this to Lock&. >> Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:93 >> +void WebGLTransformFeedback::visitReferencedJSWrappers(JSC::SlotVisitor& visitor) > > Ditto. Renamed. >> Source/WebCore/html/canvas/WebGLVertexArrayObjectBase.cpp:112 >> +void WebGLVertexArrayObjectBase::visitReferencedJSWrappers(JSC::SlotVisitor& visitor) > > Ditto. Renamed.

Created attachment 406483 [details] Patch

New patch addresses all review feedback and fixes the layout test timeouts from the previous patch, which were caused by an illegal attempt to recursively lock the object graph lock during framebuffer deletion.

Comment on attachment 406483 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406483&action=review > Source/WebCore/html/canvas/WebGLContextGroup.cpp:50 > return *(*m_contexts.begin())->graphicsContextGL(); Let's use `first()` here :) > Source/WebCore/html/canvas/WebGLContextGroup.cpp:61 > + return (*m_contexts.begin())->objectGraphLock(); Use `m_contexts.first().objectGraphLock();` > Source/WebCore/html/canvas/WebGLFramebuffer.cpp:563 > void WebGLFramebuffer::deleteObjectImpl(GraphicsContextGLOpenGL* context3d, PlatformGLObject object) Let's take `const AbstractLocker&` parameter instead. > Source/WebCore/html/canvas/WebGLProgram.cpp:161 > bool WebGLProgram::attachShader(WebGLShader* shader) Let's take `const AbstractLocker&` parameter to make it explicit that this function needs locker. > Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:61 > { Let's take `const AbstractLocker&` parameter. > Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:69 > + // Covered under the objectGraphLock in WebGL2RenderingContext::setIndexedBufferBinding. Let's take `const AbstractLocker&` parameter. > Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:94 > +{ Let's take `const AbstractLocker&` parameter. > Source/WebCore/html/canvas/WebGLVertexArrayObjectBase.cpp:54 > void WebGLVertexArrayObjectBase::setVertexAttribState(GCGLuint index, GCGLsizei bytesPerElement, GCGLint size, GCGLenum type, GCGLboolean normalized, GCGLsizei stride, GCGLintptr offset, WebGLBuffer* buffer) Let's take `const AbstractLocker&` from WebGLRenderingContextBase::vertexAttribPointer (passing `locker`). Like, ...->setVertexAttribState(locker, ...); And ...::setVertexAttribState(const AbstractLocker&, ...) { .... } > Source/WebCore/html/canvas/WebGLVertexArrayObjectBase.cpp:76 > void WebGLVertexArrayObjectBase::unbindBuffer(WebGLBuffer& buffer) Let's take `const AbstractLocker&` from WebGLRenderingContextBase::uncacheDeletedBuffer (passing `locker`). > Source/WebCore/html/canvas/WebGLVertexArrayObjectBase.cpp:112 > +void WebGLVertexArrayObjectBase::addMembersToOpaqueRoots(JSC::SlotVisitor& visitor) Let's take `const AbstractLocker&` parameter.

Comment on attachment 406483 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406483&action=review >> Source/WebCore/html/canvas/WebGLContextGroup.cpp:50 >> return *(*m_contexts.begin())->graphicsContextGL(); > > Let's use `first()` here :) Neither HashSet nor HashMap in WTF has a first() method. >> Source/WebCore/html/canvas/WebGLContextGroup.cpp:61 >> + return (*m_contexts.begin())->objectGraphLock(); > > Use `m_contexts.first().objectGraphLock();` Same answer - neither HashSet nor HashMap in WTF has a first() method. >> Source/WebCore/html/canvas/WebGLFramebuffer.cpp:563 >> void WebGLFramebuffer::deleteObjectImpl(GraphicsContextGLOpenGL* context3d, PlatformGLObject object) > > Let's take `const AbstractLocker&` parameter instead. Done. >> Source/WebCore/html/canvas/WebGLProgram.cpp:161 >> bool WebGLProgram::attachShader(WebGLShader* shader) > > Let's take `const AbstractLocker&` parameter to make it explicit that this function needs locker. Done. >> Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:61 >> { > > Let's take `const AbstractLocker&` parameter. Done. >> Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:69 >> + // Covered under the objectGraphLock in WebGL2RenderingContext::setIndexedBufferBinding. > > Let's take `const AbstractLocker&` parameter. Done. >> Source/WebCore/html/canvas/WebGLTransformFeedback.cpp:94 >> +{ > > Let's take `const AbstractLocker&` parameter. Done. >> Source/WebCore/html/canvas/WebGLVertexArrayObjectBase.cpp:54 >> void WebGLVertexArrayObjectBase::setVertexAttribState(GCGLuint index, GCGLsizei bytesPerElement, GCGLint size, GCGLenum type, GCGLboolean normalized, GCGLsizei stride, GCGLintptr offset, WebGLBuffer* buffer) > > Let's take `const AbstractLocker&` from WebGLRenderingContextBase::vertexAttribPointer (passing `locker`). > Like, > > ...->setVertexAttribState(locker, ...); > > And > > ...::setVertexAttribState(const AbstractLocker&, ...) > { > .... > } Done. >> Source/WebCore/html/canvas/WebGLVertexArrayObjectBase.cpp:76 >> void WebGLVertexArrayObjectBase::unbindBuffer(WebGLBuffer& buffer) > > Let's take `const AbstractLocker&` from WebGLRenderingContextBase::uncacheDeletedBuffer (passing `locker`). Done. >> Source/WebCore/html/canvas/WebGLVertexArrayObjectBase.cpp:112 >> +void WebGLVertexArrayObjectBase::addMembersToOpaqueRoots(JSC::SlotVisitor& visitor) > > Let's take `const AbstractLocker&` parameter. Done.

Created attachment 406554 [details] Patch

The current patch passes "const AbstractLocker&" throughout WebGL's call hierarchy to prove that the lock's held where needed. This was a substantial change involving many more methods than just those highlighted via comments in the previous code review. This passes all tests locally. During development, bugs caused hangs or assertion failures in the WebGL tests, so it seems that correctness is ensured by those tests. Please re-review. Thanks.

Comment on attachment 406554 [details] Patch If this looks OK then please go ahead and CQ it - thanks.

Comment on attachment 406554 [details] Patch r=me

Committed r265708: <https://trac.webkit.org/changeset/265708> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406554 [details].