Adds a monotonically increasing id to the frame tree

This is needed so that clients can identify when they have applied a
particular change. Also, I'm making it so the originator of a
structure change gets the notification on the client interface of the
change.

Here's an example of what this fixes:
An HTMLViewer with with two local roots, A and B.
B1 is added as a child of B and then removed.
OnFrameAdded() is seen on A and HTMLViewer attempts to add B1
back. But B1 was already removed, so that it shouldn't added it back.

The monotonically increasing id allows html viewer to identify this by
the following. When a frame is removed HTMLFrameTreeManager now adds
the id to a set. The id is removed when OnFrameRemoved is seen (this
necessitates calling OnFrameRemoved on the originator). In
OnFrameAdded if the id is in the removed set the add is ignored.

BUG=479172,490221
TEST=none
R=fsamuel@chromium.org

Review URL: https://codereview.chromium.org/1280393002

Cr-Commit-Position: refs/heads/master@{#342735}

7 files changed, 109 insertions, 23 deletions

diff --git a/components/html_viewer/ax_provider_apptest.cc b/components/html_viewer/ax_provider_apptest.cc
index ecf9eb9..7add66b 100644
--- a/components/html_viewer/ax_provider_apptest.cc
+++ b/components/html_viewer/ax_provider_apptest.cc
@@ -104,7 +104,8 @@ TEST_F(AXProviderTest, HelloWorld) {
 
     mandoline::FrameTreeClientPtr frame_tree_client;
     connection->ConnectToService(&frame_tree_client);
-    frame_tree_client->OnConnect(frame_tree_server_ptr.Pass(), array.Pass());
+    frame_tree_client->OnConnect(frame_tree_server_ptr.Pass(), 1u,
+                                 array.Pass());
   }
 
   // Connect to the AxProvider of the HTML document and get the AxTree.
diff --git a/components/html_viewer/document_resource_waiter.cc b/components/html_viewer/document_resource_waiter.cc
index 6ca5606..15029ce 100644
--- a/components/html_viewer/document_resource_waiter.cc
+++ b/components/html_viewer/document_resource_waiter.cc
@@ -18,8 +18,8 @@ DocumentResourceWaiter::DocumentResourceWaiter(GlobalState* global_state,
       document_(document),
       response_(response.Pass()),
       root_(nullptr),
-      frame_tree_client_binding_(this) {
-}
+      change_id_(0u),
+      frame_tree_client_binding_(this) {}
 
 DocumentResourceWaiter::~DocumentResourceWaiter() {
 }
@@ -28,11 +28,13 @@ void DocumentResourceWaiter::Release(
     mojo::InterfaceRequest<mandoline::FrameTreeClient>*
         frame_tree_client_request,
     mandoline::FrameTreeServerPtr* frame_tree_server,
-    mojo::Array<mandoline::FrameDataPtr>* frame_data) {
+    mojo::Array<mandoline::FrameDataPtr>* frame_data,
+    uint32_t* change_id) {
   DCHECK(IsReady());
   *frame_tree_client_request = frame_tree_client_request_.Pass();
   *frame_tree_server = server_.Pass();
   *frame_data = frame_data_.Pass();
+  *change_id = change_id_;
 }
 
 mojo::URLResponsePtr DocumentResourceWaiter::ReleaseURLResponse() {
@@ -55,8 +57,10 @@ void DocumentResourceWaiter::Bind(
 
 void DocumentResourceWaiter::OnConnect(
     mandoline::FrameTreeServerPtr server,
+    uint32 change_id,
     mojo::Array<mandoline::FrameDataPtr> frame_data) {
   DCHECK(frame_data_.is_null());
+  change_id_ = change_id;
   server_ = server.Pass();
   frame_data_ = frame_data.Pass();
   CHECK(frame_data_.size() > 0u);
@@ -65,12 +69,14 @@ void DocumentResourceWaiter::OnConnect(
     document_->LoadIfNecessary();
 }
 
-void DocumentResourceWaiter::OnFrameAdded(mandoline::FrameDataPtr frame_data) {
+void DocumentResourceWaiter::OnFrameAdded(uint32 change_id,
+                                          mandoline::FrameDataPtr frame_data) {
   // It is assumed we receive OnConnect() (which unbinds) before anything else.
   NOTREACHED();
 }
 
-void DocumentResourceWaiter::OnFrameRemoved(uint32_t frame_id) {
+void DocumentResourceWaiter::OnFrameRemoved(uint32 change_id,
+                                            uint32_t frame_id) {
   // It is assumed we receive OnConnect() (which unbinds) before anything else.
   NOTREACHED();
 }
diff --git a/components/html_viewer/document_resource_waiter.h b/components/html_viewer/document_resource_waiter.h
index d979abe..fd624cc 100644
--- a/components/html_viewer/document_resource_waiter.h
+++ b/components/html_viewer/document_resource_waiter.h
@@ -33,7 +33,8 @@ class DocumentResourceWaiter : public mandoline::FrameTreeClient {
   void Release(mojo::InterfaceRequest<mandoline::FrameTreeClient>*
                    frame_tree_client_request,
                mandoline::FrameTreeServerPtr* frame_tree_server,
-               mojo::Array<mandoline::FrameDataPtr>* frame_data);
+               mojo::Array<mandoline::FrameDataPtr>* frame_data,
+               uint32_t* change_id);
 
   mojo::URLResponsePtr ReleaseURLResponse();
 
@@ -48,9 +49,11 @@ class DocumentResourceWaiter : public mandoline::FrameTreeClient {
  private:
   // mandoline::FrameTreeClient:
   void OnConnect(mandoline::FrameTreeServerPtr server,
+                 uint32 change_id,
                  mojo::Array<mandoline::FrameDataPtr> frame_data) override;
-  void OnFrameAdded(mandoline::FrameDataPtr frame_data) override;
-  void OnFrameRemoved(uint32_t frame_id) override;
+  void OnFrameAdded(uint32 change_id,
+                    mandoline::FrameDataPtr frame_data) override;
+  void OnFrameRemoved(uint32 change_id, uint32_t frame_id) override;
   void OnFrameClientPropertyChanged(uint32_t frame_id,
                                     const mojo::String& name,
                                     mojo::Array<uint8_t> new_value) override;
@@ -61,6 +64,7 @@ class DocumentResourceWaiter : public mandoline::FrameTreeClient {
   mojo::View* root_;
   mandoline::FrameTreeServerPtr server_;
   mojo::Array<mandoline::FrameDataPtr> frame_data_;
+  uint32_t change_id_;
 
   // Once we get OnConnect() we unbind |frame_tree_client_binding_| and put it
   // here.
diff --git a/components/html_viewer/html_frame.cc b/components/html_viewer/html_frame.cc
index 9a166a9..ceb7a4a 100644
--- a/components/html_viewer/html_frame.cc
+++ b/components/html_viewer/html_frame.cc
@@ -551,18 +551,20 @@ void HTMLFrame::OnViewFocusChanged(mojo::View* gained_focus,
 }
 
 void HTMLFrame::OnConnect(mandoline::FrameTreeServerPtr server,
+                          uint32_t change_id,
                           mojo::Array<mandoline::FrameDataPtr> frame_data) {
   // OnConnect() is only sent once, and has been received (by
   // DocumentResourceWaiter) by the time we get here.
   NOTREACHED();
 }
 
-void HTMLFrame::OnFrameAdded(mandoline::FrameDataPtr frame_data) {
-  frame_tree_manager_->ProcessOnFrameAdded(this, frame_data.Pass());
+void HTMLFrame::OnFrameAdded(uint32_t change_id,
+                             mandoline::FrameDataPtr frame_data) {
+  frame_tree_manager_->ProcessOnFrameAdded(this, change_id, frame_data.Pass());
 }
 
-void HTMLFrame::OnFrameRemoved(uint32_t frame_id) {
-  frame_tree_manager_->ProcessOnFrameRemoved(this, frame_id);
+void HTMLFrame::OnFrameRemoved(uint32_t change_id, uint32_t frame_id) {
+  frame_tree_manager_->ProcessOnFrameRemoved(this, change_id, frame_id);
 }
 
 void HTMLFrame::OnFrameClientPropertyChanged(uint32_t frame_id,
diff --git a/components/html_viewer/html_frame.h b/components/html_viewer/html_frame.h
index 902ff5c..ad2a806 100644
--- a/components/html_viewer/html_frame.h
+++ b/components/html_viewer/html_frame.h
@@ -189,9 +189,11 @@ class HTMLFrame : public blink::WebFrameClient,
 
   // mandoline::FrameTreeClient:
   void OnConnect(mandoline::FrameTreeServerPtr server,
+                 uint32_t change_id,
                  mojo::Array<mandoline::FrameDataPtr> frame_data) override;
-  void OnFrameAdded(mandoline::FrameDataPtr frame_data) override;
-  void OnFrameRemoved(uint32_t frame_id) override;
+  void OnFrameAdded(uint32_t change_id,
+                    mandoline::FrameDataPtr frame_data) override;
+  void OnFrameRemoved(uint32_t change_id, uint32_t frame_id) override;
   void OnFrameClientPropertyChanged(uint32_t frame_id,
                                     const mojo::String& name,
                                     mojo::Array<uint8_t> new_value) override;
diff --git a/components/html_viewer/html_frame_tree_manager.cc b/components/html_viewer/html_frame_tree_manager.cc
index 7eae7ab..c529bda 100644
--- a/components/html_viewer/html_frame_tree_manager.cc
+++ b/components/html_viewer/html_frame_tree_manager.cc
@@ -59,8 +59,9 @@ HTMLFrame* HTMLFrameTreeManager::CreateFrameAndAttachToTree(
   mojo::InterfaceRequest<mandoline::FrameTreeClient> frame_tree_client_request;
   mandoline::FrameTreeServerPtr frame_tree_server;
   mojo::Array<mandoline::FrameDataPtr> frame_data;
+  uint32_t change_id;
   resource_waiter->Release(&frame_tree_client_request, &frame_tree_server,
-                           &frame_data);
+                           &frame_data, &change_id);
   resource_waiter.reset();
 
   HTMLFrameTreeManager* frame_tree = nullptr;
@@ -73,7 +74,7 @@ HTMLFrame* HTMLFrameTreeManager::CreateFrameAndAttachToTree(
 
   if (!frame_tree) {
     frame_tree = new HTMLFrameTreeManager(global_state);
-    frame_tree->Init(delegate, view, frame_data);
+    frame_tree->Init(delegate, view, frame_data, change_id);
     if (frame_data[0]->frame_id == view->id())
       (*instances_)[frame_data[0]->frame_id] = frame_tree;
   } else {
@@ -117,12 +118,20 @@ void HTMLFrameTreeManager::OnFrameDestroyed(HTMLFrame* frame) {
   if (frame == local_root_)
     local_root_ = nullptr;
 
+  if (!in_process_on_frame_removed_)
+    pending_remove_ids_.insert(frame->id());
+
   if (!local_root_ || !local_root_->HasLocalDescendant())
     delete this;
 }
 
 HTMLFrameTreeManager::HTMLFrameTreeManager(GlobalState* global_state)
-    : global_state_(global_state), root_(nullptr), local_root_(nullptr) {}
+    : global_state_(global_state),
+      root_(nullptr),
+      local_root_(nullptr),
+      change_id_(0u),
+      in_process_on_frame_removed_(false),
+      weak_factory_(this) {}
 
 HTMLFrameTreeManager::~HTMLFrameTreeManager() {
   DCHECK(!root_ || !local_root_);
@@ -132,7 +141,9 @@ HTMLFrameTreeManager::~HTMLFrameTreeManager() {
 void HTMLFrameTreeManager::Init(
     HTMLFrameDelegate* delegate,
     mojo::View* local_view,
-    const mojo::Array<mandoline::FrameDataPtr>& frame_data) {
+    const mojo::Array<mandoline::FrameDataPtr>& frame_data,
+    uint32_t change_id) {
+  change_id_ = change_id;
   root_ = BuildFrameTree(delegate, frame_data, local_view->id(), local_view);
   local_root_ = root_->FindFrame(local_view->id());
   CHECK(local_root_);
@@ -181,10 +192,29 @@ void HTMLFrameTreeManager::RemoveFromInstances() {
   }
 }
 
+bool HTMLFrameTreeManager::PrepareForStructureChange(HTMLFrame* source,
+                                                     uint32_t change_id) {
+  // The change ids may differ if multiple HTMLDocuments are attached to the
+  // same tree (which means we have multiple FrameTreeClients for the same
+  // tree).
+  if (change_id != (change_id_ + 1))
+    return false;
+
+  // We only process changes for the topmost local root.
+  if (source != local_root_)
+    return false;
+
+  // Update the id as the change is going to be applied (or we can assume it
+  // will be applied if we get here).
+  change_id_ = change_id;
+  return true;
+}
+
 void HTMLFrameTreeManager::ProcessOnFrameAdded(
     HTMLFrame* source,
+    uint32_t change_id,
     mandoline::FrameDataPtr frame_data) {
-  if (source != local_root_)
+  if (!PrepareForStructureChange(source, change_id))
     return;
 
   HTMLFrame* parent = root_->FindFrame(frame_data->parent_id);
@@ -199,6 +229,12 @@ void HTMLFrameTreeManager::ProcessOnFrameAdded(
     return;
   }
 
+  // Because notification is async it's entirely possible for us to create a
+  // new frame, and remove it before we get the add from the server. This check
+  // ensures we don't add back a frame we explicitly removed.
+  if (pending_remove_ids_.count(frame_data->frame_id))
+    return;
+
   HTMLFrame::CreateParams params(this, parent, frame_data->frame_id);
   // |parent| takes ownership of |frame|.
   HTMLFrame* frame = new HTMLFrame(params);
@@ -206,10 +242,13 @@ void HTMLFrameTreeManager::ProcessOnFrameAdded(
 }
 
 void HTMLFrameTreeManager::ProcessOnFrameRemoved(HTMLFrame* source,
+                                                 uint32_t change_id,
                                                  uint32_t frame_id) {
-  if (source != local_root_)
+  if (!PrepareForStructureChange(source, change_id))
     return;
 
+  pending_remove_ids_.erase(frame_id);
+
   HTMLFrame* frame = root_->FindFrame(frame_id);
   if (!frame) {
     DVLOG(1) << "OnFrameRemoved with unknown frame " << frame_id;
@@ -227,7 +266,14 @@ void HTMLFrameTreeManager::ProcessOnFrameRemoved(HTMLFrame* source,
   if (frame->IsLocal())
     return;
 
+  DCHECK(!in_process_on_frame_removed_);
+  in_process_on_frame_removed_ = true;
+  base::WeakPtr<HTMLFrameTreeManager> ref(weak_factory_.GetWeakPtr());
   frame->Close();
+  if (!ref)
+    return;  // We were deleted.
+
+  in_process_on_frame_removed_ = false;
 }
 
 void HTMLFrameTreeManager::ProcessOnFrameClientPropertyChanged(
diff --git a/components/html_viewer/html_frame_tree_manager.h b/components/html_viewer/html_frame_tree_manager.h
index 25e26f0..ea807b8 100644
--- a/components/html_viewer/html_frame_tree_manager.h
+++ b/components/html_viewer/html_frame_tree_manager.h
@@ -6,9 +6,11 @@
 #define COMPONENTS_HTML_VIEWER_HTML_FRAME_TREE_MANAGER_H_
 
 #include <map>
+#include <set>
 
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "mandoline/tab/public/interfaces/frame_tree.mojom.h"
 
 namespace blink {
@@ -50,13 +52,15 @@ class HTMLFrameTreeManager {
  private:
   friend class HTMLFrame;
   using TreeMap = std::map<uint32_t, HTMLFrameTreeManager*>;
+  using ChangeIDSet = std::set<uint32_t>;
 
   explicit HTMLFrameTreeManager(GlobalState* global_state);
   ~HTMLFrameTreeManager();
 
   void Init(HTMLFrameDelegate* delegate,
             mojo::View* local_view,
-            const mojo::Array<mandoline::FrameDataPtr>& frame_data);
+            const mojo::Array<mandoline::FrameDataPtr>& frame_data,
+            uint32_t change_id);
 
   // Creates a Frame per FrameData element in |frame_data|. Returns the root.
   HTMLFrame* BuildFrameTree(
@@ -71,13 +75,21 @@ class HTMLFrameTreeManager {
   // Invoked when a Frame is destroyed.
   void OnFrameDestroyed(HTMLFrame* frame);
 
+  // Call before applying a structure change from the server. |source| is the
+  // the source of the change, and |change_id| the id from the server. Returns
+  // true if the change should be applied, false otherwise.
+  bool PrepareForStructureChange(HTMLFrame* source, uint32_t change_id);
+
   // Each HTMLFrame delegates FrameTreeClient methods to the
   // HTMLFrameTreeManager the frame is in. HTMLFrameTreeManager only responds
   // to changes from the |local_root_| (this is because each FrameTreeClient
   // sees the same change, and a change only need be processed once).
   void ProcessOnFrameAdded(HTMLFrame* source,
+                           uint32_t change_id,
                            mandoline::FrameDataPtr frame_data);
-  void ProcessOnFrameRemoved(HTMLFrame* source, uint32_t frame_id);
+  void ProcessOnFrameRemoved(HTMLFrame* source,
+                             uint32_t change_id,
+                             uint32_t frame_id);
   void ProcessOnFrameClientPropertyChanged(HTMLFrame* source,
                                            uint32_t frame_id,
                                            const mojo::String& name,
@@ -93,6 +105,19 @@ class HTMLFrameTreeManager {
   // local.
   HTMLFrame* local_root_;
 
+  uint32_t change_id_;
+
+  // When a frame is locally removed the id is added here. When the server
+  // notifies us we remove from this set. This is done to ensure an add/remove
+  // followed by notification from the server doesn't attempt to add the frame
+  // back that was locally removed.
+  ChangeIDSet pending_remove_ids_;
+
+  // If true, we're in ProcessOnFrameRemoved().
+  bool in_process_on_frame_removed_;
+
+  base::WeakPtrFactory<HTMLFrameTreeManager> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(HTMLFrameTreeManager);
 };