fixes crash issue when create Viewer too many times

[renjianqiang]

I find that viewer will crash when i created too many viewers (the number is 16 in chrome) even i had destroy the previous viewers. Look at these code:

var viewerOldest = new Cesium.Viewer("cesiumContainer");

// then i create some other viewers, and i destroyed them when i didn't need it.
// the code can be simplified to:
for ( var  i =0; i<16; ++i){
   var viewer = new Viewer("cesiumContainer");

   // do something here...

   viewer.destroy();
}

Then, the earliest created viewer will crash. I find the chrome console output the warning:

WARNING: Too many active WebGL contexts. Oldest context will be lost.

After search some keyword, i think this crash is caused by that webgl context is not released when viewer destroyed. I make this PR to fix this problem, please let me know if i have mistakes.

[cesium-concierge]

Thanks for the pull request @renjianqiang!

• ✔️ Signed CLA found.
• ❔ CHANGES.md was not updated.
  • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
• ❔ Unit tests were not updated.
  • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

• Cesium Viewer works.
• Works in 2D/CV.

[ggetz]

Thanks @renjianqiang! It sounds like you're running into an issue we're tracking where Cesium should better handle lost WebGL contexts.

If I understand correctly, its up to the browser to handle the contexts and this change is only simulating the context lost event. See https://developer.mozilla.org/en-US/docs/Web/API/WEBGL_lose_context/loseContext

The WEBGL_lose_context.loseContext() method is part of the WebGL API and allows you to simulate losing the context of a WebGLRenderingContext context.

The fix would be to handle the event and destroy object gracefully, perhaps restoring the context if we do have the mechanism to do so.

[renjianqiang]

@ggetz, yes, you are right, i just simulate the context lost event to completely free the webgl context when destroying the Context object, otherwise too many destroyed viewers will cause the current rendering viewer which previously created crashed. It is unreasonable because these viewers have been destroyed and shouldn't have effect anymore.

And for restoring context problem, i agree with @pjcozzi that it's unlikely or unnecessarily to fully restore the render state. If we want caution users more elegantly after webgl context lost, which by accident or there are too many active webgl contexts simultaneous, we can listen for webglcontextlost event and give some tips.

[ggetz]

