From f3b5902f05e941f3f62bcd39ebb64800e40d45c1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= <mail@kkaefer.com>
Date: Thu, 12 Jul 2018 13:18:22 +0200
Subject: [node] prevent scheduled renderFinished() calls from running when
 render is canceled Reverts d5aff71 (#11948) and implements an alternative
 solution.

Even with the previous fix applied, we were calling `renderFinished()` directly when the render was canceled. This led to the following scenario (all on the main thread):
- `Map::renderStill()` callback is called, `uv_async_send` schedules a call to `renderFinished()`.
- `NodeMap::cancel()` destroys the map object, and calls `renderFinished()` directly, which cleans up the request
- `NodeMap::startRender()` is called to schedule a _new_ render, which means that a new callback will be set
- The `uv_async_t` fires and calls `renderFinished` again:
   - We don't return early as intended by the fix in #11948 since a new callback/`req` object is now available.
   - Since neither error nor image has been set, we'll see a `Didn't get an image` error message.
   - We call the callback with the error. In scenario where we're reusing Map objects, we will start a new render, which will now trigger the `Map is currently rendering an image` error in `Map::renderStill`, and the map object enters a circle in which it will now fail repeatedly with this error.

This commit fixes the issue by completely removing the existing `uv_async_t` handle that could have a scheduled call, and replacing it with a fresh instance of an `uv_async_t` handle.
---
 platform/node/src/node_map.cpp | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/platform/node/src/node_map.cpp b/platform/node/src/node_map.cpp
index 328bff1d7b..4bcd1d97bc 100644
--- a/platform/node/src/node_map.cpp
+++ b/platform/node/src/node_map.cpp
@@ -460,21 +460,13 @@ void NodeMap::startRender(NodeMap::RenderOptions options) {
 }
 
 void NodeMap::renderFinished() {
-    if (!req) {
-        // In some situations, the render finishes at the same time as we call cancel. Make sure
-        // we are only finishing a render once.
-        return;
-    }
+    assert(req);
 
     Nan::HandleScope scope;
 
     // We're done with this render call, so we're unrefing so that the loop could close.
     uv_unref(reinterpret_cast<uv_handle_t *>(async));
 
-    // There is no render pending anymore, we the GC could now delete this object if it went out
-    // of scope.
-    Unref();
-
     // Move the callback and image out of the way so that the callback can start a new render call.
     auto request = std::move(req);
     auto img = std::move(image);
@@ -527,6 +519,10 @@ void NodeMap::renderFinished() {
         };
         request->runInAsyncScope(target, callback, 1, argv);
     }
+
+    // There is no render pending anymore, we the GC could now delete this object if it went out
+    // of scope.
+    Unref();
 }
 
 /**
@@ -585,6 +581,17 @@ void NodeMap::cancel() {
     // Reset map explicitly as it resets the renderer frontend
     map.reset();
 
+    // Remove the existing async handle to flush any scheduled calls to renderFinished.
+    uv_unref(reinterpret_cast<uv_handle_t *>(async));
+    uv_close(reinterpret_cast<uv_handle_t *>(async), [] (uv_handle_t *h) {
+        delete reinterpret_cast<uv_async_t *>(h);
+    });
+    async = new uv_async_t;
+    async->data = this;
+    uv_async_init(uv_default_loop(), async, [](uv_async_t* h) {
+        reinterpret_cast<NodeMap *>(h->data)->renderFinished();
+    });
+
     frontend = std::make_unique<mbgl::HeadlessFrontend>(mbgl::Size{ 256, 256 }, pixelRatio, *this, threadpool);
     map = std::make_unique<mbgl::Map>(*frontend, mapObserver, frontend->getSize(), pixelRatio,
                                       *this, threadpool, mode);
-- 
cgit v1.2.1