Cleans up usage of ViewManagerServiceImpl::roots_

mojo/services/public/cpp/view_manager/lib/node.cc

 // Copyright 2014 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/services/public/cpp/view_manager/node.h"
 
 #include "mojo/services/public/cpp/view_manager/lib/node_private.h"
 #include "mojo/services/public/cpp/view_manager/lib/view_manager_client_impl.h"
 #include "mojo/services/public/cpp/view_manager/lib/view_private.h"
 #include "mojo/services/public/cpp/view_manager/node_observer.h"
@@ skipping 198 matching lines @@
   static_cast<ViewManagerClientImpl*>(view_manager)->AddNode(node);
   return node;
 }
 
 void Node::Destroy() {
   if (!OwnsNode(manager_, this))
     return;
 
   if (manager_)
     static_cast<ViewManagerClientImpl*>(manager_)->DestroyNode(id_);
-  while (!children_.empty())
-    children_.front()->Destroy();
+  while (!children_.empty()) {
+    Node* child = children_.front();
+    if (!OwnsNode(manager_, child)) {
+      children_.erase(children_.begin());
+      child->LocalDestroy();

[sky, 2014/07/26 16:21:39]

I think this is right, but now I'm worried that 215 needs to do the same.

[Ben Goodger (Google), 2014/07/26 16:49:10]

Yeah I would be inclined to think so too...

It should be noted in a comment here that this case basically only applies to
the window manager. No other connection has the ability to see child nodes it
doesn't own (right?).

It should be possible to test this by constructing a hierarchy with a non-owned
child & verifying that ViewManager::GetNodeById(that_id) returns NULL since
LocalDestroy should unregister it.

[sky, 2014/07/26 20:46:23]

Child nodes yes, but the root(s) of the connection are not owned either.
When I made 215 do a LocalDestroy too that broke
ViewManagerTest.DestroySecurity. In thinking about it a bit more I don't think
223 or 215 should delete. After all, unowned nodes aren't being deleted from the
server. They still exist (well, the window manager can delete things but this
code isn't allowing that). I think the erase is right with no delete. But
~ViewManagerClientImpl needs to explicitly delete else we leak these nodes.

[Ben Goodger (Google), 2014/07/26 22:56:15]

OK

+    } else {
+      child->Destroy();
+      DCHECK(std::find(children_.begin(), children_.end(), child) ==
+             children_.end());
+    }
+  }
   LocalDestroy();
 }
 
 void Node::SetBounds(const gfx::Rect& bounds) {
   if (!OwnsNode(manager_, this))
     return;
 
   if (manager_)
     static_cast<ViewManagerClientImpl*>(manager_)->SetBounds(id_, bounds);
   LocalSetBounds(bounds_, bounds);
@@ skipping 157 matching lines @@
 }
 
 void Node::LocalSetBounds(const gfx::Rect& old_bounds,
                           const gfx::Rect& new_bounds) {
   DCHECK(old_bounds == bounds_);
   ScopedSetBoundsNotifier notifier(this, old_bounds, new_bounds);
   bounds_ = new_bounds;
 }
 
 }  // namespace mojo