Abort/assertion on orderedLayers.size() == renderLayers.size()

[springmeyer]

It is currently possible to trigger an assertion in debug mode (and a crash in release mode) in render_orchestrator.cpp specifically:

Assertion failed: (orderedLayers.size() == renderLayers.size()), function createRenderTree, file ../../src/mbgl/renderer/render_orchestrator.cpp, line 249.

Steps to trigger behavior

1. Load a map with a style many layers
2. Render the map
3. Load another style into the same map, but without any layers
4. Render the map: 💥

Expected behavior

No crash or assertion when reloading a map without layers.

Actual behavior

Assertion.

It looks like a patch like this might fix this:

diff --git a/src/mbgl/renderer/render_orchestrator.cpp b/src/mbgl/renderer/render_orchestrator.cpp
index d7f13695d..590a9b85a 100644
--- a/src/mbgl/renderer/render_orchestrator.cpp
+++ b/src/mbgl/renderer/render_orchestrator.cpp
@@ -218,7 +218,7 @@ std::unique_ptr<RenderTree> RenderOrchestrator::createRenderTree(

     const LayerDifference layerDiff = diffLayers(layerImpls, updateParameters->layers);
     layerImpls = updateParameters->layers;
-    const bool layersAddedOrRemoved = !layerDiff.added.empty() || !layerDiff.removed.empty();
+    const bool layersAddedOrRemoved = layerImpls->empty() || !layerDiff.added.empty() || !layerDiff.removed.empty();

     // Remove render layers for removed layers.
     for (const auto& entry : layerDiff.removed) {

[pozdnyakov]

@springmeyer I'm having troubles reproducing this issue. I tried reproducing it with every map mode, and did not succeed. Below is the example of a unit test for Static mode:

TEST(Map, UpdateToEmptyStyle) {
    MapTest<> test{};

    test.fileSource->tileResponse = makeResponse("vector.tile");

    test.map.getStyle().loadJSON(util::read_file("test/fixtures/api/water.json"));
    test.frontend.render(test.map);

    test.map.getStyle().loadJSON(util::read_file("test/fixtures/api/empty.json"));
    test.frontend.render(test.map);
}

Updating to an empty style (as well as any other style update) is covered by const LayerDifference layerDiff = diffLayers(layerImpls, updateParameters->layers); function call, which is being there for a very long time, the erased layers go to layerDiff.removed.

Could you give more details about the styles/environment causing the assertion hit?

[springmeyer]

Could you give more details about the styles/environment causing the assertion hit?

👍 I hit while running our test suite. I'll need to circle back to fish out which styles were involved.

[springmeyer]

I've isolated a testcase now using node bindings, that asserts predictably (or segfaults if I remove the assert). But when I port it over to a gl-native unit test it does not crash. So, there may be something unique in regards to the bindings code. So, I'll close this for now and re-open if I can replicate with pure gl-native.

[springmeyer]

I've found some other workarounds, which seem to indicate the problem is related to clearData():

• The crash goes away if I don't call renderer->clearData();
• The crash also goes away if I modify clearData() in these two ways:

diff --git a/src/mbgl/renderer/render_orchestrator.cpp b/src/mbgl/renderer/render_orchestrator.cpp
index d7f13695d..b4bd725a0 100644
--- a/src/mbgl/renderer/render_orchestrator.cpp
+++ b/src/mbgl/renderer/render_orchestrator.cpp
@@ -703,7 +703,7 @@ bool RenderOrchestrator::isLoaded() const {

 void RenderOrchestrator::clearData() {
     if (!sourceImpls->empty()) sourceImpls = makeMutable<std::vector<Immutable<style::Source::Impl>>>();
-    if (!layerImpls->empty()) layerImpls = makeMutable<std::vector<Immutable<style::Layer::Impl>>>();
+    // if (!layerImpls->empty()) layerImpls = makeMutable<std::vector<Immutable<style::Layer::Impl>>>();
     if (!imageImpls->empty()) imageImpls = makeMutable<std::vector<Immutable<style::Image::Impl>>>();

     renderSources.clear();

diff --git a/src/mbgl/renderer/render_orchestrator.cpp b/src/mbgl/renderer/render_orchestrator.cpp
index d7f13695d..6be2558c6 100644
--- a/src/mbgl/renderer/render_orchestrator.cpp
+++ b/src/mbgl/renderer/render_orchestrator.cpp
@@ -708,6 +708,7 @@ void RenderOrchestrator::clearData() {

     renderSources.clear();
     renderLayers.clear();
+    orderedLayers.clear();

     crossTileSymbolIndex.reset();

cc @pozdnyakov in case you have ideas.

[springmeyer]

re-opening, found a way to replicate with just gl-native (without my custom binding code).

[springmeyer]

@pozdnyakov I've found that it is not possible to replicate this problem using the existing map unit tests. In my usage in the node-bindings (where I discovered the problem) I've worked to avoid Map::Impl::onUpdate() getting called excessively before still rendering. But the map unit tests call it several times before actually attempting to render, which I think hides the bug because state of the objects inside the RenderOrchestrator are modified slightly each time onUpdate() is called.

So I wrote a standalone testcase that replicates more closely how my bindings are calling into the renderer. What it takes to replicate is:

• Rendering with a style with layers
• Clearing data
• Rendering again with a style without layers
• Assert/crash

#include <mbgl/renderer/render_orchestrator.hpp>
#include <mbgl/style/style.hpp>
#include <mbgl/map/transform.hpp>
#include <mbgl/renderer/update_parameters.hpp>
#include <mbgl/storage/file_source.hpp>
#include <mbgl/style/style_impl.hpp>
#include <mbgl/map/map_impl.hpp>
#include <mbgl/renderer/render_tree.hpp>

int main() {
  using namespace mbgl;

  const float pixelRatio = 1.0;
  std::shared_ptr<FileSource> fileSource = nullptr;
  style::Style style(fileSource,pixelRatio);
  style.loadJSON("{ \"version\": 8, \"layers\": [{ \"id\": \"land\", \"type\": \"background\" }]}");

  style::Style style2(fileSource,pixelRatio);
  style2.loadJSON("{ \"version\": 8 }");

  optional<std::string> localFontFamily;
  RenderOrchestrator ro(false,localFontFamily);

  TimePoint timePoint = Clock::time_point::max();
  MapDebugOptions debugOptions { MapDebugOptions::NoDebug };
  const auto mode = MapMode::Tile;
  bool crossSourceCollisions = true;
  uint8_t prefetchZoomDelta = 0;
  Transform transform;
  std::unique_ptr<StillImageRequest> stillImageRequest;
  UpdateParameters params = {
      style.impl->isLoaded(),
      mode,
      pixelRatio,
      debugOptions,
      timePoint,
      transform.getState(),
      style.impl->getGlyphURL(),
      style.impl->spriteLoaded,
      style.impl->getTransitionOptions(),
      style.impl->getLight()->impl,
      style.impl->getImageImpls(),
      style.impl->getSourceImpls(),
      style.impl->getLayerImpls(),
      mapbox::base::WeakPtr<AnnotationManager>{},
      fileSource,
      prefetchZoomDelta,
      bool(stillImageRequest),
      crossSourceCollisions
  };
  auto update = std::make_shared<UpdateParameters>(std::move(params));
  ro.createRenderTree(update);
  ro.clearData();

  UpdateParameters params2 = {
      style2.impl->isLoaded(),
      mode,
      pixelRatio,
      debugOptions,
      timePoint,
      transform.getState(),
      style2.impl->getGlyphURL(),
      style2.impl->spriteLoaded,
      style2.impl->getTransitionOptions(),
      style2.impl->getLight()->impl,
      style2.impl->getImageImpls(),
      style2.impl->getSourceImpls(),
      style2.impl->getLayerImpls(),
      mapbox::base::WeakPtr<AnnotationManager>{},
      fileSource,
      prefetchZoomDelta,
      bool(stillImageRequest),
      crossSourceCollisions
  };
  auto update2 = std::make_shared<UpdateParameters>(std::move(params2));
  ro.createRenderTree(update2);
}

If I compile that locally I see:

./a.out 
2020-05-29 11:28:27.459 a.out[1449:5869654] [ERROR] {}[Style]: Failed to load sprite: Unable to find resource provider for sprite url.
2020-05-29 11:28:27.460 a.out[1449:5869654] [ERROR] {}[Style]: Failed to load sprite: Unable to find resource provider for sprite url.
Assertion failed: (orderedLayers.size() == renderLayers.size()), function createRenderTree, file /Users/danespringmeyer/projects/mbgl/src/mbgl/renderer/render_orchestrator.cpp, line 258.
[1]    1449 abort      ./a.out

[springmeyer]

My understanding is that the solution to this bug is something like:

But development moved elsewhere and therefore it never made sense to get this applied to the codebase. Closing, therefore.