Remove GetActiveEntry usage from content.

Remove GetActiveEntry usage from content.

Using GetActiveEntry can lead to subtle bugs, including security ones. See the bug entry for more details.

BUG=273710
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223982

[nasko, 2013-08-16 10:41:27]

Hey Charlie,
This is a CL to remove usage of GetActiveEntry across content. Can you review it
for me?
Thanks,
Nasko

[Charlie Reis, 2013-08-16 22:10:14]

mkosiba: Can you take a look at the question in
content/browser/android/content_view_core_impl.cc?  It dates back to our
discussion from http://crbug.com/9682.

abarth: Can you let us know whether SSLManager is intentionally interacting with
the pending (provisional) entry?

Nasko: Glad you're taking this on.  It's clear there are several bugs it has
uncovered already.  Perhaps we should go ahead and mark GetActiveEntry as
deprecated?

https://codereview.chromium.org/23022006/diff/1/content/browser/android/conte...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/23022006/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.cc:806:
web_contents_->GetController().GetVisibleEntry();
This goes against the comment, which Martin Kosiba and I added as part of
introducing GetVisibleEntry.  However, it was necessary at the time to pass the
testNotCalledOnBackForwardNavigation test in android_dbg_triggered_tests, which
you appear to have passed:
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...

I wonder if the requirements have changed.  If so, I'd say stick with
GetVisibleEntry and remove the comment.  @mkosiba, can you confirm that this is
ok now?

https://codereview.chromium.org/23022006/diff/1/content/browser/download/save...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/23022006/diff/1/content/browser/download/save...
content/browser/download/save_package.cc:258: // Instead of using
web_contents_.GetURL here, we use url() (which is the
Wow, this is confusing on multiple levels.  :)

First, we should clarify the comment to make it clear that they're talking about
the virtual URL (which is the main difference between WebContents::GetURL() and
NavigationEntry::GetURL(), the latter of which is referred to as url()).

Then there's the bigger question of whether they actually wanted this to be the
pending entry or if they really wanted the last committed entry.  Wouldn't they
want the last committed one?  What do they actually save?

(I just tested this and it looks like right click -> Save As seems kind of
broken when there's a slow pending entry.  Seems like a real bug here.)

https://codereview.chromium.org/23022006/diff/1/content/browser/ssl/ssl_manag...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/23022006/diff/1/content/browser/ssl/ssl_manag...
content/browser/ssl/ssl_manager.cc:92:
(*i)->controller()->GetLastCommittedEntry()));
I like that we're switching to GetLastCommittedEntry here, but I don't know
enough about SSLManager to be sure.  Adam, does this class expect to work with
pending NavigationEntries (i.e., provisional loads), or does it only deal with
the URL after it has committed?

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_controller_impl.cc:271:
NavigationEntryImpl* active_entry =
nit: visible_entry

Actually, should we just make this use GetTransientEntry directly?  The comment
clearly implies that's intended.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_controller_impl.cc:465: NavigationEntry*
active_entry = GetVisibleEntry();
nit: visible_entry

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_controller_impl.cc:1321:
NavigationEntryImpl::FromNavigationEntry(GetVisibleEntry());
This scares me.  We may have to come back to this one after looking carefully at
how it's used.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_controller_impl.cc:1559: details->entry
= GetVisibleEntry();
GetLastCommittedEntry, perhaps?  How would we have a pending entry here?  Or is
it the transient entry we're trying to include?  (If so, this needs a comment
about that.)

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_controller_impl_unittest.cc:256:
EXPECT_FALSE(controller.GetVisibleEntry());
Redundant with next line.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_controller_impl_unittest.cc:352:
EXPECT_EQ(controller.GetPendingEntry(), controller.GetVisibleEntry());
Redundant with next line.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_controller_impl_unittest.cc:394:
EXPECT_EQ(controller.GetPendingEntry(), controller.GetVisibleEntry());
Redundant.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_controller_impl_unittest.cc:2760:
EXPECT_EQ(url1, controller.GetVisibleEntry()->GetURL());
Redundant.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
File content/browser/web_contents/render_view_host_manager_unittest.cc (right):

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/render_view_host_manager_unittest.cc:948: // Also
we should not have a pending navigation entry.
Can you add a check that GetPendingEntry is null?

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:1915: const NavigationEntry*
entry = controller_.GetVisibleEntry();
I have similar concerns here as with the SavePackage stuff.  Doesn't that refer
to the last committed page?

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:1936: bool
WebContentsImpl::IsActiveEntry(int32 page_id) {
We should at least have a TODO to change this to IsVisibleEntry.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:2023: NavigationEntry*
active_entry = GetController().GetVisibleEntry();
nit: visible_entry

Actually, this is broken.  It's using the zoom level from the current process,
which is the last committed entry.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:2049: NavigationEntry*
active_entry = GetController().GetVisibleEntry();
nit: visible_entry

Is this correct?  I'm seeing a view source of the last committed page even when
there's a pending entry, so I'm not sure how this works.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:2866: NavigationEntry* entry =
controller_.GetVisibleEntry();
Is this buggy?  Depends on whether render_view_host always matches entry, since
we're using entry below to decide what to do with render_view_host.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:3005:
(controller_.GetVisibleEntry()->GetTransitionType() &
Can this ever be different from GetLastCommittedEntry, given that we just
committed?  (Maybe for a transient entry?)

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:3428: GURL url =
controller_.GetVisibleEntry()
This looks buggy, since GetVisibleEntry does not match GetRenderViewHost.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right):

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:474:
web_contents->GetController().GetVisibleEntry());
Perhaps we should encourage GetLastCommittedEntry here, since they're waiting
for load stop above.

