Use FrameTreeNode ID as frameId in extension APIs

extensions/browser/extension_api_frame_id_map_unittest.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/bind.h"
+#include "base/run_loop.h"
+#include "base/time/time.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "extensions/browser/extension_api_frame_id_map.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using FrameIdCallback = extensions::ExtensionApiFrameIdMap::FrameIdCallback;
+
+namespace extensions {
+
+namespace {
+
+ExtensionApiFrameId ToTestFrameId(int render_process_id, int frame_routing_id) {
+  if (render_process_id == -1 && frame_routing_id == -1)
+    return ExtensionApiFrameId();
+  // Return a deterministic value (yet different from the input) for testing.
+  return ExtensionApiFrameId(render_process_id + 1, frame_routing_id + 1);
+}
+
+class TestExtensionApiFrameIdMap : public ExtensionApiFrameIdMap {
+ public:
+  int GetInternalSize() { return frame_id_map_.size(); }
+  int GetInternalCallbackCount() { return callbacks_.size(); }
+  void TriggerInternalCleanup() {
+    // Modify the internal structure: Set the creation time to a time in the
+    // distant past and trigger a removal.
+    for (FrameIdRemovalTask& task : pending_deletions_)
+      task.creation_time = base::TimeTicks();
+    // The map cannot have invalid frame IDs, so this is a no-op.
+    RemoveFrameId(-1, -1);
+    // The only remaining task is the removal of the invalid ID above, all other
+    // pending deletion tasks should have been run by now.
+    EXPECT_EQ(1u, pending_deletions_.size());
+  }
+
+ private:
+  // ExtensionApiFrameIdMap:
+  ExtensionApiFrameId KeyToValue(const RenderFrameIdKey& key) const override {
+    return ToTestFrameId(key.render_process_id, key.frame_routing_id);
+  }
+};
+
+class ExtensionApiFrameIdMapTest : public testing::Test {
+ public:
+  ExtensionApiFrameIdMapTest()
+      : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP) {}
+
+  FrameIdCallback CreateCallback(int render_process_id, int frame_routing_id) {
+    return base::Bind(&ExtensionApiFrameIdMapTest::OnCalledCallback,
+                      base::Unretained(this), render_process_id,
+                      frame_routing_id);
+  }
+
+  void OnCalledCallback(int render_process_id,
+                        int frame_routing_id,
+                        const ExtensionApiFrameId extension_api_frame_id) {
+    // Save results for later, to see whether the ordering is OK.
+    results_.push_back(extension_api_frame_id);
+
+    // If this fails, then the mapping is completely wrong.
+    EXPECT_EQ(ToTestFrameId(render_process_id, frame_routing_id),
+              extension_api_frame_id);
+  }
+
+  const std::vector<ExtensionApiFrameId>& results() { return results_; }
+
+ private:
+  content::TestBrowserThreadBundle thread_bundle_;
+  std::vector<ExtensionApiFrameId> results_;
+
+  DISALLOW_COPY_AND_ASSIGN(ExtensionApiFrameIdMapTest);
+};
+
+}  // namespace
+
+// Tests whether getting data from the map gives consistent results.
+TEST_F(ExtensionApiFrameIdMapTest, Basics) {
+  scoped_ptr<TestExtensionApiFrameIdMap> map(new TestExtensionApiFrameIdMap());

[nasko, 2015/12/22 22:12:10]

Why not move this to be a member of the test class? There are no test cases that
use more than one map.

+  EXPECT_EQ(0, map->GetInternalSize());
+
+  EXPECT_EQ(ToTestFrameId(1, 2), map->GetFrameId(1, 2));
+  EXPECT_EQ(1, map->GetInternalSize());
+
+  // Already cached, the map size should not change.
+  EXPECT_EQ(ToTestFrameId(1, 2), map->GetFrameId(1, 2));
+  EXPECT_EQ(1, map->GetInternalSize());
+
+  EXPECT_EQ(ToTestFrameId(2, 1), map->GetFrameId(2, 1));
+  EXPECT_EQ(2, map->GetInternalSize());
+
+  // Don't save invalid values in the map because they are never deleted.
+  EXPECT_EQ(ToTestFrameId(-1, -1), map->GetFrameId(-1, -1));
+  EXPECT_EQ(2, map->GetInternalSize());
+
+  // Remove frame ID. Key erasure is not immediate but delayed.
+  map->RemoveFrameId(1, 2);
+  EXPECT_EQ(2, map->GetInternalSize());
+  map->TriggerInternalCleanup();
+  EXPECT_EQ(1, map->GetInternalSize());

[nasko, 2015/12/22 22:12:10]

If the map moves to be a class member, then you can have a wrapper to
RemoveFrameId which can trigger the cleanup and then the test case code will be
much easier to read:

EXPECT_EQ(2, map->GetInternalSize());
map->Remove(1, 2);
EXPECT_EQ(1, map->GetInternalSize());

+
+  // Non-existing frame ID.
+  map->RemoveFrameId(1, 1337);
+  EXPECT_EQ(1, map->GetInternalSize());
+  map->TriggerInternalCleanup();
+  EXPECT_EQ(1, map->GetInternalSize());
+
+  // Non-existing frame ID (because it's already removed).
+  map->RemoveFrameId(1, 2);
+  EXPECT_EQ(1, map->GetInternalSize());
+  map->TriggerInternalCleanup();
+  EXPECT_EQ(1, map->GetInternalSize());
+
+  // Invalid frame ID.
+  map->RemoveFrameId(-1, -1);
+  EXPECT_EQ(1, map->GetInternalSize());
+  map->TriggerInternalCleanup();
+  EXPECT_EQ(1, map->GetInternalSize());
+}
+
+TEST_F(ExtensionApiFrameIdMapTest, GetFrameIdOnIO) {
+  scoped_ptr<TestExtensionApiFrameIdMap> map(new TestExtensionApiFrameIdMap());
+  EXPECT_EQ(0, map->GetInternalSize());
+
+  // Two identical calls, should be processed at the next message loop.
+  map->GetFrameIdOnIO(1, 2, CreateCallback(1, 2));
+  EXPECT_EQ(1, map->GetInternalCallbackCount());
+  EXPECT_EQ(0, map->GetInternalSize());
+
+  map->GetFrameIdOnIO(1, 2, CreateCallback(1, 2));
+  EXPECT_EQ(2, map->GetInternalCallbackCount());
+  EXPECT_EQ(0, map->GetInternalSize());
+
+  // First get the frame ID on IO (queued on message loop), then on UI.
+  map->GetFrameIdOnIO(2, 1, CreateCallback(2, 1));
+  EXPECT_EQ(3, map->GetInternalCallbackCount());
+  EXPECT_EQ(0, map->GetInternalSize());
+
+  EXPECT_EQ(ToTestFrameId(2, 1), map->GetFrameId(2, 1));
+  EXPECT_EQ(1, map->GetInternalSize());
+
+  // First get the frame ID on UI, then on IO (queued on message loop).
+  EXPECT_EQ(ToTestFrameId(3, 1), map->GetFrameId(3, 1));
+  EXPECT_EQ(2, map->GetInternalSize());
+
+  map->GetFrameIdOnIO(3, 1, CreateCallback(3, 1));
+  EXPECT_EQ(4, map->GetInternalCallbackCount());
+  EXPECT_EQ(2, map->GetInternalSize());
+
+  // A call identical to the very first call.
+  map->GetFrameIdOnIO(1, 2, CreateCallback(1, 2));
+  EXPECT_EQ(5, map->GetInternalCallbackCount());
+  EXPECT_EQ(2, map->GetInternalSize());
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(0, map->GetInternalCallbackCount());
+
+  EXPECT_EQ(3, map->GetInternalSize());
+  EXPECT_EQ(5U, results().size());
+
+  // Callbacks should be run in order.
+  EXPECT_EQ(ToTestFrameId(1, 2), results()[0]);
+  EXPECT_EQ(ToTestFrameId(1, 2), results()[1]);
+  EXPECT_EQ(ToTestFrameId(2, 1), results()[2]);
+  EXPECT_EQ(ToTestFrameId(3, 1), results()[3]);
+  EXPECT_EQ(ToTestFrameId(1, 2), results()[4]);
+
+  // Request the frame ID for input that was already looked up. Should complete
+  // synchronously.
+  map->GetFrameIdOnIO(1, 2, CreateCallback(1, 2));
+  EXPECT_EQ(0, map->GetInternalCallbackCount());
+  EXPECT_EQ(3, map->GetInternalSize());
+  EXPECT_EQ(6U, results().size());
+  EXPECT_EQ(ToTestFrameId(1, 2), results()[5]);
+
+  // Queue frame removal and look up frame ID. Frame ID should not be removed
+  // yet and GetFrameIdOnIO() should complete synchronously.
+  map->RemoveFrameId(1, 2);
+  map->GetFrameIdOnIO(1, 2, CreateCallback(1, 2));
+  EXPECT_EQ(0, map->GetInternalCallbackCount());
+  EXPECT_EQ(3, map->GetInternalSize());
+  EXPECT_EQ(7U, results().size());
+  EXPECT_EQ(ToTestFrameId(1, 2), results()[6]);
+
+  // Now fire the timer that triggers frame removal, and request the frame ID.
+  // Thread hopping should be necessary again.
+  map->TriggerInternalCleanup();
+  EXPECT_EQ(2, map->GetInternalSize());
+  map->GetFrameIdOnIO(1, 2, CreateCallback(1, 2));
+  EXPECT_EQ(1, map->GetInternalCallbackCount());
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(0, map->GetInternalCallbackCount());
+  EXPECT_EQ(3, map->GetInternalSize());
+  EXPECT_EQ(8U, results().size());
+  EXPECT_EQ(ToTestFrameId(1, 2), results()[7]);
+}
+
+}  // namespace extensions