Cleans up usage of ViewManagerServiceImpl::roots_

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

Index: mojo/services/public/cpp/view_manager/lib/node.cc
diff --git a/mojo/services/public/cpp/view_manager/lib/node.cc b/mojo/services/public/cpp/view_manager/lib/node.cc
index 292cf29e381ca53f29914b2495ccabbdf1598d71..84d214ec29faa4093679e28d431b8c6e88be84e3 100644
--- a/mojo/services/public/cpp/view_manager/lib/node.cc
+++ b/mojo/services/public/cpp/view_manager/lib/node.cc
@@ -216,8 +216,17 @@ void Node::Destroy() {
 
   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();
 }