|RenderFrameHost*| shouldn't be used as a key by BlinkTestController.

Addresses can be reused and therefore pointers shouldn't be used as
container keys (unless the code maintaining the container can guarantee
that the old entry will be destroyed before the pointer gets freed).
In particular, using |RenderFrameHost*| as a key of BlinkTestController's
layout_test_control_map_ was incorrect and led to timeouts described
in more details in https://crbug.com/834185#c13 - #c14.

This CL replaces
    std::map<RenderFrameHost*, ...>
with
    std::map<std::pair<int, int>, ...>
and uses (process id, frame routing id) as a key.

After this CL around ~200 flaky timeouts from
FlagExpectations/site-per-process seem to avoided.  Before this CL, I
could reliably repro a timeout in
- http/tests/fetch/window/thorough/redirect-nocors-base-https-other-https.html
- http/tests/fetch/serviceworker-proxied/thorough/scheme-data-base-https-other-https.html
when running it 20 iterations.  After this CL, I cannot repro any
timeouts when running all ~200 tests 20 times:

    $ DISPLAY=:20 third_party/WebKit/Tools/Scripts/run-webkit-tests \
        -t rel --no-retry --additional-driver-flag=--site-per-process \
        --iterations=20 \
        --exit-after-n-failures=1 --exit-after-n-crashes-or-timeouts=1 \
        --additional-driver-flag=--no-sandbox \
        --additional-driver-flag=--isolate-origins=http://www.web-platform.test:8001/,http://www1.web-platform.test:8001/,http://www2.web-platform.test:8001/,http://xn--n8j6ds53lwwkrqhv28a.web-platform.test:8001/,http://xn--lve-6lad.web-platform.test:8001/,http://www.web-platform.test:8081/,http://www1.web-platform.test:8081/,http://www2.web-platform.test:8081/,http://xn--n8j6ds53lwwkrqhv28a.web-platform.test:8081/,http://xn--lve-6lad.web-platform.test:8081/,https://www.web-platform.test:8444/,https://www1.web-platform.test:8444/,https://www2.web-platform.test:8444/,https://xn--n8j6ds53lwwkrqhv28a.web-platform.test:8444/,https://xn--lve-6lad.web-platform.test:8444/ \
        --test-list=$HOME/scratch/test-list
    ...
    Found 206 tests; running 206 (20 times each: --repeat-each=1 --iterations=20), skipping 0.
    ...
    All 4120 tests ran as expected.

Bug: 801992, 834185, 711468, 859988
Change-Id: I9f2c1abf87027fcb3824c54e597e561ac72babb9
Reviewed-on: https://chromium-review.googlesource.com/1182316
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584841}

content/shell/browser/layout_test/blink_test_controller.cc

@@ -1261,19 +1261,23 @@ void BlinkTestController::OnBlockThirdPartyCookies(bool block) {
 
 mojom::LayoutTestControl* BlinkTestController::GetLayoutTestControlPtr(
     RenderFrameHost* frame) {
-  if (layout_test_control_map_.find(frame) == layout_test_control_map_.end()) {
-    frame->GetRemoteAssociatedInterfaces()->GetInterface(
-        &layout_test_control_map_[frame]);
-    layout_test_control_map_[frame].set_connection_error_handler(
+  auto key = std::make_pair<int, int>(frame->GetProcess()->GetID(),
+                                      frame->GetRoutingID());
+  if (layout_test_control_map_.find(key) == layout_test_control_map_.end()) {
+    mojom::LayoutTestControlAssociatedPtr& new_ptr =
+        layout_test_control_map_[key];
+    frame->GetRemoteAssociatedInterfaces()->GetInterface(&new_ptr);
+    new_ptr.set_connection_error_handler(
         base::BindOnce(&BlinkTestController::HandleLayoutTestControlError,
-                       weak_factory_.GetWeakPtr(), frame));
+                       weak_factory_.GetWeakPtr(), key));
   }
-  DCHECK(layout_test_control_map_[frame].get());
-  return layout_test_control_map_[frame].get();
+  DCHECK(layout_test_control_map_[key].get());
+  return layout_test_control_map_[key].get();
 }
 
-void BlinkTestController::HandleLayoutTestControlError(RenderFrameHost* frame) {
-  layout_test_control_map_.erase(frame);
+void BlinkTestController::HandleLayoutTestControlError(
+    const std::pair<int, int>& key) {
+  layout_test_control_map_.erase(key);
 }
 
 BlinkTestController::Node::Node() = default;

content/shell/browser/layout_test/blink_test_controller.h

@@ -10,6 +10,7 @@
 #include <ostream>
 #include <set>
 #include <string>
+#include <utility>
 
 #include "base/cancelable_callback.h"
 #include "base/files/file_path.h"
@@ -241,7 +242,7 @@ class BlinkTestController : public WebContentsObserver,
                                          const std::string& argument);
   void OnBlockThirdPartyCookies(bool block);
   mojom::LayoutTestControl* GetLayoutTestControlPtr(RenderFrameHost* frame);
-  void HandleLayoutTestControlError(RenderFrameHost* frame);
+  void HandleLayoutTestControlError(const std::pair<int, int>& key);
 
   void OnCleanupFinished();
   void OnCaptureDumpCompleted(mojom::LayoutTestDumpPtr dump);
@@ -327,7 +328,11 @@ class BlinkTestController : public WebContentsObserver,
   bool waiting_for_main_frame_dump_ = false;
 
   // Map from one frame to one mojo pipe.
-  std::map<RenderFrameHost*, mojom::LayoutTestControlAssociatedPtr>
+  //
+  // The key is a pair of (process id, frame routing id).
+  // TODO(lukasza): Use content::GlobalFrameRoutingID instead of std::pair<...>
+  // once it is exposed via content/public/browser API.
+  std::map<std::pair<int, int>, mojom::LayoutTestControlAssociatedPtr>
       layout_test_control_map_;
 #if defined(OS_ANDROID)
   // Because of the nested message pump implementation, Android needs to allow