Move most visited item state info from InstantController to InstantService.

chrome/common/instant_restricted_id_cache.h

Index: chrome/common/instant_restricted_id_cache.h
diff --git a/chrome/common/instant_restricted_id_cache.h b/chrome/common/instant_restricted_id_cache.h
index b0c2b3ba05e44eb294577ccdb18d3ad7222274b4..5935ea29c09eb250d2892bab60dec4fc58f0b538 100644
--- a/chrome/common/instant_restricted_id_cache.h
+++ b/chrome/common/instant_restricted_id_cache.h
@@ -69,6 +69,8 @@ class InstantRestrictedIDCache {
   FRIEND_TEST_ALL_PREFIXES(InstantRestrictedIDCacheTest, ManualIDGeneration);
   FRIEND_TEST_ALL_PREFIXES(InstantRestrictedIDCacheTest, MixIDGeneration);
   FRIEND_TEST_ALL_PREFIXES(InstantRestrictedIDCacheTest, AddEmptySet);
+  FRIEND_TEST_ALL_PREFIXES(InstantRestrictedIDCacheTest,
+                           AddItemsWithRestrictedID);
 
   typedef base::MRUCache<InstantRestrictedID, T> CacheImpl;
 
@@ -126,10 +128,15 @@ void InstantRestrictedIDCache<T>::AddItemsWithRestrictedID(
     ids_added.insert(item_id.first);
 
     cache_.Put(item_id.first, item_id.second);
-    if (i == 0)
-      last_add_start_ = --cache_.rend();
     last_restricted_id_ = std::max(item_id.first, last_restricted_id_);
   }
+
+  // cache_.Put() can invalidate the iterator |last_add_start_| is pointing to.

[Jered, 2013/05/24 15:58:21]

So were you seeing last_add_start_ reset for i == 0, and then become invalidated
inside the loop above?

From reading mru_cache, I think that should only happen if items contains
duplicate restricted ids or is larger than the cache capacity. Both seem like
deeper problems. Backing up by items.size() would not be correct in those cases
because we did not add that many items.

[Jered, 2013/05/24 19:23:02]

Ok, got it. This is sort of nasty and subtle. :-)

To avoid actually having a dummy before begin(), reverse_iterator defines
rbegin() to be end() and rend() maps to begin(). operator* accesses i-1. So
--rend() actually points at the last element of the _previously_ added MV items,
it's just that operator* makes it look like the first added item from this
batch. Yuck.

So, in your unit test, when erase() removes item 12, i.e. the last item of the
previously added batch, that invalidates last_add_start_.

I'm not sure why we're not seeing a bazillion renderer crashes because of this.

Anyway, I am fine with this fix. I would be _even happier_ with never ever using
reverse_iterators at all, because this behavior is really confusing and
dangerous.

[kmadhusu, 2013/05/24 20:12:23]

Yes. I completely agree. Reverse_iterator is very confusing. I will  clean up
this file in a separate CL.

+  // Therefore, update |last_add_start_| after adding all the items to the
+  // |cache_|.
+  last_add_start_ = cache_.rend();
+  for (size_t i = 0; i < items.size(); ++i)
+    --last_add_start_;
 }
 
 template <typename T>