Do not use GetPendingEntryIndex in TabRestoreServiceHelper::PopulateTab.

components/sessions/core/tab_restore_service_helper.cc

 // Copyright 2012 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 "components/sessions/core/tab_restore_service_helper.h"
 
 #include <inttypes.h>
 #include <stddef.h>
 
 #include <algorithm>
@@ skipping 392 matching lines @@
       return ValidateWindow(static_cast<const Window&>(entry));
   }
   NOTREACHED();
   return false;
 }
 
 void TabRestoreServiceHelper::PopulateTab(Tab* tab,
                                           int index,
                                           LiveTabContext* context,
                                           LiveTab* live_tab) {
-  const int pending_index = live_tab->GetPendingEntryIndex();
   int entry_count =
       live_tab->IsInitialBlankNavigation() ? 0 : live_tab->GetEntryCount();
-  if (entry_count == 0 && pending_index == 0)

[Eugene But (OOO till 7-30), 2017/03/21 22:31:14]

When entry_count is 0, then pending_index is always -1, so this should never be
true:
https://cs.chromium.org/chromium/src/content/public/browser/navigation_contro...

[Charlie Reis, 2017/03/22 22:49:49]

Yes, I can't think of a case that both entry_count and pending_index would be 0.

I thought about the case of opening a slow URL in a new tab (when entry_count is
0 but there's a pending entry), but that indeed has pending_index -1.  (Note
that we *do* get here in that case, because we don't filter out tabs with zero
entries until AddEntry/FilterEntry/IsTabInteresting after this call.  But
entry_count is zero, so the loop below does nothing.)

-    entry_count++;
   tab->navigations.resize(static_cast<int>(entry_count));
   for (int i = 0; i < entry_count; ++i) {
-    SerializedNavigationEntry entry = (i == pending_index)
-                                          ? live_tab->GetPendingEntry()
-                                          : live_tab->GetEntryAtIndex(i);
+    SerializedNavigationEntry entry = live_tab->GetEntryAtIndex(i);

[Eugene But (OOO till 7-30), 2017/03/21 22:31:15]

Because |entry_count| does not include pending item, there is no need to restore
pending item.

[sky, 2017/03/22 03:20:16]

Are you sure about that? I thought reload and possibly other things making the
pending index point to an existing one.

[Eugene But (OOO till 7-30), 2017/03/22 14:38:40]

It is true that reload and back-forward navigation make pending index point to
existing one. However in that case |entry_count| will not be 0, so the previous
2 lines I deleted will not execute. Also |i == pending_index)| will be true iff
item is both bending and committed, so |live_tab->GetEntryAtIndex(i)| and
|live_tab->GetPendingEntry()| will return the same result.

[Charlie Reis, 2017/03/22 22:49:49]

Yeah, pointing to an existing one is fine-- your CL wouldn't affect that.  It
would only matter if GetPendingEntry() didn't return GetEntryAtIndex(i) when
pending_entry_index_ is i.  That seems like it can only happen if i is -1.

Since i can't be -1 here (given the loop bounds), I think your change is
probably safe.

     tab->navigations[i] = entry;
   }
   tab->timestamp = TimeNow();
   tab->current_navigation_index = live_tab->GetCurrentEntryIndex();
   tab->tabstrip_index = index;
 
   tab->extension_app_id = client_->GetExtensionAppIDForTab(live_tab);
 
   tab->user_agent_override = live_tab->GetUserAgentOverride();
 
@@ skipping 121 matching lines @@
         tab.browser_id = new_id;
     }
   }
 }
 
 base::Time TabRestoreServiceHelper::TimeNow() const {
   return time_factory_ ? time_factory_->TimeNow() : base::Time::Now();
 }
 
 }  // namespace sessions