WebGL: extension string is available even though Emscripten doesn't implement it #20798
Comments
|
cc @juj but I'm going to look into this for a bit. |
|
This is known in particular to affect Filament with EXT_clip_control: Possible solutions I can think of:
Options 1 and 2 will not guard us against this happening again, unless the WebGL working group adopts a policy that they'll stop reusing the upstream "EXT_" extension names and only use "WEBGL_" extension names. Perhaps more importantly, none of these will prevent Chrome's shipment of these extensions from breaking existing content (like existing Filament-based renderers). An alternative:
This is maybe technically a bit heavy-handed as a workaround for an Emscripten problem, but given the ecosystem impact I think it is the most prudent. cc @kdashg |
|
Actually, per that analysis, I'm probably going to revert these two new extensions from Chrome. This will make the repro I provided stop working in Canary. Anyway, that's the end of my investigation for now. |
|
Emscripten should not be auto-concatenating an EXTENSIONS string from getSupportedExtensions(). |
Problem: - Emscripten exposes the GL_EXTENSIONS directly from WebGL. - Emscripten does not implement glClipControlEXT or glPolygonOffsetClampEXT entry points. - Chrome 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 (entry points are NULL). Partially reverts https://crrev.com/c/5021828 Fixed: 1502022 Bug: 1501471 Bug: emscripten-core/emscripten#20798 Bug: KhronosGroup/WebGL#3604 Change-Id: If672d9ed825eaa6f50ccf0a937dd12ce72c36b8f Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5068523 Reviewed-by: Gregg Tavares <[email protected]> Auto-Submit: Kai Ninomiya <[email protected]> Commit-Queue: Kai Ninomiya <[email protected]> Cr-Commit-Position: refs/heads/main@{#1230453}
Please reopen. I don't think this is resolved until this happens. |
|
Why? |
|
After #20802 Emscripten only constructs EXTENSIONS by concatenating getSupportedExtensions() when GetProcAddress is disabled. This should be OK because any mention of a function that Emscripten doesn't implement will cause a linker error. When GetProcAddress is enabled, EXTENSIONS is constructed by allowlist. |
|
A couple of comments on #20802 indicate that the new build flag Given the experience with this bug, it seems risky for Emscripten to automatically pass through support for all of the browser's supported WebGL extensions. Could Emscripten instead make the behavior of I'm taking the liberty of reopening this issue while making this comment. Not sure of the status of things right now, but perhaps #20802 could be reverted and redone in a way that no new build flag is added. The current status quo is not really workable from the WebGL WG's point of view - we don't want to have to rename every extension that's exposed to the web into the |
It is inherently risky to blindly concat the EXTENSIONS string. The blind-concat approach is a hack that mostly worked, but it's caused this backwards compat issue that is pushing your bug upstream into the working group's process for extension naming. Continuing to do so is to continue to exacerbate this issue. I believe this is not difficult, and it's important to us (the WebGL WG, future users of webgl's extensions), it would really great to just fix this. |
The issue described in the PR is just how new settings work in emscripten. The solution is the pass In terms is backing out #20802, it sounds like your concerns would be alleviated if we simply made emscripten/src/library_webgl.js Line 747 in f3c8edc |
|
What is the reasoning for not always using an allowlist, even for |
Code size, I think, but it can also be useful for applications to use extensions (without entrypoints), before Emscripten explicitly allowlists them. On the flip side, I'm still not clear what the benefit would be of using an allowlist when GetProcAddress isn't available? |
|
Without an allowlist, emscripten is misrepresenting the extensions that it supports. Usually the disadvantages are subtle, but it is a class of errors that can constrain the direction of the webgl wg, and should not continue. This is a large enough project that being a good web citizen matters, and we are coming off of a relatively severe issue with a number of already-deployed probably-wont-be-patched apps. Proactive engineering would be asking ourselves "how can we avoid this whole class of issues in the future", not just the narrow defect identified here. Also, realistically do you believe code size is a concern here? It is an ~30loc addition to have an allowlist. |
|
IIUC the code size issue stems from GetProcAddress. I think we can make the allowlist unconditional, especially if we can avoid including it apps that don't query extension at all (which seems perfectly do-able). I'm sure we can fine a way to fix this issue fully, and to everybodys's satisfaction. |
|
Sounds great @sbc100 . I'm not sure how many Emscripten apps build with Alternatively, the (trivial) list of known WebGL 1 and 2 extensions could be factored out of the |
|
Here is a possible followup to always use the allowlist: #20955 |
|
Dear Ken and Kelsey, it is frustrating to me to read your comments, when it is clear that you haven't taken the time to read through the original PR, but are still vocally opining on what should be done here. Kelsey, you write
which is incorrect. With the code change that landed, Emscripten accurately represents the extensions that it supports, as the PR fixes the root source of the incompatibility that caused the lack of support.
Exactly. Proactive engineering also asks how to fix a defect in a way that caters to all users of the project, instead of fixing a problem in a bad way that would adversely impact 95% of users to fix an issue that only the other 5% of the users had in the first place.
I authored the exact fix you requested, in a way that does not add dead code to developers who aren't bitten by this issue at all. I did not do a tradeoff. It is not about either-or. But it is about catering to all users of the project that they get what they need, without impacting the other users. I did what you asked, but somehow you are still complaining that this issue has gone unaddressed. Ken, you complain that you want the allowlist unconditionally to apply to all users to cater to WebGL WG, because you say, that some users have had issues with building with a new version of Emscripten. Where is the logic in that? A WebGL spec problem is a WebGL spec problem, and a SDK update problem is a SDK update problem. Let's not deliberately conflate the two issues.
The original issue was never an untested code path in Emscripten. Please can you please sit down and take the time to read to understand the issue before writing opiniated posts that end in passive-aggressive thankyous? I am not fighting against you here, but can we try to get to some signal over noise. The PR #20948 will flip the build flag to opt-out from opt-in to not disrupt people who are upgrading. I am on holiday break since last weekend, and back to office start of next year. If there are further issues after that, I'll be happy to look into those, but let's try to bring those issues up in forms of concrete code and workflow examples to get to a good signal level? |
|
@juj I apologize - I didn't make the time to read your PR, but instead based my statements on abstract understanding of the problem. I'll do better in the future. I also didn't mean to be passive aggressive, and will be more conscious of that in my communications. @kainino0x , @sbc100 and I just had a video chat (thanks Kai for setting it up) which was helpful in getting everyone on the same page. Sam will be reviewing/merging your PR #20948 and following up. We're in agreement that it's safest to always filter the WebGL extensions supported by the browser via an allowlist in Emscripten's runtime. |
|
I also need to apologize to @kdashg, for rejecting your concerns and not trusting your expertise. I didn't spend enough time considering it to be as confident as I was that it would be safe to pass through the extension list even without GetProcAddress, or to estimate the code size cost, and so I shouldn't have taken as hard of a line as I did. On a different note I have also come around to agree that we do need the allowlist always, even when neither GetProcAddress nor If we go forward with this then I think the Emscripten side of things will be satisfactorily resolved (pending Jukka's input) with perhaps some future work to try to switch GL_ENABLE_GET_PROC_ADDRESS back to default-disabled after some time. After that it will still be on our plates to figure out how we can ship these extensions in WebGL, either by doing some kind of ecosystem survey to find out what will break, or by the WebGL WG renaming any future extensions that add entrypoints. TBD. |
|
Thanks all, much appreciated. I think it is fine to have GL_ENABLE_GET_PROC_ADDRESS be enabled by default even to the end of time, and have people discover to manually disable it when they know they don't need it. That way everyone gets the allowlist by default, and can opt in to optimizing their code when they know to do so. |
|
Here is my understanding:
WebGL extensions with the same names as their corresponding (but distinct) OpenGL extensions are intended to behave predominately the same, but they are not guaranteed to be exactly the same. Emscripten is assuming they are exactly the same, and that is the remaining defect here. I do not understand the dead-code/code-size argument, because while it does technically increase code size, I do not understand why the solution I proposed three weeks ago would constitute a materially-significant increase in code size. I am however excited for the work going on to fix these issues, and I particularly look forward to #20955 merging, which to me should be required in order to close this issue. PS: I particularly care because this is a library that I've made non-trivial contributions to! The texEnv/CTexUnit JIT code was me ten years ago, and I worked with @kripken on a number of other early issues at that time. |
Here we use the JS library dependency mechanism to include this code only when its needed. Sadly this will end up being preset in almost all GL-using programs because most of them will end up using `emscriptenWebGLGet` (e.g. any app that calls `glGetInteger`, `glGetString`, etc) Fixes: emscripten-core#20798
Here we use the JS library dependency mechanism to include this code only when its needed. Sadly this will end up being preset in almost all GL-using programs because most of them will end up using `emscriptenWebGLGet` (e.g. any app that calls `glGetInteger`, `glGetString`, etc) Fixes: emscripten-core#20798
Here we use the JS library dependency mechanism to include this code only when its needed. Sadly this will end up being preset in almost all GL-using programs because most of them will end up using `emscriptenWebGLGet` (e.g. any app that calls `glGetInteger`, `glGetString`, etc) Fixes: emscripten-core#20798
|
Let's try to take a step back to get a bigger picture. There are a lot of users who use Emscripten to develop WebGL targeting applications. In the order of most web-native (least ported) to least web-native (most ported):
This future-WebGL-extensions problem never touched developers in group 1 since they develop a direct web facing rendering engine to begin with, so they have developed and tested their engine against the WebGL spec. Just like people hand-writing a JS WebGL engine would. It neither never touched developers in groups 2 or 3 since they don't use extensions. And it is not a problem for developers in group 4 either, because they have changed their codebase to not rely on GetProcAddress or GL_SUPPORT_AUTOMATIC_ENABLE_EXTENSIONS, but statically write the code by hand at compile and link time to target the extensions they need. This future-WebGL-extensions problem specifically touched developers in group 5, who compiled a codebase without adjusting their engine code to that they compiled to the web. The PR #20802 sealed the Emscripten side problem that users in group 5 might face. The PR #20948 further fixes two problems:
The comments above are arguing that we should unconditionally force all Emscripten users to take this allowlist. This would include developers 1. - 4. above, who have codebases that are statically audited to be immune to the whole problem. It would be poor engineering judgement to do so. Fixing the problem that affected users in camp 5 in a way that all other users will also suffer, who provably never had any issues with the problem would be a naive fix.
The thing to understand about code size, is that code size bloat is not a bottleneck like CPU or GPU hotspots are. Instead, code size bloat is something that happens to the codebase by hundreds of these types "this is not a material increase" decisions. We get these kinds of comments almost weekly, when someone posts a PR with their favorite feature, that would increase the code size unconditionally for all users. (off the top of my head, recently with someone attempting to add High DPI CSS coordinate transformation support, because of course everyone wants the support to calculate their coords in retina coordinates). We need to strive to not add code to the compiler so that it would leak to users to whom the code does not relate to in any way. That is simply not a sustainable way to develop a compiler. More than half a decade ago, we regularly got complaints that Emscripten was bloated, and the output was a mess to understand. There were a stream of developers who were commenting that once LLVM adds support for WebAssembly, they can finally get rid of all Emscripten bloat and directly compile code with LLVM to Wasm instead for a leaner result. These opinions rose exactly from the background that people were put off from Emscripten practices having this kind of casual attitude to code size. Still today, Emscripten's MINIMAL_RUNTIME feature is the most upvoted feature to ever land: #7923 . It optimized the runtime by -90% for small pages (used for e.g. small playable WebGL ads) by specifically eradicating hundreds of different small locations in the codebase, where a "it's just a few lines of code" choice had been made before. However, please understand that I am not arguing on terms of a code size tradeoff that we should not land an allowlist. We definitely need to. It is not a tradeoff question, for the people who are in camp 5. Camp 5 people shall get that allowlist imposed on them. But we should not land an allowlist in a way to users who are provably immune to the whole spec problem, from being affected them in the first place. That would be poor judgement. The fix to group 5 must be done in a way that users in groups 1-4 will still have the way to build code that does not have unnecessary dead weight in their builds that does not have any effect to their code. Users 1-4 produce code that does not have flows that could enable future extensions even if they appear in glGet() list for them. They have no other way, for the same reason that JS developers who hand-write JS WebGL engines don't have a way of activating future native extensions: there is no way for them to activate those extensions in advance. Another thing to understand about code size, which in public opinion has even the bigger impact, and the reason why Hacker News and StackOverflows have had negative opinions about Emscripten: is that code bloat adds cognitive bloat to developers' opinions and the process of learning. Many developers read the generated build output to learn to understand how Emscripten works. When their build output has hundreds of places of not-immaterial-increases, they may be turned off by the generated code on the basis that Emscripten authors were lazy, and the compiler must be poorly written. I got to read these comments about monthly in the era when I was at Mozilla. In comment #20798 (comment) I mentioned that we could keep the allowlist enabled by default indefinitely, and have developers need to opt-out from it. That is already a casually lazy overdecision on behalf of all users, but if it helps the unease, then that would be ok. At least then developers in camps 1-4 will still have a way to instruct the compiler to generate the code they want to produce (and that is still 100% provably free from defects by static analysis). A couple of times now I have read above a comment that Emscripten returning all extensions in glGet is a defect, but that by itself is not a problem, and after the PRs that have already landed, I argue, cannot lead to future problems. The rationale for that is that codebases 1-4, again, is that even if their code sees an unfiltered future extension, their codebases do not leverage that extension, because the codebases have not been statically written to do so. Only codebases of form 5 would attempt to use, but they are already the ones that have been subjected to the allowlist, since they use GetProcAddress (and now with the other PR, GL_SUPPORT_AUTOMATIC_ENABLE_EXTENSIONS). So, to summarize, after 20802 and 20948 have landed, I do not think there is anything more to do here. We should not land a PR that unconditionally imposes an non-optoutable allowlist to all users, because that would be irrelevant noise for several users at best. In my previous message I asked
So if after the above PRs, we should still adjust the code after those PRs, I would really like to learn about the possible contingency and workflow cases, ideally with code examples of where things would go wrong, that I have missed here. |
|
To be clear, #20955 does not unconditionally include the allowlist, it includes it precisely for codebases that depend on it. In its current incarnation that means any program that uses GetProcAddress or via glGetString. My understanding is that this would not force these extra bytes on users of group 1 and 2 above. The allowlist would not be included for those users since they are using webgl APIs directly, right? |
However, I do get your point that there will then exist a way for groups 1 and 2 to directly target the web if they avoid |
|
I can tell that you are unconvinced that returning getSupportedExtensions in EXTENSIONS unchecked could turn malignant (anymore), and I can tell that you are terrified of code size issues.
I hear you that you think this is not a problem, but I am advising that it is risky to keep doing this. An example is extensions that have no entrypoints that can be statically excluded by not using getProcAddress, but where there are for example added functionality for GLSL. There is the opportunity for the WG to believe we are making a trivial exclusion from a GLSL extension, that then comes around to bite a deployed app that expects the e.g. desktop GL version of the extension. I can tell you that there is this risk because I am one of the ones in the back room trying to give close and careful consideration to extension proposals, but also in recent months I have been pressured to reduce the care and stringency with which I review WebGL extension proposals to accelerate their roll-outs. Put another way: I bet that, even given your mitigations, I would be able to adversarially craft a WebGL extension that would break some Emscripten apps that you believe to be immune. Your risk is that a group that you don't participate in can (accidentally!) cause behavioral changes in apps long-deployed with your framework, which risks very unhappy deployment owners, and risks incurring reputational damage regarding Emscripten's stability and robustness in "finished" deployments. If you are willing to risk this for the sake of these arrays of strings, that's your project's choice, but that's not the guarantee that us browsers are (capable of) giving you, and we unfortunately can't guarantee that we won't then (accidentally, unintentionally!) break your users' deployments. That is what I see as the price of cutting corners to save some ~800 bytes here. |
There is absolutely no need to go into emotional characterization ad hominems here. We can surely keep discussing this issue as the issue without painting the people discussing it.
This is not an example that I could see. The thing to understand is that the fix does not apply to individual extensions at a time, but to the whole compiled codebase at a time. So in order for a codebase to be bit by this issue via an extension that does not have entrypoints, they would have to first a) hand-write a change into their application to statically link all their other extensions that do have entry points (taking them away from the "blind port" realm), b) turn off the automatic enable extensions linker flag, then c) manually hand-write new code into their codebase to blindly enable these extensions that don't yet exist, and d) never test the code they hand-wrote to see that it doesn't work today. Stating again, they would have to do active work to take their engine from being a "I blindly compiled an existing GL engine" port, carefully refit that engine to statically link to everything, and statically enable all the extensions, including the ones that don't exist on the web, and then decide "nope, we actually aren't going to test any of what we wrote will work" to plant their time bomb.
Please understand, for the third time maybe now, that I am not proposing to cut corners. The solutions that have already landed apply widely to all compiled code and is an opt-out to which you have to actively write code if you started from a port. I am proposing to find a solution that can fit every Emscripten developer, without getting immediate reputational harm from other users in the groups 1-2 I mentioned earlier, had we applied naive fixes. It would be condescending to web developers for Emscripten to mandate that "we think you can never be trusted to know what code you write, so we will always impose the allowlist to all your codebases, no matter how first-tier web experience you may think you are targeting with your code." If the current guardrails still don't seem enough, then we can look into adding more, for example, even if a developer has a) migrated away from the "it's a port" GL realm by having refitted all their GetProcAddress spots, and they have b) refitted their code to hand-write their GL extension enables and c) have explicitly disabled the auto enable extensions mode, we could still look at d) printing a development mode warning when attempting to manually enable an extension that does not exist in WebGL (a check that gets optimized out for them in release builds), or e) adding even an extra level of opt-out field to get to the level of paranoia. But we simply should not use a blunt tool to wholesale think there couldn't possibly exist codebases out there that know they intend what they write. |
|
Hey friends, the last few comments seem to be getting a little heated. I admit I do not fully understand the technical details, but I'm worried there might be some misunderstandings here. What do you think about scheduling a meeting to discuss this in more detail offline? I think that might be productive. |
|
I think that's a great idea. In the new year, maybe? (Happy solstice! :)) I apologize that "terrified" felt like an ad hominem. My intent was to characterize very strong degree and not to connote emotion or any irrationality. (I would describe myself as being terrified of e.g. raw pointer member fields in c++!) Sorry that I didn't take enough care in wording here! |
|
The new year is probably best, yeah. I'm about to go out myself, and will mostly be out until Jan 2 or so. I'll send out an email when I'm back! With that said, since I know the least about GL, don't wait for me if we do arrive at a compromise acceptable to everyone meanwhile. But if not, hopefully a meeting next year can help. |
Here we use the JS library dependency mechanism to include this code only when its needed. Sadly this will end up being preset in almost all GL-using programs because most of them will end up using `emscriptenWebGLGet` (e.g. any app that calls `glGetInteger`, `glGetString`, etc) Fixes: emscripten-core#20798
Here we use the JS library dependency mechanism to include this code only when its needed. Sadly this will end up being preset in almost all GL-using programs because most of them will end up using `emscriptenWebGLGet` (e.g. any app that calls `glGetInteger`, `glGetString`, etc) Fixes: emscripten-core#20798
Here we use the JS library dependency mechanism to include this code only when its needed. Sadly this will end up being preset in almost all GL-using programs because most of them will end up using `emscriptenWebGLGet` (e.g. any app that calls `glGetInteger`, `glGetString`, etc) Fixes: emscripten-core#20798
Here we use the JS library dependency mechanism to include this code only when its needed. Sadly this will end up being preset in almost all GL-using programs because most of them will end up using `emscriptenWebGLGet` (e.g. any app that calls `glGetInteger`, `glGetString`, etc) Fixes: emscripten-core#20798
Here we use the JS library dependency mechanism to include this code only when its needed. Sadly this will end up being preset in almost all GL-using programs because most of them will end up using `emscriptenWebGLGet` (e.g. any app that calls `glGetInteger`, `glGetString`, etc) Fixes: emscripten-core#20798
Here we use the JS library dependency mechanism to include this code only when its needed. Sadly this will end up being preset in almost all GL-using programs because most of them will end up using `emscriptenWebGLGet` (e.g. any app that calls `glGetInteger`, `glGetString`, etc) Fixes: emscripten-core#20798
Here we use the JS library dependency mechanism to include this code only when its needed. Sadly this will end up being preset in almost all GL-using programs because most of them will end up using `emscriptenWebGLGet` (e.g. any app that calls `glGetInteger`, `glGetString`, etc) Fixes: emscripten-core#20798
Here we use the JS library dependency mechanism to include this code only when its needed. Sadly this will end up being preset in almost all GL-using programs because most of them will end up using `emscriptenWebGLGet` (e.g. any app that calls `glGetInteger`, `glGetString`, etc) Fixes: #20798
Please include the following in your bug report:
Version of emscripten/emsdk:
Failing command line in full:
emcc main.cpp -o index.html -g -sUSE_WEBGL2Full link command and output with
-vappended:No build failure
Repro code: https://github.com/kainino0x/emscripten-extension-bug
Repro page: https://kai.graphics/emscripten-extension-bug/
Chrome Canary (as of 121.0.6153.0) recently enabled 4 new WebGL extensions:
EXT_clip_control
EXT_depth_clamp
EXT_polygon_offset_clamp
WEBGL_polygon_mode
https://chromiumdash.appspot.com/commit/40900b5e6698955aba32af9679519104c3bad20f
Emscripten passes through the extension strings from WebGL, but it does not actually implement the new entry points glClipControlEXT and glPolygonOffsetClampEXT. These will fail to link if you try to use them directly - but they'll return NULL if you try to get them with GetProcAddress, causing a crash in existing builds if they contain code to use these extensions:
To reproduce this make sure you have an updated Canary and EXT_polygon_offset_clamp is available on webglreport.com.
(If on Mac, you may need to switch to the Metal backend at
chrome://flags/#use-angle.)EDIT: I am going to revert these new extensions from Chrome per my analysis below. Hopefully a repro isn't really necessary but if needed, you can use an older Chromium snapshot:
Follow the Puppeteer instructions here:
https://www.chromium.org/getting-involved/download-chromium/#chrome-for-testing
Or use these snapshots:
https://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html?prefix=Linux_x64/1229878/ (not tried)
https://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html?prefix=Win_x64/1229878/ (not tried)
https://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html?prefix=Mac_Arm/1229875/ (although I can't actually get this to work on my ARM mac)
https://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html?prefix=Mac/1229878/ (nor these, though I thought they were dual-architecture binaries)
The text was updated successfully, but these errors were encountered: