Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2740013008:
Fix tab restore for view-source Chrome extension pages. (Closed)
|
Created:
3 years, 9 months ago by alexmos Modified:
3 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, Avi (use Gerrit), jam, creis+watch_chromium.org, ajwong+watch_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix 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}
Committed: https://chromium.googlesource.com/chromium/src/+/94875b3b4d4a1fc047b99214f30078f3780259f0
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address Charlie's comments #
Total comments: 2
Patch Set 3 : Charlie's nit #
Messages
Total messages: 22 (13 generated)
The CQ bit was checked by alexmos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
alexmos@chromium.org changed reviewers: + creis@chromium.org
Charlie, can you please take a look? This just adds chrome-extension to the list of allowed view-source schemes, which avoids the view-source restore bug. I think we'll still need a check for an empty SiteInstance in ShouldAllowOpenURL, and I'll continue working on that in another CL (still looking a sensible repro). One tricky part here is about collisions between view-source and other URL rewriters - see comment below. Also, I've filed https://crbug.com/700610 to fix the other problems with content::HandleViewSource. https://codereview.chromium.org/2740013008/diff/1/content/browser/browser_url... File content/browser/browser_url_handler_impl.cc (right): https://codereview.chromium.org/2740013008/diff/1/content/browser/browser_url... content/browser/browser_url_handler_impl.cc:100: AddHandlerPair(&HandleViewSource, &ReverseViewSource); The reason for this move is that otherwise, when committing the restored view-source tab, we ended up showing chrome://bookmarks instead of view-source:chrome-extension://eemcgdkfndhakfknompkggombfjjjeno/main.html, with the latter being what you'd get if you just pressed ctrl-U. The reason is that on restore, we go through RendererDidNavigateToExistingPage -> entry->update_virtual_url_with_url() is true -> UpdateVirtualURLToURL() -> BrowserURLHandlerImpl::ReverseURLRewrite, which passes the URL through a list of reverse rewrite handlers. For a normal extension, ReverseViewSource ensures that the visible URL is prepended with view-source:. However, for the internal URL for bookmarks (chrome-extension://eemcgdkfndhakfknompkggombfjjjeno/main.html#1), ExtensionWebUI::HandleChromeURLOverrideReverse is called first and reverses the URL into chrome://bookmarks, and because it returns true, the view-source rewriter never runs, so we never prepend the view-source. I'm not sure this is a good way to fix this, but it makes the restored URL match the behavior with just pressing ctrl-U on chrome://bookmarks. Interestingly, for ctrl-U, we also go through RendererDidNavigateToExistingPage, but there the entry->update_virtual_url_with_url() is false, so we never call the UpdateVirtualURLToURL and don't have this problem. I'm actually a bit confused why this is an existing-page navigation and not a new-page - maybe you know why? Alternatively, we could consider making both rewriters run, and showing view-source:chrome://bookmarks, but then we'd need to also fix UpdateVirtualURLToURL never being called on a plain ctrl-U to keep the two consistent.
Description was changed from
==========
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
==========
to
==========
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
==========
Yes, seems like the right thing to do. LGTM with some thoughts below. Thanks! https://codereview.chromium.org/2740013008/diff/1/chrome/browser/chrome_conte... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2740013008/diff/1/chrome/browser/chrome_conte... chrome/browser/chrome_content_browser_client.cc:1322: additional_schemes->push_back(extensions::kExtensionScheme); I hate to add more plumbing, but I wonder if this line belongs in ChromeContentBrowserClientExtensionsPart. Devlin would probably know. https://codereview.chromium.org/2740013008/diff/1/chrome/browser/tab_contents... File chrome/browser/tab_contents/view_source_browsertest.cc (right): https://codereview.chromium.org/2740013008/diff/1/chrome/browser/tab_contents... chrome/browser/tab_contents/view_source_browsertest.cc:202: bookmarks_tab->GetMainFrame()->GetLastCommittedURL(); Maybe verify that this URL has a chrome-extension:// scheme? (If that changes in the future, this test might become meaningless.) https://codereview.chromium.org/2740013008/diff/1/content/browser/browser_url... File content/browser/browser_url_handler_impl.cc (right): https://codereview.chromium.org/2740013008/diff/1/content/browser/browser_url... content/browser/browser_url_handler_impl.cc:38: // the WebUI schemes defined by the ContentBrowserClient. Let's update this comment as well. https://codereview.chromium.org/2740013008/diff/1/content/browser/browser_url... content/browser/browser_url_handler_impl.cc:100: AddHandlerPair(&HandleViewSource, &ReverseViewSource); On 2017/03/11 02:12:26, alexmos wrote: > The reason for this move is that otherwise, when committing the restored > view-source tab, we ended up showing chrome://bookmarks instead of > view-source:chrome-extension://eemcgdkfndhakfknompkggombfjjjeno/main.html, with > the latter being what you'd get if you just pressed ctrl-U. > > The reason is that on restore, we go through RendererDidNavigateToExistingPage > -> entry->update_virtual_url_with_url() is true -> UpdateVirtualURLToURL() -> > BrowserURLHandlerImpl::ReverseURLRewrite, which passes the URL through a list of > reverse rewrite handlers. For a normal extension, ReverseViewSource ensures > that the visible URL is prepended with view-source:. However, for the internal > URL for bookmarks > (chrome-extension://eemcgdkfndhakfknompkggombfjjjeno/main.html#1), > ExtensionWebUI::HandleChromeURLOverrideReverse is called first and reverses the > URL into chrome://bookmarks, and because it returns true, the view-source > rewriter never runs, so we never prepend the view-source. Wow, this rewriting logic is crazy. It's always bothered me that it's an arbitrary transform that is treated like it's reversible, so I'm not surprised to see outcomes like this... It's fairly low on my list of overhauls to take on, though. > > I'm not sure this is a good way to fix this, but it makes the restored URL match > the behavior with just pressing ctrl-U on chrome://bookmarks. Interestingly, > for ctrl-U, we also go through RendererDidNavigateToExistingPage, but there the > entry->update_virtual_url_with_url() is false, so we never call the > UpdateVirtualURLToURL and don't have this problem. I'm actually a bit confused > why this is an existing-page navigation and not a new-page - maybe you know why? I bet that's because Ctrl+U clones the tab and modifies the "last committed entry" of the cloned tab to put it into view-source mode. In that sense, it's treated more like a history navigation. > > Alternatively, we could consider making both rewriters run, and showing > view-source:chrome://bookmarks, but then we'd need to also fix > UpdateVirtualURLToURL never being called on a plain ctrl-U to keep the two > consistent. Interesting-- I think it would be a nice outcome to see view-source:chrome://bookmarks, since the extension URL isn't really supposed to get exposed like this. Then again, I'm not sure what the implications of the UpdateVirtualURLToURL change are. I'd say switch it over if it's easy, but don't worry about it if it seems risky. https://codereview.chromium.org/2740013008/diff/1/content/public/browser/cont... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2740013008/diff/1/content/public/browser/cont... content/public/browser/content_browser_client.h:225: std::vector<std::string>* additional_schemes) {} I wonder if there's a risk that this breaks view-source in other content embedders, like Opera? They won't know to implement this right away, and suddenly their WebUI schemes won't allow view-source. It'd be a bit inconsistent with other methods in here, but we could make the default implementation return GetAdditionalWebUISchemes (in the .cc file, plus documented here) to avoid the issue.
The CQ bit was checked by alexmos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for reviewing! https://codereview.chromium.org/2740013008/diff/1/chrome/browser/chrome_conte... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2740013008/diff/1/chrome/browser/chrome_conte... chrome/browser/chrome_content_browser_client.cc:1322: additional_schemes->push_back(extensions::kExtensionScheme); On 2017/03/15 15:03:31, Charlie Reis (slow) wrote: > I hate to add more plumbing, but I wonder if this line belongs in > ChromeContentBrowserClientExtensionsPart. Devlin would probably know. Double-checked this with Devlin, and he's ok with leaving this as-is, given all the other usage of kExtensionScheme. Moving more/all of it out into ChromeContentBrowserClientExtensionsPart can be done separately in the future. https://codereview.chromium.org/2740013008/diff/1/chrome/browser/tab_contents... File chrome/browser/tab_contents/view_source_browsertest.cc (right): https://codereview.chromium.org/2740013008/diff/1/chrome/browser/tab_contents... chrome/browser/tab_contents/view_source_browsertest.cc:202: bookmarks_tab->GetMainFrame()->GetLastCommittedURL(); On 2017/03/15 15:03:31, Charlie Reis (slow) wrote: > Maybe verify that this URL has a chrome-extension:// scheme? (If that changes > in the future, this test might become meaningless.) Done. https://codereview.chromium.org/2740013008/diff/1/content/browser/browser_url... File content/browser/browser_url_handler_impl.cc (right): https://codereview.chromium.org/2740013008/diff/1/content/browser/browser_url... content/browser/browser_url_handler_impl.cc:38: // the WebUI schemes defined by the ContentBrowserClient. On 2017/03/15 15:03:31, Charlie Reis (slow) wrote: > Let's update this comment as well. Done. https://codereview.chromium.org/2740013008/diff/1/content/browser/browser_url... content/browser/browser_url_handler_impl.cc:100: AddHandlerPair(&HandleViewSource, &ReverseViewSource); On 2017/03/15 15:03:31, Charlie Reis (slow) wrote: > On 2017/03/11 02:12:26, alexmos wrote: > > The reason for this move is that otherwise, when committing the restored > > view-source tab, we ended up showing chrome://bookmarks instead of > > view-source:chrome-extension://eemcgdkfndhakfknompkggombfjjjeno/main.html, > with > > the latter being what you'd get if you just pressed ctrl-U. > > > > The reason is that on restore, we go through RendererDidNavigateToExistingPage > > -> entry->update_virtual_url_with_url() is true -> UpdateVirtualURLToURL() -> > > BrowserURLHandlerImpl::ReverseURLRewrite, which passes the URL through a list > of > > reverse rewrite handlers. For a normal extension, ReverseViewSource ensures > > that the visible URL is prepended with view-source:. However, for the > internal > > URL for bookmarks > > (chrome-extension://eemcgdkfndhakfknompkggombfjjjeno/main.html#1), > > ExtensionWebUI::HandleChromeURLOverrideReverse is called first and reverses > the > > URL into chrome://bookmarks, and because it returns true, the view-source > > rewriter never runs, so we never prepend the view-source. > > Wow, this rewriting logic is crazy. It's always bothered me that it's an > arbitrary transform that is treated like it's reversible, so I'm not surprised > to see outcomes like this... It's fairly low on my list of overhauls to take > on, though. > > > > > I'm not sure this is a good way to fix this, but it makes the restored URL > match > > the behavior with just pressing ctrl-U on chrome://bookmarks. Interestingly, > > for ctrl-U, we also go through RendererDidNavigateToExistingPage, but there > the > > entry->update_virtual_url_with_url() is false, so we never call the > > UpdateVirtualURLToURL and don't have this problem. I'm actually a bit > confused > > why this is an existing-page navigation and not a new-page - maybe you know > why? > > I bet that's because Ctrl+U clones the tab and modifies the "last committed > entry" of the cloned tab to put it into view-source mode. In that sense, it's > treated more like a history navigation. > > > > > Alternatively, we could consider making both rewriters run, and showing > > view-source:chrome://bookmarks, but then we'd need to also fix > > UpdateVirtualURLToURL never being called on a plain ctrl-U to keep the two > > consistent. > > Interesting-- I think it would be a nice outcome to see > view-source:chrome://bookmarks, since the extension URL isn't really supposed to > get exposed like this. Then again, I'm not sure what the implications of the > UpdateVirtualURLToURL change are. I'd say switch it over if it's easy, but > don't worry about it if it seems risky. > Fixing the default ctrl-U path seems a bit non-trivial, so I'll put it on my TODO list and try to find time to follow up in a separate CL. https://codereview.chromium.org/2740013008/diff/1/content/public/browser/cont... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2740013008/diff/1/content/public/browser/cont... content/public/browser/content_browser_client.h:225: std::vector<std::string>* additional_schemes) {} On 2017/03/15 15:03:31, Charlie Reis (slow) wrote: > I wonder if there's a risk that this breaks view-source in other content > embedders, like Opera? They won't know to implement this right away, and > suddenly their WebUI schemes won't allow view-source. > > It'd be a bit inconsistent with other methods in here, but we could make the > default implementation return GetAdditionalWebUISchemes (in the .cc file, plus > documented here) to avoid the issue. Good idea. Done.
alexmos@chromium.org changed reviewers: + avi@chromium.org
Avi, can you please review chrome/browser/tab_contents/view_source_browsertest.cc for OWNERS?
lgtm
Thanks! LGTM with nit. https://codereview.chromium.org/2740013008/diff/1/content/browser/browser_url... File content/browser/browser_url_handler_impl.cc (right): https://codereview.chromium.org/2740013008/diff/1/content/browser/browser_url... content/browser/browser_url_handler_impl.cc:100: AddHandlerPair(&HandleViewSource, &ReverseViewSource); On 2017/03/16 20:07:30, alexmos wrote: > On 2017/03/15 15:03:31, Charlie Reis (slow) wrote: > > On 2017/03/11 02:12:26, alexmos wrote: > > > The reason for this move is that otherwise, when committing the restored > > > view-source tab, we ended up showing chrome://bookmarks instead of > > > view-source:chrome-extension://eemcgdkfndhakfknompkggombfjjjeno/main.html, > > with > > > the latter being what you'd get if you just pressed ctrl-U. > > > > > > The reason is that on restore, we go through > RendererDidNavigateToExistingPage > > > -> entry->update_virtual_url_with_url() is true -> UpdateVirtualURLToURL() > -> > > > BrowserURLHandlerImpl::ReverseURLRewrite, which passes the URL through a > list > > of > > > reverse rewrite handlers. For a normal extension, ReverseViewSource ensures > > > that the visible URL is prepended with view-source:. However, for the > > internal > > > URL for bookmarks > > > (chrome-extension://eemcgdkfndhakfknompkggombfjjjeno/main.html#1), > > > ExtensionWebUI::HandleChromeURLOverrideReverse is called first and reverses > > the > > > URL into chrome://bookmarks, and because it returns true, the view-source > > > rewriter never runs, so we never prepend the view-source. > > > > Wow, this rewriting logic is crazy. It's always bothered me that it's an > > arbitrary transform that is treated like it's reversible, so I'm not surprised > > to see outcomes like this... It's fairly low on my list of overhauls to take > > on, though. > > > > > > > > I'm not sure this is a good way to fix this, but it makes the restored URL > > match > > > the behavior with just pressing ctrl-U on chrome://bookmarks. > Interestingly, > > > for ctrl-U, we also go through RendererDidNavigateToExistingPage, but there > > the > > > entry->update_virtual_url_with_url() is false, so we never call the > > > UpdateVirtualURLToURL and don't have this problem. I'm actually a bit > > confused > > > why this is an existing-page navigation and not a new-page - maybe you know > > why? > > > > I bet that's because Ctrl+U clones the tab and modifies the "last committed > > entry" of the cloned tab to put it into view-source mode. In that sense, it's > > treated more like a history navigation. > > > > > > > > Alternatively, we could consider making both rewriters run, and showing > > > view-source:chrome://bookmarks, but then we'd need to also fix > > > UpdateVirtualURLToURL never being called on a plain ctrl-U to keep the two > > > consistent. > > > > Interesting-- I think it would be a nice outcome to see > > view-source:chrome://bookmarks, since the extension URL isn't really supposed > to > > get exposed like this. Then again, I'm not sure what the implications of the > > UpdateVirtualURLToURL change are. I'd say switch it over if it's easy, but > > don't worry about it if it seems risky. > > > > Fixing the default ctrl-U path seems a bit non-trivial, so I'll put it on my > TODO list and try to find time to follow up in a separate CL. Acknowledged. https://codereview.chromium.org/2740013008/diff/20001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2740013008/diff/20001/content/public/browser/... content/public/browser/content_browser_client.h:223: // Returns a list of additional schemes allowed for view-source. Don't forget to document what it defaults to, so implementors know to include that behavior in their overrides.
Thanks! https://codereview.chromium.org/2740013008/diff/20001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2740013008/diff/20001/content/public/browser/... content/public/browser/content_browser_client.h:223: // Returns a list of additional schemes allowed for view-source. On 2017/03/16 20:26:05, Charlie Reis (slow) wrote: > Don't forget to document what it defaults to, so implementors know to include > that behavior in their overrides. Done.
The CQ bit was checked by alexmos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/2740013008/#ps40001 (title: "Charlie's nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489696266280540,
"parent_rev": "36000eb193f6d73843b190d01de049fdb983647b", "commit_rev":
"94875b3b4d4a1fc047b99214f30078f3780259f0"}
Message was sent while issue was closed.
Description was changed from
==========
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
==========
to
==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/94875b3b4d4a1fc047b99214f300...
==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/94875b3b4d4a1fc047b99214f300... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
