Harden DisplayItemList in release mode not to crash on cached display item failures.

Source/platform/graphics/paint/DisplayItemList.cpp

 // 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 "config.h"
 #include "platform/graphics/paint/DisplayItemList.h"
 
 #include "platform/NotImplemented.h"
 #include "platform/RuntimeEnabledFeatures.h"
 #include "platform/TraceEvent.h"
@@ skipping 138 matching lines @@
     // Only need to index DrawingDisplayItems and FIXME: BeginSubtreeDisplayItems.
     if (!DisplayItem::isDrawingType(type))
         return;
 
     DisplayItemIndicesByClientMap::iterator it = displayItemIndicesByClient.find(client);
     Vector<size_t>& indices = it == displayItemIndicesByClient.end() ?
         displayItemIndicesByClient.add(client, Vector<size_t>()).storedValue->value : it->value;
     indices.append(index);
 }
 
-DisplayItems::Iterator DisplayItemList::findOutOfOrderCachedItem(DisplayItems::Iterator& currentIt, const DisplayItem::Id& id, DisplayItem::Type matchingType, DisplayItemIndicesByClientMap& displayItemIndicesByClient)
+DisplayItems::Iterator DisplayItemList::findOutOfOrderCachedItem(DisplayItems::Iterator currentIt, const DisplayItem::Id& id, DisplayItem::Type matchingType, DisplayItemIndicesByClientMap& displayItemIndicesByClient)
 {
     ASSERT(DisplayItem::isCachedType(id.type));
     ASSERT(clientCacheIsValid(id.client));
 
     size_t foundIndex = findMatchingItemFromIndex(id, matchingType, displayItemIndicesByClient, m_currentDisplayItems);
     if (foundIndex != kNotFound)
         return m_currentDisplayItems.iteratorAt(foundIndex);
 
     return findOutOfOrderCachedItemForward(currentIt, id, matchingType, displayItemIndicesByClient);
 }
 
 // Find forward for the item and index all skipped indexable items.
-DisplayItems::Iterator DisplayItemList::findOutOfOrderCachedItemForward(DisplayItems::Iterator& currentIt, const DisplayItem::Id& id, DisplayItem::Type matchingType, DisplayItemIndicesByClientMap& displayItemIndicesByClient)
+DisplayItems::Iterator DisplayItemList::findOutOfOrderCachedItemForward(DisplayItems::Iterator currentIt, const DisplayItem::Id& id, DisplayItem::Type matchingType, DisplayItemIndicesByClientMap& displayItemIndicesByClient)
 {
     DisplayItems::Iterator currentEnd = m_currentDisplayItems.end();
     for (; currentIt != currentEnd; ++currentIt) {
         DisplayItems::ItemHandle item = *currentIt;
         if (!item.isGone()
             && DisplayItem::isDrawingType(item.type())
             && m_validlyCachedClients.contains(item.client())) {
             if (item.id().equalToExceptForType(id, matchingType))
                 return currentIt;
 
@@ skipping 79 matching lines @@
                 updatedList.appendByMoving(currentIt);
             } else {
                 DisplayItems::Iterator foundIt = findOutOfOrderCachedItem(currentIt, newDisplayItem.id(), matchingType, displayItemIndicesByClient);
                 isSynchronized = (foundIt == currentIt);
 
 #ifndef NDEBUG
                 if (foundIt == currentEnd) {
                     showDebugData();
                     WTFLogAlways("CachedDisplayItem %s not found in m_currentDisplayItems\n",
                         newDisplayItem.asDebugString().utf8().data());
+                    ASSERT_NOT_REACHED();

[Xianzhu, 2015/06/18 00:03:26]

We should put a 'continue;' after the above statement, otherwise we'll crash at
line 281.

[chrishtr, 2015/06/18 00:08:53]

Done.

                 }
 #endif
-                // TODO(chrishtr): downgrade to ASSERT as this goes to beta users, and replace with bailing out of the rest of the merge.
-                // This assert hits if we couldn't find the cached display item in our cache, which should be impossible.
-                RELEASE_ASSERT(foundIt != currentEnd);
+                // If foundIt == currentEnd, it means that we did not find the cached display item. This should be impossible, but may occur
+                // if there is a bug in the system, such as under-invalidation, incorrect cache checking or duplicate display ids. In this case,
+                // attempt to recover rather than crashing or bailing on display of the rest of the display list.
+                if (foundIt != currentEnd)

[Xianzhu, 2015/06/18 00:03:26]

Would it be better to put this 'if' and the 'if' at line 268 into an 'if-else'?

[chrishtr, 2015/06/18 00:08:53]

It's inside an ifndef NDEBUG block, prefer to keep this separate.

+                    currentIt = foundIt;
 
                 updatedList.appendByMoving(foundIt);
             }
         } else {
 #if ENABLE(ASSERT)
             if (RuntimeEnabledFeatures::slimmingPaintUnderInvalidationCheckingEnabled())
                 checkCachedDisplayItemIsUnchanged(newDisplayItem, displayItemIndicesByClient);
             else
                 ASSERT(!DisplayItem::isDrawingType(newDisplayItem.type()) || newDisplayItem.skippedCache() || !clientCacheIsValid(newDisplayItem.client()));
 #endif // ENABLE(ASSERT)
@@ skipping 187 matching lines @@
 
 void DisplayItemList::replay(GraphicsContext& context) const
 {
     TRACE_EVENT0("blink,benchmark", "DisplayItemList::replay");
     ASSERT(m_newDisplayItems.isEmpty());
     for (auto& displayItem : m_currentDisplayItems)
         displayItem.replay(context);
 }
 
 } // namespace blink