search

KhronosGroup / WebGL

The Official Khronos WebGL Repository
Other
2.63k stars 668 forks source link

Shipping extensions with GLES names can break existing content #3604

Open kainino0x opened 8 months ago

kainino0x commented 8 months ago

Please see this bug for details: https://github.com/emscripten-core/emscripten/issues/20798

tl;dr:

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 8 months ago

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 8 months ago

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 8 months ago

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 8 months ago

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 8 months ago

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

kdashg commented 8 months ago

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 8 months ago

Jukka has a fix up: https://github.com/emscripten-core/emscripten/pull/20802

MarkCallow commented 8 months ago

That PR does not fix the problem. It disables GetProcAddress thus preventing people from encountering the problem but in the process breaks any app that really needs to use GetProcAddress. A new compile option has to be specified to have GetProcAddress. There is a new PR #20948 https://github.com/emscripten-core/emscripten/pull/20948 to flip the flag so GetProcAddress is available by default and the flag disables it. I do not understand why they are reluctant to fix the extension list they advertise. There is mention of some people wanting to see the unfiltered WebGL list but I don’t truly understand why. Possibly something to do with extensions that have no functions. But a filtered list could still include those extensions.

Regards

-Mark

On Nov 30, 2023, at 10:25, Kai Ninomiya @.***> wrote:

Jukka has a fix up: emscripten-core/emscripten#20802 https://github.com/emscripten-core/emscripten/pull/20802 — Reply to this email directly, view it on GitHub https://github.com/KhronosGroup/WebGL/issues/3604#issuecomment-1832957755, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAREASYVUKGCNV6LSKDZY53YG7OCLAVCNFSM6AAAAAA76VEXICVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZSHE2TONZVGU. You are receiving this because you are subscribed to this thread.