[core] fix loading of stale must-revalidate resources

In https://github.com/mapbox/mapbox-gl-native/pull/9670, we implemented support for the Cache-Control: must-revalidate header. While we now respect this header and don't show resources that are stale and have this header set, the optional cache request never completes. This means we're also never going to try to actually get a fresh tile and just never show this tile anymore.

This commit fixes this by responding with a Not Found error when the resource is unusable (= stale and has must-revalidate set). Since we actually still have the data (but can't use it due to caching rules), we're responding with the data as well. To avoid a second cache hit, tile_loader_impl.hpp now passes on the data from the Optional to the Required request so that it can be reused when we get a 304 Not Modified response from the server.

3 files changed, 96 insertions, 9 deletions

diff --git a/platform/default/default_file_source.cpp b/platform/default/default_file_source.cpp
index 1e855dd9f7..3fdb03e6b4 100644
--- a/platform/default/default_file_source.cpp
+++ b/platform/default/default_file_source.cpp
@@ -126,9 +126,8 @@ public:
             tasks[req] = localFileSource->request(resource, callback);
         } else {
             // Try the offline database
-            Resource revalidation = resource;
-
-            const bool hasPrior = resource.priorEtag || resource.priorModified || resource.priorExpires;
+            const bool hasPrior = resource.priorEtag || resource.priorModified ||
+                                  resource.priorExpires || resource.priorData;
             if (!hasPrior || resource.necessity == Resource::Optional) {
                 auto offlineResponse = offlineDatabase->get(resource);
 
@@ -142,27 +141,34 @@ public:
                 }
 
                 if (offlineResponse) {
-                    revalidation.priorModified = offlineResponse->modified;
-                    revalidation.priorExpires = offlineResponse->expires;
-                    revalidation.priorEtag = offlineResponse->etag;
+                    resource.priorModified = offlineResponse->modified;
+                    resource.priorExpires = offlineResponse->expires;
+                    resource.priorEtag = offlineResponse->etag;
 
                     // Don't return resources the server requested not to show when they're stale.
                     // Even if we can't directly use the response, we may still use it to send a
                     // conditional HTTP request.
                     if (offlineResponse->isUsable()) {
                         callback(*offlineResponse);
+                    } else if (resource.necessity == Resource::Optional) {
+                        // Instead of the data that we got, return a not found error so that
+                        // underlying implementations know about the fact that we couldn't find
+                        // usable cache data.
+                        offlineResponse->error = std::make_unique<Response::Error>(
+                            Response::Error::Reason::NotFound, "Cached resource is unusable");
+                        callback(*offlineResponse);
                     } else {
                         // Since we can't return the data immediately, we'll have to hold on so that
                         // we can return it later in case we get a 304 Not Modified response.
-                        revalidation.priorData = offlineResponse->data;
+                        resource.priorData = offlineResponse->data;
                     }
                 }
             }
 
             // Get from the online file source
             if (resource.necessity == Resource::Required) {
-                tasks[req] = onlineFileSource.request(revalidation, [=] (Response onlineResponse) mutable {
-                    this->offlineDatabase->put(revalidation, onlineResponse);
+                tasks[req] = onlineFileSource.request(resource, [=] (Response onlineResponse) mutable {
+                    this->offlineDatabase->put(resource, onlineResponse);
                     callback(onlineResponse);
                 });
             }
diff --git a/src/mbgl/tile/tile_loader_impl.hpp b/src/mbgl/tile/tile_loader_impl.hpp
index 899cbaf9b0..598ec32c10 100644
--- a/src/mbgl/tile/tile_loader_impl.hpp
+++ b/src/mbgl/tile/tile_loader_impl.hpp
@@ -61,7 +61,10 @@ void TileLoader<T>::loadOptional() {
             // When the optional request could not be satisfied, don't treat it as an error.
             // Instead, we make sure that the next request knows that there has been an optional
             // request before by setting one of the prior* fields.
+            resource.priorModified = res.modified;
             resource.priorExpires = Timestamp{ Seconds::zero() };
+            resource.priorEtag = res.etag;
+            resource.priorData = res.data;
         } else {
             loadedData(res);
         }
diff --git a/test/storage/default_file_source.test.cpp b/test/storage/default_file_source.test.cpp
index b8aebae7e7..b5686b5ffe 100644
--- a/test/storage/default_file_source.test.cpp
+++ b/test/storage/default_file_source.test.cpp
@@ -571,3 +571,81 @@ TEST(DefaultFileSource, TEST_REQUIRES_SERVER(SetResourceTransform)) {
 
     loop.run();
 }
+
+// Test that a stale cache file that has must-revalidate set will trigger a response.
+TEST(DefaultFileSource, TEST_REQUIRES_SERVER(RespondToStaleMustRevalidate)) {
+    util::RunLoop loop;
+    DefaultFileSource fs(":memory:", ".");
+
+    Resource resource { Resource::Unknown, "http://127.0.0.1:3000/revalidate-same" };
+    resource.necessity = Resource::Necessity::Optional;
+
+    // using namespace std::chrono_literals;
+
+    // Put an existing value in the cache that has expired, and has must-revalidate set.
+    Response response;
+    response.data = std::make_shared<std::string>("Cached value");
+    response.modified = Timestamp(Seconds(1417392000)); // December 1, 2014
+    response.expires = Timestamp(Seconds(1417392000));
+    response.mustRevalidate = true;
+    response.etag.emplace("snowfall");
+    fs.put(resource, response);
+
+    std::unique_ptr<AsyncRequest> req;
+    req = fs.request(resource, [&](Response res) {
+        req.reset();
+        ASSERT_TRUE(res.error.get());
+        EXPECT_EQ(Response::Error::Reason::NotFound, res.error->reason);
+        EXPECT_EQ("Cached resource is unusable", res.error->message);
+        EXPECT_FALSE(res.notModified);
+        ASSERT_TRUE(res.data.get());
+        EXPECT_EQ("Cached value", *res.data);
+        ASSERT_TRUE(res.expires);
+        EXPECT_EQ(Timestamp{ Seconds(1417392000) }, *res.expires);
+        EXPECT_TRUE(res.mustRevalidate);
+        ASSERT_TRUE(res.modified);
+        EXPECT_EQ(Timestamp{ Seconds(1417392000) }, *res.modified);
+        ASSERT_TRUE(res.etag);
+        EXPECT_EQ("snowfall", *res.etag);
+
+        resource.priorEtag = res.etag;
+        resource.priorModified = res.modified;
+        resource.priorExpires = res.expires;
+        resource.priorData = res.data;
+
+        loop.stop();
+    });
+
+    loop.run();
+
+    // Now run this request again, with the data we gathered from the previous stale/unusable
+    // request. We're replacing the data so that we can check that the DefaultFileSource doesn't
+    // attempt another database access if we already have the value.
+    resource.necessity = Resource::Necessity::Required;
+    resource.priorData = std::make_shared<std::string>("Prior value");
+
+    req = fs.request(resource, [&](Response res) {
+        req.reset();
+        ASSERT_EQ(nullptr, res.error.get());
+        // Since the data was found in the cache, we're doing a revalidation request. Yet, since
+        // this request hasn't returned data before, we're setting notModified to false in the
+        // OnlineFileSource to ensure that requestors know that this is the first time they're
+        // seeing this data.
+        EXPECT_FALSE(res.notModified);
+        ASSERT_TRUE(res.data.get());
+        // Ensure that it's the value that we manually inserted into the cache rather than the value
+        // the server returns, since we should be executing a revalidation request which doesn't
+        // return new data, only a 304 Not Modified response.
+        EXPECT_EQ("Prior value", *res.data);
+        ASSERT_TRUE(res.expires);
+        EXPECT_LE(util::now(), *res.expires);
+        EXPECT_TRUE(res.mustRevalidate);
+        ASSERT_TRUE(res.modified);
+        EXPECT_EQ(Timestamp{ Seconds(1417392000) }, *res.modified);
+        ASSERT_TRUE(res.etag);
+        EXPECT_EQ("snowfall", *res.etag);
+        loop.stop();
+    });
+
+    loop.run();
+}