gl.detachShader breaks shader program

https://bugs.webkit.org/show_bug.cgi?id=137689
<rdar://problem/34025056>

Reviewed by Sam Weinig.

Source/WebCore:

It should be possible to compile shaders, attach them to a program,
link the program, detach the shaders, delete the shaders, and then
ask for the uniform and attribute locations. That is, once you've
linked, the shaders can be thrown away.

We were using the attached shaders to look up uniform locations, so
we now keep around a separate map that remembers what shaders were
attached when the program links.

This fixes the bug, but the whole area is still a bit messy. For one,
we're keeping around all the shader information even after it is
no longer used.
See https://bugs.webkit.org/show_bug.cgi?id=98204

Test: fast/canvas/webgl/detachShader-before-accessing-uniform.html

* platform/graphics/GraphicsContext3D.h: Add another map to remember
what shaders were used when a program was linked.
* platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:
(WebCore::GraphicsContext3D::mappedSymbolInShaderSourceMap): New helper
to look up a name in our source maps.
(WebCore::GraphicsContext3D::mappedSymbolName): Use the helper, and look
at linked shaders if there are no attached shaders.
(WebCore::GraphicsContext3D::originalSymbolInShaderSourceMap): Does the
reverse of the above.
(WebCore::GraphicsContext3D::originalSymbolName):
(WebCore::GraphicsContext3D::linkProgram): Add to the new map.
(WebCore::GraphicsContext3D::deleteProgram): Delete the program from
our shader entries.

LayoutTests:

Test that detaches and deletes shaders after linking, but before
asking for uniform locations.

* fast/canvas/webgl/detachShader-before-accessing-uniform-expected.txt: Added.
* fast/canvas/webgl/detachShader-before-accessing-uniform.html: Added.

Canonical link: https://commits.webkit.org/193180@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@221831 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog

@@ -1,3 +1,17 @@
+2017-09-08  Dean Jackson  <dino@apple.com>
+
+        gl.detachShader breaks shader program
+        https://bugs.webkit.org/show_bug.cgi?id=137689
+        <rdar://problem/34025056>
+
+        Reviewed by Sam Weinig.
+
+        Test that detaches and deletes shaders after linking, but before
+        asking for uniform locations.
+
+        * fast/canvas/webgl/detachShader-before-accessing-uniform-expected.txt: Added.
+        * fast/canvas/webgl/detachShader-before-accessing-uniform.html: Added.
+
 2017-09-09  Per Arne Vollan  <pvollan@apple.com>
 
         Mark http/tests/misc/object-embedding-svg-delayed-size-negotiation-2.htm as flaky.

LayoutTests/fast/canvas/webgl/detachShader-before-accessing-uniform-expected.txt

@@ -0,0 +1,7 @@
+Vertex Shader compiled.
+Fragment Shader compiled.
+Program linked. Detaching and deleting shaders.
+Color uniform location was identified.
+Position attribute location was identified.
+Drawn.
+

LayoutTests/fast/canvas/webgl/detachShader-before-accessing-uniform.html

