There is a missing validation when it is read from the map. Iterators should always have to be checked if point to the end of the container.
Created attachment 208172 [details] map iterator validation
LGTM
Comment on attachment 208172 [details] map iterator validation View in context: https://bugs.webkit.org/attachment.cgi?id=208172&action=review > Source/WebCore/ChangeLog:15 > + No new tests. Covered by existing tests. > + LayoutTests/webgl/conformance/glsl/functions/glsl-function-abs.html > + LayoutTests/webgl/conformance/glsl/functions/glsl-function-acos.html > + LayoutTests/webgl/conformance/glsl/functions/glsl-function-length.html These tests currently crash, or exhibit bad behavior? If so, we should unskip them with this change.
(In reply to comment #3) > (From update of attachment 208172 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208172&action=review > > > Source/WebCore/ChangeLog:15 > > + No new tests. Covered by existing tests. > > + LayoutTests/webgl/conformance/glsl/functions/glsl-function-abs.html > > + LayoutTests/webgl/conformance/glsl/functions/glsl-function-acos.html > > + LayoutTests/webgl/conformance/glsl/functions/glsl-function-length.html > > These tests currently crash, or exhibit bad behavior? > If so, we should unskip them with this change. Thank you for your review. These tests works fine. I ran them in the minibrowser directly. I ran Khronos conformance tests too. This patch does not break any webgl test.
(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 208172 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=208172&action=review > > > > > Source/WebCore/ChangeLog:15 > > > + No new tests. Covered by existing tests. > > > + LayoutTests/webgl/conformance/glsl/functions/glsl-function-abs.html > > > + LayoutTests/webgl/conformance/glsl/functions/glsl-function-acos.html > > > + LayoutTests/webgl/conformance/glsl/functions/glsl-function-length.html > > > > These tests currently crash, or exhibit bad behavior? > > If so, we should unskip them with this change. > > Thank you for your review. > These tests works fine. I ran them in the minibrowser directly. I ran Khronos conformance tests too. This patch does not break any webgl test. If these tests work fine without the change, how is this "covered by existing tests"?
Comment on attachment 208172 [details] map iterator validation View in context: https://bugs.webkit.org/attachment.cgi?id=208172&action=review > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:467 > + if (result == m_shaderSourceMap.end()) > + return; what situation does the patch help?
(In reply to comment #6) > (From update of attachment 208172 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208172&action=review > > > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:467 > > + if (result == m_shaderSourceMap.end()) > > + return; > > what situation does the patch help? It avoids possibly bug/crash. Assume m_shaderSourceMap does not contain search object. Then result iterator will point to m_shaderSourceMap.end() and read from it should crash. Look in other methods with use of m_shaderSourceMap.find. There are that checks too. I think someone forgot about this check here.
Comment on attachment 208172 [details] map iterator validation This patch isn't necessary (though it would be harmless). We call "m_extensions->getTranslatedShaderSourceANGLE(shader)", which does it's own check of the m_shaderSourceMap, and returns an empty string if there is no shader source entry. We check for the empty string and return prior to hitting the point where you propose adding a check. I don't think this change would actually be executed.