New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 699428 link

Starred by 2 users

Regression : Blank source page appears after clicking on 'Reopen closed tab' option.

Reported by yfulgaon...@etouch.net, Mar 8 2017

Issue description

Chrome 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
 
Actual_Result.mp4
1005 KB View Download
Expected_Result.mp4
1021 KB View Download
Cc: rbasuvula@chromium.org
Labels: hasbisect-per-revision ReleaseBlock-Stable
Owner: nasko@chromium.org
Status: Assigned (was: Unconfirmed)
Using the per-revision bisect providing the bisect results,
Good build:58.0.3025.0 (Revision: 453134).
Bad build:58.0.3026.0  (Revision: 453454).

You are probably looking for a change made after 453317 (known good), but no later than 453318 (first known bad).

CHANGE-LOG URL:
---------------
https://chromium.googlesource.com/chromium/src/+log/09ac3a21d0b908ca26d0898f58ea77be7e9462d2..9e1897b5c7dab9c190fc3c919a1c4662af63b3e6

From the CL above, assigning the issue to the concern owner

@nasko : Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner.

Review-Url: https://codereview.chromium.org/2715363002
Note :Able to reproduce the issue in Win 10.0,Ubuntu 14.04 & Mac 10.12.3 and Able to reproduce in latest Canary #58.0.3033.0
Adding Release Block-Stable for this issue.Please remove if not the case.

Comment 2 by creis@chromium.org, Mar 8 2017

Cc: nasko@chromium.org creis@chromium.org
Components: Internals>Sandbox>SiteIsolation UI>Browser>Navigation
Owner: alex...@chromium.org
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.
Cc: rdevlin....@chromium.org
Status: Started (was: Assigned)
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.
Correction in #3: HasSite returns *true*, not false (given that the SiteInstance is not empty).
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.

Comment 6 by creis@chromium.org, 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.
Project Member

Comment 7 by bugdroid1@chromium.org, 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

Labels: Needs-Feedback
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!!
699428.mp4
477 KB View Download
Cc: sureshkumari@chromium.org
Labels: -Needs-Feedback TE-Verified-M59 TE-Verified-59.0.3044.0
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!!
Win-699428.mp4
586 KB View Download
Labels: Merge-Request-58
Requesting to merge this to M58.  The fix has been verified on canary per #10 and should be low-risk enough to merge.
Status: Fixed (was: Started)
Project Member

Comment 13 by sheriffbot@chromium.org, Mar 17 2017

Labels: -Merge-Request-58 Hotlist-Merge-Approved Merge-Approved-58
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
Labels: TE-verified-59.0.3046.0
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.
Project Member

Comment 15 by bugdroid1@chromium.org, Mar 20 2017

Labels: -merge-approved-58 merge-merged-3029
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

Labels: TE-Verified-M58 TE-Verified-58.0.3029.33
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!!
699428.mp4
624 KB View Download

Sign in to add a comment