@@ -0,0 +1,122 @@
+<style>
+canvas {
+    width: 200px;
+    height: 200px;
+}
+</style>
+</head>
+<script id="vertexShaderSource" type="text/glsl">
+attribute vec4 position;
+void main() {
+  gl_Position = position;
+}
+</script>
+<script id="fragmentShaderSource" type="text/glsl">
+precision mediump float;
+uniform vec4 color;
+void main() {
+  gl_FragColor = color;
+}
+</script>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+function output(msg) {
+    let d = document.querySelector("div");
+    d.innerHTML += `${msg}<br>`;
+}
+
+function drawTriangle() {
+
+    let canvas = document.querySelector("canvas");
+    canvas.width = 200;
+    canvas.height = 200;
+    let gl = canvas.getContext("webgl");
+
+    let vertexShader = gl.createShader(gl.VERTEX_SHADER);
+    gl.shaderSource(vertexShader, document.getElementById("vertexShaderSource").textContent);
+    gl.compileShader(vertexShader);
+    if (!gl.getShaderParameter(vertexShader, gl.COMPILE_STATUS)) {
+        output("Vertex Shader failed to compile.");
+        console.log(gl.getShaderInfoLog(vertexShader));
+        return;
+    }
+    output("Vertex Shader compiled.");
+
+    let fragmentShader = gl.createShader(gl.FRAGMENT_SHADER);
+    gl.shaderSource(fragmentShader, document.getElementById("fragmentShaderSource").textContent);
+    gl.compileShader(fragmentShader);
+    if (!gl.getShaderParameter(fragmentShader, gl.COMPILE_STATUS)) {
+        output("Fragment Shader failed to compile.");
+        console.log(gl.getShaderInfoLog(fragmentShader));
+        return;
+    }
+    output("Fragment Shader compiled.");
+
+    let program = gl.createProgram();
+    gl.attachShader(program, vertexShader);
+    gl.attachShader(program, fragmentShader);
+    gl.linkProgram(program);
+
+    if (!gl.getProgramParameter(program, gl.LINK_STATUS)) {
+        output("Unable to link shaders into program.");
+        return;
+    }
+    output("Program linked. Detaching and deleting shaders.");
+
+    gl.detachShader(program, vertexShader);
+    gl.detachShader(program, fragmentShader);
+    gl.deleteShader(vertexShader);
+    gl.deleteShader(fragmentShader);
+
+    gl.useProgram(program);
+
+    let colorUniform = gl.getUniformLocation(program, "color");
+    if (colorUniform)
+        output("Color uniform location was identified.");
+    else {
+        output("FAIL: Color uniform location was not found.");
+        return;
+    }
+
+    let positionAttribute = gl.getAttribLocation(program, "position");
+    if (positionAttribute >= 0)
+        output("Position attribute location was identified.");
+    else {
+        output("FAIL: Position attribute location was not found.");
+        return;
+    }
+
+    gl.enableVertexAttribArray(positionAttribute);
+
+    let vertices = new Float32Array([
+       -0.8, -0.3,
+       0.7, -0.8,
+       0.55, 0.75
+    ]);
+
+    let triangleBuffer = gl.createBuffer();
+
+    gl.bindBuffer(gl.ARRAY_BUFFER, triangleBuffer);
+    gl.bufferData(gl.ARRAY_BUFFER, vertices, gl.STATIC_DRAW);
+
+    gl.clearColor(0, 0, 0, 1);
+    gl.clear(gl.COLOR_BUFFER_BIT);
+
+    let now = Date.now();
+    gl.uniform4fv(colorUniform, [1.0, 0.0, 0.0, 1.0]);
+
+    gl.bindBuffer(gl.ARRAY_BUFFER, triangleBuffer);
+    gl.vertexAttribPointer(positionAttribute, 2, gl.FLOAT, false, 0, 0);
+
+    gl.drawArrays(gl.TRIANGLES, 0, 3);
+    output("Drawn.");
+}
+
+window.addEventListener("load", drawTriangle, false);
+</script>
+<body>
+    <canvas></canvas>
+    <div></div>
+</body>

Source/WebCore/ChangeLog

@@ -1,3 +1,41 @@
+2017-09-08  Dean Jackson  <dino@apple.com>
+
+        gl.detachShader breaks shader program
+        https://bugs.webkit.org/show_bug.cgi?id=137689
+        <rdar://problem/34025056>
+
+        Reviewed by Sam Weinig.
+
+        It should be possible to compile shaders, attach them to a program,
+        link the program, detach the shaders, delete the shaders, and then
+        ask for the uniform and attribute locations. That is, once you've
+        linked, the shaders can be thrown away.
+
+        We were using the attached shaders to look up uniform locations, so
+        we now keep around a separate map that remembers what shaders were
+        attached when the program links.
+
+        This fixes the bug, but the whole area is still a bit messy. For one,
+        we're keeping around all the shader information even after it is
+        no longer used.
+        See https://bugs.webkit.org/show_bug.cgi?id=98204
+
+        Test: fast/canvas/webgl/detachShader-before-accessing-uniform.html
+
+        * platform/graphics/GraphicsContext3D.h: Add another map to remember
+        what shaders were used when a program was linked.
+        * platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:
+        (WebCore::GraphicsContext3D::mappedSymbolInShaderSourceMap): New helper
+        to look up a name in our source maps.
+        (WebCore::GraphicsContext3D::mappedSymbolName): Use the helper, and look
+        at linked shaders if there are no attached shaders.
+        (WebCore::GraphicsContext3D::originalSymbolInShaderSourceMap): Does the
+        reverse of the above.
+        (WebCore::GraphicsContext3D::originalSymbolName):
+        (WebCore::GraphicsContext3D::linkProgram): Add to the new map.
+        (WebCore::GraphicsContext3D::deleteProgram): Delete the program from
+        our shader entries.
+
 2017-09-09  Mark Lam  <mark.lam@apple.com>
 
         Avoid duplicate computations of ExecState::vm().