@renjianqiang Regarding the process of destroying the viewer, I think the actual fix would need to address the root cause of viewer holding onto extra memory, (see #9298) which would be more involved.

For the WebGL context, I think we would want to remove the loseContext call and instead properly handle the 'webglcontextlost' event and warn the user, as you suggest. Would you be willing to work on that change here?

[renjianqiang]

I‘m pleasure to add some handler for 'webglcontextlost' event. Up to now i think the loseContext call is necessary, maybe the viewer holding onto extra memory is the root cause, and i will try to dig deep into the #9298 in the next few days.

…

---Original--- From: "Gabby ***@***.***> Date: Thu, Jan 20, 2022 23:22 PM To: ***@***.***>; Cc: ***@***.******@***.***>; Subject: Re: [CesiumGS/cesium] fixes crash issue when create Viewer too manytimes (PR #10011) @renjianqiang Regarding the process of destroying the viewer, I think the actual fix would need to address the root cause of viewer holding onto extra memory, (see #9298) which would be more involved. For the WebGL context, I think we would want to remove the loseContext call and instead properly handle the 'webglcontextlost' event and warn the user, as you suggest. Would you be willing to work on that change here? — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you were mentioned.Message ID: ***@***.***>

[renjianqiang]

@ggetz, after a few days of trying to figure out the issue #9298, i make sure you are right that the extra memory is the root cause. The root cause is detached window memory leaks.

In short, the cesium widget and other object keep a reference to a DOM element which prevents the element from being garbage collected since it can still be accessed, i.e., canvas is not destroyed even the viewer has been destroyed. So the webgl context is not released by chrome.

I try to fix this problem by set the cesium object's properties to undefined when object is destroyed and it worked, the cesium objects are disappeared in heap snapshot as expected, the webgl context has lost after viewer destroyed.

But the question is that i don't know if the problem has been fully solved, and there are must more files require modify, and after that i find the some test case also need be modified otherwise failed. I think i should create another PR and spend more time on it.

[ggetz]

@renjianqiang Yes, I think it's best to address #9298 separately. I would also suggest summarizing your fix there before opening a PR so we can discuss the right approach and test cases. Thanks!

[renjianqiang]

Ok, my fix to this issue is to modify the destroyObject.js :

// function destroyObject
// ...
for (var key in object) {
  if (typeof object[key] === "function") {
    object[key] = throwOnDestroyed;
  } else if (typeof object[key] === "object") {
    Object.defineProperty(object, key, { value: undefined }); // add this line to unset references
  }
}

And then i do some modifications to other files:

// Source\Scene\Cesium3DTileset.js
// Cesium3DTileset.prototype.destroy 
// line 2894
while (stack.length > 0) {
  var tile = stack.pop();
  // tile.destroy();

  var children = tile.children;
  var length = children.length;
  for (var i = 0; i < length; ++i) {
    stack.push(children[i]);
  }
  tile.destroy(); // move to here because of tile.children is undefined after tile is destroyed
}

// Source\Widgets\Viewer\Viewer.js
// Viewer.prototype.destroy
// line 1755
if (defined(this._selectionIndicator)) {
  this._element.removeChild(this._selectionIndicator.container);
  this._selectionIndicator = this._selectionIndicator.destroy();
}

// destroy inspector if it has attached to viewer
if(defined(this.cesiumInspector)){
  this.cesiumInspector.destroy();
}

if(defined(this.cesium3DTilesInspector)){
  this.cesium3DTilesInspector.destroy();
}

And the follow is partial modifications for test case:

// Specs\Scene\Cesium3DTilesetSpec.js
// "destroys before external tileset JSON file finishes loading"
// line 2468
expect(statistics.numberOfPendingRequests).toEqual(1);
var rootContentReadyPromise = root.contentReadyPromise; // add line to record promise before set to undefined
scene.primitives.remove(tileset);

// return root.contentReadyPromise
return rootContentReadyPromise
// ...

// Source\Scene\Primitive.js
// function setReady(primitive, frameState, state, error) {
// line 2555
frameState.afterRender.push(function () {
 // add handler if primitive is destroyed during current render frame
  if (primitive.isDestroyed()) {
    return;
  }
// ...

Please review my modifications, and give me some advice, Thanks @ggetz !

[renjianqiang]

Now, the cesium object will gc successfully after destroyed:

But it looks like there are still some object is unreleased:

[renjianqiang]

I also find there are maybe some problems in Source\Widgets\ClockViewModel.js and Source\Widgets\Animation\AnimationViewModel.js.

// Source\Widgets\ClockViewModel.js
// line 40
this.startTime = knockout.observable(clock.startTime);
this.startTime.equalityComparer = JulianDate.equals;
// this.startTime.subscribe(function (value) {
//   clock.startTime = value;
//   this.synchronize();
// }, this);

// maybe we should record the subscription and dispose in destroy function
this._subscriptions.push(
    this.startTime.subscribe(function (value) {
      clock.startTime = value;
      this.synchronize();
    }, this)
  );

// ClockViewModel.prototype.destroy 
// line 189
var subscriptions = this._subscriptions;
for (var i = 0, len = subscriptions.length; i < len; i++) {
  subscriptions[i].dispose();
}

// Source\Widgets\Animation\AnimationViewModel.js
// maybe we should add destroy function for AnimationViewModel and call it in Animation.prototype.destroy function
AnimationViewModel.prototype.destroy = function () {
  var that = this;
  this._definedProperties.forEach(function (property) {
    knockout.getObservable(that, property).dispose();
  });
  
  destroyObject(this);
};

I'm not sure if we need to make this change, and i'd like to have your suggestions!

[cesium-concierge]

Thanks again for your contribution @renjianqiang!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

[cesium-concierge]

Thanks again for your contribution @renjianqiang!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

[cesium-concierge]

Thanks again for your contribution @renjianqiang!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

[ItayBa]

Did anyone solved it?

[cesium-concierge]

Thanks again for your contribution @renjianqiang!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

[anthonylowther21]

Can this PR either be rejected or accepted?

We have this exact issue. When the map is loaded and unloaded too many times, the first map screen turns white, and we get the rror that too many webgl contexts exist

[danrockstheplanet]

This is still an issue. I'm seeing it one of my projects.

[cesium-concierge]

Thanks again for your contribution @renjianqiang!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

[ggetz]

I'm closing this issue due to inactivity. If you believe this is still an issue, please feel free to re-open and we'll give this a fresh review. Thanks!

[anthonylowther21]

@ggetz, this is still an issue on our projects. Did this PR get rejected, or did this get resolved in a separate PR?

[danrockstheplanet]

@ggetz, I'm also interested in knowing the answer to @anthonylowther21's question. Has this been resolved? If so, can you tell us when?