KhronosGroup/WebGL

Shipping extensions with GLES names can break existing content

Opened this issue · 8 comments

kainino0x commented

Please see this bug for details: emscripten-core/emscripten#20798

tl;dr:

  • Emscripten exposes the GL_EXTENSIONS directly from WebGL.
  • Emscripten does not implement glClipControlEXT or glPolygonOffsetClampEXT.
  • Browser ships EXT_clip_control or EXT_polygon_offset_clamp.
  • Existing pages on the Web that were compiled using Emscripten, from C/C++ code that supported these extensions (like Filament), start crashing

I have a few ideas about this but my suggestion on that bug is for the WebGL group to simply not ship new EXT_ extensions that add new functions, and instead rebrand them as WEBGL_.

kdashg commented

Emscripten should be allowlisting extensions, and only adding them to its extension string if they are recognized and implemented. It is a defect in emscripten that it hacks together an extension string like this.

kdashg commented

Why do they start crashing? For example, in Firefox, we only try to dlsym pfns for e.g. glClipControlEXT when we see EXT_clip_control in the extension string, but we soft-assert and mark-as-disabled if e.g. EXT_clip_control is in EXTENSIONS but dlsym("glClipControlEXT") -> nullptr.
Are apps not being careful here?

kainino0x commented

Emscripten should be allowlisting extensions, and only adding them to its extension string if they are recognized and implemented. It is a defect in emscripten that it hacks together an extension string like this.

I agree, but realistically there is a bunch of content out there that has this bug and is not going to get rebuilt with a new version of Emscripten. And unfortunately I don't think we could really do browser telemetry to determine what sites depend on this behavior in order to do a proper "deprecate and remove" process.

Are apps not being careful here?

Filament, at least, is not, and that is a pretty well battle-tested engine (on Android mostly), so I guess they have not had issues with it. AFAIK the presence of the string is supposed to guarantee the presence of the function(s), so this would only be needed if there's a driver bug?

greggman commented

If I understand the issue

  // Original native C/C++ code. Runs on native OpenGL ES

  bool supportsEXTClipControl = !!strstr(glGetString(GL_EXTENSIONS), "GL_EXT_clip_control");
  
  ...

  if (supportsEXTClipControl) {
    glClipControlEXT(...);   // works in native, crashes on emscripten since
                             // glClipControlEXT is not connected to anything.
                             // Or, if it's connected to a no-op then it's not
                             // actually doing the thing advertised as supported.
  }
kainino0x commented

Correct, I have some sample code in the Emscripten bug too:
https://github.com/kainino0x/emscripten-extension-bug/blob/main/main.cpp

kdashg commented

Driver bugs are a reality, and we've hit this class of 'em before. That's why Firefox is paranoid about it.
I do not want to change prefixes because of a poor reason like this, so I'd like to consider other avenues before deciding.

Before moving forward with any changes to our process, I must insist that Emscripten fix its defect here before it causes further harm.

kainino0x commented

Jukka has a fix up: emscripten-core/emscripten#20802

  • 🚀1
MarkCallow commented