Keep a copy of page id in RenderViewHost.

content/renderer/render_frame_impl.cc

 // Copyright 2013 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_frame_impl.h"
 
 #include <map>
 #include <string>
 
 #include "base/auto_reset.h"
@@ skipping 2212 matching lines @@
       // If the history list is valid, our list of page IDs should be correct.
       DCHECK(render_view_->history_list_length_ <= 0 ||
              render_view_->history_list_offset_ < 0 ||
              render_view_->history_list_offset_ >=
                  render_view_->history_list_length_ ||
              render_view_->history_page_ids_[render_view_->history_list_offset_]
                   == render_view_->page_id_);
     }
   }
 
+  Send(new FrameHostMsg_DidAssignPageId(routing_id_, render_view_->page_id_));

[Charlie Reis, 2014/09/08 16:04:37]

This part looks new to me, but I think it makes sense, to protect us from
unexpected behavior in the observers.

Note that there is a branch above that doesn't assign page_id_ (if the inner
check on line 2214 fails).  I don't know what an "unsuccessful history
navigation" is, so I'm not sure if it matters if we send the old page ID to the
browser in that case.  Probably shouldn't be a problem, but maybe we have a test
case that hits that branch to help us understand it?

[Avi (use Gerrit), 2014/09/08 16:40:48]

Yes, this is new. There is so much uncertainty about doing stuff before sending
DidCommitProvisionalLoad or whether we process it, etc, that I just wanted to
say f-it and just ipc.
I'm pretty sure sending an unneccessary IPC is OK here. Do we have a test case?
I don't know. I'd need to look.

[Charlie Reis, 2014/09/08 16:51:00]

I tossed in a NOTREACHED in the else block and these tests in
content_browsertests hit it:

    RenderViewHostTest.BasicRenderFrameHost
    RenderViewHostTest.ReleaseSessionOnCloseACK
    RenderViewImplTest.ImeComposition
    RenderViewImplTest.NavigateFrame

Perhaps they can help explain that case?

I agree that this will likely be fine, but it'd be nice to know why we're
getting here.

[Avi (use Gerrit), 2014/09/08 17:02:09]

It would indeed be good to know what's going on. During the next cycle of tests
(apply patch, crash on the canary, revert) I'll dig there.

[Charlie Reis, 2014/09/08 17:28:53]

I took a closer look, and we get there for this commit_type:
    // Reloads, client redirects, etc. Loads that neither originate from
    // nor add entries to the back/forward list.
    WebHistoryInertCommit

Sure enough, reloads make us hit my NOTREACHED.  But in that case, we should
just be telling the browser something it already knows with the new IPC, so I
think this change is fine.

+
   FOR_EACH_OBSERVER(RenderViewObserver, render_view_->observers_,
                     DidCommitProvisionalLoad(frame, is_new_navigation));
   FOR_EACH_OBSERVER(RenderFrameObserver, observers_,
                     DidCommitProvisionalLoad(is_new_navigation));
 
   if (!frame->parent()) {  // Only for top frames.
     RenderThreadImpl* render_thread_impl = RenderThreadImpl::current();
     if (render_thread_impl) {  // Can be NULL in tests.
       render_thread_impl->histogram_customizer()->
           RenderViewNavigatedToHost(GURL(GetLoadingUrl()).host(),
@@ skipping 1632 matching lines @@
 
 #if defined(ENABLE_BROWSER_CDMS)
 RendererCdmManager* RenderFrameImpl::GetCdmManager() {
   if (!cdm_manager_)
     cdm_manager_ = new RendererCdmManager(this);
   return cdm_manager_;
 }
 #endif  // defined(ENABLE_BROWSER_CDMS)
 
 }  // namespace content