From 6dfb4ecc439b2a50b65f396142885c47161af28b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= <mail@kkaefer.com>
Date: Fri, 15 Sep 2017 23:35:26 +0200
Subject: [core] keep tiles renderable even if a subsequent error occurs

Since 9a9408e8111bcdcd0fcb9a93112d61ab8fce0601, we marked tiles as non-renderable if an error occured. This lead to situations where a tile was loaded + parsed successfully, then a revalidation attempt occured (e.g. because the resource was stale) which failed. In this case, we used to mark the tile as non-renderable although we could've used the perfectly parsed (stale) resource.
---
 src/mbgl/tile/geometry_tile.cpp |  2 --
 src/mbgl/tile/raster_tile.cpp   |  3 ---
 test/tile/geojson_tile.test.cpp | 37 +++++++++++++++++++++++++++++++++++++
 test/tile/raster_tile.test.cpp  | 14 ++++++++++++++
 4 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp
index 29ba7d42cd..c4d5d3bae3 100644
--- a/src/mbgl/tile/geometry_tile.cpp
+++ b/src/mbgl/tile/geometry_tile.cpp
@@ -58,7 +58,6 @@ void GeometryTile::cancel() {
 
 void GeometryTile::setError(std::exception_ptr err) {
     loaded = true;
-    renderable = false;
     observer->onTileError(*this, err);
 }
 
@@ -144,7 +143,6 @@ void GeometryTile::onPlacement(PlacementResult result) {
 void GeometryTile::onError(std::exception_ptr err) {
     loaded = true;
     pending = false;
-    renderable = false;
     observer->onTileError(*this, err);
 }
     
diff --git a/src/mbgl/tile/raster_tile.cpp b/src/mbgl/tile/raster_tile.cpp
index b1a901e565..770561e703 100644
--- a/src/mbgl/tile/raster_tile.cpp
+++ b/src/mbgl/tile/raster_tile.cpp
@@ -29,7 +29,6 @@ void RasterTile::cancel() {
 
 void RasterTile::setError(std::exception_ptr err) {
     loaded = true;
-    renderable = false;
     observer->onTileError(*this, err);
 }
 
@@ -49,9 +48,7 @@ void RasterTile::onParsed(std::unique_ptr<Bucket> result) {
 }
 
 void RasterTile::onError(std::exception_ptr err) {
-    bucket.reset();
     loaded = true;
-    renderable = false;
     observer->onTileError(*this, err);
 }
 
diff --git a/test/tile/geojson_tile.test.cpp b/test/tile/geojson_tile.test.cpp
index a0383f06c9..80e215a98d 100644
--- a/test/tile/geojson_tile.test.cpp
+++ b/test/tile/geojson_tile.test.cpp
@@ -70,3 +70,40 @@ TEST(GeoJSONTile, Issue7648) {
         test.loop.runOnce();
     }
 }
+
+// Tests that tiles remain renderable if they have been renderable and then had an error sent to
+// them, e.g. when revalidating/refreshing the request.
+TEST(GeoJSONTile, Issue9927) {
+    GeoJSONTileTest test;
+
+    CircleLayer layer("circle", "source");
+
+    mapbox::geometry::feature_collection<int16_t> features;
+    features.push_back(mapbox::geometry::feature<int16_t> {
+        mapbox::geometry::point<int16_t>(0, 0)
+    });
+
+    GeoJSONTile tile(OverscaledTileID(0, 0, 0), "source", test.tileParameters, features);
+
+    tile.setLayers({{ layer.baseImpl }});
+    tile.setPlacementConfig({});
+
+    while (!tile.isComplete()) {
+        test.loop.runOnce();
+    }
+
+    ASSERT_TRUE(tile.isRenderable());
+    ASSERT_NE(nullptr, tile.getBucket(*layer.baseImpl));
+
+    // Make sure that once we've had a renderable tile and then receive erroneous data, we retain
+    // the previously rendered data and keep the tile renderable.
+    tile.setError(std::make_exception_ptr(std::runtime_error("Connection offline")));
+    ASSERT_TRUE(tile.isRenderable());
+    ASSERT_NE(nullptr, tile.getBucket(*layer.baseImpl));
+
+    // Then simulate a parsing failure and make sure that we keep it renderable in this situation
+    // as well.
+    tile.onError(std::make_exception_ptr(std::runtime_error("Parse error")));
+    ASSERT_TRUE(tile.isRenderable());
+    ASSERT_NE(nullptr, tile.getBucket(*layer.baseImpl));
+ }
diff --git a/test/tile/raster_tile.test.cpp b/test/tile/raster_tile.test.cpp
index f841a82e68..596f97884c 100644
--- a/test/tile/raster_tile.test.cpp
+++ b/test/tile/raster_tile.test.cpp
@@ -60,6 +60,20 @@ TEST(RasterTile, onParsed) {
     EXPECT_TRUE(tile.isRenderable());
     EXPECT_TRUE(tile.isLoaded());
     EXPECT_TRUE(tile.isComplete());
+
+    // Make sure that once we've had a renderable tile and then receive erroneous data, we retain
+    // the previously rendered data and keep the tile renderable.
+    tile.setError(std::make_exception_ptr(std::runtime_error("Connection offline")));
+    EXPECT_TRUE(tile.isRenderable());
+    EXPECT_TRUE(tile.isLoaded());
+    EXPECT_TRUE(tile.isComplete());
+
+    // Then simulate a parsing failure and make sure that we keep it renderable in this situation
+    // as well.
+    tile.onError(std::make_exception_ptr(std::runtime_error("Parse error")), 0);
+    ASSERT_TRUE(tile.isRenderable());
+    EXPECT_TRUE(tile.isLoaded());
+    EXPECT_TRUE(tile.isComplete());
 }
 
 TEST(RasterTile, onParsedEmpty) {
-- 
cgit v1.2.1