Regression : Blank source page appears after clicking on 'Reopen closed tab' option.
Reported by
yfulgaon...@etouch.net,
Mar 8 2017
|
||||||||||||
Issue descriptionChrome Version : 59.0.3034.0 (Official Build) 3164ef05aedfd208cf1d9e43558d7e8d59b14a2c-refs/heads/master@{#455336} 32/64 bit OS : Windows (7,8,10), Linux (14.04 LTS), Mac (10.11.6, 10.12.1, 10.12) What steps will reproduce the problem? 1. Launch chrome, navigate to chrome://bookmarks and press 'Ctrl' + 'U' to view page source. 2. Close the page source code tab, right click on 'Bookmark Manager' tab and select 'Reopen closed tab' option. 3. Observe. Actual : Blank source page appears after clicking on 'Reopen closed tab' option. Expected : Source page should load properly after clicking on 'Reopen closed tab' option. This is a regression issue broken in ‘M-58’, below is the Manual Regression range and will soon update other info. Good build : 58.0.3025.0 Bad build : 58.0.3026.0
,
Mar 8 2017
Thanks for the report! This appears to affect reopening any view-source:chrome-extension:// URL when running in --isolate-extensions mode. That mode is now enabled at 100% on M57 and M56 as well, so the bug also applies there. It does not affect other view source cases (e.g., web pages or chrome:// URLs). When repro'ing it, I saw crash ID f280044920000000, which matches issue 698709. That appears to be a DumpWithoutCrashing that indicates the navigation was blocked in ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL, because blob and filesystem URLs aren't considered web-accessible. That restriction dates back to a security bug ( issue 656752 ) that alexmos@ fixed in r429532. Unclear why view-source would be using a blob or filesystem URL, but we can definitely look closer at this point. Alex seems to have a lead on it at issue 696034.
,
Mar 9 2017
Quick update: the DumpWithoutCrashing that's triggered here is not the one for blob/filesystem URLs, but rather the one below it in ShouldAllowOpenURL, which means that the site_url isn't HTTP/HTTPS/chrome-extension, and the navigation is to a non-web-accessible chrome-extension resource. Specifically, the site_url here is "about:". Note that it's not empty, and HasSite on the passed-in SiteInstance returns false. This is surprising, and digging a bit further, I found that when the tab restore starts, as part of reading the serialized navigation data in ContentSerializedNavigationBuilder::ToNavigationEntry, we pass the URL through content::HandleViewSource, which does *not* include "chrome-extension" as a valid view-source URL, rewriting the destination URL to "about:blank" and returning false. Hence, when we initiate tab restore, the navigation goes to "about:blank" which results in creating an "about:" SiteInstance. Then we go down the transfer path for view-source:chrome-extension://blah/, which consults ShouldAllowOpenURL from RequestTransferURL, which blocks it. I'm not yet sure how the "about:blank" navigation actually ends up redirecting to view-source:chrome-extension://blah/. The changes that Devlin made as part of issue 659798 to remove WebUI from extensions might be related to how "chrome-extension" ended up not in this list. Adding the extension scheme to the list in content::HandleViewSource mostly fixes this bug and gets the restored view-source page to load, though the "view-source" prefix is missing in the omnibox URL. I'm looking into that.
,
Mar 9 2017
Correction in #3: HasSite returns *true*, not false (given that the SiteInstance is not empty).
,
Mar 9 2017
A couple more interesting details. Regarding how an about:blank navigation still ends up redirecting to view-source:chrome-extension://blahblah/: the tab restore is considered a history navigation, so in the renderer, RenderFrameImpl::NavigateInternal generates the navigation request using WebLocalFrameImpl::requestFromHistoryItem(), which reuses the URL that was in the PageState (which wasn't rewritten by HandleViewSource) and ignores the about:blank in commont_params.url. Hence, the request that goes out on the network stack is still view-source:chrome-extension://blahblah/. I suspect any history navigation will bypass the HandleViewSource's blocks this way. That helps understand why this still works without --isolate-extensions: content::HandleViewSource still blocks the view-source navigation and rewrites the destination to about:blank, but then the history navigation above happens to the correct view-source URL, and then we decide that a transfer is not needed, bypassing ShouldAllowOpenURL. So without --isolate-extensions, we happily load the restored view-source chrome-extension URL inside the "about:" SiteInstance.
,
Mar 9 2017
Good find on the PageState/HistoryItem. Sigh. I have plans to de-dupe the fields in NavigationEntry and PageState (such as URL) so that they don't diverge like this. That'll happen as we change PageState to FrameState.
,
Mar 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/94875b3b4d4a1fc047b99214f30078f3780259f0 commit 94875b3b4d4a1fc047b99214f30078f3780259f0 Author: alexmos <alexmos@chromium.org> Date: Thu Mar 16 22:19:01 2017 Fix tab restore for view-source Chrome extension pages. Previously, for non-web-accessible pages, this was blocked by the check in ShouldAllowOpenURL, and generated a DumpWithoutCrashing report because the source SiteInstance ("about:") was not HTTP/HTTPS/extension and the target URL was not a WAR. The source SiteInstance was wrong because content::HandleViewSource disallowed view-source navigations to the chrome-extension scheme and overwrote the destination URL to about:blank. See full analysis in issue 699428. The fix adds chrome-extension to the list of schemes allowed for view-source. It also fixes an issue where the restored view-source tab's visible URL ended up at chrome://bookmarks, rather than view-source:chrome-extension://<bookmark_extension_id>/. BUG= 699428 ,698709,696034,700610 Review-Url: https://codereview.chromium.org/2740013008 Cr-Commit-Position: refs/heads/master@{#457582} [modify] https://crrev.com/94875b3b4d4a1fc047b99214f30078f3780259f0/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/94875b3b4d4a1fc047b99214f30078f3780259f0/chrome/browser/chrome_content_browser_client.h [modify] https://crrev.com/94875b3b4d4a1fc047b99214f30078f3780259f0/chrome/browser/tab_contents/view_source_browsertest.cc [modify] https://crrev.com/94875b3b4d4a1fc047b99214f30078f3780259f0/content/browser/browser_url_handler_impl.cc [modify] https://crrev.com/94875b3b4d4a1fc047b99214f30078f3780259f0/content/public/browser/content_browser_client.cc [modify] https://crrev.com/94875b3b4d4a1fc047b99214f30078f3780259f0/content/public/browser/content_browser_client.h
,
Mar 17 2017
Tested the issue on Windows-7,Mac-10.12.3 and Linux Ubuntu-14.04 using chrome version#59.0.3043.0 with the steps mentioned in comment#0. Observed that the blank source page appears after clicking on 'Reopen closed tab' option. Seems that the issue has not been fixed yet. Please find the attached screen cast for the same. Thanks!!
,
Mar 17 2017
,
Mar 17 2017
Looks like fix missed # 59.0.3043.0 and is available in chrome version # 59.0.3044.0(Revision-457648). Observed that the fix is working as expected on Windows-7,Mac-10.12.3 and Linux Ubuntu-14.04 using chrome version#59.0.3044.0. Hence adding TE-Verified labels. Please find the attached screen cast for the same. Thanks!!
,
Mar 17 2017
Requesting to merge this to M58. The fix has been verified on canary per #10 and should be low-risk enough to merge.
,
Mar 17 2017
,
Mar 17 2017
Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 20 2017
Retested above issue on Windows(7,8,8.1,10),Mac(10.11.6, 10.12.1, 10.12) and Linux(14.04 LTS) OS using latest canary #59.0.3046.0 and issue has been fixed. It is working as intended. Thank you.
,
Mar 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7495c6fd9e5f9823396b508f4145450589853391 commit 7495c6fd9e5f9823396b508f4145450589853391 Author: Alex Moshchuk <alexmos@chromium.org> Date: Mon Mar 20 17:10:18 2017 Fix tab restore for view-source Chrome extension pages (Merge to M58). Previously, for non-web-accessible pages, this was blocked by the check in ShouldAllowOpenURL, and generated a DumpWithoutCrashing report because the source SiteInstance ("about:") was not HTTP/HTTPS/extension and the target URL was not a WAR. The source SiteInstance was wrong because content::HandleViewSource disallowed view-source navigations to the chrome-extension scheme and overwrote the destination URL to about:blank. See full analysis in issue 699428. The fix adds chrome-extension to the list of schemes allowed for view-source. It also fixes an issue where the restored view-source tab's visible URL ended up at chrome://bookmarks, rather than view-source:chrome-extension://<bookmark_extension_id>/. BUG= 699428 ,698709,696034,700610 Review-Url: https://codereview.chromium.org/2740013008 Cr-Commit-Position: refs/heads/master@{#457582} (cherry picked from commit 94875b3b4d4a1fc047b99214f30078f3780259f0) Review-Url: https://codereview.chromium.org/2760023002 . Cr-Commit-Position: refs/branch-heads/3029@{#301} Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471} [modify] https://crrev.com/7495c6fd9e5f9823396b508f4145450589853391/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/7495c6fd9e5f9823396b508f4145450589853391/chrome/browser/chrome_content_browser_client.h [modify] https://crrev.com/7495c6fd9e5f9823396b508f4145450589853391/chrome/browser/tab_contents/view_source_browsertest.cc [modify] https://crrev.com/7495c6fd9e5f9823396b508f4145450589853391/content/browser/browser_url_handler_impl.cc [modify] https://crrev.com/7495c6fd9e5f9823396b508f4145450589853391/content/public/browser/content_browser_client.cc [modify] https://crrev.com/7495c6fd9e5f9823396b508f4145450589853391/content/public/browser/content_browser_client.h
,
Mar 22 2017
Verified the issue on Mac os 10.12.3 , ubuntu 14.04 and windows 7 using chrome M58 #58.0.3029.33 and issue is fixed. Hence adding TE-Verified labels. Please find the attached screen cast for reference. Thanks!! |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by rbasuvula@chromium.org
, Mar 8 2017Labels: hasbisect-per-revision ReleaseBlock-Stable
Owner: nasko@chromium.org
Status: Assigned (was: Unconfirmed)