Issue metadata
Sign in to add a comment
|
View Frame Source for iframe of a file:// URL shows main frame's filename
Reported by
jm.acun...@gmail.com,
Oct 25 2016
|
||||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.71 Safari/537.36 Steps to reproduce the problem: - Writes in the Chrome address bar: data: text/html <iframe src='https://abc.xyz'></iframe> - Right 'View Frame Source code' - Title tab: view-source:https://abc.xyz - Ok Now we do the same from a file on the local machine: - Right 'View Frame Source code' - Title tab: Local file name (although it shows the favicon correctly) What is the expected behavior? Title tab: view-source:Domain Name What went wrong? Title tab: Local file name Did this work before? N/A Chrome version: 54.0.2840.71 Channel: stable OS Version: 6.3 Flash Version: Shockwave Flash 23.0 r0
,
Oct 26 2016
Tested the issue on windows-7, Mac-10.11.4 and linux ubuntu 14.04 using chrome stable version 54.0.2840.71 and latest canary 56.0.2899.0 with below steps 1. opened chrome 2.Navigated to the page https://abc.xyz 3.Clicked on View page source and observed that "view-source:https://abc.xyz/" in the address bar 4.Same behaviour observed in Firefox too with the above steps. Could you please let us know your expected behaviour and if possible provide the screenshot to triage the issue better. Thanks.
,
Oct 26 2016
Hello, I sorry not having explained clearly. The static html page in local machine contains an iframe whose src is https://abc.xyz The error occurs when you click on the iframe and click view source code iframe <html> <body> <iframe src="https://abc.xyz"></iframe> </body> </html>
,
Oct 26 2016
sureshkumari: Please stop adding OS-Linux and OS-Mac when you test on those OSes. The labels are for where the bug *reproduces*.
,
Nov 1 2016
Tested the issue on windows-7 using chrome stable version 54.0.2840.71 with below steps 1.Opened chrome 2.Created abcd.html file using the code provided in comment#3 <html> <body> <iframe src="https://abc.xyz"></iframe> </body> </html> 3.opened abcd.html in chrome browser and clicked on view source, observed abcd.html as Title of the page. 4.opened a new tab and navigated to the following url "data: text/html; charset=utf-8, <iframe src="https://abc.xyz.com"> width="1000" height="900"</iframe>" 5. clicked on page source, observed iframe url as title of the page. In step-5, are you expecting title of a page should be "View source : https://abc.xyz"?? please check the attached screecast for reference and let us know anything missed here to reproduce the issue. Thanks.
,
Nov 7 2016
For me the error occurs when a local page, you visualize the frame source with src = https://abc.xyz.com: - the title of the tab is abcd.html (wrong) Mozilla Firefox correctly shows the title of the tab: - the title of the tab is https://abc.xyz/ (correct)
,
Nov 8 2016
Able to reproduce the issue on windows-7 using chrome stable version 54.0.2840.87 and canary 56.0.2910.0 This is regression issue broken in M54.Please find the bisect information as below Narrow Bisect:: =============== Good :54.0.2795.0 -- (build revision 405182) Bad:: 54.0.2797.0 -- (build revision 405656) ChangeLog: ================ https://chromium.googlesource.com/chromium/src/+log/54c7739244a8870372e88f991579e6ad42872d6a..6f0c1ec23bc1eabe221fccdb4b28b1d96273a705 Possible suspect ================== 6f0c1ec23bc1eabe221fccdb4b28b1d96273a705 Review URL: https://codereview.chromium.org/2086423005 afakhry@ could you please look into this issue if it is related to your change,else please help us in finding the appropriate owner for this issue. Thanks,
,
Nov 14 2016
creis@ This regression was caused by this line [https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_commands.cc?type=cs&q=ViewSource&sq=package:chromium&l=1212] being changed from: // Do not restore title, derive it from the url. last_committed_entry->SetTitle(base::string16()); to: // Do not restore title, derive it from the url. view_source_contents->UpdateTitleForEntry(last_committed_entry, base::string16()); WebContentsImpl::UpdateTitleForEntry() does never set an empty title on the NavigationEntry, instead gets the file name from the URL [https://cs.chromium.org/chromium/src/content/browser/web_contents/web_contents_impl.cc?type=cs&q=WebContentsImpl::UpdateTitleForEntry&sq=package:chromium&l=3871]. I think I will revert back this line if that's ok with you. This doesn't affect the task manager showing the correct title.
,
Nov 14 2016
Updating the summary, assuming I understand the bug correctly. I see the bug you're describing, but I'm hesitant to fix it by reverting that line. Won't that break notifications in all view source cases? Even if the task manger does the right thing for some other reason, it seems like the wrong thing to do. Should we be handling this case differently because we're basically loading a subframe in the cloned main frame's entry? There may be more bugs hiding here.
,
Nov 15 2016
M54 is already in Stable now, may be we could take the fix for M55.Lopping to folks who are involved.
,
Nov 15 2016
A friendly reminder that M55 Stable is launch is coming soon! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch ASAP so it gets enough baking time in Beta (before Stable promotion). Thank you! Also due to Thanksgiving holidays in US, please make sure fix is ready and merged to M55 latest by 5:00 PM PT Friday, 11/18/16 (sooner the better).
,
Nov 15 2016
Re#9: creis@, I dug into it more today. The reason it used to work before was that we cleared the title of the nav_entry which also cleared that |cached_display_title_|. WebContentsImpl::GetTitle() will be called several times returning the wrong (unexpected) title, until we receive a message that we did Navigate, which sets the URL of the nav_entry and clears the |cached_display_title_| again: |#2 0x2b8f44714a17 content::NavigationEntryImpl::SetURL() |#3 0x2b8f447061e5 content::NavigationControllerImpl::RendererDidNavigateToNewPage() |#4 0x2b8f44704fec content::NavigationControllerImpl::RendererDidNavigate() |#5 0x2b8f44732d15 content::NavigatorImpl::DidNavigate() |#6 0x2b8f4473d567 content::RenderFrameHostImpl::OnDidCommitProvisionalLoad() |#7 0x2b8f4473a798 content::RenderFrameHostImpl::OnMessageReceived() |#8 0x2b8f44c5fd12 content::RenderProcessHostImpl::OnMessageReceived() |#9 0x2b8f42fd5178 IPC::ChannelProxy::Context::OnDispatchMessage() After that, WebContentsImpl::GetTitle() returns the expected title by calling NavigationEntryImpl::GetTitleForDisplay() which extracts the title from the virtual_url which we set earlier in ViewSource() to be: "view-source:https://abc.xyz" ----> After my change, by calling WebContentsImpl::UpdateTitleForEntry() we actually set the |title_| member of the nav_entry, which always has the priority, and hence we don't get the title from the virtual_url. The fix I'm proposing is to clear the nav_entry's URL when we set its virtual URL. This way, WebContentsImpl::UpdateTitleForEntry() won't hit this condition: [https://cs.chromium.org/chromium/src/content/browser/web_contents/web_contents_impl.cc?type=cs&q=WebContentsImpl::UpdateTitleForEntry&sq=package:chromium&l=3884] and the title will be actually cleared. The correct URL will be updated when we receive the message. Like this: https://codereview.chromium.org/2501083004 What do you think?
,
Nov 17 2016
Thanks for looking into it, and sorry I missed the bug comment. I left some comments on the review, and we can continue the discussion there. (I'm not sure if that approach is safe, but maybe we can find something.)
,
Nov 18 2016
Given the timing (M55 reaching stable soon), relatively low severity (i.e., cosmetic issue in a rare case, already exists on stable), and uncertainty around how to fix, I think it's ok for this to target M56 instead. That said, let's try to resolve the questions on the CL and find a way forward, so we can get the fix merged into the M56 branch early in the beta.
,
Nov 28 2016
Still we are able to reproduce the issue on Windows 7 with Chrome Stable version-54.0.2840.99 & latest Canary version-57.0.2931.0. afakhry@ Please take a look into this issue. Thank you.
,
Nov 28 2016
Thanks for rechecking. I already have and the fix CL is still under review.
,
Nov 29 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4d116bd4f43359ce057a60c82c2188c93d49a638 commit 4d116bd4f43359ce057a60c82c2188c93d49a638 Author: afakhry <afakhry@chromium.org> Date: Tue Nov 29 17:25:25 2016 Fix View Frame Source for iframe of a file:// URL shows main frame's filename View Frame Source for iframe of a file:// URL used to show the main frame's filename instead of the expected title derived from the iframe's page URL in the form of "view-source:url". This CL fixes this and adds a browser test. BUG= 659040 TEST=browser_tests --gtest_filter=ChromeNavigationBrowserTest.TestViewFrameSource Review-Url: https://codereview.chromium.org/2501083004 Cr-Commit-Position: refs/heads/master@{#435021} [modify] https://crrev.com/4d116bd4f43359ce057a60c82c2188c93d49a638/chrome/browser/chrome_navigation_browsertest.cc [modify] https://crrev.com/4d116bd4f43359ce057a60c82c2188c93d49a638/chrome/browser/ui/browser_commands.cc [modify] https://crrev.com/4d116bd4f43359ce057a60c82c2188c93d49a638/testing/buildbot/filters/site-per-process.browser_tests.filter
,
Nov 29 2016
,
Nov 30 2016
Your change meets the bar and is auto-approved for M56 (branch: 2924)
,
Dec 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e13c83eaae5c04aecfb3a7b75921be2b20b07a67 commit e13c83eaae5c04aecfb3a7b75921be2b20b07a67 Author: Ahmed Fakhry <afakhry@google.com> Date: Thu Dec 01 02:10:48 2016 [Merge to M56] Fix View Frame Source for iframe of a file:// URL shows main frame's filename View Frame Source for iframe of a file:// URL used to show the main frame's filename instead of the expected title derived from the iframe's page URL in the form of "view-source:url". This CL fixes this and adds a browser test. TBR=creis@chromium.org,sky@chromium.org BUG= 659040 TEST=browser_tests --gtest_filter=ChromeNavigationBrowserTest.TestViewFrameSource Review-Url: https://codereview.chromium.org/2501083004 Cr-Commit-Position: refs/heads/master@{#435021} (cherry picked from commit 4d116bd4f43359ce057a60c82c2188c93d49a638) Review URL: https://codereview.chromium.org/2538213003 . Cr-Commit-Position: refs/branch-heads/2924@{#232} Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059} [modify] https://crrev.com/e13c83eaae5c04aecfb3a7b75921be2b20b07a67/chrome/browser/chrome_navigation_browsertest.cc [modify] https://crrev.com/e13c83eaae5c04aecfb3a7b75921be2b20b07a67/chrome/browser/ui/browser_commands.cc [modify] https://crrev.com/e13c83eaae5c04aecfb3a7b75921be2b20b07a67/testing/buildbot/filters/site-per-process.browser_tests.filter
,
Dec 1 2016
,
Dec 2 2016
Tested on windows 7 & 10 using chrome dev M56 #56.0.2924.14 and issue is still reproduced. The view frame source page title is still displayed wrong when compared with other browsers. Attached screenshot for reference. @afakhry--Could you please look into this. Thanks!
,
Dec 2 2016
Assigning to the owner for updates.
,
Dec 2 2016
What is wrong with the title in the screenshot? This is the expected title! view-source:<iframe-url>
,
Dec 2 2016
Agreed-- that's expected behavior, and it matches what was described as expected in the bug report. hodda@: What were you expecting to see?
,
Dec 2 2016
(Sorry, I meant hdodda@ in comment 26)
,
Dec 5 2016
Thanks for the update , as per the comment #25  adding TE-Verified Labels. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by ligim...@chromium.org
, Oct 25 2016