Bug 214765 - [WebGL2] expando-loss and expando-loss-2 conformance tests are failing
Summary: [WebGL2] expando-loss and expando-loss-2 conformance tests are failing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords: InRadar
Depends on: 215396
Blocks: 126404 189641 189672 215394 215433
  Show dependency treegraph
 
Reported: 2020-07-24 15:34 PDT by Kenneth Russell
Modified: 2020-08-14 15:06 PDT (History)
14 users (show)

See Also:


Attachments
Patch (19.36 KB, patch)
2020-08-10 16:11 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (62.63 KB, patch)
2020-08-10 21:27 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (62.58 KB, patch)
2020-08-11 09:43 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (83.25 KB, patch)
2020-08-12 10:05 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (84.07 KB, patch)
2020-08-12 16:48 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (144.24 KB, patch)
2020-08-13 16:28 PDT, Kenneth Russell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2020-07-24 15:34:54 PDT
Per Bug 189641 and Bug 189672 the following two WebGL conformance tests are failing:

webgl/2.0.0/conformance/misc/expando-loss.html
webgl/2.0.0/conformance2/misc/expando-loss-2.html

the reason is that the WebGL rendering context maintains RefPtrs to objects which are latched into the OpenGL context state, but those implicitly retain weak pointers to their JavaScript wrappers.

https://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/heap/Strong.h appears to be designed to hold a strong reference to its target JavaScript object, and it is used in a select few places in WebCore - for example, https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/bindings/js/WorkerScriptController.h .

In order to fix the expando-loss failures, it seems likely that WebGLRenderingContextBase and WebGL2RenderingContext will have to retain strong references to the objects which are latched in to the context state, and which can be queried back from the context - like WebGLTextures, via bindTexture and getParameter(TEXTURE_BINDING_2D). In this case it seems the TextureUnitState will need to maintain Strong<JSWebGLTexture> instead of RefPtr<WebGLTexture>.

This will have to be carefully designed. Currently WebGLRenderingContextBase::m_contextObjects and WebGLContextGroup::m_groupObjects maintain weak downward references to all of the objects they allocated, and WebGLContextObject::m_context and WebGLSharedObject::m_contextGroup maintain weak upward references to the context which allocated them. Reference cycles must not be introduced.

A similar restructuring was done in Chromium in http://crbug.com/485634 . There was a significant bug tail associated with this change; a couple of the performance regressions were http://crbug.com/569668 and http://crbug.com/608576 . Care must be taken to not run into similar issues in WebCore.
Comment 1 Radar WebKit Bug Importer 2020-07-31 15:35:15 PDT
<rdar://problem/66401953>
Comment 2 Dean Jackson 2020-08-07 16:48:13 PDT
Adding Sam and Chris who might have advice here.
Comment 3 Dean Jackson 2020-08-07 17:46:00 PDT
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
Comment 4 Sam Weinig 2020-08-07 18:02:39 PDT
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.
Comment 5 Sam Weinig 2020-08-07 18:09:35 PDT
(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.
Comment 6 Sam Weinig 2020-08-07 18:20:55 PDT
(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.
Comment 7 Kenneth Russell 2020-08-10 12:42:46 PDT
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.
Comment 8 Kenneth Russell 2020-08-10 16:11:48 PDT
Created attachment 406340 [details]
Patch
Comment 9 Kenneth Russell 2020-08-10 16:13:49 PDT
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.
Comment 10 Kenneth Russell 2020-08-10 16:22:54 PDT
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.
Comment 11 Kenneth Russell 2020-08-10 16:28:24 PDT
Looks like this patch only works in Mac Debug builds - which is the flavor I was testing.
Comment 12 Kenneth Russell 2020-08-10 18:31:50 PDT
ysuzuki@ helped me figure out what was wrong with the earlier patch - have a version that's working now. Obsoleting the old one.
Comment 13 Kenneth Russell 2020-08-10 21:27:15 PDT
Created attachment 406359 [details]
Patch
Comment 14 Darin Adler 2020-08-11 08:45:35 PDT
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.
Comment 15 Kenneth Russell 2020-08-11 09:43:13 PDT
Thanks Darin for your review and Yusuke for your help on this patch. Removing the extraneous namespace comments and CQ'ing.
Comment 16 Kenneth Russell 2020-08-11 09:43:42 PDT
Created attachment 406387 [details]
Patch
Comment 17 EWS 2020-08-11 10:30:00 PDT
Committed r265502: <https://trac.webkit.org/changeset/265502>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406387 [details].
Comment 18 Yusuke Suzuki 2020-08-11 14:23:36 PDT
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.
Comment 19 Darin Adler 2020-08-11 14:25:33 PDT
Oops, guess I should not have done review+. Concerned about the threading issue!
Comment 20 Kenneth Russell 2020-08-11 14:29:51 PDT
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.
Comment 21 Kenneth Russell 2020-08-11 14:47:06 PDT
Filed Bug 215394 about addressing ysuzuki@'s review feedback.
Comment 22 WebKit Commit Bot 2020-08-11 15:00:25 PDT
Re-opened since this is blocked by bug 215396
Comment 23 Kenneth Russell 2020-08-11 15:01:55 PDT
After discussion with ysuzuki@ on Slack, reverting the above patch in Bug 215396 because designing the locking mechanism is going to take some time.
Comment 24 Kenneth Russell 2020-08-11 15:17:42 PDT
Revert landed in r265523. Will re-attempt.
Comment 25 Kenneth Russell 2020-08-12 10:05:38 PDT
Created attachment 406462 [details]
Patch
Comment 26 Kenneth Russell 2020-08-12 10:08:46 PDT
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 27 Yusuke Suzuki 2020-08-12 14:58:51 PDT
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 28 Kenneth Russell 2020-08-12 16:43:28 PDT
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.
Comment 29 Kenneth Russell 2020-08-12 16:48:09 PDT
Created attachment 406483 [details]
Patch
Comment 30 Kenneth Russell 2020-08-12 16:49:04 PDT
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 31 Yusuke Suzuki 2020-08-12 17:03:05 PDT
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 32 Kenneth Russell 2020-08-13 16:27:56 PDT
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.
Comment 33 Kenneth Russell 2020-08-13 16:28:08 PDT
Created attachment 406554 [details]
Patch
Comment 34 Kenneth Russell 2020-08-13 16:33:24 PDT
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 35 Kenneth Russell 2020-08-13 18:43:45 PDT
Comment on attachment 406554 [details]
Patch

If this looks OK then please go ahead and CQ it - thanks.
Comment 36 Yusuke Suzuki 2020-08-14 14:57:09 PDT
Comment on attachment 406554 [details]
Patch

r=me
Comment 37 EWS 2020-08-14 15:06:46 PDT
Committed r265708: <https://trac.webkit.org/changeset/265708>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406554 [details].