Fix tab restore for view-source Chrome extension pages.

content/browser/browser_url_handler_impl.cc

Index: content/browser/browser_url_handler_impl.cc
diff --git a/content/browser/browser_url_handler_impl.cc b/content/browser/browser_url_handler_impl.cc
index a79b89dda4c26494bbacbabd92cc3fa70ec0e578..a81a713ec8ae26246125092a15461a6b1d256cc3 100644
--- a/content/browser/browser_url_handler_impl.cc
+++ b/content/browser/browser_url_handler_impl.cc
@@ -39,7 +39,7 @@ static bool HandleViewSource(GURL* url, BrowserContext* browser_context) {
     std::vector<std::string> all_allowed_sub_schemes;
     for (size_t i = 0; i < arraysize(default_allowed_sub_schemes); ++i)
       all_allowed_sub_schemes.push_back(default_allowed_sub_schemes[i]);
-    GetContentClient()->browser()->GetAdditionalWebUISchemes(
+    GetContentClient()->browser()->GetAdditionalViewSourceSchemes(
         &all_allowed_sub_schemes);
 
     bool is_sub_scheme_allowed = false;
@@ -95,10 +95,11 @@ BrowserURLHandlerImpl::BrowserURLHandlerImpl() :
     fixup_handler_(nullptr) {
   AddHandlerPair(&DebugURLHandler, BrowserURLHandlerImpl::null_handler());
 
-  GetContentClient()->browser()->BrowserURLHandlerCreated(this);
-
-  // view-source:
+  // view-source: should take precedence over other rewriters, so it's
+  // important to add it before calling up to the content client.
   AddHandlerPair(&HandleViewSource, &ReverseViewSource);

[alexmos, 2017/03/11 02:12:26]

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.

[Charlie Reis, 2017/03/15 15:03:31]

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 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.
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.

[alexmos, 2017/03/16 20:07:30]

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.

[Charlie Reis, 2017/03/16 20:26:05]

Acknowledged.

+
+  GetContentClient()->browser()->BrowserURLHandlerCreated(this);
 }
 
 BrowserURLHandlerImpl::~BrowserURLHandlerImpl() {