New issue
Advanced search Search tips

Issue 618553 link

Starred by 1 user
Status: Fixed
Owner:
dominickn@chromium.org
Closed: Jun 2016
Cc:
clamy@chromium.org
creis@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

NavigationHandle does not report view-source component of URL

Project Member Reported by dominickn@chromium.org, Jun 9 2016 
I stumbled on an interesting discrepancy in NavigationHandle::GetURL versus WebContents::GetLastCommittedURL.

The SavePageBrowserTest.SaveViewSourceHTMLOnly browser test navigates to a view source URL - "view-source:http://mock.http/save_page/a.htm". In WebContentsObserver::DidFinishNavigation, after checking that the navigation was committed and in the main frame and without error, navigation_handle->GetURL() returned "http://mock.http/save_page/a.htm", without the view-source: prefix. However, calling web_contents()->GetLastCommittedURL() returned the link with the correct view-source prefix.

Is there a reason why the navigation handle doesn't have the view-source component in the URL? Is it to do with how view source is implemented?

As an aside, this inconsistency tripped an ASAN bot into exposing a memory leak in HostContentSettingsMap, so it was helpful in a way! :)

Comment 1 by creis@chromium.org, Jun 9 2016 

Yeah, this is a confusing bit about view-source, but I'm not sure it's something to change.

view-source is actually a mode that a RenderFrameHost is put into, after which it's told to navigate to URLs without the view-source prefix.  The view-source prefix that's visible in the omnibox is actually the "virtual URL" (which gets used in a few other contexts as well).

If you ask NavigationEntry::GetURL(), you'll get the same answer as NavigationHandle::GetURL(), which has no prefix.  It happens that WebContents::GetLastCommittedURL() returns NavigationEntry::GetVirtualURL(), partly due to legacy behavior where the callers expect it.

I suppose we could rename WebContents::GetLastCommittedURL to GetLastCommittedVirtualURL, but that's a bit verbose.  Maybe this is just a matter of clarifying the comments in web_contents.h, or else a WontFix.

(Anyway, glad it was useful to track down the memory leak.)  :)

Comment 2 by dominickn@chromium.org, Jun 9 2016 

Thanks for the context. An update to the comment in web_contents.h SGTM just to clarify what's going on. :)
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 21 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/319d693f722a8bc60ca52b6735b8efae7875d5d0

commit 319d693f722a8bc60ca52b6735b8efae7875d5d0
Author: dominickn <dominickn@chromium.org>
Date: Tue Jun 21 00:59:26 2016

Clarify that WebContents::GetLastCommittedURL returns the virtual URL.

NavigationHandle::GetURL is not necessarily consistent with
WebContents::GetLastCommittedURL, as the URL being navigated to may be
changed for display in the URL bar. For example, when viewing the page
source, NavigationHandle::GetURL returns the page's URL, but
WebContents::GetLastCommittedURL returns the URL prefixed with
"view-source:". This CL notes the difference in the header comments for
both methods.

BUG= 618553 

Review-Url: https://codereview.chromium.org/2076063002
Cr-Commit-Position: refs/heads/master@{#400857}

[modify] https://crrev.com/319d693f722a8bc60ca52b6735b8efae7875d5d0/content/public/browser/navigation_handle.h
[modify] https://crrev.com/319d693f722a8bc60ca52b6735b8efae7875d5d0/content/public/browser/web_contents.h

Comment 4 by dominickn@chromium.org, Jun 21 2016

Cc: clamy@chromium.org
Owner: dominickn@chromium.org
Status: Fixed (was: Untriaged)
►

Sign in to add a comment