fix: prevent clearFadeHold undefined error

[charlieforward9]

This issue resolves an issue where Maplibre crashes due to a crash throwing cannot read undefined: clearFadeHold when Maplibre is being controlled by DeckGL and animated with HubbleGL.

The error does not occur repeatably, but it occurs enough to be an issue for end users that cannot be avoided.

Debugging to the root cause is unrealistic for us since our API with Maplibre is abstracted quite significantly by using react-map-gl/maplibre and delegating the control of the viewState to DeckGL & HubbleGl.

Happy to learn more here...

Launch Checklist

• Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
• Briefly describe the changes in this PR.
• Link to related issues.
• Include before/after visuals or gifs if this PR includes visual changes.
• Write tests for all new functionality.
• Document any changes to public APIs.
• Post benchmark scores.
• Add an entry to CHANGELOG.md under the ## main section.

[HarelM]

Thanks for taking the time to open this PR.
I think a test is needed here in order to be able to reproduce the problem.
Otherwise, it might be that the cause of this issue is using some non public API as part of wrapping libraries, and I'm that case I don't think the fix should be here...

[charlieforward9]

This is the stacktrace, if that can help you help me figure out how to write a test here

Uncaught TypeError: Cannot read properties of undefined (reading 'clearFadeHold')
    at be.update (maplibre-gl-YXZ2VXOW.js?v=ce64ef21:11794:50)
    at bi._updateSources (maplibre-gl-YXZ2VXOW.js?v=ce64ef21:15672:71)
    at e.Map._render (maplibre-gl-YXZ2VXOW.js?v=ce64ef21:19932:437)
    at e.Map.redraw (maplibre-gl-YXZ2VXOW.js?v=ce64ef21:19937:119)
    at maplibre-gl-YXZ2VXOW.js?v=ce64ef21:19435:79
    at s2 (maplibre-gl-YXZ2VXOW.js?v=ce64ef21:17100:34)
    at maplibre-gl-YXZ2VXOW.js?v=ce64ef21:17102:67
    at ResizeObserver.<anonymous> (maplibre-gl-YXZ2VXOW.js?v=ce64ef21:19438:22)

[HarelM]

Can you reproduce the issue with maplibre only? Without any wrapper? Doing this will help define the relevant test...

[charlieforward9]

Can you reproduce the issue with maplibre only? Without any wrapper? Doing this will help define the relevant test...

Disassembling our wrapper logic while preserving the control logic that led to this issue enough to make a reproduction would take a significant amount of time for us.

I am quite the beginner in maplibre, but I just subscribed to repo updates. I am a bit more experienced in the vis.gl side of things. I can keep this in mind as I learn more but short term I cannot be of much value.

Its fair to say that this conditional operator doesnt manipulating the behavior of this in any negative way. So could we see this merged and add the bug as an issue or point of discussion instead?

[HarelM]

I prefer to have a better understanding of the issue, it might be that this is not the right place to fix it but rather in a different place where it makes more sense.
This is a patch, not a fix, and therefore I prefer to have a deeper understanding of what's causing this.
Also, as I said, it might be a bug in the outside code that gets to this method, and needs to be fixed there.

[charlieforward9]

@HarelM this error is resolved when removing the terrain source config, but again, this libary is too nested to provide a reproduction at this moment, so I will reach out to the wrappers' developers to see what insights they may be able to provide.

const MAP_STYLE: StyleSpecification = {
  version: 8,
  projection: {
    type: "globe",
  },
  sources: {
    satellite: {
      url: "https://api.maptiler.com/tiles/satellite-v2/tiles.json?key=get_your_own_OpIi9ZULNHzrESv6T2vL",
      type: "raster",
    },
    terrainSource: {
      type: "raster-dem",
      url: "https://api.maptiler.com/tiles/terrain-rgb/tiles.json?key=get_your_own_OpIi9ZULNHzrESv6T2vL",
      tileSize: 256,
    },
  },
  layers: [
    {
      id: "Satellite",
      type: "raster",
      source: "satellite",
    },
  ],
  sky: {
    "atmosphere-blend": [
      "interpolate",
      ["linear"],
      ["zoom"],
      0,
      1,
      5,
      1,
      10,
      0.5,
      15,
      0.2,
    ],
  },
  //FIXME: Error: Cannot read properties of undefined (reading 'clearFadeHold') when using terrain, removing this prevents the error from throwing.
  terrain: {
    source: "terrainSource",
    exaggeration: 1,
  },
};

[viernullvier]

FYI: I've been seeing these crashes as well in a completely different context (Angular + MapBox Draw), so it's definitely not related to the specific wrapper.

The crash is able to stall the map instance for good because there is no error cleanup in the render task queue. If any function in the task queue throws, it gets stuck in the "running" state indefinitely and rejects any subsequent calls to _render with Attempting to run(), but is already running.. The crash can be easily triggered from the outside by calling map._renderTaskQueue.add(functionThatThrows).

I don't think this is the right place to fix it though - adding error handling to the render task queue will probably impact performance massively, and render tasks should not be expected to error out.

It is, however, a good place for a workaround - I'm currently probing for the crashed/stuck/stalled state after enabling terrain and overriding the task queue state if the probe fails:

requestAnimationFrame(() => {
  try {
    map._render(Date.now())
  } catch {
    map._renderTaskQueue._currentlyRunning = false
    map._render(Date.now())
  }
})

I'm still investigating if/how I can minimally reproduce the clearFadeHold issue, but even in my project it only happens under very specific circumstances, albeit reliably.

[viernullvier]

Managed to reproduce it semi-reliably - here's a slightly modified version of the 3d-terrain.html example:
https://jsbin.com/qihavanivo/edit?html,output
It's possible that this example doesn't trigger the bug immediately, but only after interacting or after a hard refresh.

The issue happens during motion, especially zooming. From time to time, there's a single tile kept in retain that belongs to one of the lowest zoom levels, but doesn't actually exist in this._tiles anymore.

[HarelM]

@viernullvier thanks for looking into this!
From your reproduction and explanation it seems that there's a problem in the retained tiles code, right?
It would be great if you could investigate it further and find a way to reproduce this, even using a unit test and then we can decide what's the best way to fix it.
Bottom line, if this is the right way to fix it, so be it, but there's a need for a test to make sure we cover this code change for future issues.