[mkosiba (inactive), 2013-08-19 14:49:37]

LGTM for content_view_core_impl changes (but I'm not an OWNER)

Dan, Ali - this should be fine but it might make sense to apply the CL locally
and run some RDS tests (URL intercepting doesn't have integration tests, so
you'd have to do it manually Ali) just to be extra sure.

https://codereview.chromium.org/23022006/diff/1/content/browser/android/conte...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/23022006/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.cc:806:
web_contents_->GetController().GetVisibleEntry();
On 2013/08/16 22:10:14, creis wrote:
> This goes against the comment, which Martin Kosiba and I added as part of
> introducing GetVisibleEntry.  However, it was necessary at the time to pass
the
> testNotCalledOnBackForwardNavigation test in android_dbg_triggered_tests,
which
> you appear to have passed:
>
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...
> 
> I wonder if the requirements have changed.  If so, I'd say stick with
> GetVisibleEntry and remove the comment.  @mkosiba, can you confirm that this
is
> ok now?

Hey! Thanks for remembering about this! We use a transition type to mark
API-initiated navigations now (instead of stashing their URL) which is why the
test passes with this CL.
Changing this here will change the WebView.getUrl semantics slightly, but in
this case it should be fine.

https://codereview.chromium.org/23022006/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.cc:1434: NavigationEntry* entry =
web_contents_->GetController().GetVisibleEntry();
@apiccion
FYI. This should be fine but just to be extra sure we should check that
navigations from chrome://* pages don't trigger intercepting after this lands.
Could you take care of that? Want me to file a bug?

https://codereview.chromium.org/23022006/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.cc:1550: NavigationEntry* entry =
web_contents_->GetController().GetVisibleEntry();
@dfalcantara:
FYI. I think this should be fine for RDS since the user is toggling the setting
for whatever URL we're displaying in the address bar.

[gone, 2013-08-19 18:40:39]

https://codereview.chromium.org/23022006/diff/1/content/browser/android/conte...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/23022006/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.cc:1550: NavigationEntry* entry =
web_contents_->GetController().GetVisibleEntry();
On 2013/08/19 14:49:38, mkosiba wrote:
> @dfalcantara:
> FYI. I think this should be fine for RDS since the user is toggling the
setting
> for whatever URL we're displaying in the address bar.

Should be alright if the visible entry updates before the page has completely
loaded (in case users try to get the desktop site before the page has finished
loading).

[nasko, 2013-09-12 17:56:30]

Hey guys,
I'm renewing this CL after being away for a while. 

I've added apiccion@, to answer android questions.
Adam, can you look at the SSL manager question Charlie has?

Thanks in advance.

https://codereview.chromium.org/23022006/diff/1/content/browser/android/conte...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/23022006/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.cc:806:
web_contents_->GetController().GetVisibleEntry();
On 2013/08/19 14:49:38, mkosiba wrote:
> On 2013/08/16 22:10:14, creis wrote:
> > This goes against the comment, which Martin Kosiba and I added as part of
> > introducing GetVisibleEntry.  However, it was necessary at the time to pass
> the
> > testNotCalledOnBackForwardNavigation test in android_dbg_triggered_tests,
> which
> > you appear to have passed:
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...
> > 
> > I wonder if the requirements have changed.  If so, I'd say stick with
> > GetVisibleEntry and remove the comment.  @mkosiba, can you confirm that this
> is
> > ok now?
> 
> Hey! Thanks for remembering about this! We use a transition type to mark
> API-initiated navigations now (instead of stashing their URL) which is why the
> test passes with this CL.
> Changing this here will change the WebView.getUrl semantics slightly, but in
> this case it should be fine. 

Done.

https://codereview.chromium.org/23022006/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.cc:1434: NavigationEntry* entry =
web_contents_->GetController().GetVisibleEntry();
On 2013/08/19 14:49:38, mkosiba wrote:
> @apiccion
> FYI. This should be fine but just to be extra sure we should check that
> navigations from chrome://* pages don't trigger intercepting after this lands.
> Could you take care of that? Want me to file a bug?

@mkosiba, @apiccion
What is the conclusion on this? Is the change fine? Do we need to file any bugs?

https://codereview.chromium.org/23022006/diff/1/content/browser/android/conte...
content/browser/android/content_view_core_impl.cc:1550: NavigationEntry* entry =
web_contents_->GetController().GetVisibleEntry();
On 2013/08/19 18:40:39, dfalcantara wrote:
> On 2013/08/19 14:49:38, mkosiba wrote:
> > @dfalcantara:
> > FYI. I think this should be fine for RDS since the user is toggling the
> setting
> > for whatever URL we're displaying in the address bar.
> 
> Should be alright if the visible entry updates before the page has completely
> loaded (in case users try to get the desktop site before the page has finished
> loading).

Done.

https://codereview.chromium.org/23022006/diff/1/content/browser/download/save...
File content/browser/download/save_package.cc (right):

https://codereview.chromium.org/23022006/diff/1/content/browser/download/save...
content/browser/download/save_package.cc:258: // Instead of using
web_contents_.GetURL here, we use url() (which is the
On 2013/08/16 22:10:14, creis wrote:
> Wow, this is confusing on multiple levels.  :)
> 
> First, we should clarify the comment to make it clear that they're talking
about
> the virtual URL (which is the main difference between WebContents::GetURL()
and
> NavigationEntry::GetURL(), the latter of which is referred to as url()).
> 
> Then there's the bigger question of whether they actually wanted this to be
the
> pending entry or if they really wanted the last committed entry.  Wouldn't
they
> want the last committed one?  What do they actually save?
> 
> (I just tested this and it looks like right click -> Save As seems kind of
> broken when there's a slow pending entry.  Seems like a real bug here.)

Done.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_controller_impl.cc:271:
NavigationEntryImpl* active_entry =
On 2013/08/16 22:10:14, creis wrote:
> nit: visible_entry
> 
> Actually, should we just make this use GetTransientEntry directly?  The
comment
> clearly implies that's intended.

Done.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_controller_impl.cc:465: NavigationEntry*
active_entry = GetVisibleEntry();
On 2013/08/16 22:10:14, creis wrote:
> nit: visible_entry

Done.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_controller_impl.cc:1559: details->entry
= GetVisibleEntry();
On 2013/08/16 22:10:14, creis wrote:
> GetLastCommittedEntry, perhaps?  How would we have a pending entry here?  Or
is
> it the transient entry we're trying to include?  (If so, this needs a comment
> about that.)

I'm not sure we can have a transient entry at the time of commit. I will use
LastCommitted.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_controller_impl_unittest.cc:256:
EXPECT_FALSE(controller.GetVisibleEntry());
On 2013/08/16 22:10:14, creis wrote:
> Redundant with next line.

Done.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_controller_impl_unittest.cc:352:
EXPECT_EQ(controller.GetPendingEntry(), controller.GetVisibleEntry());
On 2013/08/16 22:10:14, creis wrote:
> Redundant with next line.

Done.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_controller_impl_unittest.cc:394:
EXPECT_EQ(controller.GetPendingEntry(), controller.GetVisibleEntry());
On 2013/08/16 22:10:14, creis wrote:
> Redundant.

Done.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_controller_impl_unittest.cc:2760:
EXPECT_EQ(url1, controller.GetVisibleEntry()->GetURL());
On 2013/08/16 22:10:14, creis wrote:
> Redundant.

Done.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
File content/browser/web_contents/render_view_host_manager_unittest.cc (right):

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/render_view_host_manager_unittest.cc:948: // Also
we should not have a pending navigation entry.
On 2013/08/16 22:10:14, creis wrote:
> Can you add a check that GetPendingEntry is null?

Done.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:1915: const NavigationEntry*
entry = controller_.GetVisibleEntry();
On 2013/08/16 22:10:14, creis wrote:
> I have similar concerns here as with the SavePackage stuff.  Doesn't that
refer
> to the last committed page?

Done.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:1936: bool
WebContentsImpl::IsActiveEntry(int32 page_id) {
On 2013/08/16 22:10:14, creis wrote:
> We should at least have a TODO to change this to IsVisibleEntry.

Done.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:2023: NavigationEntry*
active_entry = GetController().GetVisibleEntry();
On 2013/08/16 22:10:14, creis wrote:
> nit: visible_entry
> 
> Actually, this is broken.  It's using the zoom level from the current process,
> which is the last committed entry.

Done.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:2049: NavigationEntry*
active_entry = GetController().GetVisibleEntry();
On 2013/08/16 22:10:14, creis wrote:
> nit: visible_entry
> 
> Is this correct?  I'm seeing a view source of the last committed page even
when
> there's a pending entry, so I'm not sure how this works.

The URL passed in isn't used. Eventually down the road, we end up calling
GetLastCommittedURL, so that's why it works. I've made those two match.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:2866: NavigationEntry* entry =
controller_.GetVisibleEntry();
On 2013/08/16 22:10:14, creis wrote:
> Is this buggy?  Depends on whether render_view_host always matches entry,
since
> we're using entry below to decide what to do with render_view_host.

I'm not sure I follow your comment, but the code has changed and the entry is
only used to judge view-source mode. We can't be in view-source for pending
entries, hence I'm changing this to be LastCommitted.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:3005:
(controller_.GetVisibleEntry()->GetTransitionType() &
On 2013/08/16 22:10:14, creis wrote:
> Can this ever be different from GetLastCommittedEntry, given that we just
> committed?  (Maybe for a transient entry?)

I think you are right and it can't be different if we just committed.
Furthermore, I don't see how we can have a transient entry at the time of
commit. In case of SSL warnings, it is displayed before we even complete the
connection and send the request, so to even receive a response (aka commit), we
need to have agreed on the interstitial page. In the case of safe browsing, we
need to have loaded the content and check it, which is also post-commit.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_impl.cc:3428: GURL url =
controller_.GetVisibleEntry()
On 2013/08/16 22:10:14, creis wrote:
> This looks buggy, since GetVisibleEntry does not match GetRenderViewHost.

Done.

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_view_aura_browsertest.cc (right):

https://codereview.chromium.org/23022006/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_view_aura_browsertest.cc:474:
web_contents->GetController().GetVisibleEntry());
On 2013/08/16 22:10:14, creis wrote:
> Perhaps we should encourage GetLastCommittedEntry here, since they're waiting
> for load stop above.

Done.

[abarth-chromium, 2013-09-12 19:54:09]

On 2013/08/16 22:10:14, creis wrote:
> abarth: Can you let us know whether SSLManager is intentionally interacting
with
> the pending (provisional) entry?

Yeah, that seems totally wrong.  I'd expect it to use the LastCommittedEntry as
this CL does.

[nasko, 2013-09-13 00:48:01]

On 2013/09/12 19:54:09, abarth wrote:
> On 2013/08/16 22:10:14, creis wrote:
> > abarth: Can you let us know whether SSLManager is intentionally interacting
> with
> > the pending (provisional) entry?
> 
> Yeah, that seems totally wrong.  I'd expect it to use the LastCommittedEntry
as
> this CL does.

Can I get an LGTM for content/browser/ssl/ then?

[abarth-chromium, 2013-09-13 03:18:31]

On 2013/09/13 00:48:01, nasko wrote:
> On 2013/09/12 19:54:09, abarth wrote:
> > On 2013/08/16 22:10:14, creis wrote:
> > > abarth: Can you let us know whether SSLManager is intentionally
interacting
> > with
> > > the pending (provisional) entry?
> > 
> > Yeah, that seems totally wrong.  I'd expect it to use the LastCommittedEntry
> as
> > this CL does.
> 
> Can I get an LGTM for content/browser/ssl/ then?

Lgtm

[nasko, 2013-09-13 22:54:26]

Adding rdsmith@ for OWNERs review of chrome/browser/download.

[Randy Smith (Not in Mondays), 2013-09-16 16:26:03]

On 2013/09/13 22:54:26, nasko wrote:
> Adding rdsmith@ for OWNERs review of chrome/browser/download.

As noted in private email, making this change removes the point of the test. 
The test is based on doing a save *before* the commit happens.  That can happen
if someone chooses SavePageAs from the drop down menu before the page commit
occurs.  It would be hard to do, but possible, and caused a crash pre-156538
fix.  So I'd like to find some other way to solve whatever problem you had with
that test.

[Charlie Reis, 2013-09-16 23:10:53]

Getting close.  Looks like we still need to resolve Save As with no committed
entry, and a few other comments below.

https://codereview.chromium.org/23022006/diff/41001/chrome/browser/download/s...
File chrome/browser/download/save_page_browsertest.cc (right):

https://codereview.chromium.org/23022006/diff/41001/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:596: // The navigation needs to
at least commit, otherwise there will be nothing
I agree with rdsmith: this test appears to be a regression test for the case
that Save As is initiated before commit.  We should keep the test that way to
make sure Chrome does something reasonable in that situation.  (Sounds like
you're making progress on this, based on the email thread.)

https://codereview.chromium.org/23022006/diff/41001/content/browser/accessibi...
File content/browser/accessibility/accessibility_ui.cc (right):

https://codereview.chromium.org/23022006/diff/41001/content/browser/accessibi...
content/browser/accessibility/accessibility_ui.cc:86:
rvh->GetProcess()->GetID(),
I forget-- did we come to a resolution on this?  We've got GetURL,
GetVisibleEntry, and an rvh which could theoretically be last committed,
pending, or neither.  I wonder if there's some way to ensure we'll get a
consistent descriptor here.

Feel free to just make it a TODO, since I'm not sure what the answer is and
GetVisibleEntry seems like the right place to get the favicon.

https://codereview.chromium.org/23022006/diff/41001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/23022006/diff/41001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:822: GURL url = entry ?
entry->GetVirtualURL() : GURL::EmptyGURL();
These lines can be replaced with web_contents_->GetVisibleURL() now.  They only
used this approach before to explicitly use the active entry, which isn't
necessary any longer.

https://codereview.chromium.org/23022006/diff/41001/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/23022006/diff/41001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1322:
NavigationEntryImpl::FromNavigationEntry(GetVisibleEntry());
To be honest, I'm not sure what this is expecting.  Let's just keep your
GetVisibleEntry change and move this line below Jochen's TODO comment to clean
it up.

https://codereview.chromium.org/23022006/diff/41001/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/23022006/diff/41001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl_unittest.cc:374:
EXPECT_EQ(controller.GetVisibleEntry(), controller.GetVisibleEntry());
This is a no-op.

https://codereview.chromium.org/23022006/diff/41001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl_unittest.cc:416:
EXPECT_EQ(controller.GetVisibleEntry(), controller.GetVisibleEntry());
No-op.

https://codereview.chromium.org/23022006/diff/41001/content/browser/web_conte...
File content/browser/web_contents/render_view_host_manager_unittest.cc (right):

https://codereview.chromium.org/23022006/diff/41001/content/browser/web_conte...
content/browser/web_contents/render_view_host_manager_unittest.cc:953:
EXPECT_TRUE(contents()->GetController().GetPendingEntry() == NULL);
nit: Just use EXPECT_FALSE and we won't need the "== NULL"

https://codereview.chromium.org/23022006/diff/41001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/23022006/diff/41001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:2923: NavigationEntry* entry =
controller_.GetPendingEntry();
Your reply said you were changing this to LastCommitted, but it's Pending here.

Are we sure that |render_view_host| corresponds to the pending entry, and that a
pending entry exists?  Seems plausible, which would make this right.

[nasko, 2013-09-18 17:03:17]

As per email discussion, I've removed the changes to Save As and addressed the
rest of the comments.

https://codereview.chromium.org/23022006/diff/41001/chrome/browser/download/s...
File chrome/browser/download/save_page_browsertest.cc (right):

https://codereview.chromium.org/23022006/diff/41001/chrome/browser/download/s...
chrome/browser/download/save_page_browsertest.cc:596: // The navigation needs to
at least commit, otherwise there will be nothing
On 2013/09/16 23:10:54, creis wrote:
> I agree with rdsmith: this test appears to be a regression test for the case
> that Save As is initiated before commit.  We should keep the test that way to
> make sure Chrome does something reasonable in that situation.  (Sounds like
> you're making progress on this, based on the email thread.)

Reverting this in favor of more complete fix as discussed over email.

https://codereview.chromium.org/23022006/diff/41001/content/browser/accessibi...
File content/browser/accessibility/accessibility_ui.cc (right):

https://codereview.chromium.org/23022006/diff/41001/content/browser/accessibi...
content/browser/accessibility/accessibility_ui.cc:86:
rvh->GetProcess()->GetID(),
On 2013/09/16 23:10:54, creis wrote:
> I forget-- did we come to a resolution on this?  We've got GetURL,
> GetVisibleEntry, and an rvh which could theoretically be last committed,
> pending, or neither.  I wonder if there's some way to ensure we'll get a
> consistent descriptor here.
> 
> Feel free to just make it a TODO, since I'm not sure what the answer is and
> GetVisibleEntry seems like the right place to get the favicon.

I've added a TODO, since this code will need more changes. I'm not yet 100% sure
how I'll solve the WebContents GetTitle problem, so I'll fix this once this is
known.

https://codereview.chromium.org/23022006/diff/41001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/23022006/diff/41001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:822: GURL url = entry ?
entry->GetVirtualURL() : GURL::EmptyGURL();
On 2013/09/16 23:10:54, creis wrote:
> These lines can be replaced with web_contents_->GetVisibleURL() now.  They
only
> used this approach before to explicitly use the active entry, which isn't
> necessary any longer.

Very good point!

https://codereview.chromium.org/23022006/diff/41001/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/23022006/diff/41001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1322:
NavigationEntryImpl::FromNavigationEntry(GetVisibleEntry());
On 2013/09/16 23:10:54, creis wrote:
> To be honest, I'm not sure what this is expecting.  Let's just keep your
> GetVisibleEntry change and move this line below Jochen's TODO comment to clean
> it up.

Done.

https://codereview.chromium.org/23022006/diff/41001/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/23022006/diff/41001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl_unittest.cc:374:
EXPECT_EQ(controller.GetVisibleEntry(), controller.GetVisibleEntry());
On 2013/09/16 23:10:54, creis wrote:
> This is a no-op.

Done.

https://codereview.chromium.org/23022006/diff/41001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl_unittest.cc:416:
EXPECT_EQ(controller.GetVisibleEntry(), controller.GetVisibleEntry());
On 2013/09/16 23:10:54, creis wrote:
> No-op.

Done.

https://codereview.chromium.org/23022006/diff/41001/content/browser/web_conte...
File content/browser/web_contents/render_view_host_manager_unittest.cc (right):

https://codereview.chromium.org/23022006/diff/41001/content/browser/web_conte...
content/browser/web_contents/render_view_host_manager_unittest.cc:953:
EXPECT_TRUE(contents()->GetController().GetPendingEntry() == NULL);
On 2013/09/16 23:10:54, creis wrote:
> nit: Just use EXPECT_FALSE and we won't need the "== NULL"

I kept it consistent with the surrounding code, which all uses EXPECT_TRUE with
explicit check inside. If you feel strongly about this, I'll put together
another CL and change all instances to consistently use the other pattern.

https://codereview.chromium.org/23022006/diff/41001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/23022006/diff/41001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:2923: NavigationEntry* entry =
controller_.GetPendingEntry();
On 2013/09/16 23:10:54, creis wrote:
> Your reply said you were changing this to LastCommitted, but it's Pending
here.
> 
> Are we sure that |render_view_host| corresponds to the pending entry, and that
a
> pending entry exists?  Seems plausible, which would make this right.

I did change it to LastCommitted, but that broke the NTPViewSource unittest.
According to it, we expect to send ViewMsg_EnableViewSourceMode prior to
receiving FrameNavigate. Thinking a bit more about it, we create RenderViewHost
when we are about to navigate in a new tab or when we are doing a cross process
navigation. In those cases, navigating to view-source URL will be the pending
navigation and not the last committed one, unless I'm mistaken.

[Randy Smith (Not in Mondays), 2013-09-18 17:12:44]

My understanding from the email thread was that this CL would silently abort a
SavePage if the it was attempted before the page was loaded.  Are you doing
that?  I didn't see the code I expected to see for that purpose.

[nasko, 2013-09-18 17:14:30]

On 2013/09/18 17:12:44, rdsmith wrote:
> My understanding from the email thread was that this CL would silently abort a
> SavePage if the it was attempted before the page was loaded.  Are you doing
> that?  I didn't see the code I expected to see for that purpose.

No, I opted for no changes in this CL and separate CL for the Save As work.

[Randy Smith (Not in Mondays), 2013-09-18 17:30:57]

On 2013/09/18 17:14:30, nasko wrote:
> On 2013/09/18 17:12:44, rdsmith wrote:
> > My understanding from the email thread was that this CL would silently abort
a
> > SavePage if the it was attempted before the page was loaded.  Are you doing
> > that?  I didn't see the code I expected to see for that purpose.
> 
> No, I opted for no changes in this CL and separate CL for the Save As work.

How does that avoid the crash?

[nasko, 2013-09-18 17:32:14]

On 2013/09/18 17:30:57, rdsmith wrote:
> On 2013/09/18 17:14:30, nasko wrote:
> > On 2013/09/18 17:12:44, rdsmith wrote:
> > > My understanding from the email thread was that this CL would silently
abort
> a
> > > SavePage if the it was attempted before the page was loaded.  Are you
doing
> > > that?  I didn't see the code I expected to see for that purpose.
> > 
> > No, I opted for no changes in this CL and separate CL for the Save As work.
> 
> How does that avoid the crash?

There is no crash : ) There was crash in an intermediate version of my CL. Since
I'm not making any changes, it is back to how it works today.

[Randy Smith (Not in Mondays), 2013-09-18 17:45:01]

On 2013/09/18 17:32:14, nasko wrote:
> On 2013/09/18 17:30:57, rdsmith wrote:
> > On 2013/09/18 17:14:30, nasko wrote:
> > > On 2013/09/18 17:12:44, rdsmith wrote:
> > > > My understanding from the email thread was that this CL would silently
> abort
> > a
> > > > SavePage if the it was attempted before the page was loaded.  Are you
> doing
> > > > that?  I didn't see the code I expected to see for that purpose.
> > > 
> > > No, I opted for no changes in this CL and separate CL for the Save As
work.
> > 
> > How does that avoid the crash?
> 
> There is no crash : ) There was crash in an intermediate version of my CL.
Since
> I'm not making any changes, it is back to how it works today.

Ok :-}.  LGTM.

[Charlie Reis, 2013-09-18 18:36:04]

LGTM

https://codereview.chromium.org/23022006/diff/41001/content/browser/web_conte...
File content/browser/web_contents/render_view_host_manager_unittest.cc (right):

https://codereview.chromium.org/23022006/diff/41001/content/browser/web_conte...
content/browser/web_contents/render_view_host_manager_unittest.cc:953:
EXPECT_TRUE(contents()->GetController().GetPendingEntry() == NULL);
On 2013/09/18 17:03:18, nasko wrote:
> On 2013/09/16 23:10:54, creis wrote:
> > nit: Just use EXPECT_FALSE and we won't need the "== NULL"
> 
> I kept it consistent with the surrounding code, which all uses EXPECT_TRUE
with
> explicit check inside. If you feel strongly about this, I'll put together
> another CL and change all instances to consistently use the other pattern.

Ok.  Feel free to send a cleanup CL to me after this and I'll approve it.

[commit-bot: I haz the power, 2013-09-18 20:25:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/23022006/66001

[commit-bot: I haz the power, 2013-09-18 23:20:10]

Change committed as 223982