Fix initial erroneous navigation in iframe to not add history entry.

content/renderer/render_view_impl.cc

 // Copyright (c) 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 "content/renderer/render_view_impl.h"
 
 #include <algorithm>
 #include <cmath>
 
 #include "base/auto_reset.h"
@@ skipping 3531 matching lines @@
       document_state->set_request_time(Time::FromDoubleT(event_time));
   }
 
   // Start time is only set after request time.
   document_state->set_start_load_time(Time::Now());
 
   bool is_top_most = !frame->parent();
   if (is_top_most) {
     navigation_gesture_ = WebUserGestureIndicator::isProcessingUserGesture() ?
         NavigationGestureUser : NavigationGestureAuto;
-  } else if (frame->parent()->isLoading()) {
-    // Take note of AUTO_SUBFRAME loads here, so that we can know how to
-    // load an error page.  See didFailProvisionalLoad.
+  } else if (ds->replacesCurrentHistoryItem() ||
+      !frame->hasCommittedRealDocument()) {

[darin (slow to review), 2013/09/20 22:09:38]

I'm having a hard time understanding what the hasCommittedRealDocument() check
is doing.

If we haven't committed a real document, then it means we are either showing the
initial empty document or we have started a provisional load on the frame that
hasn't committed yet. In this latter case, how do you know it is AUTO_SUBFRAME?
The case of replacesCurrentHistoryItem() returning false corresponds to the case
of creating a new back/forward entry. AUTO_SUBFRAME should never be used when we
create a new back/forward entry.

I'm confused.

[nasko, 2013/09/20 22:30:00]

The problem with solely relying on replacesCurrentHistoryItem is with this code
fragment:

var f = document.createElement('iframe');
document.body.appendChild(f);

This delivers a didStartProvisionalLoad for "about:blank" and
replacesCurrentHistoryItem returns false. This would mean that we need to add a
session history entry for this navigation, which isn't correct, as this is not a
real document load.

This is why I'm checking whether we have loaded a real document in the frame, so
I can disambiguate this case.

+    // Subframe navigations, which don't add session history items must be
+    // marked with AUTO_SUBFRAME. See also didFailProvisionalLoad for how we
+    // handle loading of error pages.
     document_state->navigation_state()->set_transition_type(
         PAGE_TRANSITION_AUTO_SUBFRAME);
   }
 
   FOR_EACH_OBSERVER(
       RenderViewObserver, observers_, DidStartProvisionalLoad(frame));
 
   Send(new ViewHostMsg_DidStartProvisionalLoadForFrame(
        routing_id_, frame->identifier(),
        frame->parent() ? frame->parent()->identifier() : -1,
@@ skipping 2935 matching lines @@
   for (size_t i = 0; i < icon_urls.size(); i++) {
     WebURL url = icon_urls[i].iconURL();
     if (!url.isEmpty())
       urls.push_back(FaviconURL(url,
                                 ToFaviconType(icon_urls[i].iconType())));
   }
   SendUpdateFaviconURL(urls);
 }
 
 }  // namespace content