Fix crashes in GeoJsonDocumentRasterOverlayTileProvider::loadTileImage

[baruchInsert-tech]

Summary

Three bugs in GeoJsonDocumentRasterOverlayTileProvider::loadTileImage that caused native crashes when roaming around a tileset with a GeoJSON raster overlay.

Details

Bug 1: Dangling reference capture of _tree

In GeoJsonDocumentRasterOverlayTileProvider::loadTileImage(), the runInWorkerThread lambda captured _tree by reference (&tree = this->_tree). All other captures in the lambda were correctly by value.

RasterOverlayTileProvider is reference-counted via ReferenceCountedNonThreadSafe, and the lambda did not capture any reference-counted pointer to the tile provider. When tiles were unloaded (e.g., camera moves away), the tile provider could be destroyed before queued worker thread tasks executed, leaving the &tree reference dangling.

Fix:

• Changed _tree from Quadtree to std::shared_ptr<Quadtree> so worker thread lambdas can share ownership
• Capture pTree by value (shared_ptr copy) in the lambda to keep the quadtree alive
• Also capture _pDocument by value since the Quadtree holds raw const GeoJsonObject* pointers into the document data

Bug 2: Dangling raw pointer in QuadtreeGeometryData::pStyle

QuadtreeGeometryData stored a const VectorStyle* raw pointer to the tile provider's _defaultStyle. When the tile provider was destroyed, worker threads still accessed the freed style through this pointer.

Fix:

• Changed QuadtreeGeometryData::pStyle from const VectorStyle* to VectorStyle style stored by value

Bug 3: Zero-dimension texture size crash

The texture size computation could produce a zero width or height when getTargetScreenPixels() returns a small value in one dimension. Dividing by maximumScreenSpaceError and truncating to int produced 0, causing Blend2D to crash when creating a zero-size image.

Fix:

• Clamp textureSize to a minimum of 1×1 with glm::max(..., glm::ivec2(1))

Closes #1313

@j9liu

[azrogers]

@baruchInsert-tech The first and third changes you list seem like good fixes. However, I'm not sure about the second one. The pStyle pointer will be a pointer to either the style field on a GeoJsonObject in _pDocument, or it will be a pointer to the default style. It should not cause problems if it is a pointer to a style field, as _pDocument being a shared pointer ensures that this pointer will be valid for as long as the worker thread maintains a copy of the shared pointer. I can see how it might cause problems if it is a pointer to the default style, because the default style is stored on the tile provider and so could be invalid by the time the worker thread is rasterizing the tile. However, I would prefer not to copy the style for every object as this will increase memory usage (64 bytes for VectorStyle versus 4 or 8 bytes for a pointer) which could be significant for large datasets. I would suggest instead adding the _defaultStyle to the quadtree and passing that to addPrimitivesToData so that we can have a pointer to the default style that we can be sure will have the same lifetime as the worker thread lambda. This will involve re-ordering the initialization so that the quadtree object exists before addPrimitivesToData is called, but this shouldn't be too much of an issue.

Besides that, this change is looking good!

[baruchInsert-tech]

@azrogers
Thanks for the review! I agree that copying the full VectorStyle for every object is wasteful. I've pushed a new commit that implements your suggestion:

• Reverted QuadtreeGeometryData::pStyle back to a raw pointer
• Added defaultStyle to the Quadtree struct
• Re-ordered buildQuadtree() so the Quadtree (with its defaultStyle) is created before addPrimitivesToData is called, ensuring all pStyle pointers to tree.defaultStyle remain valid for the lifetime of the shared_ptr
• Removed _defaultStyle from the tile provider since the quadtree now owns it

[azrogers]

It doesn't look like that change quite did it. Ironically, I don't experience any crashes using the GeoJsonDocumentRasterOverlay normally, but with this PR I see a crash immediately. It looks like the style pointers aren't quite being handled properly. Have you run into this in testing?

[baruchInsert-tech]

@azrogers

the crash happens when using Cesium for Unity: I load a tileset, draw a polygon over an area and when zooming in and roaming around, the Unity editor/application crashes after a couple of seconds.

I tracked down the root cause, and it also explains the 3 failing CI tests (can render lines with bounding box height set by pixels, ...by meters, ...wrapping around the earth).

The problem is that buildQuadtree() returns a Quadtree by value, and the constructor does:

this->_pTree = std::make_shared(buildQuadtree(...));

Inside buildQuadtree, addPrimitivesToData stores raw pointers like pStyle = &tree.defaultStyle - pointing into the
local Quadtree. When the function returns, make_shared move-constructs the Quadtree into a new heap allocation.
defaultStyle now lives at a different address, but all the pStyle raw pointers in data still point to the old
destroyed local. Every access to pStyle after that is a use-after-free - which causes the crash, and also makes the
rasterized test images produce garbage output.

Fix: allocate the Quadtree on the heap first, then populate it in-place so &tree.defaultStyle always refers to the final address:

this->_pTree = std::make_shared();
buildQuadtree(
*this->_pTree,
this->_pDocument,
geoJsonOptions.defaultStyle,
geoJsonOptions.ellipsoid);

With buildQuadtree changed from returning by value to taking a Quadtree& parameter. Pushed the fix.

[azrogers]

Thanks @baruchInsert-tech!