Avoid weird nested runloops while verifying model in sync tests

The use of WaitForGetFavicon() is unnecessary and misleading because
it introduces a nested runloop where many things can happen, including
model changes that call
BookmarksMatchVerifierChecker::IsExitConditionSatisfied() and hence
make it reentrant.

It is unnecessary because, starting with
55b3fe20, a loading favicon is
guaranteed to ultimately notify a model change, which will trigger
MultiClientStatusChangeChecker::OnStateChanged() and hence
IsExitConditionSatisfied().

Bug: 783774
Change-Id: I02c7623e3bb173b25b9c715b7bdf9c646300e3ee
Reviewed-on: https://chromium-review.googlesource.com/785091
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Pavel Yatsuk <pavely@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522089}

chrome/browser/sync/test/integration/bookmarks_helper.cc

@@ -80,21 +80,12 @@ class SignalEventTask : public history::HistoryDBTask {
 class FaviconChangeObserver : public bookmarks::BookmarkModelObserver {
  public:
   FaviconChangeObserver(BookmarkModel* model, const BookmarkNode* node)
-      : model_(model),
-        node_(node),
-        wait_for_load_(false) {
+      : model_(model), node_(node) {
     model->AddObserver(this);
   }
   ~FaviconChangeObserver() override { model_->RemoveObserver(this); }
-  void WaitForGetFavicon() {
-    DCHECK(!run_loop_.running());
-    wait_for_load_ = true;
-    content::RunThisRunLoop(&run_loop_);
-    ASSERT_TRUE(node_->is_favicon_loaded());
-  }
   void WaitForSetFavicon() {
     DCHECK(!run_loop_.running());
-    wait_for_load_ = false;
     content::RunThisRunLoop(&run_loop_);
   }
 
@@ -127,16 +118,13 @@ class FaviconChangeObserver : public bookmarks::BookmarkModelObserver {
                                      const BookmarkNode* node) override {}
   void BookmarkNodeFaviconChanged(BookmarkModel* model,
                                   const BookmarkNode* node) override {
-    if (model == model_ && node == node_ && run_loop_.running()) {
-      if (!wait_for_load_ || (wait_for_load_ && node->is_favicon_loaded()))
-        run_loop_.Quit();
-    }
+    if (model == model_ && node == node_)
+      run_loop_.Quit();
   }
 
  private:
   BookmarkModel* model_;
   const BookmarkNode* node_;
-  bool wait_for_load_;
   base::RunLoop run_loop_;
   DISALLOW_COPY_AND_ASSIGN(FaviconChangeObserver);
 };
@@ -221,9 +209,10 @@ struct FaviconData {
   GURL icon_url;
 };
 
-// Gets the favicon and icon URL associated with |node| in |model|.
-FaviconData GetFaviconData(BookmarkModel* model,
-                           const BookmarkNode* node) {
+// Gets the favicon and icon URL associated with |node| in |model|. Returns
+// nullopt if the favicon is still loading.
+base::Optional<FaviconData> GetFaviconData(BookmarkModel* model,
+                                           const BookmarkNode* node) {
   // If a favicon wasn't explicitly set for a particular URL, simply return its
   // blank favicon.
   if (!urls_with_favicons_ ||
@@ -233,10 +222,12 @@ FaviconData GetFaviconData(BookmarkModel* model,
   // If a favicon was explicitly set, we may need to wait for it to be loaded
   // via BookmarkModel::GetFavicon(), which is an asynchronous operation.
   if (!node->is_favicon_loaded()) {
-    FaviconChangeObserver observer(model, node);
     model->GetFavicon(node);
-    observer.WaitForGetFavicon();
+    // Favicon still loading, no data available just yet.
+    return base::nullopt;
   }
+
+  // Favicon loaded: return actual image.
   return FaviconData(model->GetFavicon(node),
                      node->icon_url() ? *node->icon_url() : GURL());
 }
@@ -266,9 +257,7 @@ void SetFaviconImpl(Profile* profile,
 
     // Wait for the favicon for |node| to be invalidated.
     observer.WaitForSetFavicon();
-    // Wait for the BookmarkModel to fetch the updated favicon and for the new
-    // favicon to be sent to BookmarkChangeProcessor.
-    GetFaviconData(model, node);
+    model->GetFavicon(node);
 }
 
 // Expires the favicon for |profile| and |node|. |profile| may be
@@ -315,9 +304,7 @@ void DeleteFaviconMappingsImpl(Profile* profile,
 
   // Wait for the favicon for |node| to be invalidated.
   observer.WaitForSetFavicon();
-  // Wait for the BookmarkModel to fetch the updated favicon and for the new
-  // favicon to be sent to BookmarkChangeProcessor.
-  GetFaviconData(model, node);
+  model->GetFavicon(node);
 }
 
 // Wait for all currently scheduled tasks on the history thread for all
@@ -360,14 +347,19 @@ bool FaviconsMatch(BookmarkModel* model_a,
                    BookmarkModel* model_b,
                    const BookmarkNode* node_a,
                    const BookmarkNode* node_b) {
-  FaviconData favicon_data_a = GetFaviconData(model_a, node_a);
-  FaviconData favicon_data_b = GetFaviconData(model_b, node_b);
+  base::Optional<FaviconData> favicon_data_a = GetFaviconData(model_a, node_a);
+  base::Optional<FaviconData> favicon_data_b = GetFaviconData(model_b, node_b);
+
+  // If either of the two favicons is still loading, let's return false now
+  // because observers will get notified when the load completes.
+  if (!favicon_data_a.has_value() || !favicon_data_b.has_value())
+    return false;
 
-  if (favicon_data_a.icon_url != favicon_data_b.icon_url)
+  if (favicon_data_a->icon_url != favicon_data_b->icon_url)
     return false;
 
-  gfx::Image image_a = favicon_data_a.image;
-  gfx::Image image_b = favicon_data_b.image;
+  gfx::Image image_a = favicon_data_a->image;
+  gfx::Image image_b = favicon_data_b->image;
 
   if (image_a.IsEmpty() && image_b.IsEmpty())
     return true;  // Two empty images are equivalent.

components/bookmarks/browser/bookmark_model.h

@@ -147,8 +147,11 @@ class BookmarkModel : public BookmarkUndoProvider,
             const BookmarkNode* new_parent,
             int index);
 
-  // Returns the favicon for |node|. If the favicon has not yet been
-  // loaded it is loaded and the observer of the model notified when done.
+  // Returns the favicon for |node|. If the favicon has not yet been loaded,
+  // a load will be triggered and the observer of the model notified when done.
+  // This also means that, on return, the node's state is guaranteed to be
+  // either LOADED_FAVICON (if it was already loaded prior to the call) or
+  // LOADING_FAVICON (with the exception of folders, where the call is a no-op).
   const gfx::Image& GetFavicon(const BookmarkNode* node);
 
   // Returns the type of the favicon for |node|. If the favicon has not yet