Source/WebCore/platform/graphics/GraphicsContext3D.h

@@ -1348,9 +1348,14 @@ class GraphicsContext3D : public RefCounted<GraphicsContext3D> {
         }
     };
 
+    // FIXME: Shaders are never removed from this map, even if they and their program are deleted.
+    // This is bad, and it also relies on the fact we never reuse Platform3DObject numbers.
     typedef HashMap<Platform3DObject, ShaderSourceEntry> ShaderSourceMap;
     ShaderSourceMap m_shaderSourceMap;
 
+    typedef HashMap<Platform3DObject, std::pair<Platform3DObject, Platform3DObject>> LinkedShaderMap;
+    LinkedShaderMap m_linkedShaderMap;
+
     struct ActiveShaderSymbolCounts {
         Vector<GC3Dint> filteredToActualAttributeIndexMap;
         Vector<GC3Dint> filteredToActualUniformIndexMap;
@@ -1377,6 +1382,8 @@ class GraphicsContext3D : public RefCounted<GraphicsContext3D> {
     String mappedSymbolName(Platform3DObject program, ANGLEShaderSymbolType, const String& name);
     String mappedSymbolName(Platform3DObject shaders[2], size_t count, const String& name);
     String originalSymbolName(Platform3DObject program, ANGLEShaderSymbolType, const String& name);
+    std::optional<String> mappedSymbolInShaderSourceMap(Platform3DObject shader, ANGLEShaderSymbolType, const String& name);
+    std::optional<String> originalSymbolInShaderSourceMap(Platform3DObject shader, ANGLEShaderSymbolType, const String& name);
 
     std::unique_ptr<ShaderNameHash> nameHashMapForShaders;
 

Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp

@@ -965,23 +965,43 @@ static String generateHashedName(const String& name)
     return builder.toString();
 }
 
+std::optional<String> GraphicsContext3D::mappedSymbolInShaderSourceMap(Platform3DObject shader, ANGLEShaderSymbolType symbolType, const String& name)
+{
+    auto result = m_shaderSourceMap.find(shader);
+    if (result == m_shaderSourceMap.end())
+        return std::nullopt;
+
+    const auto& symbolMap = result->value.symbolMap(symbolType);
+    auto symbolEntry = symbolMap.find(name);
+    if (symbolEntry == symbolMap.end())
+        return std::nullopt;
+
+    auto& mappedName = symbolEntry->value.mappedName;
+    return String(mappedName.c_str(), mappedName.length());
+}
+
 String GraphicsContext3D::mappedSymbolName(Platform3DObject program, ANGLEShaderSymbolType symbolType, const String& name)
 {
     GC3Dsizei count = 0;
     Platform3DObject shaders[2] = { };
     getAttachedShaders(program, 2, &count, shaders);
 
     for (GC3Dsizei i = 0; i < count; ++i) {
-        ShaderSourceMap::iterator result = m_shaderSourceMap.find(shaders[i]);
-        if (result == m_shaderSourceMap.end())
-            continue;
+        auto mappedName = mappedSymbolInShaderSourceMap(shaders[i], symbolType, name);
+        if (mappedName)
+            return mappedName.value();
+    }
 
-        const ShaderSymbolMap& symbolMap = result->value.symbolMap(symbolType);
-        ShaderSymbolMap::const_iterator symbolEntry = symbolMap.find(name);
-        if (symbolEntry != symbolMap.end()) {
-            const std::string& mappedName = symbolEntry->value.mappedName;
-            return String(mappedName.c_str(), mappedName.length());
-        }
+    // We might have detached or deleted the shaders after linking.
+    auto result = m_linkedShaderMap.find(program);
+    if (result != m_linkedShaderMap.end()) {
+        auto linkedShaders = result->value;
+        auto mappedName = mappedSymbolInShaderSourceMap(linkedShaders.first, symbolType, name);
+        if (mappedName)
+            return mappedName.value();
+        mappedName = mappedSymbolInShaderSourceMap(linkedShaders.second, symbolType, name);
+        if (mappedName)
+            return mappedName.value();
     }
 
     if (symbolType == SHADER_SYMBOL_TYPE_ATTRIBUTE && !name.isEmpty()) {
@@ -991,7 +1011,7 @@ String GraphicsContext3D::mappedSymbolName(Platform3DObject program, ANGLEShader
             nameHashMapForShaders = std::make_unique<ShaderNameHash>();
         setCurrentNameHashMapForShader(nameHashMapForShaders.get());
 
-        String generatedName = generateHashedName(name);
+        auto generatedName = generateHashedName(name);
 
         setCurrentNameHashMapForShader(nullptr);
 
@@ -1002,23 +1022,43 @@ String GraphicsContext3D::mappedSymbolName(Platform3DObject program, ANGLEShader
 
     return name;
 }
-
+
+std::optional<String> GraphicsContext3D::originalSymbolInShaderSourceMap(Platform3DObject shader, ANGLEShaderSymbolType symbolType, const String& name)
+{
+    auto result = m_shaderSourceMap.find(shader);
+    if (result == m_shaderSourceMap.end())
+        return std::nullopt;
+
+    const auto& symbolMap = result->value.symbolMap(symbolType);
+    for (const auto& symbolEntry : symbolMap) {
+        if (name == symbolEntry.value.mappedName.c_str())
+            return symbolEntry.key;
+    }
+    return std::nullopt;
+}
+
 String GraphicsContext3D::originalSymbolName(Platform3DObject program, ANGLEShaderSymbolType symbolType, const String& name)
 {
     GC3Dsizei count;
     Platform3DObject shaders[2];
     getAttachedShaders(program, 2, &count, shaders);
 
     for (GC3Dsizei i = 0; i < count; ++i) {
-        ShaderSourceMap::iterator result = m_shaderSourceMap.find(shaders[i]);
-        if (result == m_shaderSourceMap.end())
-            continue;
-
-        const ShaderSymbolMap& symbolMap = result->value.symbolMap(symbolType);
-        for (const auto& symbolEntry : symbolMap) {
-            if (name == symbolEntry.value.mappedName.c_str())
-                return symbolEntry.key;
-        }
+        auto originalName = originalSymbolInShaderSourceMap(shaders[i], symbolType, name);
+        if (originalName)
+            return originalName.value();
+    }
+
+    // We might have detached or deleted the shaders after linking.
+    auto result = m_linkedShaderMap.find(program);
+    if (result != m_linkedShaderMap.end()) {
+        auto linkedShaders = result->value;
+        auto originalName = originalSymbolInShaderSourceMap(linkedShaders.first, symbolType, name);
+        if (originalName)
+            return originalName.value();
+        originalName = originalSymbolInShaderSourceMap(linkedShaders.second, symbolType, name);
+        if (originalName)
+            return originalName.value();
     }
 
     if (symbolType == SHADER_SYMBOL_TYPE_ATTRIBUTE && !name.isEmpty()) {
@@ -1193,6 +1233,14 @@ void GraphicsContext3D::linkProgram(Platform3DObject program)
 {
     ASSERT(program);
     makeContextCurrent();
+
+    GC3Dsizei count = 0;
+    Platform3DObject shaders[2] = { };
+    getAttachedShaders(program, 2, &count, shaders);
+
+    if (count == 2)
+        m_linkedShaderMap.set(program, std::make_pair(shaders[0], shaders[1]));
+
     ::glLinkProgram(program);
 }
 
@@ -1904,6 +1952,7 @@ void GraphicsContext3D::deleteFramebuffer(Platform3DObject framebuffer)
 void GraphicsContext3D::deleteProgram(Platform3DObject program)
 {
     makeContextCurrent();
+    m_shaderProgramSymbolCountMap.remove(program);
     glDeleteProgram(program);
 }