Assert sequence validity on non-thread-safe RefCount manipulations (2)

mojo/public/cpp/bindings/lib/multiplex_router.cc

Index: mojo/public/cpp/bindings/lib/multiplex_router.cc
diff --git a/mojo/public/cpp/bindings/lib/multiplex_router.cc b/mojo/public/cpp/bindings/lib/multiplex_router.cc
index 2da459ae8f8ca5ee8704164bbeefaad3a93a2c88..022b6b2e3707988abb77f182ffb7c0ffaf1dcabd 100644
--- a/mojo/public/cpp/bindings/lib/multiplex_router.cc
+++ b/mojo/public/cpp/bindings/lib/multiplex_router.cc
@@ -440,6 +440,7 @@ InterfaceId MultiplexRouter::AssociateInterface(
     } while (base::ContainsKey(endpoints_, id));
 
     InterfaceEndpoint* endpoint = new InterfaceEndpoint(this, id);
+    endpoint->DisableSequenceConsistencyAssertions();
     endpoints_[id] = endpoint;
     if (encountered_error_)
       UpdateEndpointStateMayRemove(endpoint, PEER_ENDPOINT_CLOSED);
@@ -960,6 +961,7 @@ MultiplexRouter::InterfaceEndpoint* MultiplexRouter::FindOrInsertEndpoint(
   InterfaceEndpoint* endpoint = FindEndpoint(id);
   if (!endpoint) {
     endpoint = new InterfaceEndpoint(this, id);
+    endpoint->DisableSequenceConsistencyAssertions();

[gab, 2017/03/24 15:46:45]

That seems like a very large hammer, can we use the scoped object where relevant
instead? Otherwise how do we know all callers are truly doing the right thing
with the received pointer? I'd much rather not have
DisableSequenceConsistencyAssertions() if we can help it (and otherwise it
should have a large comment saying what it's there for and not to use it in new
use cases).

Also, outside the scope of this CL but why is this stored in a raw ptr and
returned as a raw ptr if it's RefCounted..?

[yzshen1, 2017/03/24 16:58:09]

I am the one who suggested that the check should be disabled at the object
level, instead of using the scoped-allow object. :)

That is because:
- The scoped-allow object disables the check for *all* ref-counted objects in
its scope.
- When we want to disable the check for an object, it is a promise to behave
correctly for all usage of that object across its lifetime. That is not a
per-callsite choice. For example, all callsites should use the same lock to
guard addref/release for that object.

Considering these reasons, I don't think the scoped-allow approach is at the
right level.
(I think this is a question to me.)
It saves one unnecessary add-ref for the return value of FindEndpoint(). I agree
that it is not a big difference though.

     endpoints_[id] = endpoint;
     if (inserted)
       *inserted = true;