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

mojo/public/cpp/bindings/lib/multiplex_router.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 "mojo/public/cpp/bindings/lib/multiplex_router.h"
 
 #include <stdint.h>
 
 #include <utility>
 
@@ skipping 422 matching lines @@
     MayAutoLock locker(&lock_);
     do {
       if (next_interface_id_value_ >= kInterfaceIdNamespaceMask)
         next_interface_id_value_ = 1;
       id = next_interface_id_value_++;
       if (set_interface_id_namespace_bit_)
         id |= kInterfaceIdNamespaceMask;
     } while (base::ContainsKey(endpoints_, id));
 
     InterfaceEndpoint* endpoint = new InterfaceEndpoint(this, id);
+    endpoint->DisableSequenceConsistencyAssertions();
     endpoints_[id] = endpoint;
     if (encountered_error_)
       UpdateEndpointStateMayRemove(endpoint, PEER_ENDPOINT_CLOSED);
     endpoint->set_handle_created();
   }
 
   if (!NotifyAssociation(&handle_to_send, id)) {
     // The peer handle of |handle_to_send|, which is supposed to join this
     // associated group, has been closed.
     {
@@ skipping 500 matching lines @@
     InterfaceId id,
     bool* inserted) {
   AssertLockAcquired();
   // Either |inserted| is nullptr or it points to a boolean initialized as
   // false.
   DCHECK(!inserted || !*inserted);
 
   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;
   }
 
   return endpoint;
 }
 
 MultiplexRouter::InterfaceEndpoint* MultiplexRouter::FindEndpoint(
     InterfaceId id) {
   AssertLockAcquired();
   auto iter = endpoints_.find(id);
   return iter != endpoints_.end() ? iter->second.get() : nullptr;
 }
 
 void MultiplexRouter::AssertLockAcquired() {
 #if DCHECK_IS_ON()
   if (lock_)
     lock_->AssertAcquired();
 #endif
 }
 
 }  // namespace internal
 }  // namespace mojo