Add RoutingIDIssuer to ensure the correctness of ID to be routed.

content/browser/renderer_host/routing_id_issuer.cc

Index: content/browser/renderer_host/routing_id_issuer.cc
diff --git a/content/browser/renderer_host/routing_id_issuer.cc b/content/browser/renderer_host/routing_id_issuer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..435871249015de591ee28d13021944b0029f1d4b
--- /dev/null
+++ b/content/browser/renderer_host/routing_id_issuer.cc
@@ -0,0 +1,63 @@
+// Copyright (c) 2012 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 "content/browser/renderer_host/routing_id_issuer.h"
+
+namespace content {
+
+namespace {
+
+const int kManglerBit = 3;
+const int kIDBit = 31 - kManglerBit;
+const int kManglerWrap = 1 << kManglerBit;
+const int kManglerMask = kManglerWrap - 1;
+const int kIDWrap = 1 << kIDBit;
+const int kIDMask = kIDWrap - 1;
+
+int g_manglerSequence = 0;

[ncarter (slow), 2014/10/13 22:51:33]

Agree with nasko; this should probably be a StaticAtomicSequenceNumber

[Hajime Morrita, 2014/10/13 23:39:17]

Done.

+int g_disablingIDManglingCount = 0;
+
+int GetNextMangler() {
+  ++g_manglerSequence;
+  g_manglerSequence = g_manglerSequence % kManglerWrap;

[ncarter (slow), 2014/10/13 22:51:33]

You might want to consider skipping mangler ID's of 0.

One class of bug we're trying to defend against is random small numbers (or
routing ID's allocated by different kinds of process hosts) being used as id's
here.

[Hajime Morrita, 2014/10/13 23:39:17]

Makes sense. Changed to always return non-zero mangler.

+  return g_manglerSequence;
+}
+
+}  // namespace
+
+scoped_ptr<RoutingIDIssuer> RoutingIDIssuer::Create() {
+  if (0 < g_disablingIDManglingCount)
+    return make_scoped_ptr(new RoutingIDIssuer(0));
+
+  DCHECK(0 == g_disablingIDManglingCount);
+  return make_scoped_ptr(new RoutingIDIssuer(GetNextMangler()));
+}
+
+scoped_ptr<RoutingIDIssuer> RoutingIDIssuer::CreateWithMangler(int mangler) {
+  return make_scoped_ptr(new RoutingIDIssuer(mangler));
+}
+
+RoutingIDIssuer::RoutingIDIssuer(int mangler) : mangler_(mangler) {
+  DCHECK(0 <= mangler_ && mangler_ < kManglerWrap) << mangler_
+                                                   << ">=" << kManglerWrap;
+}
+
+int RoutingIDIssuer::IssueNext() {
+  return (mangler_ << kIDBit) | ((next_.GetNext() + 1) & kIDMask);

[ncarter (slow), 2014/10/13 22:51:33]

I had an idea for a sort of different algorithm here, but one which has some
nice properties and some other properties that are not quite as nice as what
you've implemented. It's probably overkill, but let me know what you think:

== 

The idea would be that rather than rotating through a series of
g_manglerSequence values for the top 3 bits, we could instead use hashing to
eliminate some percentage of the possible values.

One seed value would be known at compile time, e.g., a static string like
"RenderProcessHost" -- call this value the flavor_seed. The other could be an
instance-specific value, probably the process_id.

The body of IssueNext would look like:
  while(1) {
    int route_id = next_.GetNext() + 1;
    int flavorhashinput[2] = {route_id, hash_of_flavor_seed_};
    if (base::Hash(flavorhashinput) % 2 == 0)
      continue;
    int instancehashinput[2] = {route_id, process_id_};
    if (base::Hash(instancehashinput) % 2 == 0)
      continue;
    return route_id;
  }

==

The first benefit: when we have a route_id, we can ask (statically), is this
likely a "RenderProcessHost" route id by computing the first hash. And if we
have a route_id and the process_id, then we can compute (statically) the second
value. I think the static nature of this would make for nice asserts, since
sometimes we wind up with route_id's far away from any pointer to a
RoutingIDIssuer.

Note that the use of process_id_ is optional -- that's appropriate if we're
worried about bugs where route_id's wind up paired with the wrong process_id; in
the case of the current bug, it seems that it's possible. If we don't use
process_id, then I think this scheme becomes even more test-friendly, since the
routing_id sequences would become very predictable.

We could conceivably go further, and pick (by brute force) specific flavor_seed_
values that have a low rate of overlap when hashed with small route_id values.

[Hajime Morrita, 2014/10/13 23:39:17]

Thanks for the suggestion! I'd rather avoid using process id as we don't have
one when
RenderProcessHostImpl is created and set the value afterword can possibly
introduce
strange race conditions.

Having the flavor seed sounds promising. Probably we could use something like
FROM_HERE?

Let me land this land this naive version for now.
I don't have good idea how to collect these detailed metrics from wild.
Probably we need some preparation to make it available.
It'd better to add such kind of infrastructure as well if we add more
sophisticated metrics.

+}
+
+bool RoutingIDIssuer::IsProbablyValid(int issued_id) const {
+  return mangler_ == ((issued_id >> kIDBit) & kManglerMask);

[ncarter (slow), 2014/10/13 22:51:33]

can't issued_id be a negative value? If so then this right shift operation isn't
defined?

[Hajime Morrita, 2014/10/13 23:39:17]

Oops. Fixing.

+}
+
+void RoutingIDIssuer::StartDisablingIDMangling() {
+  g_disablingIDManglingCount++;
+}
+
+void RoutingIDIssuer::StopDisablingIDMangling() {
+  g_disablingIDManglingCount--;
+  DCHECK(0 <= g_disablingIDManglingCount);
+}
+
+}  // namespace content