Bug 43820 – Regression in linking of programs

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kenneth Russell]

https://bugs.webkit.org/show_bug.cgi?id=41380 introduced a regression in the linking of programs when the shaders are first attached to a program, then deleted, and then the program is linked. The WebGLShader object has its m_object cleared when deleteShader is called from JavaScript. Later, during linkProgram, the WebGLRenderingContext attempts to look up the WebGLShaderObject associated with the program and fails.

The validation of the attached shaders' types must be done with GraphicsContext3D calls instead, calling getShaderiv(shader, SHADER_TYPE, &val).

[Zhenyao Mo]

The real problem is when we deleteShader, if a shader is attached to a program, it's not really deleted; instead, DELETE_STATUS is set to true.  Currently in deleteShader, the name in WebGLShader is always set to 0.  Therefore, when we call getAttachedShaders(), we can't find any matching WebGLShader objects.

The fix should be tracking whether a shader is attached and don't set the name in WebGLShader to 0 when deleteShader if it is attached.

[Kenneth Russell]

(In reply to comment #1)
> The real problem is when we deleteShader, if a shader is attached to a program, it's not really deleted; instead, DELETE_STATUS is set to true.  Currently in deleteShader, the name in WebGLShader is always set to 0.  Therefore, when we call getAttachedShaders(), we can't find any matching WebGLShader objects.
> 
> The fix should be tracking whether a shader is attached and don't set the name in WebGLShader to 0 when deleteShader if it is attached.

This isn't the only situation in which the WebGLObject wrappers might get out of sync with the underlying OpenGL resources. Another example would be WebGLRenderbuffer. When a renderbuffer is attached to a framebuffer object, the framebuffer references it, and even if the renderbuffer is deleted, it is still referenced until detached from the framebuffer (or the framebuffer is deleted). In order to properly handle lookups of these renderbuffers, we would need to also track whether they are attached to a framebuffer at the time they are deleted. This tracking logic will quickly become complicated; it will be needed for nearly every WebGLObject subclass.

I believe the correct fix is to avoid looking up the WebGLShader object at all, and simply perform the needed queries against the underlying OpenGL shader object.

[Zhenyao Mo]

(In reply to comment #2)
> (In reply to comment #1)
> > The real problem is when we deleteShader, if a shader is attached to a program, it's not really deleted; instead, DELETE_STATUS is set to true.  Currently in deleteShader, the name in WebGLShader is always set to 0.  Therefore, when we call getAttachedShaders(), we can't find any matching WebGLShader objects.
> > 
> > The fix should be tracking whether a shader is attached and don't set the name in WebGLShader to 0 when deleteShader if it is attached.
> 
> This isn't the only situation in which the WebGLObject wrappers might get out of sync with the underlying OpenGL resources. Another example would be WebGLRenderbuffer. When a renderbuffer is attached to a framebuffer object, the framebuffer references it, and even if the renderbuffer is deleted, it is still referenced until detached from the framebuffer (or the framebuffer is deleted). In order to properly handle lookups of these renderbuffers, we would need to also track whether they are attached to a framebuffer at the time they are deleted. This tracking logic will quickly become complicated; it will be needed for nearly every WebGLObject subclass.
> 
> I believe the correct fix is to avoid looking up the WebGLShader object at all, and simply perform the needed queries against the underlying OpenGL shader object.

If we don't track WebGLShader objects, in the case of getAttachedShaders() we have to create a second WebGLShader object where the underlying name is the same as the original WebGLShader object.  This will really be confusing.

So at least for the case of WebGLShader, I think we should do the tracking.

[Kenneth Russell]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > The real problem is when we deleteShader, if a shader is attached to a program, it's not really deleted; instead, DELETE_STATUS is set to true.  Currently in deleteShader, the name in WebGLShader is always set to 0.  Therefore, when we call getAttachedShaders(), we can't find any matching WebGLShader objects.
> > > 
> > > The fix should be tracking whether a shader is attached and don't set the name in WebGLShader to 0 when deleteShader if it is attached.
> > 
> > This isn't the only situation in which the WebGLObject wrappers might get out of sync with the underlying OpenGL resources. Another example would be WebGLRenderbuffer. When a renderbuffer is attached to a framebuffer object, the framebuffer references it, and even if the renderbuffer is deleted, it is still referenced until detached from the framebuffer (or the framebuffer is deleted). In order to properly handle lookups of these renderbuffers, we would need to also track whether they are attached to a framebuffer at the time they are deleted. This tracking logic will quickly become complicated; it will be needed for nearly every WebGLObject subclass.
> > 
> > I believe the correct fix is to avoid looking up the WebGLShader object at all, and simply perform the needed queries against the underlying OpenGL shader object.
> 
> If we don't track WebGLShader objects, in the case of getAttachedShaders() we have to create a second WebGLShader object where the underlying name is the same as the original WebGLShader object.  This will really be confusing.
> 
> So at least for the case of WebGLShader, I think we should do the tracking.

OK, I see your point about the public APIs needing to work properly. Agreed, the best way to make both work is to track whether a given shader has really been deleted, or whether it is still referenced from a program.

[Zhenyao Mo]

Created attachment 64184 [details]
patch

The test is in sync with khronos.

Also tested: the previously failed WebGL Google demos run OK with this patch.

[Kenneth Russell]

Comment on attachment 64184 [details]
patch

Overall the patch looks good; thanks for taking care of this quickly. The new tests look good.

WebCore/html/canvas/WebGLProgram.h:76
 +      WebGLShader* m_fragmentShader;
There's a deep issue here related to the lifetime of WebGLObjects. Because they are reference counted, they can theoretically be deleted at any time. (The global HashSet of RefPtrs to WebGLObjects in the WebGLRenderingContext prevents this somewhat, but there are still problems because the order the objects are stored in that HashSet is not guaranteed, so neither is deletion order.) These raw pointers may therefore go bad spontaneously. I think the only solution is to make these RefPtrs. I also think it would be great if we could just reuse the RefCounted mechanism rather than adding the new onAttached/onDetached, but I think it would be tricky -- likely involving logic like "if (refCount() <= 2) m_object = 0;" -- and also I'm not 100% sure we would be able to intercept the ref() / deref() calls since they aren't virtual in RefCountedBase. (I think since RefPtr always calls against the most specifically available subclass that we could simply shadow the superclass version and explicitly call it.)

Please make these RefPtrs and re-test. Thanks.

[Zhenyao Mo]

Created attachment 64281 [details]
revised patch: use RefPtr for shader members in WebGLProgram

no other change

[Kenneth Russell]

Comment on attachment 64281 [details]
revised patch: use RefPtr for shader members in WebGLProgram

Looks good to me -- assuming this change builds and was re-tested.

[Zhenyao Mo]

Created attachment 64282 [details]
revised patch: no need to initialize RefPtr.

[Kenneth Russell]

Comment on attachment 64282 [details]
revised patch: no need to initialize RefPtr.

Looks fine.

[Dimitri Glazkov (Google)]

Comment on attachment 64282 [details]
revised patch: no need to initialize RefPtr.

Ok, but the attach/detach logic looks very similar to RefCounted. Can we just reuse that somehow?

[Kenneth Russell]

(In reply to comment #11)
> (From update of attachment 64282 [details])
> Ok, but the attach/detach logic looks very similar to RefCounted. Can we just reuse that somehow?

Mo and I talked about this, but at least for the initial version we'd like to keep it separate. More thought is needed on exactly how the RefCounted reference count maps to the numbers we would need to track to decide when to delete the underlying OpenGL resource (see my comment above about "refCount() <= 2").

[Zhenyao Mo]

Committed r65330: <http://trac.webkit.org/changeset/65330>

[WebKit Review Bot]

http://trac.webkit.org/changeset/65330 might have broken SnowLeopard Intel Release (Tests)