Remove about:srcdoc url conversion.

content/browser/renderer_host/render_process_host_impl.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Represents the browser side of the browser <--> renderer communication
 // channel. There will be one RenderProcessHost per renderer process.
 
 #include "content/browser/renderer_host/render_process_host_impl.h"
 
 #include <algorithm>
@@ skipping 2339 matching lines @@
     // This is because the browser treats navigation to an empty GURL as a
     // navigation to the home page. This is often a privileged page
     // (chrome://newtab/) which is exactly what we don't want.
     *url = GURL(url::kAboutBlankURL);
     return;
   }
 
   if (url->SchemeIs(url::kAboutScheme)) {
     // The renderer treats all URLs in the about: scheme as being about:blank.
     // Canonicalize about: URLs to about:blank.
-    *url = GURL(url::kAboutBlankURL);
+    // An exception is made for about:srcdoc.
+    if (*url != kAboutSrcDocURL)
+      *url = GURL(url::kAboutBlankURL);
+    return;

[Charlie Reis, 2016/11/16 18:20:22]

Was this return added for ChildPolicySecurityPolicyImpl::CanRequestURL rejecting
about:srcdoc?  I think that brings up a good point.

First, we should update CanRequestURL to allow about:srcdoc the same way it
allows about:blank.  Otherwise its other callers might get the wrong answer
(e.g., if the srcdoc page tries to make an XmlHttpRequest).

Second, Nick points out that such a change would make this whole block
unnecessary.  CanRequestURL will return true for about:blank and about:srcdoc,
and false for all other about: URLs, causing us to rewrite them to about:blank
on line 2371 below.  That seems like a nice cleanup to me, keeping the logic in
fewer places.

[arthursonzogni, 2016/11/17 17:04:58]

Yes it was.
To be honest, I initially tried what you suggest when I saw that the "return"
was needed here.
The problem is that we can't just remove this block of code because comparison
to about:blank made in CanRequestURL is not case-sensitive. Mixing not
case-sensitive and case-sensitive test are not a good idea. Comparison to
about:blank are case-sensitive everywhere except here.

For instance, if the user click on <a href="about:BLANK"></a>, it will cause a
browser crash because of this inconsistency. 

FYI the specification doesn't says that the about:xxxx URL are not
case-sensitive. So:
GURL("about:blank") != GURL(about:BLANK)
even if hostname are not case-sensitive.
GURL("www.chromium.org/xX") == GURL("www.CHROMIUM.org/xX")

I think we could make CanRequestURL and CanCommitURL case-sensitive. What do you
think?

[Charlie Reis, 2016/11/22 01:01:20]

Yes, I think that makes sense, assuming we don't expect the renderer to allow an
about:BlAnK commit, etc.  That would make it consistent with all the other
checks.

   }
 
   if (!policy->CanRequestURL(rph->GetID(), *url)) {
     // If this renderer is not permitted to request this URL, we invalidate the
     // URL.  This prevents us from storing the blocked URL and becoming confused
     // later.
     VLOG(1) << "Blocked URL " << url->spec();
     *url = GURL(url::kAboutBlankURL);
   }
 }
@@ skipping 614 matching lines @@
   LOG(ERROR) << "Terminating render process for bad Mojo message: " << error;
 
   // The ReceivedBadMessage call below will trigger a DumpWithoutCrashing. Alias
   // enough information here so that we can determine what the bad message was.
   base::debug::Alias(&error);
   bad_message::ReceivedBadMessage(render_process_id,
                                   bad_message::RPH_MOJO_PROCESS_ERROR);
 }
 
 }  // namespace content