Skip to content

Force webgl context removal in Context.prototype.destroy #12693

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ethanchristensen01
Copy link

@ethanchristensen01 ethanchristensen01 commented Jun 25, 2025

Description

Try to force webgl context loss in Context.prototype.destroy so resources can be cleared when the CesiumWidget/Viewer are destroyed.

Issue number and link

#11533 (#11533)

Testing plan

With this sandcastle below, click "test button" to open a dialog with a cesium viewer inside, and click the "x" button to close it and destroy the viewer. Repeat this process 16 times.

Before, this would have crashed Cesium (specifically on Chrome, I think).

https://sandcastle.cesium.com/#c=nVYLb9s2EP4rhLfBMmrTstMMheMEQ9MNGNChxTJsA+ICpSRKIkKTGkn50SD/fUe9TCpygs0wYOl49913T3ojNiKWQhsUU83K7a0UhjBBFbpGouR8I1D1GdRZdkqtGqcG7RjdU7WYNg/L02kLIkyFf78IcThFlzj80mqkpYgNkwIxwcxt5S1gyQQ9tgpnmABaIuNyC9A4o+ZnTu3j++OviTU/GZ/4Wep0j2of+M9KFvRQp65f5DxaEoYezKfCktUrTw8+iv5TUm3+olHGFytkVEmnvsbeHj2zQ4jwIie1hX/05L46L09OdPN5ExzWkGSKExqV2V0u978osqX6M1V3FJgnELvvoLGKQUsRnPLjHzLwqCUQDRPERrtqc3ZLlIEnIi5wquT2A80UpTqoy3sffpk2lb5fwOMiDMOJlwOpGBw3kL085JQkTGSdq9+IybGRv4OUCB30kBAqmInzc9qzi8u+vpKcnwf3Mu1ZJqVqCId44dfg9KaoKZVoctqKn/wZ0VCUD4xwmUEtUsI1dU9jWQoDB6E/OElrcK7Vx7XGuIvg2VhBwUzQGyfw9eaNP1+SUwxAwdeYs/ih4fP9Y/X79BU5GfIC8ZuqJoNjTrT+yLTBJAGK1mA86bee3STOyI/7i+Zk4SSyi6r2dMulpn5wDTi27avkMRj06665AdaKbuWO9om3E9gHXLwcyAngbA1tid6XxkgxnmApqgpct6EGEye6upg9Rq/iR0ZUmXoN3Uuq42Q0Ha21OXJ60wp/YttCKoNKxQOM54ZuC06A3Dwq4wdqcKz15Moq52bLu3mKZHJ0a7VniYEZhkXxw9VJmlOW5eaZeEtUxuwYOrIC+qtaGq4QSqdSLvcrlLMkoeKqX8CKx03Cdv4OGmZzllGHhnsFn8XdFfX/YnX6HduCzyJ5cKEKqVm9kkgEc1sa6iCqGnAZhsXBERtZWC+e7NuMiYQeVuhyIKi6F867TtmBJlfuNZsavw6VS1dwSsFuP5yDXX7ldqOGpjqukJCCDtFeuuARiR8yBdsqWSGVRSSAvxnNF/84eTFAbOccPQ54jriMH15ODobhmsV2ZP5riRavVyiFTppp9o1CsJfeSVwqLdUKHDF74Q6lZzGQ+7c+fpv6ntj+z5kRzjLgX1/oZxL9XZqmr7iuk1a/refuFlnbCWTJ9WbEZXX3f4LBhbRvRjfrfHHzsRZijNdzeF3PQf2ZpZGSR0RZk/55tc6tSjNBoNNyWkfVsm0guuW7GaGKHwjrZTOrm3pps+PW4gIEAGctUQ21nte/LYGGTJ9ub1OAw5bl+R2Swl11B26dGAeiPHUkgFa+aokbtS6I6Ay6tm3120sCLP6GQoHuyfJcAEsnZU2HvYNF1vVV9dIu7neQNURKI+FPTq9e1fO/

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

Thank you for the pull request, @ethanchristensen01!

✅ We can confirm we have a CLA on file for you.

@javagl
Copy link
Contributor

javagl commented Jun 26, 2025

As mentioned in the issue, it looks like this change ~"serves its purpose", and I don't see too much potential for any "negative side-effects" (What could be a "negative side effect" of destroying something, except for that thing being destroyed...?)

When trying it out with the sandcastles in the issue, I wondered whether this can be sensibly covered with unit tests. Could it, at least in theory, be possible to do some setTimeout-trickery to create (and destroy) 17 (!) viewers in quick sequence? Is it necessary to add some delay between creation and destruction? I haven't tried that yet, just wondering...

EDIT: The failing test is only linting. npm run prettier should fix that.

@ethanchristensen01
Copy link
Author

Running prettier on my machine changes hundreds of files. On the file modified here, it removes all trailing commas among other things. Is there some configuration for prettier that isn't currently kept in the repo?

@javagl
Copy link
Contributor

javagl commented Jun 30, 2025

I just ran it locally on this branch. There are some changes that I cannot explain (and they already have been merged into main via 194f9f3 - no idea where these changes are coming from - maybe @jjhembd knows?). But apart from that, the changes should really be about newlines in the file that was modified here:

Cesium PR Formatting

@ethanchristensen01
Copy link
Author

Ah, that explains why I couldn't see any changes. I'll fix that in about 16 hours probably.

@jjhembd
Copy link
Contributor

jjhembd commented Jul 1, 2025

@javagl the commit 194f9f3 was just aligning #12558 with main. As you can see from the overall file diff from that PR, those newlines from 194f9f3 were in CHANGES.md on the main branch prior to the PR.

I think the actual source is the prettier update in #12698 (4 days ago). Make sure you have the latest version of prettier installed locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants