UMA data collector for cross-site documents(XSD)

UMA data collector for cross-site documents(XSD)

Intercept cross-site documents and apply a couple of blocking filters to measure how our cross-site document blocking policy affects the renderer behavior. This doesn't actually block anything, but just records UMA data about how these filters work. It does three things: 1) whitelists legitimate XSDs (responses with whitelisted mime types or with valid CORS headers) 2) applies an appropriate content sniffing algorithm depending on the mime type of the response, 3) if it is sniffed as a blocked document, reports its status code and the context (img, script, etc) where the request is originally issued to measure the compatibility impact of the blocking.

BUG=268640
Related Doc: https://docs.google.com/a/google.com/document/d/1nB3GruRqQmtA7OPZZAhWOsZDvfWpYpKQXE3cxGxTVfs/edit

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219383

[dsjang, 2013-08-06 16:48:59]

Disruptive formatting in weburlloader_impl.cc has been reverted.

[nasko, 2013-08-06 17:29:27]

A bunch of comments. I will review the weburlloader_impl.cc in the properly
formatted way later.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
File webkit/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:30: std::map<unsigned,
WebURLRequest::TargetType>
Why are these needed in this file? They are already in the header as private
variables.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:56: 
nit: no need for empty line here.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:100: LOG(INFO) <<
"SiteIsolationPolicy.CorsisSafe:";
nit: CorsIsSafe

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:107: //
XSDP.All.NNav.NSafeScheme.NSMIMEType.NSCORS.%MIMECODE from now
What is the meaning of this string? Putting a comment that outlines how the UMA
stats are organized and their meaning will be very very useful. Without that,
I'm somewhat lost in subsequent code.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:132: if
(url_responsedata_map_.count(response_url_str) == 0)
Is this a valid case? When will we have seen a request, but receive no response?

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:264: StringToLowerASCII(&mime_type);
The mime_type string is UTF8, yet you are using ASCII function. This doesn't
seem right.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:271: if (!strcmp(document_mime_types[i],
mime_type.data())) {
This is unsafe comparison, you should be bounding memory access by the size of
the inputs.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:276: if (i == 0) {
Using constants like these seems unclean to me. Is there a better way to
structure this?

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:289: bool
SiteIsolationPolicy::IsSafeScheme(GURL& url) {
What about blob URLs? Are those safe too?

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:302:
net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
Is there a reason we are deviating from how SiteInstance does same site checks?
The code there does exclude private registries.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:311: bool
SiteIsolationPolicy::IsFrameNotCommitted(WebKit::WebFrame* frame) {
This name is a bit confusing, can you put a description for it?

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:327: // Strip quotes off from it. This is
non-standard practice, but many
Don't we have code that parses the header that we can reuse? I'm always worried
about duplicating parsing logic.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:344: if (access_control_origin == "*")
You talk about cookie-tagged requests, but there is no such check. Am I missing
something?

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:349: // correct. If this doesn't start
with a scheme(http://, https://),
This sounds scary. Is this correct?

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:426: state = 1;
Please use symbolic names or describe what those numbers mean.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:502: }
Leave an empty line and put a // namespace comment.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
File webkit/child/site_isolation_policy.h (right):

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.h:73: static void DidReceiveData(const char*
payload,
Why is this method without a comment? It seems out of place when the rest have
very detailed comments.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.h:90: // Returns if this response's scheme is
not security relevant. For
nit: s/if/whether/ here and all the following comments of the same form.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.h:91: // example, this returns true for data:
Really? It returns a type according to the declaration, not a boolean.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.h:127: static std::map<unsigned,
WebURLRequest::TargetType> id_target_map_;
Be explicit in type declaration. unsigned what?

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.h:135: static std::map<unsigned, std::string>
id_url_map_;
Same here, be explicit.

[dsjang, 2013-08-07 00:19:06]

Nasko's	feedback has been applied. Scheme whitelisting is switched to
blacklisting(http,https).

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
File webkit/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:30: std::map<unsigned,
WebURLRequest::TargetType>
On 2013/08/06 17:29:27, nasko wrote:
> Why are these needed in this file? They are already in the header as private
> variables.

Done.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:56: 
On 2013/08/06 17:29:27, nasko wrote:
> nit: no need for empty line here.

Done.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:100: LOG(INFO) <<
"SiteIsolationPolicy.CorsisSafe:";
On 2013/08/06 17:29:27, nasko wrote:
> nit: CorsIsSafe

Done.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:107: //
XSDP.All.NNav.NSafeScheme.NSMIMEType.NSCORS.%MIMECODE from now
On 2013/08/06 17:29:27, nasko wrote:
> What is the meaning of this string? Putting a comment that outlines how the
UMA
> stats are organized and their meaning will be very very useful. Without that,
> I'm somewhat lost in subsequent code.

Done.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:132: if
(url_responsedata_map_.count(response_url_str) == 0)
url_responsedata_map_ only maintains url for cross-site responses detected by
DidReceiveResponse(). Therefore, if the url for received data does not exist in
url_responsedata_map_, it means that this data is not for a cross-site response.
So we skip the check.

On 2013/08/06 17:29:27, nasko wrote:
> Is this a valid case? When will we have seen a request, but receive no
response?

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:264: StringToLowerASCII(&mime_type);
I found that all the crawled mime types are originally lowercase on the top 50k
sites. And existing mime type handling code doesn't do lowercasing. I'll remove
the StringToLowerASCII call entirely here.

On 2013/08/06 17:29:27, nasko wrote:
> The mime_type string is UTF8, yet you are using ASCII function. This doesn't
> seem right.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:271: if (!strcmp(document_mime_types[i],
mime_type.data())) {
Switched to std::string::operator==().

On 2013/08/06 17:29:27, nasko wrote:
> This is unsafe comparison, you should be bounding memory access by the size of
> the inputs.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:276: if (i == 0) {
On 2013/08/06 17:29:27, nasko wrote:
> Using constants like these seems unclean to me. Is there a better way to
> structure this?

Done.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:289: bool
SiteIsolationPolicy::IsSafeScheme(GURL& url) {
Switched to blacklisting from whitelisting.

On 2013/08/06 17:29:27, nasko wrote:
> What about blob URLs? Are those safe too?

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:302:
net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
I thought allowing private registries here enables sites to use a finer
definition of sites. Knowing SiteIsolation is using only the public registry,
I'll switch to the public one.

On 2013/08/06 17:29:27, nasko wrote:
> Is there a reason we are deviating from how SiteInstance does same site
checks?
> The code there does exclude private registries.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:311: bool
SiteIsolationPolicy::IsFrameNotCommitted(WebKit::WebFrame* frame) {
On 2013/08/06 17:29:27, nasko wrote:
> This name is a bit confusing, can you put a description for it?

Done.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:327: // Strip quotes off from it. This is
non-standard practice, but many
On 2013/08/06 17:29:27, nasko wrote:
> Don't we have code that parses the header that we can reuse? I'm always
worried
> about duplicating parsing logic.

Done.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:344: if (access_control_origin == "*")
Yes, it has to be incorporated here in the future, but we allow for them to pass
here even thought they can be blocked by the actual CORS policy.

On 2013/08/06 17:29:27, nasko wrote:
> You talk about cookie-tagged requests, but there is no such check. Am I
missing
> something?

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:349: // correct. If this doesn't start
with a scheme(http://, https://),
This was actually scarily wrong :-). I found out the Blink's CORS implemtation
doesn't allow it. This code will be gone too.

On 2013/08/06 17:29:27, nasko wrote:
> This sounds scary. Is this correct?

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.cc:426: state = 1;
On 2013/08/06 17:29:27, nasko wrote:
> Please use symbolic names or describe what those numbers mean.

Done.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
File webkit/child/site_isolation_policy.h (right):

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.h:73: static void DidReceiveData(const char*
payload,
On 2013/08/06 17:29:27, nasko wrote:
> Why is this method without a comment? It seems out of place when the rest have
> very detailed comments.

Done.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.h:90: // Returns if this response's scheme is
not security relevant. For
On 2013/08/06 17:29:27, nasko wrote:
> nit: s/if/whether/ here and all the following comments of the same form.

Done.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.h:91: // example, this returns true for data:
On 2013/08/06 17:29:27, nasko wrote:
> Really? It returns a type according to the declaration, not a boolean.

Done.

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.h:127: static std::map<unsigned,
WebURLRequest::TargetType> id_target_map_;
Decided to leave "unsigned" unchanged to be consistent with existing code.

On 2013/08/06 17:29:27, nasko wrote:
> Be explicit in type declaration. unsigned what?

https://codereview.chromium.org/22254005/diff/1/webkit/child/site_isolation_p...
webkit/child/site_isolation_policy.h:135: static std::map<unsigned, std::string>
id_url_map_;
Decided to leave "unsigned" unchanged to be consistent with existing code.

On 2013/08/06 17:29:27, nasko wrote:
> Same here, be explicit.

[Charlie Reis, 2013-08-07 21:02:02]

Just from an initial glance, I think we need to step back and talk about what
data is being collected and what the naming scheme for the UMA stats should be.

I'm also uneasy with having custom parsers here, since they're likely to
introduce vulnerabilities and are unlikely to be accurate.  If we decide to
include them, we would need to be exceptionally clear about how we're relying on
them and why they're safe.

A few random nits below, but I think we'll want to find some time to discuss.

https://codereview.chromium.org/22254005/diff/20001/content/renderer/render_f...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/22254005/diff/20001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:76: using
webkit_glue::SiteIsolationPolicy;
nit: Alphabetical order.

https://codereview.chromium.org/22254005/diff/20001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:622: //
SiteIsolationPolicy::DidFinishLoading(response.url());
Maybe you can clarify the comment?  Do you mean that this needs to be matched by
a call to DidFinishLoading with the same URL as in the response?  Is that the
right method name?

https://codereview.chromium.org/22254005/diff/20001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/20001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:27: using WebKit::WebDocument;
These should be alphabetized.

https://codereview.chromium.org/22254005/diff/20001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:50: id_target_map_[identifier] =
target_type;
I can't understand this comment or code.  It looks like you're going to set
id_target_map_[identifier] to target_type regardless below, so why bother with
this?

https://codereview.chromium.org/22254005/diff/20001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:145:
UMA_HISTOGRAM_COUNTS("XSDP.XSD.DataLength", length);
Why would we need to collect this?

https://codereview.chromium.org/22254005/diff/20001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.h (right):

https://codereview.chromium.org/22254005/diff/20001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:52: // The correct calling sequence of the
following three functions are
A class-level comment should describe what the class is for.  If this comment is
accurate, it should be below the public: line with a blank line between it and
the WillSendRequest line.  It's confusing to me, though, since it sounds like
you require the 4th/5th one as well.

https://codereview.chromium.org/22254005/diff/20001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:98: // Returns whether response's scheme is
not security relevant. For
This comment and the function name don't agree.

https://codereview.chromium.org/22254005/diff/20001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:120: static bool
IsFrameNotCommitted(WebFrame* frame);
This name is hard to understand (vs the comment).  Sounds like whether the frame
is committed is an implementation detail.

https://codereview.chromium.org/22254005/diff/20001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:135: // Maintain data between
willSendRequest() -> didReceiveResponse()
-> is confusing here because it has a specific C++ meaning.  Just say "and".

Please explain what the key is.

[dsjang, 2013-08-08 21:21:00]

Bucket names are reorganized.

https://codereview.chromium.org/22254005/diff/20001/content/renderer/render_f...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/22254005/diff/20001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:76: using
webkit_glue::SiteIsolationPolicy;
On 2013/08/07 21:02:02, creis wrote:
> nit: Alphabetical order.

Done.

https://codereview.chromium.org/22254005/diff/20001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:622: //
SiteIsolationPolicy::DidFinishLoading(response.url());
On 2013/08/07 21:02:02, creis wrote:
> Maybe you can clarify the comment?  Do you mean that this needs to be matched
by
> a call to DidFinishLoading with the same URL as in the response?  Is that the
> right method name?

Done.

https://codereview.chromium.org/22254005/diff/20001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/20001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:27: using WebKit::WebDocument;
On 2013/08/07 21:02:02, creis wrote:
> These should be alphabetized.

Done.

https://codereview.chromium.org/22254005/diff/20001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:50: id_target_map_[identifier] =
target_type;
On 2013/08/07 21:02:02, creis wrote:
> I can't understand this comment or code.  It looks like you're going to set
> id_target_map_[identifier] to target_type regardless below, so why bother with
> this?

Done.

https://codereview.chromium.org/22254005/diff/20001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:145:
UMA_HISTOGRAM_COUNTS("XSDP.XSD.DataLength", length);
I wanted to see if that's not the case that most of the cross-site documents are
passed due to the short length of the first network packet. This might not be
useful.

On 2013/08/07 21:02:02, creis wrote:
> Why would we need to collect this?

https://codereview.chromium.org/22254005/diff/20001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.h (right):

https://codereview.chromium.org/22254005/diff/20001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:52: // The correct calling sequence of the
following three functions are
On 2013/08/07 21:02:02, creis wrote:
> A class-level comment should describe what the class is for.  If this comment
is
> accurate, it should be below the public: line with a blank line between it and
> the WillSendRequest line.  It's confusing to me, though, since it sounds like
> you require the 4th/5th one as well.

Done.

https://codereview.chromium.org/22254005/diff/20001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:98: // Returns whether response's scheme is
not security relevant. For
On 2013/08/07 21:02:02, creis wrote:
> This comment and the function name don't agree.

Done.

https://codereview.chromium.org/22254005/diff/20001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:120: static bool
IsFrameNotCommitted(WebFrame* frame);
How about FrameInNavigation?

On 2013/08/07 21:02:02, creis wrote:
> This name is hard to understand (vs the comment).  Sounds like whether the
frame
> is committed is an implementation detail.

https://codereview.chromium.org/22254005/diff/20001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:135: // Maintain data between
willSendRequest() -> didReceiveResponse()
On 2013/08/07 21:02:02, creis wrote:
> -> is confusing here because it has a specific C++ meaning.  Just say "and".
> 
> Please explain what the key is.

Done.

[Charlie Reis, 2013-08-09 00:39:02]

I'm only up to the macros, but here's my review comments so far.  I can finish
the rest in the morning.  (Our discussion today helped a lot.)

https://codereview.chromium.org/22254005/diff/20001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/20001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:131: WebURL& web_response_url) {
Add a comment to the .h file that there's a risk that multiple requests can be
made for the same URL, but we're ignoring it because the response will almost
always be the same.

https://codereview.chromium.org/22254005/diff/26001/content/renderer/render_f...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/22254005/diff/26001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:621: // Calling this will do bookkeeping
of an object related to the
will update bookkeeping related to the response for later use.

https://codereview.chromium.org/22254005/diff/26001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:622: // response for late uses. We have to
make it sure to call either
make sure

https://codereview.chromium.org/22254005/diff/26001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:625: // to free the object.
free the bookkeeping data.

I think this whole comment block belongs on the
SiteIsolationPolicy::DidReceiveResponse declaration, rather than at the call
site.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:44: // TargetIsSubresource no matter what
the original target_type was.
Much clearer.  Thanks.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:54: UMA_HISTOGRAM_COUNTS("XSDP.ALL", 1);
For the naming scheme, let's stick to SiteIsolation as the prefix, since we've
used that for all our other histograms.  This one can be
SiteIsolation.AllResponses, and the others can be SiteIsolation.XSD.MimeType,
etc.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:60: // See if this is for navigation. If
it is, let it pass.
"let it pass" -> "don't block it, under the assumption that we will put it in an
appropriate process."

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:66: GURL
frame_origin(frame->document().securityOrigin().toString().utf8());
I don't think you need the utf8() call here, do you?

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:68: // TODO(dsjang): Find out all network
related schemes here.
Is there more to be done here?

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:89: // There was a possiblity that a CORS
request preceded by a
Do we need this comment?  Or perhaps we can just skip ahead to "Every CORS
request should have the Access-Control-Allow-Origin header, even if it is
preceded by a pre-flight request."

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:95: .httpHeaderField(
Style nit: Better to response and .httpHeaderField on the same line.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:107: // TODO(dsjang): Apply X-Content-Type
option here.
What does this mean?

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:122: #define COUNT_BLOCK(BUCKET_PREFIX) \
Introducing new macros is generally frowned upon:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Prepro...

I can see how it's helping here if you can't pass a variable name to
UMA_HISTOGRAM_COUNTS, so maybe we can meet some of the requirements of the style
guide by using a more unique name (since it's global in scope) and #undef'ing it
afterward.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.h (right):

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:24: IsHTML = 0,
nit: Drop the "Is" from these names.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:47: unsigned identifier;
request_identifier

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:57: // track of the sequence of network
requests made for the original
This wording is a bit awkward.  The identifier represents a request, not a
sequence of them.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:58: // url.  target_type gets sometimes
misleading especially when it
Can you elaborate a bit?  Hard for others to know what "sometimes misleading"
means.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:59: // has TargetIsSubresource. We should
not depend on target_type to
has -> is

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:66: // erase the slot.
What slot?

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:74: // if is the case, this is going to
record various kinds of UMA data
if -> this
going to record -> records

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:86: // didFailLoad() are used for
successful loading, failed one,
successful loading and failed loading, respectively.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:92: // Returns the canonical mime type code
of the mime type of
Everything from here down should be a private helper method.  We have a
FRIEND_TEST_ALL_PREFIXES macro that will let you test them (see
render_view_impl.h for an example).

Also, please elaborate on what "canonical" means.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:97: // Returns whether response's scheme is
network relevant. This
Is "network relevant" the right name for this?  FTP is a network scheme.  Maybe
something like IsBlockableScheme?

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:99: static bool IsNetworkScheme(GURL&
frame_origin);
Most of these should probably be const GURL&.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:104: // Returns if a valid CORS's set for
frame_origin. This is very
"Returns if there is a valid CORS header for frame_origin."

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:119: static bool
IsFrameInNavigation(WebFrame* frame);
Perhaps IsFrameNavigating?

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:126: static bool SniffForJS(const char*
data, size_t length);
Why isn't this Sniff function with the others?

Also, if there's a way to separate out the stuff we would need to block requests
from the stuff that's only needed for UMA stats (even if it's just TODO comments
to remove the latter), that will make it clearer which parts of the code are
intended to stick around for a while.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:144: // This is a map that maps the
identifier of a response to the
This maps the identifier...

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:145: // response's url. This is used to
free REsponseMetaData in
typo: REsponse

[dsjang, 2013-08-09 01:31:23]

https://codereview.chromium.org/22254005/diff/26001/content/renderer/render_f...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/22254005/diff/26001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:621: // Calling this will do bookkeeping
of an object related to the
On 2013/08/09 00:39:03, creis wrote:
> will update bookkeeping related to the response for later use.

Done.

https://codereview.chromium.org/22254005/diff/26001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:622: // response for late uses. We have to
make it sure to call either
On 2013/08/09 00:39:03, creis wrote:
> make sure

Done.

https://codereview.chromium.org/22254005/diff/26001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:625: // to free the object.
On 2013/08/09 00:39:03, creis wrote:
> free the bookkeeping data.
> 
> I think this whole comment block belongs on the
> SiteIsolationPolicy::DidReceiveResponse declaration, rather than at the call
> site.

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:44: // TargetIsSubresource no matter what
the original target_type was.
On 2013/08/09 00:39:03, creis wrote:
> Much clearer.  Thanks.

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:54: UMA_HISTOGRAM_COUNTS("XSDP.ALL", 1);
On 2013/08/09 00:39:03, creis wrote:
> For the naming scheme, let's stick to SiteIsolation as the prefix, since we've
> used that for all our other histograms.  This one can be
> SiteIsolation.AllResponses, and the others can be SiteIsolation.XSD.MimeType,
> etc.

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:60: // See if this is for navigation. If
it is, let it pass.
On 2013/08/09 00:39:03, creis wrote:
> "let it pass" -> "don't block it, under the assumption that we will put it in
an
> appropriate process."

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:66: GURL
frame_origin(frame->document().securityOrigin().toString().utf8());
On 2013/08/09 00:39:03, creis wrote:
> I don't think you need the utf8() call here, do you?

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:68: // TODO(dsjang): Find out all network
related schemes here.
On 2013/08/09 00:39:03, creis wrote:
> Is there more to be done here?

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:89: // There was a possiblity that a CORS
request preceded by a
On 2013/08/09 00:39:03, creis wrote:
> Do we need this comment?  Or perhaps we can just skip ahead to "Every CORS
> request should have the Access-Control-Allow-Origin header, even if it is
> preceded by a pre-flight request."

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:95: .httpHeaderField(
On 2013/08/09 00:39:03, creis wrote:
> Style nit: Better to response and .httpHeaderField on the same line.

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:107: // TODO(dsjang): Apply X-Content-Type
option here.
I'm planning to detect X-Content-Type: nosniff header here so that we block
everything with a blacklisted mime type and "X-Content-Type: nosniff". I'll add
it for the next patch.

On 2013/08/09 00:39:03, creis wrote:
> What does this mean?

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:122: #define COUNT_BLOCK(BUCKET_PREFIX) \
On 2013/08/09 00:39:03, creis wrote:
> Introducing new macros is generally frowned upon:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Prepro...
> 
> I can see how it's helping here if you can't pass a variable name to
> UMA_HISTOGRAM_COUNTS, so maybe we can meet some of the requirements of the
style
> guide by using a more unique name (since it's global in scope) and #undef'ing
it
> afterward.

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.h (right):

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:24: IsHTML = 0,
On 2013/08/09 00:39:03, creis wrote:
> nit: Drop the "Is" from these names.

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:47: unsigned identifier;
On 2013/08/09 00:39:03, creis wrote:
> request_identifier

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:57: // track of the sequence of network
requests made for the original
On 2013/08/09 00:39:03, creis wrote:
> This wording is a bit awkward.  The identifier represents a request, not a
> sequence of them.

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:58: // url.  target_type gets sometimes
misleading especially when it
On 2013/08/09 00:39:03, creis wrote:
> Can you elaborate a bit?  Hard for others to know what "sometimes misleading"
> means.

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:59: // has TargetIsSubresource. We should
not depend on target_type to
On 2013/08/09 00:39:03, creis wrote:
> has -> is

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:66: // erase the slot.
On 2013/08/09 00:39:03, creis wrote:
> What slot?

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:74: // if is the case, this is going to
record various kinds of UMA data
On 2013/08/09 00:39:03, creis wrote:
> if -> this
> going to record -> records

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:86: // didFailLoad() are used for
successful loading, failed one,
On 2013/08/09 00:39:03, creis wrote:
> successful loading and failed loading, respectively.

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:92: // Returns the canonical mime type code
of the mime type of
On 2013/08/09 00:39:03, creis wrote:
> Everything from here down should be a private helper method.  We have a
> FRIEND_TEST_ALL_PREFIXES macro that will let you test them (see
> render_view_impl.h for an example).
> 
> Also, please elaborate on what "canonical" means.

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:97: // Returns whether response's scheme is
network relevant. This
On 2013/08/09 00:39:03, creis wrote:
> Is "network relevant" the right name for this?  FTP is a network scheme. 
Maybe
> something like IsBlockableScheme?

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:99: static bool IsNetworkScheme(GURL&
frame_origin);
On 2013/08/09 00:39:03, creis wrote:
> Most of these should probably be const GURL&.

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:104: // Returns if a valid CORS's set for
frame_origin. This is very
On 2013/08/09 00:39:03, creis wrote:
> "Returns if there is a valid CORS header for frame_origin."

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:119: static bool
IsFrameInNavigation(WebFrame* frame);
On 2013/08/09 00:39:03, creis wrote:
> Perhaps IsFrameNavigating?

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:126: static bool SniffForJS(const char*
data, size_t length);
On 2013/08/09 00:39:03, creis wrote:
> Why isn't this Sniff function with the others?
> 
> Also, if there's a way to separate out the stuff we would need to block
requests
> from the stuff that's only needed for UMA stats (even if it's just TODO
comments
> to remove the latter), that will make it clearer which parts of the code are
> intended to stick around for a while.

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:144: // This is a map that maps the
identifier of a response to the
On 2013/08/09 00:39:03, creis wrote:
> This maps the identifier...

Done.

https://codereview.chromium.org/22254005/diff/26001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:145: // response's url. This is used to
free REsponseMetaData in
On 2013/08/09 00:39:03, creis wrote:
> typo: REsponse

Done.

[Charlie Reis, 2013-08-09 18:48:38]

Thanks!  More comments on the rest below.  I agree with Nasko that it would help
to have a comment somewhere explaining your UMA stat naming scheme.

Can you also take a pass over the CL description to clean it up a bit?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:54:
UMA_HISTOGRAM_COUNTS("SiteIsolation.ALL", 1);
ALL -> AllResponses

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:145: // We only record XSDs whose content
is actually non-zero.
Make sure XSDs is spelled out somewhere (perhaps in the class-level comment of
the .h file).

Also, what do you mean by "content is actually non-zero"?  The length is
non-zero?  Or that we didn't see zero responses?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:160: // Record the entire number of
responses with a specific mime
number of XSD reponses

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:161: // type(text/html, text/xml, etc).
nit: space after type

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:167: // HTML/JSON/XML. So if the status
code is an error status code, it
it -> blocking it

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:170: // sniffed as a text document. 2)
then, this blocking only breaks
nit: Either capitalize each sentence or use a semicolon instead of a period
between the numbered phrases.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:173: // with unaffected status code(e.g,
404). *) the renderer is
nit: space after code
Also, why "*)" rather than "3)"?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:178: // This is only used for measuring
false-negative analysis for
nit: "measuring" and "analysis" are redundant here.  Drop "measuring".

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:189: "SiteIsolation.XSD.MimeType.HTML");
I don't think we need "MimeType" in the stat name (here and below).

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:215: DCHECK(false);
Use NOTREACHED() instead.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:222: #undef
SITE_ISOLATION_POLICY_SNIFF_AND_COUNT
Probably should undef these in reverse order, since SNIFF_AND_COUNT depends on
the others.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:246: static const char TEXT_HTML[] =
"text/html";
Constants should be formatted as kTextHtml, etc.  See content_constants.cc for
examples.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:253: static const char TEXT_PLAIN[] =
"text/json";
Shouldn't this be text/plain?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:260: // 50k sites related to HTML, XML,
JSON, Plain.
This comment belongs above your constants.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:285: bool
SiteIsolationPolicy::IsSameSite(const GURL& frame_origin,
This seems like it should be modeled more after SiteInstance::IsSameWebSite. 
You can ignore GetEffectiveURL, but what about URLs that aren't valid?  Also,
SameDomainOrHost seems like a more convenient check than calling
GetDomainAndRegistry twice.

I don't think you need an equivalent to IsURLSameAsAnySiteInstance, since the
kChromeUI* URLs aren't visible in the renderer.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:298: // "finer grained" sites than only
using public ones.
Just change this to be a TODO to use INCLUDE_PRIVATE_REGISTRIES when
http://crbug.com/7988 is fixed.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:318: // navigation or not by far.
nit: drop "by far"

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:329: // TODO(dsjang): Is this actually
true? The server seems to return
Can we resolve this TODO?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:342: // documents alllowed by this. We
have to have t a way to see if
typos: alllowed, t

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:357: // allowed.
Please move this comment to the top of this function.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:361: WebKit::WebSecurityOrigin
cors_security_origin =
Why send this through WebSecurityOrigin if we're just converting it back to a
GURL?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:366: LOG(ERROR) <<
cors_security_origin.toString().utf8();
We'll need to get rid of all the log statements before committing.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:370: bool
SiteIsolationPolicy::SniffForHTML(const char* data, size_t length) {
Can you put a comment somewhere saying which function this code is based on?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:371: // TODO(dsjang): The content sniffer
used by Chrome and Firefox are
This part of the comment isn't a TODO.  However, it may be worth adding a TODO
to parameterize the original sniffing logic based on whether to include <!-- as
a valid signature or not, so that we can remove this function.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:408: // finds 1) {, 2) "or', 3) : in the
order. This is intentionally not
finds {, "/', and : in that order.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:410: // trusted code base as small as
possible. State 4 is a dead state.
I'd change this sentence to just say that we're avoiding adding a dependency on
a regular expression library.  The trusted computing base argument could go
either way: a library would be more vetted than this hand-coded parser but also
larger and more complex.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:437: if (c == ':') {
nit: No braces needed.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:441: default:
NOTREACHED()

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:448: bool
SiteIsolationPolicy::DoSignatureMatching(const char* data,
MatchesSignature might be a better name.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:459: if (!(c == ' ' || c == '\r' || c ==
'\n' || c == '\t')) {
nit: No braces needed on one-line clause.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:474: bool
SiteIsolationPolicy::IsErrorStatusCode(int status_code) {
The only place you use this is when deciding if it's renderable, so it seems
strange to invert the result.  Perhaps this should be IsRenderableStatusCode
instead of IsErrorStatusCode.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:488: // TODO(dsjang): This is a real
hacking. The only purpose of this
hacking -> hack

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:490: // data can be JavaScript.(superset
of JS). This function will be
nit: space, not period, after JavaScript

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:491: // removed for the production code.
will be removed once UMA stats are gathered.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:495: if (strncmp(data, "var ", 4) == 0) {
nit: No brace.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.h (right):

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:23: struct ResponseMetaData {
There are some compile errors for this on linux_clang:
http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/13...

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:117: // Returns whether this scheme is a
target of XSDP. This returns
Note: we haven't defined XSDP anywhere in the code.  We should either introduce
that term in a class-level comment or spell it out here.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:138: // Returns whether the given frame is
nagivating. When this is true,
typo: navigating

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy_unittest.cc (right):

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy_unittest.cc:16:
EXPECT_FALSE(SiteIsolationPolicy::IsBlockableScheme(data_url));
Also test that http/https are true and ftp is false.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy_unittest.cc:26:
EXPECT_TRUE(SiteIsolationPolicy::IsSameSite(url2, url0));
Also test that http://a.com, https://b.com, about:blank, and the empty URL are
not considered the same site as url0.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy_unittest.cc:67: const char* non_json_data =
"\t\t\r\n   { name : \"chrome\", ";
Also test for "foo({"name" : "chrome","

https://codereview.chromium.org/22254005/diff/32001/webkit/child/weburlloader...
File webkit/child/weburlloader_impl.cc (right):

https://codereview.chromium.org/22254005/diff/32001/webkit/child/weburlloader...
webkit/child/weburlloader_impl.cc:40: 
nit: Remove extra blank line.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/weburlloader...
webkit/child/weburlloader_impl.cc:381: // OnReceivedData().
I agree that this should only be used temporarily, but have you thought about
how we will do blocking without it?

[nasko, 2013-08-09 19:07:28]

Looks much better!
The one thing I haven't done is verify your state machine for parsing JSON.

https://codereview.chromium.org/22254005/diff/32001/content/renderer/render_f...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/22254005/diff/32001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:623:
SiteIsolationPolicy::DidReceiveResponse(frame, identifier, response);
All other calls are at the end of the respective function. Why is this not
following the same pattern?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:54:
UMA_HISTOGRAM_COUNTS("SiteIsolation.ALL", 1);
nit: TOTAL?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:145: // We only record XSDs whose content
is actually non-zero.
How do we know the content is non-zero based on the URL?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:215: DCHECK(false);
nit: You can add << "message" to clarify why this is failing, which will print
to the console. It saves a roundtrip to the source code location to figure out
the cause of break.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:257: LOG(ERROR) << "mimetype:" <<
mime_type << "==[" << TEXT_HTML << "]";
Why is this TEXT_HTML there? Wouldn't it always print the same?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:263: } else if
(LowerCaseEqualsASCII(mime_type, TEXT_XML) ||
nit: Have you ordered these in order of decreasing probability of occurrence?
I'd do it to have higher likelihood of short circuiting the if statement.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:293: // TODO(dsjang): Is there any reason
why we don't use
This will be good to iron out before we commit the code. Changing UMA stats
semantics later can bring confusion when looking over the full collection.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:318: // navigation or not by far.
Awesome comment! Thank you! I've learned something new : ).

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:342: // documents alllowed by this. We
have to have t a way to see if
nit: Not sure what "t a way" stands for. To find a way?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:411: const int INIT_ST = 0;
nit: I'd use less abbreviation in these, but it is up to you.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:462: ++data;
Why are you modifying the input parameter? You also don't reset it across
outside for loop iterations, so this seems wrong on first glance.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:465: if (length < signature_length)
Is there a reason why we can't put this before we start comparing? If we don't
have enough data from the wire to compare to the current signature, we shouldn't
even try it, right?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.h (right):

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:56: // Register target_type information for
identifier which identifies
nit: |identifier|

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:59: // not depend on target_type to decide
if a request is for
nit: |target_type|

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:64: // Register the header information of
the response data. This
nit: Registers

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:69: // to free the bookkepping data.
TODO(dsjang): There's a possibility
nit: TODO on new line.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:70: // that two distinct responses
(identified by different identifiers)
What is an example of this? It seems strange enough case to warrant a small
example.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:87: // called at the end of the
nit: combine with next line, too much white space without ending an actual
sentence.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:117: // Returns whether this scheme is a
target of XSDP. This returns
nit: This is the first time I see XSDP in this file, will be nice to have it
expanded at least once.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:124: // Returns whether there's a valid
CORS header for
nit: Another instance of too much white space at the end of the line.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:127: // sites as our security domain, not
origins. TODO(dsjang): this
nit: TODO on new line

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:151: // TODO(dsjang): this is only needed
for collecting UMA stat.
Is there a reason for the TODO? Are you planning to remove those? Same for the
one below.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:170: static std::map<unsigned, std::string>
id_url_map_;
If the second member is URL, why are you storing it as a string?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy_unittest.cc (right):

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy_unittest.cc:16:
EXPECT_FALSE(SiteIsolationPolicy::IsBlockableScheme(data_url));
Why not test other schemes? How would this test catch regressions in
IsBlockableScheme?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy_unittest.cc:26:
EXPECT_TRUE(SiteIsolationPolicy::IsSameSite(url2, url0));
Why not add negative tests as well?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/weburlloader...
File webkit/child/weburlloader_impl.cc (right):

https://codereview.chromium.org/22254005/diff/32001/webkit/child/weburlloader...
webkit/child/weburlloader_impl.cc:40: 
nit: New empty line, not needed.

[dsjang, 2013-08-12 22:56:17]

Change the way that static globals are handled to be compatible with Clang++
rules & merge LKGR into the local branch.

https://codereview.chromium.org/22254005/diff/32001/content/renderer/render_f...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/22254005/diff/32001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:623:
SiteIsolationPolicy::DidReceiveResponse(frame, identifier, response);
This is called at front since didReceiveResponse is returned immediately in the
next line when the frame is not navigating. If we inserted our hookup call at
the end, we won't see any sub resource requests.

On 2013/08/09 19:07:28, nasko wrote:
> All other calls are at the end of the respective function. Why is this not
> following the same pattern?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:54:
UMA_HISTOGRAM_COUNTS("SiteIsolation.ALL", 1);
On 2013/08/09 18:48:38, creis wrote:
> ALL -> AllResponses

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:54:
UMA_HISTOGRAM_COUNTS("SiteIsolation.ALL", 1);
AllResponses seems more informative than TOTAL to me. Let me know if you have a
better idea.

On 2013/08/09 19:07:28, nasko wrote:
> nit: TOTAL?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:145: // We only record XSDs whose content
is actually non-zero.
This means that we can't see zero responses here. Moved this comment to .h file.

On 2013/08/09 18:48:38, creis wrote:
> Make sure XSDs is spelled out somewhere (perhaps in the class-level comment of
> the .h file).
> 
> Also, what do you mean by "content is actually non-zero"?  The length is
> non-zero?  Or that we didn't see zero responses?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:145: // We only record XSDs whose content
is actually non-zero.
We don't know. What I meant was that DidReceiveData() is never called for zero
responses.

On 2013/08/09 19:07:28, nasko wrote:
> How do we know the content is non-zero based on the URL?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:160: // Record the entire number of
responses with a specific mime
On 2013/08/09 18:48:38, creis wrote:
> number of XSD reponses

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:161: // type(text/html, text/xml, etc).
On 2013/08/09 18:48:38, creis wrote:
> nit: space after type

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:167: // HTML/JSON/XML. So if the status
code is an error status code, it
On 2013/08/09 18:48:38, creis wrote:
> it -> blocking it

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:170: // sniffed as a text document. 2)
then, this blocking only breaks
On 2013/08/09 18:48:38, creis wrote:
> nit: Either capitalize each sentence or use a semicolon instead of a period
> between the numbered phrases.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:178: // This is only used for measuring
false-negative analysis for
On 2013/08/09 18:48:38, creis wrote:
> nit: "measuring" and "analysis" are redundant here.  Drop "measuring".

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:189: "SiteIsolation.XSD.MimeType.HTML");
On 2013/08/09 18:48:38, creis wrote:
> I don't think we need "MimeType" in the stat name (here and below).

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:215: DCHECK(false);
On 2013/08/09 18:48:38, creis wrote:
> Use NOTREACHED() instead.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:215: DCHECK(false);
This is very useful. Thanks for letting me know that.

On 2013/08/09 19:07:28, nasko wrote:
> nit: You can add << "message" to clarify why this is failing, which will print
> to the console. It saves a roundtrip to the source code location to figure out
> the cause of break.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:222: #undef
SITE_ISOLATION_POLICY_SNIFF_AND_COUNT
On 2013/08/09 18:48:38, creis wrote:
> Probably should undef these in reverse order, since SNIFF_AND_COUNT depends on
> the others.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:246: static const char TEXT_HTML[] =
"text/html";
On 2013/08/09 18:48:38, creis wrote:
> Constants should be formatted as kTextHtml, etc.  See content_constants.cc for
> examples.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:253: static const char TEXT_PLAIN[] =
"text/json";
On 2013/08/09 18:48:38, creis wrote:
> Shouldn't this be text/plain?

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:257: LOG(ERROR) << "mimetype:" <<
mime_type << "==[" << TEXT_HTML << "]";
On 2013/08/09 19:07:28, nasko wrote:
> Why is this TEXT_HTML there? Wouldn't it always print the same?

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:260: // 50k sites related to HTML, XML,
JSON, Plain.
On 2013/08/09 18:48:38, creis wrote:
> This comment belongs above your constants.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:263: } else if
(LowerCaseEqualsASCII(mime_type, TEXT_XML) ||
On 2013/08/09 19:07:28, nasko wrote:
> nit: Have you ordered these in order of decreasing probability of occurrence?
> I'd do it to have higher likelihood of short circuiting the if statement.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:293: // TODO(dsjang): Is there any reason
why we don't use
On 2013/08/09 19:07:28, nasko wrote:
> This will be good to iron out before we commit the code. Changing UMA stats
> semantics later can bring confusion when looking over the full collection.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:298: // "finer grained" sites than only
using public ones.
On 2013/08/09 18:48:38, creis wrote:
> Just change this to be a TODO to use INCLUDE_PRIVATE_REGISTRIES when
> http://crbug.com/7988 is fixed.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:318: // navigation or not by far.
Thank you here! I'm so glad that you got something from reviewing my code.

On 2013/08/09 19:07:28, nasko wrote:
> Awesome comment! Thank you! I've learned something new : ).

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:318: // navigation or not by far.
On 2013/08/09 19:07:28, nasko wrote:
> Awesome comment! Thank you! I've learned something new : ).

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:318: // navigation or not by far.
On 2013/08/09 18:48:38, creis wrote:
> nit: drop "by far"

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:329: // TODO(dsjang): Is this actually
true? The server seems to return
third_party/WebKit/Source/core/loader/CrossOriginAccessControl.cpp is not using
this heuristic. So removed this line.

On 2013/08/09 18:48:38, creis wrote:
> Can we resolve this TODO?

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:342: // documents alllowed by this. We
have to have t a way to see if
On 2013/08/09 19:07:28, nasko wrote:
> nit: Not sure what "t a way" stands for. To find a way?

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:342: // documents alllowed by this. We
have to have t a way to see if
On 2013/08/09 18:48:38, creis wrote:
> typos: alllowed, t

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:357: // allowed.
On 2013/08/09 18:48:38, creis wrote:
> Please move this comment to the top of this function.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:361: WebKit::WebSecurityOrigin
cors_security_origin =
On 2013/08/09 18:48:38, creis wrote:
> Why send this through WebSecurityOrigin if we're just converting it back to a
> GURL?

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:366: LOG(ERROR) <<
cors_security_origin.toString().utf8();
Sure. I'll do that when the CL is ready for committing.

On 2013/08/09 18:48:38, creis wrote:
> We'll need to get rid of all the log statements before committing.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:370: bool
SiteIsolationPolicy::SniffForHTML(const char* data, size_t length) {
On 2013/08/09 18:48:38, creis wrote:
> Can you put a comment somewhere saying which function this code is based on?

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:371: // TODO(dsjang): The content sniffer
used by Chrome and Firefox are
On 2013/08/09 18:48:38, creis wrote:
> This part of the comment isn't a TODO.  However, it may be worth adding a TODO
> to parameterize the original sniffing logic based on whether to include <!--
as
> a valid signature or not, so that we can remove this function.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:408: // finds 1) {, 2) "or', 3) : in the
order. This is intentionally not
On 2013/08/09 18:48:38, creis wrote:
> finds {, "/', and : in that order.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:410: // trusted code base as small as
possible. State 4 is a dead state.
On 2013/08/09 18:48:38, creis wrote:
> I'd change this sentence to just say that we're avoiding adding a dependency
on
> a regular expression library.  The trusted computing base argument could go
> either way: a library would be more vetted than this hand-coded parser but
also
> larger and more complex.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:437: if (c == ':') {
On 2013/08/09 18:48:38, creis wrote:
> nit: No braces needed.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:441: default:
On 2013/08/09 18:48:38, creis wrote:
> NOTREACHED()

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:459: if (!(c == ' ' || c == '\r' || c ==
'\n' || c == '\t')) {
On 2013/08/09 18:48:38, creis wrote:
> nit: No braces needed on one-line clause.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:462: ++data;
On 2013/08/09 19:07:28, nasko wrote:
> Why are you modifying the input parameter? You also don't reset it across
> outside for loop iterations, so this seems wrong on first glance.

Thanks a lot!! Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:465: if (length < signature_length)
On 2013/08/09 19:07:28, nasko wrote:
> Is there a reason why we can't put this before we start comparing? If we don't
> have enough data from the wire to compare to the current signature, we
shouldn't
> even try it, right?

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:474: bool
SiteIsolationPolicy::IsErrorStatusCode(int status_code) {
On 2013/08/09 18:48:38, creis wrote:
> The only place you use this is when deciding if it's renderable, so it seems
> strange to invert the result.  Perhaps this should be IsRenderableStatusCode
> instead of IsErrorStatusCode.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:488: // TODO(dsjang): This is a real
hacking. The only purpose of this
On 2013/08/09 18:48:38, creis wrote:
> hacking -> hack

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:490: // data can be JavaScript.(superset
of JS). This function will be
On 2013/08/09 18:48:38, creis wrote:
> nit: space, not period, after JavaScript

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:491: // removed for the production code.
On 2013/08/09 18:48:38, creis wrote:
> will be removed once UMA stats are gathered.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:495: if (strncmp(data, "var ", 4) == 0) {
On 2013/08/09 18:48:38, creis wrote:
> nit: No brace.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.h (right):

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:23: struct ResponseMetaData {
On 2013/08/09 18:48:38, creis wrote:
> There are some compile errors for this on linux_clang:
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/13...

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:56: // Register target_type information for
identifier which identifies
On 2013/08/09 19:07:28, nasko wrote:
> nit: |identifier|

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:59: // not depend on target_type to decide
if a request is for
On 2013/08/09 19:07:28, nasko wrote:
> nit: |target_type|

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:64: // Register the header information of
the response data. This
On 2013/08/09 19:07:28, nasko wrote:
> nit: Registers

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:69: // to free the bookkepping data.
TODO(dsjang): There's a possibility
On 2013/08/09 19:07:28, nasko wrote:
> nit: TODO on new line.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:70: // that two distinct responses
(identified by different identifiers)
On 2013/08/09 19:07:28, nasko wrote:
> What is an example of this? It seems strange enough case to warrant a small
> example.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:87: // called at the end of the
On 2013/08/09 19:07:28, nasko wrote:
> nit: combine with next line, too much white space without ending an actual
> sentence.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:117: // Returns whether this scheme is a
target of XSDP. This returns
On 2013/08/09 18:48:38, creis wrote:
> Note: we haven't defined XSDP anywhere in the code.  We should either
introduce
> that term in a class-level comment or spell it out here.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:117: // Returns whether this scheme is a
target of XSDP. This returns
On 2013/08/09 19:07:28, nasko wrote:
> nit: This is the first time I see XSDP in this file, will be nice to have it
> expanded at least once.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:124: // Returns whether there's a valid
CORS header for
On 2013/08/09 19:07:28, nasko wrote:
> nit: Another instance of too much white space at the end of the line.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:127: // sites as our security domain, not
origins. TODO(dsjang): this
On 2013/08/09 19:07:28, nasko wrote:
> nit: TODO on new line

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:138: // Returns whether the given frame is
nagivating. When this is true,
On 2013/08/09 18:48:38, creis wrote:
> typo: navigating

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:151: // TODO(dsjang): this is only needed
for collecting UMA stat.
They are going to be deleted when deployed to do actual blocking. This is added
to the comment.

On 2013/08/09 19:07:28, nasko wrote:
> Is there a reason for the TODO? Are you planning to remove those? Same for the
> one below.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:170: static std::map<unsigned, std::string>
id_url_map_;
I thought that a string representation is more compact than URL.

On 2013/08/09 19:07:28, nasko wrote:
> If the second member is URL, why are you storing it as a string?

[Charlie Reis, 2013-08-13 00:53:19]

Thanks for all the changes.  Looks like there were some missed comments from
last time, though, so I pointed them out.  Beyond a few other issues pointed out
here, I think we're close, so let's start removing the LOG statements and get it
ready to commit.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:32: // Chromium flags on.
No need for this comment.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:103: // TODO(dsjang): Apply X-Content-Type
option here.
Are we hoping to resolve this before committing?  Be sure to make it clear that
this is about the no-sniff header, which will allow us to block responses
without sniffing them.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:118: return;
No need for an empty return statement here.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:121: #define
SITE_ISOLATION_POLICY_COUNT_BLOCK(BUCKET_PREFIX) \
Let's add a comment about why these macros are needed, with a TODO to remove
them after we've collected the UMA data.  (The fact that they make assumptions
about what variables are in scope, like is_sniffed_for_js, makes them very
fragile, so I'd like to make it clear that they're temporary.)

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:163: // Record the number of responses
with a specific mime
Please be specific about "number of cross-site document responses", since this
is very different from "AllResponses" above.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:176: // with unaffected status code(e.g,
404). *) the renderer is
Looks like you missed my earlier comment about this one.

nit: space after code
Also, why "*)" rather than "3)"?

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:183: // non-blocked resources.
nit: You can move some of this to the previous line.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:274: LOG(ERROR) << "mimetype:" <<
mime_type;
Please remove all LOG statements from the CL, since we're getting ready to
commit it.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:300: bool
SiteIsolationPolicy::IsSameSite(const GURL& frame_origin,
Looks like you missed my earlier comment about SiteInstance::IsSameWebSite.

This should check whether the URLs are valid, and it should use SameDomainOrHost
instead of calling GetDomainAndRegistry twice.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:358: // TODO(dsjang): examine
createFromString()'s behavior for a URL
Have you checked this?

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:409: // dependency on a regular expression
library.  The trusted
Heh, I didn't mean for you to include the second sentence.  :)  Please drop the
"The trusted computing base" sentence.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:466: data = data + start;
I'm with Nasko that we should not modify the input parameter.  Maybe just put
this in a new variable.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:499: // removed once UMA stats are
gathers.
nit: gathers -> gathered.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:505: ++data;
Should probably avoid modifying the input parameter here as well.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.h (right):

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:23: // |SiteIsolationPolicy| implements the
cross-site document blocking
nit: No |'s needed around the class name here or below.  We tend to use them
just for function arguments.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:24: // policy(XSDP) for Site Isolation.
XSDP will monitor network
nit: Please use a space before open parens in comments like this, to help
distinguish it from function calls.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:25: // responses to a renderer and blocks
illegal responses so that a
nit: blocks -> block

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:30: // |SiteIsolationPolicy| gathers are as
follows:
Part of this can fit on the previous line, right?  In general, we try to use the
full 80 characters when writing a paragraph comment.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:32: // SiteIsolation.AllResponses : # of
all network responses
nit: End with a period.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:38: //   blocked # of  cross-site document
response by [%MIMETYPE] sniffing.
nit: extra space after "of"
nit: responses grouped by sniffed MIME type.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:41: //   out of
SiteIsolation.XSD.[%MIMETYPE].Blocked
nit: End with a period, here and below.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:42: //
SiteIsolation.XSD.[%MIMETYPE].Blocked.NotRenderableStatusCode :
NonRenderableStatusCode

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:43: //   # of responses with not renderable
status code,
with a non-renderable

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:45: //
SiteIsolation.XSD.[%MIMETYPE].NotBlocked
nit: End with a colon, here and for the MaybeJS case.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:48: //   # of responses sniffed for of
SiteIsolation.XSD.[%MIMETYPE].NotBlocked.
I think there's a word missing here.  # of responses that are plausibly sniffed
to be JavaScript?

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:86: typedef std::map<unsigned, std::string>
IdUrlMap;
Converting a GURL to a string can lose information.  Correctness matters more
than compactness here, so let's use GURLs in these maps.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:108: // src="urlA"> on the same page, they
will be two calls of
nit: they -> there

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:126: // called at the end of
thetransaction. WebURLLoaderImpl::didReceivedData()
typo: thetransaction

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:139: 
nit: No blank line here.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:156: // policy(XSDP). This returns true
only for http://* and https://
nit: No * needed if you don't have it on both.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy_unittest.cc (right):

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy_unittest.cc:14: TEST(SiteIsolationPolicyTest,
IsBlockableScheme) {
I think you missed all 5 of our comments about this file from patch set 6.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/weburlloader...
File webkit/child/weburlloader_impl.cc (right):

https://codereview.chromium.org/22254005/diff/69001/webkit/child/weburlloader...
webkit/child/weburlloader_impl.cc:40: 
There are still a few unresolved comments about this file in patch set 6.

[dsjang, 2013-08-13 20:54:48]

More test cases are added and code formatting & comments are improved.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy_unittest.cc (right):

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy_unittest.cc:16:
EXPECT_FALSE(SiteIsolationPolicy::IsBlockableScheme(data_url));
On 2013/08/09 18:48:38, creis wrote:
> Also test that http/https are true and ftp is false.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy_unittest.cc:26:
EXPECT_TRUE(SiteIsolationPolicy::IsSameSite(url2, url0));
On 2013/08/09 18:48:38, creis wrote:
> Also test that http://a.com, https://b.com, about:blank, and the empty URL are
> not considered the same site as url0.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/site_isolati...
webkit/child/site_isolation_policy_unittest.cc:67: const char* non_json_data =
"\t\t\r\n   { name : \"chrome\", ";
On 2013/08/09 18:48:38, creis wrote:
> Also test for "foo({"name" : "chrome","

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:32: // Chromium flags on.
On 2013/08/13 00:53:19, creis wrote:
> No need for this comment.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:103: // TODO(dsjang): Apply X-Content-Type
option here.
On 2013/08/13 00:53:19, creis wrote:
> Are we hoping to resolve this before committing?  Be sure to make it clear
that
> this is about the no-sniff header, which will allow us to block responses
> without sniffing them.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:118: return;
On 2013/08/13 00:53:19, creis wrote:
> No need for an empty return statement here.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:121: #define
SITE_ISOLATION_POLICY_COUNT_BLOCK(BUCKET_PREFIX) \
On 2013/08/13 00:53:19, creis wrote:
> Let's add a comment about why these macros are needed, with a TODO to remove
> them after we've collected the UMA data.  (The fact that they make assumptions
> about what variables are in scope, like is_sniffed_for_js, makes them very
> fragile, so I'd like to make it clear that they're temporary.)

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:163: // Record the number of responses
with a specific mime
On 2013/08/13 00:53:19, creis wrote:
> Please be specific about "number of cross-site document responses", since this
> is very different from "AllResponses" above.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:176: // with unaffected status code(e.g,
404). *) the renderer is
On 2013/08/13 00:53:19, creis wrote:
> Looks like you missed my earlier comment about this one.
> 
> nit: space after code
> Also, why "*)" rather than "3)"?

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:183: // non-blocked resources.
On 2013/08/13 00:53:19, creis wrote:
> nit: You can move some of this to the previous line.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:274: LOG(ERROR) << "mimetype:" <<
mime_type;
On 2013/08/13 00:53:19, creis wrote:
> Please remove all LOG statements from the CL, since we're getting ready to
> commit it.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:300: bool
SiteIsolationPolicy::IsSameSite(const GURL& frame_origin,
On 2013/08/13 00:53:19, creis wrote:
> Looks like you missed my earlier comment about SiteInstance::IsSameWebSite.
> 
> This should check whether the URLs are valid, and it should use
SameDomainOrHost
> instead of calling GetDomainAndRegistry twice.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:358: // TODO(dsjang): examine
createFromString()'s behavior for a URL
this comment should have been gone since we don't use createFromString()
anymore.

On 2013/08/13 00:53:19, creis wrote:
> Have you checked this?

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:409: // dependency on a regular expression
library.  The trusted
On 2013/08/13 00:53:19, creis wrote:
> Heh, I didn't mean for you to include the second sentence.  :)  Please drop
the
> "The trusted computing base" sentence.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:466: data = data + start;
On 2013/08/13 00:53:19, creis wrote:
> I'm with Nasko that we should not modify the input parameter.  Maybe just put
> this in a new variable.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:499: // removed once UMA stats are
gathers.
On 2013/08/13 00:53:19, creis wrote:
> nit: gathers -> gathered.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:505: ++data;
On 2013/08/13 00:53:19, creis wrote:
> Should probably avoid modifying the input parameter here as well.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.h (right):

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:23: // |SiteIsolationPolicy| implements the
cross-site document blocking
On 2013/08/13 00:53:19, creis wrote:
> nit: No |'s needed around the class name here or below.  We tend to use them
> just for function arguments.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:24: // policy(XSDP) for Site Isolation.
XSDP will monitor network
On 2013/08/13 00:53:19, creis wrote:
> nit: Please use a space before open parens in comments like this, to help
> distinguish it from function calls.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:25: // responses to a renderer and blocks
illegal responses so that a
On 2013/08/13 00:53:19, creis wrote:
> nit: blocks -> block

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:30: // |SiteIsolationPolicy| gathers are as
follows:
On 2013/08/13 00:53:19, creis wrote:
> Part of this can fit on the previous line, right?  In general, we try to use
the
> full 80 characters when writing a paragraph comment.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:32: // SiteIsolation.AllResponses : # of
all network responses
On 2013/08/13 00:53:19, creis wrote:
> nit: End with a period.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:38: //   blocked # of  cross-site document
response by [%MIMETYPE] sniffing.
On 2013/08/13 00:53:19, creis wrote:
> nit: extra space after "of"
> nit: responses grouped by sniffed MIME type.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:41: //   out of
SiteIsolation.XSD.[%MIMETYPE].Blocked
On 2013/08/13 00:53:19, creis wrote:
> nit: End with a period, here and below.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:42: //
SiteIsolation.XSD.[%MIMETYPE].Blocked.NotRenderableStatusCode :
On 2013/08/13 00:53:19, creis wrote:
> NonRenderableStatusCode

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:43: //   # of responses with not renderable
status code,
On 2013/08/13 00:53:19, creis wrote:
> with a non-renderable

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:45: //
SiteIsolation.XSD.[%MIMETYPE].NotBlocked
On 2013/08/13 00:53:19, creis wrote:
> nit: End with a colon, here and for the MaybeJS case.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:48: //   # of responses sniffed for of
SiteIsolation.XSD.[%MIMETYPE].NotBlocked.
On 2013/08/13 00:53:19, creis wrote:
> I think there's a word missing here.  # of responses that are plausibly
sniffed
> to be JavaScript?

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:86: typedef std::map<unsigned, std::string>
IdUrlMap;
On 2013/08/13 00:53:19, creis wrote:
> Converting a GURL to a string can lose information.  Correctness matters more
> than compactness here, so let's use GURLs in these maps.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:108: // src="urlA"> on the same page, they
will be two calls of
On 2013/08/13 00:53:19, creis wrote:
> nit: they -> there

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:126: // called at the end of
thetransaction. WebURLLoaderImpl::didReceivedData()
On 2013/08/13 00:53:19, creis wrote:
> typo: thetransaction

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:139: 
On 2013/08/13 00:53:19, creis wrote:
> nit: No blank line here.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:156: // policy(XSDP). This returns true
only for http://* and https://
On 2013/08/13 00:53:19, creis wrote:
> nit: No * needed if you don't have it on both.

Done.

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy_unittest.cc (right):

https://codereview.chromium.org/22254005/diff/69001/webkit/child/site_isolati...
webkit/child/site_isolation_policy_unittest.cc:14: TEST(SiteIsolationPolicyTest,
IsBlockableScheme) {
On 2013/08/13 00:53:19, creis wrote:
> I think you missed all 5 of our comments about this file from patch set 6.

Done.

[Charlie Reis, 2013-08-13 21:09:03]

LGTM with nits.  I think you can send it to Darin once these are fixed.

https://codereview.chromium.org/22254005/diff/87001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/87001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:114: // UMA_HISTOGRAM_* macros do not
accept variabls as their bucket names. As a
nit: variables

https://codereview.chromium.org/22254005/diff/87001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:317: const GURL& response_url) {
Please add a check for whether either URL is invalid, and if so, return false.

https://codereview.chromium.org/22254005/diff/87001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.h (right):

https://codereview.chromium.org/22254005/diff/87001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:55: //   # of responses that are plausibly
sniffed to be JavaScript?
nit: End with period, not question mark.

https://codereview.chromium.org/22254005/diff/87001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:131: // end of thetransaction. 
WebURLLoaderImpl::didReceivedData() is not a place
nit: the transaction

[dsjang, 2013-08-13 21:49:52]

darin: the goal of this CL is described in detail at the end of this document
about our proposal for blocking cross-site documents :
https://docs.google.com/a/google.com/document/d/1nB3GruRqQmtA7OPZZAhWOsZDvfWp...

Moreover, you can refer to the bug cited by the CL too.

Thank you for your code review in advance!

https://codereview.chromium.org/22254005/diff/32001/webkit/child/weburlloader...
File webkit/child/weburlloader_impl.cc (right):

https://codereview.chromium.org/22254005/diff/32001/webkit/child/weburlloader...
webkit/child/weburlloader_impl.cc:40: 
On 2013/08/09 18:48:38, creis wrote:
> nit: Remove extra blank line.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/weburlloader...
webkit/child/weburlloader_impl.cc:40: 
On 2013/08/09 19:07:28, nasko wrote:
> nit: New empty line, not needed.

Done.

https://codereview.chromium.org/22254005/diff/32001/webkit/child/weburlloader...
webkit/child/weburlloader_impl.cc:381: // OnReceivedData().
On 2013/08/09 18:48:38, creis wrote:
> I agree that this should only be used temporarily, but have you thought about
> how we will do blocking without it?

Done.

https://codereview.chromium.org/22254005/diff/87001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/87001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:114: // UMA_HISTOGRAM_* macros do not
accept variabls as their bucket names. As a
On 2013/08/13 21:09:03, creis wrote:
> nit: variables

Done.

https://codereview.chromium.org/22254005/diff/87001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:317: const GURL& response_url) {
On 2013/08/13 21:09:03, creis wrote:
> Please add a check for whether either URL is invalid, and if so, return false.

Done.

https://codereview.chromium.org/22254005/diff/87001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.h (right):

https://codereview.chromium.org/22254005/diff/87001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:55: //   # of responses that are plausibly
sniffed to be JavaScript?
On 2013/08/13 21:09:03, creis wrote:
> nit: End with period, not question mark.

Done.

https://codereview.chromium.org/22254005/diff/87001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:131: // end of thetransaction. 
WebURLLoaderImpl::didReceivedData() is not a place
On 2013/08/13 21:09:03, creis wrote:
> nit: the transaction

Done.

[darin (slow to review), 2013-08-14 05:23:26]

https://codereview.chromium.org/22254005/diff/77001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/77001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:122: #define
SITE_ISOLATION_POLICY_COUNT_BLOCK(BUCKET_PREFIX) \
nit: Can you do all of this without macros?  Even if it means doing some string
concatenation at runtime, I suspect it'll be worth making the code more
readable.

https://codereview.chromium.org/22254005/diff/77001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:283: static const char kTextHtml[] =
"text/html";
nit: for constants like these, it is more canonical to put them at file scope in
an anonymous namespace sans static keyword.

https://codereview.chromium.org/22254005/diff/77001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:296: } else if
(LowerCaseEqualsASCII(mime_type, kTextPlain)) {
nit: no need for "else" after "return"

https://codereview.chromium.org/22254005/diff/77001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.h (right):

https://codereview.chromium.org/22254005/diff/77001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:17: using WebKit::WebFrame;
nit: using declarations are not allowed in header files as they pollute other
.cc files.

https://codereview.chromium.org/22254005/diff/77001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:57: struct ResponseMetaData {
nit: Perhaps this should be inside an internal namespace or made an inner class
of SiteIsolationPolicy since it is not intended as public API?

https://codereview.chromium.org/22254005/diff/77001/webkit/child/weburlloader...
File webkit/child/weburlloader_impl.cc (right):

https://codereview.chromium.org/22254005/diff/77001/webkit/child/weburlloader...
webkit/child/weburlloader_impl.cc:34: #include
"webkit/child/site_isolation_policy.h"
We are trying not to add more code to src/webkit/ as it is all intended to be
moved to src/content/, so this change is creating more work for someone.

Of course it would be a layering violation to call code from src/content/ in
src/webkit/.

One idea would be to move your metrics gathering to src/content/ entirely.  I
think you could easily move this code to
src/content/child/resource_dispatcher.cc, and then the site_isolation_policy
would be able to be src/content/ as well.

https://codereview.chromium.org/22254005/diff/77001/webkit/child/weburlloader...
webkit/child/weburlloader_impl.cc:378: // TODO(dsjang): A temporary hack to
connect the lastest response observed by
nit: lastest -> latest

[dsjang, 2013-08-14 20:47:03]

As darin suggested, site_isolation_policy.cc is moved to /content/child. I'd
like to get reviews about the hooking code in resource_dispatcher.* and the code
dealing with "<!--" for HTML sniffing in SiteIsolationPolicy::SniffForHTML().

https://codereview.chromium.org/22254005/diff/77001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/77001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:122: #define
SITE_ISOLATION_POLICY_COUNT_BLOCK(BUCKET_PREFIX) \
We're forced to use a constant string bucket name for UMA_HISTOGRAM_COUNTS in my
experience. I looked at the code of histogram, and I couldn't find any way to
use a variable string for a bucket name. Please let me know if there's a way.

On 2013/08/14 05:23:27, darin wrote:
> nit: Can you do all of this without macros?  Even if it means doing some
string
> concatenation at runtime, I suspect it'll be worth making the code more
> readable.

https://codereview.chromium.org/22254005/diff/77001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:283: static const char kTextHtml[] =
"text/html";
On 2013/08/14 05:23:27, darin wrote:
> nit: for constants like these, it is more canonical to put them at file scope
in
> an anonymous namespace sans static keyword.

Done.

https://codereview.chromium.org/22254005/diff/77001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.cc:296: } else if
(LowerCaseEqualsASCII(mime_type, kTextPlain)) {
On 2013/08/14 05:23:27, darin wrote:
> nit: no need for "else" after "return"

Done.

https://codereview.chromium.org/22254005/diff/77001/webkit/child/site_isolati...
File webkit/child/site_isolation_policy.h (right):

https://codereview.chromium.org/22254005/diff/77001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:17: using WebKit::WebFrame;
On 2013/08/14 05:23:27, darin wrote:
> nit: using declarations are not allowed in header files as they pollute other
> .cc files.

Done.

https://codereview.chromium.org/22254005/diff/77001/webkit/child/site_isolati...
webkit/child/site_isolation_policy.h:57: struct ResponseMetaData {
On 2013/08/14 05:23:27, darin wrote:
> nit: Perhaps this should be inside an internal namespace or made an inner
class
> of SiteIsolationPolicy since it is not intended as public API?

Done.

https://codereview.chromium.org/22254005/diff/77001/webkit/child/weburlloader...
File webkit/child/weburlloader_impl.cc (right):

https://codereview.chromium.org/22254005/diff/77001/webkit/child/weburlloader...
webkit/child/weburlloader_impl.cc:34: #include
"webkit/child/site_isolation_policy.h"
On 2013/08/14 05:23:27, darin wrote:
> We are trying not to add more code to src/webkit/ as it is all intended to be
> moved to src/content/, so this change is creating more work for someone.
> 
> Of course it would be a layering violation to call code from src/content/ in
> src/webkit/.
> 
> One idea would be to move your metrics gathering to src/content/ entirely.  I
> think you could easily move this code to
> src/content/child/resource_dispatcher.cc, and then the site_isolation_policy
> would be able to be src/content/ as well.

Done.

https://codereview.chromium.org/22254005/diff/77001/webkit/child/weburlloader...
webkit/child/weburlloader_impl.cc:378: // TODO(dsjang): A temporary hack to
connect the lastest response observed by
On 2013/08/14 05:23:27, darin wrote:
> nit: lastest -> latest

Done.

[dsjang, 2013-08-16 17:52:40]

SiteIsolationPolicy is converted to a ResourceLoaderBridge::Peer which wraps the
original Peer used by IPCResourceLoaderBridge. The original Peer works as a main
bridge between the IPC layer and WebURLLoader connected to WebKit.

Placing document blocking logic here has some benefits :

1) most of the data we need are already there, so bookkeeping is not needed
anymore (resource_type, first network packet)

2) blocking logic is located at the end of the renderer. So we can block
documents very early before it affects WebKit. This can help us experience the
compatibility issue in a more realistic setting: this is closer to doing the
blocking in the browser process.

The url of a frame that initiates the request is missing in IPCResourceLoader,
and I couldn't use routing_id to query RenderView there, since it violates the
content module hierarchy. Instead, I attached the URL of a frame to
RequestExtraData() in RenderFrameImpl when a request is issued.

[Charlie Reis, 2013-08-19 16:20:05]

Just a few minor comments while we wait for Darin's thoughts on the approach.

https://codereview.chromium.org/22254005/diff/159001/content/child/request_ex...
File content/child/request_extra_data.h (right):

https://codereview.chromium.org/22254005/diff/159001/content/child/request_ex...
content/child/request_extra_data.h:51: WebKit::WebString frame_origin_;
It seems like we should store this in a GURL if possible, if that's how we're
going to use it.

https://codereview.chromium.org/22254005/diff/159001/content/child/resource_d...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/22254005/diff/159001/content/child/resource_d...
content/child/resource_dispatcher.cc:144: // TODO(dsjang): Consult the expert
when this branch is taken.
What are you trying to figure out?

https://codereview.chromium.org/22254005/diff/159001/content/child/resource_d...
content/child/resource_dispatcher.cc:185: // is security irrelevant.
Any luck tracking this down?

https://codereview.chromium.org/22254005/diff/159001/content/child/resource_d...
content/child/resource_dispatcher.cc:189: // This is buggy.
You're storing it in a normal pointer.  :)

https://codereview.chromium.org/22254005/diff/159001/content/child/resource_d...
content/child/resource_dispatcher.cc:366: 
nit: Remove empty line.

https://codereview.chromium.org/22254005/diff/159001/content/child/site_isola...
File content/child/site_isolation_policy.h (right):

https://codereview.chromium.org/22254005/diff/159001/content/child/site_isola...
content/child/site_isolation_policy.h:132:
webkit_glue::ResourceLoaderBridge::Peer* original_peer_;
Style nit: Fields go below methods.

https://codereview.chromium.org/22254005/diff/159001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/22254005/diff/159001/content/renderer/render_...
content/renderer/render_frame_impl.cc:70: 
nit: No extra line here.

[dsjang, 2013-08-19 21:37:17]

https://codereview.chromium.org/22254005/diff/159001/content/child/request_ex...
File content/child/request_extra_data.h (right):

https://codereview.chromium.org/22254005/diff/159001/content/child/request_ex...
content/child/request_extra_data.h:51: WebKit::WebString frame_origin_;
The first object we get from WebKit is SecurityOrigin. Can we just store
SecurityOrigin here? It belongs to WebCore.

On 2013/08/19 16:20:06, creis wrote:
> It seems like we should store this in a GURL if possible, if that's how we're
> going to use it.

https://codereview.chromium.org/22254005/diff/159001/content/child/resource_d...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/22254005/diff/159001/content/child/resource_d...
content/child/resource_dispatcher.cc:144: // TODO(dsjang): Consult the expert
when this branch is taken.
I'm trying to make sure that the lack of request_info.extra_data implies that
this request is not-security relevant.

On 2013/08/19 16:20:06, creis wrote:
> What are you trying to figure out?

https://codereview.chromium.org/22254005/diff/159001/content/child/resource_d...
content/child/resource_dispatcher.cc:185: // is security irrelevant.
Resolved this issue, and added a comment about the situation.

On 2013/08/19 16:20:06, creis wrote:
> Any luck tracking this down?

https://codereview.chromium.org/22254005/diff/159001/content/child/resource_d...
content/child/resource_dispatcher.cc:189: // This is buggy.
On 2013/08/19 16:20:06, creis wrote:
> You're storing it in a normal pointer.  :)

Done.

https://codereview.chromium.org/22254005/diff/159001/content/child/resource_d...
content/child/resource_dispatcher.cc:366: 
On 2013/08/19 16:20:06, creis wrote:
> nit: Remove empty line.

Done.

https://codereview.chromium.org/22254005/diff/159001/content/child/site_isola...
File content/child/site_isolation_policy.h (right):

https://codereview.chromium.org/22254005/diff/159001/content/child/site_isola...
content/child/site_isolation_policy.h:132:
webkit_glue::ResourceLoaderBridge::Peer* original_peer_;
On 2013/08/19 16:20:06, creis wrote:
> Style nit: Fields go below methods.

Done.

https://codereview.chromium.org/22254005/diff/159001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/22254005/diff/159001/content/renderer/render_...
content/renderer/render_frame_impl.cc:70: 
On 2013/08/19 16:20:06, creis wrote:
> nit: No extra line here.

Done.

[darin (slow to review), 2013-08-20 23:47:05]

If I were writing this patch, I probably wouldn't attempt chaining
ResourceLoaderBridge::Peer objects because of the fact that they get re-created
and the SiteIsolationPolicy object is not really interested in all of the
events.

It seems like what you had in a previous version of the patch, in which
SiteIsolationPolicy had a set of non-virtual public methods, was simpler.  It is
OK for ResourceDispatcher to know about SiteIsolationPolicy (since it is
responsible for creating it), and it is OK for ResourceDispatcher to know to
poke SiteIsolationPolicy at particular times.  I think the result will be
simpler.

https://codereview.chromium.org/22254005/diff/123002/content/child/request_ex...
File content/child/request_extra_data.cc (right):

https://codereview.chromium.org/22254005/diff/123002/content/child/request_ex...
content/child/request_extra_data.cc:8: using WebKit::WebSecurityOrigin;
nit: not needed

https://codereview.chromium.org/22254005/diff/123002/content/child/request_ex...
File content/child/request_extra_data.h (right):

https://codereview.chromium.org/22254005/diff/123002/content/child/request_ex...
content/child/request_extra_data.h:11: #include
"third_party/WebKit/public/web/WebSecurityOrigin.h"
nit: looks like WebSecurityOrigin.h is not needed.

https://codereview.chromium.org/22254005/diff/123002/content/child/resource_d...
File content/child/resource_dispatcher.h (right):

https://codereview.chromium.org/22254005/diff/123002/content/child/resource_d...
content/child/resource_dispatcher.h:47: int
AddPendingRequest(SiteIsolationPolicy* callback,
nit: it is a bit confusing to refer to something that is a "policy" as a
"callback"

https://codereview.chromium.org/22254005/diff/123002/content/child/site_isola...
File content/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/123002/content/child/site_isola...
content/child/site_isolation_policy.cc:289: original_peer_->OnReceivedData(data,
length, encoded_data_length);
nit: having the same code in both branches indicates that the code should be
moved to after the branch.  there is no need for "else" here.

it looks like you have made this code capable of not only performing
measurements but also being close to what would be needed for enforcement. 
maybe it would be better to just focus initially on what is needed to perform
measurements.

[dsjang, 2013-08-21 18:49:56]

SiteIsolationPolicy is converted into a purely static class, 
a command-line argument --cross-site-document-blocking is added to turn on
actual blocking. Test cases are added to test blocking too.

https://codereview.chromium.org/22254005/diff/123002/content/child/request_ex...
File content/child/request_extra_data.cc (right):

https://codereview.chromium.org/22254005/diff/123002/content/child/request_ex...
content/child/request_extra_data.cc:8: using WebKit::WebSecurityOrigin;
On 2013/08/20 23:47:05, darin wrote:
> nit: not needed

Done.

https://codereview.chromium.org/22254005/diff/123002/content/child/request_ex...
File content/child/request_extra_data.h (right):

https://codereview.chromium.org/22254005/diff/123002/content/child/request_ex...
content/child/request_extra_data.h:11: #include
"third_party/WebKit/public/web/WebSecurityOrigin.h"
On 2013/08/20 23:47:05, darin wrote:
> nit: looks like WebSecurityOrigin.h is not needed.

Done.

https://codereview.chromium.org/22254005/diff/123002/content/child/resource_d...
File content/child/resource_dispatcher.h (right):

https://codereview.chromium.org/22254005/diff/123002/content/child/resource_d...
content/child/resource_dispatcher.h:47: int
AddPendingRequest(SiteIsolationPolicy* callback,
On 2013/08/20 23:47:05, darin wrote:
> nit: it is a bit confusing to refer to something that is a "policy" as a
> "callback"

Done.

https://codereview.chromium.org/22254005/diff/123002/content/child/site_isola...
File content/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/123002/content/child/site_isola...
content/child/site_isolation_policy.cc:289: original_peer_->OnReceivedData(data,
length, encoded_data_length);
I'm switching back to the older version of SiteIsolationPolicy with only static
members, and the enforcement part is moved from SiteIsolationPolicy to 
ResourceDispatcher::OnReceivedData(), which calls
SiteIsolationPolicy::OnReceivedData().

On 2013/08/20 23:47:05, darin wrote:
> nit: having the same code in both branches indicates that the code should be
> moved to after the branch.  there is no need for "else" here.
> 
> it looks like you have made this code capable of not only performing
> measurements but also being close to what would be needed for enforcement. 
> maybe it would be better to just focus initially on what is needed to perform
> measurements.

[dsjang, 2013-08-22 00:15:11]

Blocking logic gets simpler, and test cases for SiteIsolationPolicy are moved to
/content from /chrome.

[Charlie Reis, 2013-08-22 18:23:29]

On one hand, this CL is a bit larger due to including the blocking logic and
blocking tests, but on the other hand, the blocking tests are nice in that they
demonstrate we will be measuring the right things.  I'm always a fan of tests,
especially when measurements are involved.

Lots of minor cleanup comments below.

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
content/child/resource_dispatcher.cc:102: // The security origin of the frame
that initiates this request
nit: End with period.

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
content/child/resource_dispatcher.cc:428: bool ok_response =
SiteIsolationPolicy::OnReceivedData(
This name doesn't describe what the function is doing.  Perhaps something more
like block_response = SiteIsolationPolicy::ShouldBlockResponse?

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
content/child/resource_dispatcher.cc:435: // if data_offset is not-zero, this is
not the first data packet for
Nit: capitalize sentence.

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
content/child/resource_dispatcher.cc:439: // one-character string as response
data : this is needed to trigger
nit: Remove the colon and just have two sentences.

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
content/child/resource_dispatcher.cc:500: // We update the response_url here so
that we can sent it to
nit: sent -> send

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
content/child/resource_dispatcher.cc:530: // be a better way to connect the
received data to the response_url
I don't understand this comment.
1) Are you planning to remove this when we're done logging, even if the blocking
code stays?
2) What's wrong with this way?

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
content/child/resource_dispatcher.cc:553: 
nit: Don't add extra line.

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
File content/child/resource_dispatcher.h (right):

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
content/child/resource_dispatcher.h:98: // Original requested url
nit: End with period.

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
File content/child/resource_dispatcher_unittest.cc (right):

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
content/child/resource_dispatcher_unittest.cc:180: GURL(),
Style nit: There's room for GURL() on the existing line, right?  No need to
split it into 3 lines.

https://codereview.chromium.org/22254005/diff/208001/content/child/site_isola...
File content/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/208001/content/child/site_isola...
content/child/site_isolation_policy.cc:258: 
nit: Only one blank line between blocks/functions, here and below.

https://codereview.chromium.org/22254005/diff/208001/content/child/site_isola...
File content/child/site_isolation_policy.h (right):

https://codereview.chromium.org/22254005/diff/208001/content/child/site_isola...
content/child/site_isolation_policy.h:63: // TODO(dsjang): There's a possibility
that two distinct responses (identified
Is this still possible?  It looks like we always use request ID now.

https://codereview.chromium.org/22254005/diff/208001/content/child/site_isola...
File content/child/site_isolation_policy_browsertest.cc (right):

https://codereview.chromium.org/22254005/diff/208001/content/child/site_isola...
content/child/site_isolation_policy_browsertest.cc:13: // The goal of these
tests is to "simulate" exploited renderer processes, which
The goal of these tests is to "simulate" -> These tests simulate

https://codereview.chromium.org/22254005/diff/208001/content/child/site_isola...
content/child/site_isolation_policy_browsertest.cc:17: // invalidated inside the
renderer.
invalidated -> turned off

https://codereview.chromium.org/22254005/diff/208001/content/child/site_isola...
content/child/site_isolation_policy_browsertest.cc:56: // inspect if any of them
were successfully received. The XHR requests will
inspects

https://codereview.chromium.org/22254005/diff/208001/content/child/site_isola...
content/child/site_isolation_policy_browsertest.cc:65: std::string status;
Can you add a comment here saying that the test will return 1 from the
DOMAutomationController if it succeeds?  Otherwise it's hard to tell what's
happening here.

https://codereview.chromium.org/22254005/diff/208001/content/public/common/co...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/22254005/diff/208001/content/public/common/co...
content/public/common/content_switches.cc:51: const char
kCrossSiteDocumentBlocking[]     = "cross-site-document-blocking";
Can you rename this to kBlockCrossSiteDocuments (block-cross-site-documents)?

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
File content/test/data/cross_site_document_request.html (right):

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request.html:4: /* This test shows that
cross-site documents are blocked by SiteIsolationPolicy
Style nit: We don't tend to use /* */ comments for documentation.  It would be
fine to put this description in the body of the page, or just use // comments
for it.

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request.html:5: even if the Same Origin
Policy is turned on in the renderer. The same origin
nit: turned off

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request.html:15: text/xml,
application/json), and correctly sniffed as the content type that they
and -> that are

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request.html:16: claim to be. We also
block text/plain documents when their body look like one of
looks

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request.html:33: if
((blockedResourceUrls.indexOf(resourceUrl) != -1 && xhr.responseText != " ") ||
80 chars isn't required in HTML files, but it might make this easier to read.

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request.html:38:
//domAutomationController.send(0);
Should this be uncommented?

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request.html:46: ("\n" + prefix +
"response to " + resourceUrl + "(" + xhr.getResponseHeader("content-type") + ")
" + (xhr.responseText == " " ? "blocked" : "not-blocked"));
nit: Please wrap this line.

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request.html:60: //all the test cases are
successfully passed.
nit: Capitalize All and put a space before it.

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request.html:70:
'http://bar.com/files/main.html');
nit: This can all fit on one line.

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
File content/test/data/cross_site_document_request_target.html (right):

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request_target.html:2: <!-- This page
tests that the renderer process does not crash even the response
nit: Just move this text into the body of the page rather than in a comment.

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request_target.html:17: <!-- image -->
nit: No need for the redundant HTML comment if you label it in the body of the
page.

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request_target.html:30: should be fixed
later.  -->
Can you clarify what "should be fixed" means?

https://codereview.chromium.org/22254005/diff/208001/content/test/data/site_i...
File content/test/data/site_isolation/comment_js.html (right):

https://codereview.chromium.org/22254005/diff/208001/content/test/data/site_i...
content/test/data/site_isolation/comment_js.html:1: <!-- this is a valid
javascript content -->
nit for all of these files: Capitalize sentence, end with a period.

content -> document
Maybe mention "despite having an HTML comment."

https://codereview.chromium.org/22254005/diff/208001/net/tools/testserver/tes...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/22254005/diff/208001/net/tools/testserver/tes...
net/tools/testserver/testserver.py:303: 'txt' : 'text/plain'
nit: Alphabetize.

[dsjang, 2013-08-22 19:05:54]

Comments and naming are improved

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
content/child/resource_dispatcher.cc:102: // The security origin of the frame
that initiates this request
On 2013/08/22 18:23:30, creis wrote:
> nit: End with period.

Done.

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
content/child/resource_dispatcher.cc:428: bool ok_response =
SiteIsolationPolicy::OnReceivedData(
On 2013/08/22 18:23:30, creis wrote:
> This name doesn't describe what the function is doing.  Perhaps something more
> like block_response = SiteIsolationPolicy::ShouldBlockResponse?

Done.

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
content/child/resource_dispatcher.cc:435: // if data_offset is not-zero, this is
not the first data packet for
On 2013/08/22 18:23:30, creis wrote:
> Nit: capitalize sentence.

Done.

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
content/child/resource_dispatcher.cc:439: // one-character string as response
data : this is needed to trigger
On 2013/08/22 18:23:30, creis wrote:
> nit: Remove the colon and just have two sentences.

Done.

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
content/child/resource_dispatcher.cc:500: // We update the response_url here so
that we can sent it to
On 2013/08/22 18:23:30, creis wrote:
> nit: sent -> send

Done.

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
content/child/resource_dispatcher.cc:530: // be a better way to connect the
received data to the response_url
With the blocking logic, everything about SiteIsolationPolicy will remain here.
The comment was just an old one to go away.

On 2013/08/22 18:23:30, creis wrote:
> I don't understand this comment.
> 1) Are you planning to remove this when we're done logging, even if the
blocking
> code stays?
> 2) What's wrong with this way?

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
content/child/resource_dispatcher.cc:553: 
On 2013/08/22 18:23:30, creis wrote:
> nit: Don't add extra line.

Done.

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
File content/child/resource_dispatcher.h (right):

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
content/child/resource_dispatcher.h:98: // Original requested url
On 2013/08/22 18:23:30, creis wrote:
> nit: End with period.

Done.

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
File content/child/resource_dispatcher_unittest.cc (right):

https://codereview.chromium.org/22254005/diff/208001/content/child/resource_d...
content/child/resource_dispatcher_unittest.cc:180: GURL(),
On 2013/08/22 18:23:30, creis wrote:
> Style nit: There's room for GURL() on the existing line, right?  No need to
> split it into 3 lines.

Done.

https://codereview.chromium.org/22254005/diff/208001/content/child/site_isola...
File content/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/208001/content/child/site_isola...
content/child/site_isolation_policy.cc:258: 
On 2013/08/22 18:23:30, creis wrote:
> nit: Only one blank line between blocks/functions, here and below.

Done.

https://codereview.chromium.org/22254005/diff/208001/content/child/site_isola...
File content/child/site_isolation_policy.h (right):

https://codereview.chromium.org/22254005/diff/208001/content/child/site_isola...
content/child/site_isolation_policy.h:63: // TODO(dsjang): There's a possibility
that two distinct responses (identified
On 2013/08/22 18:23:30, creis wrote:
> Is this still possible?  It looks like we always use request ID now.

Done.

https://codereview.chromium.org/22254005/diff/208001/content/child/site_isola...
File content/child/site_isolation_policy_browsertest.cc (right):

https://codereview.chromium.org/22254005/diff/208001/content/child/site_isola...
content/child/site_isolation_policy_browsertest.cc:13: // The goal of these
tests is to "simulate" exploited renderer processes, which
On 2013/08/22 18:23:30, creis wrote:
> The goal of these tests is to "simulate" -> These tests simulate

Done.

https://codereview.chromium.org/22254005/diff/208001/content/child/site_isola...
content/child/site_isolation_policy_browsertest.cc:17: // invalidated inside the
renderer.
On 2013/08/22 18:23:30, creis wrote:
> invalidated -> turned off

Done.

https://codereview.chromium.org/22254005/diff/208001/content/child/site_isola...
content/child/site_isolation_policy_browsertest.cc:56: // inspect if any of them
were successfully received. The XHR requests will
On 2013/08/22 18:23:30, creis wrote:
> inspects

Done.

https://codereview.chromium.org/22254005/diff/208001/content/child/site_isola...
content/child/site_isolation_policy_browsertest.cc:65: std::string status;
On 2013/08/22 18:23:30, creis wrote:
> Can you add a comment here saying that the test will return 1 from the
> DOMAutomationController if it succeeds?  Otherwise it's hard to tell what's
> happening here.

Done.

https://codereview.chromium.org/22254005/diff/208001/content/public/common/co...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/22254005/diff/208001/content/public/common/co...
content/public/common/content_switches.cc:51: const char
kCrossSiteDocumentBlocking[]     = "cross-site-document-blocking";
On 2013/08/22 18:23:30, creis wrote:
> Can you rename this to kBlockCrossSiteDocuments (block-cross-site-documents)?

Done.

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
File content/test/data/cross_site_document_request.html (right):

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request.html:4: /* This test shows that
cross-site documents are blocked by SiteIsolationPolicy
On 2013/08/22 18:23:30, creis wrote:
> Style nit: We don't tend to use /* */ comments for documentation.  It would be
> fine to put this description in the body of the page, or just use // comments
> for it.

Done.

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request.html:5: even if the Same Origin
Policy is turned on in the renderer. The same origin
On 2013/08/22 18:23:30, creis wrote:
> nit: turned off

Done.

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request.html:15: text/xml,
application/json), and correctly sniffed as the content type that they
On 2013/08/22 18:23:30, creis wrote:
> and -> that are

Done.

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request.html:16: claim to be. We also
block text/plain documents when their body look like one of
On 2013/08/22 18:23:30, creis wrote:
> looks

Done.

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request.html:33: if
((blockedResourceUrls.indexOf(resourceUrl) != -1 && xhr.responseText != " ") ||
On 2013/08/22 18:23:30, creis wrote:
> 80 chars isn't required in HTML files, but it might make this easier to read.

Done.

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request.html:38:
//domAutomationController.send(0);
On 2013/08/22 18:23:30, creis wrote:
> Should this be uncommented?

Done.

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request.html:46: ("\n" + prefix +
"response to " + resourceUrl + "(" + xhr.getResponseHeader("content-type") + ")
" + (xhr.responseText == " " ? "blocked" : "not-blocked"));
On 2013/08/22 18:23:30, creis wrote:
> nit: Please wrap this line.

Done.

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request.html:60: //all the test cases are
successfully passed.
On 2013/08/22 18:23:30, creis wrote:
> nit: Capitalize All and put a space before it.

Done.

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request.html:70:
'http://bar.com/files/main.html');
On 2013/08/22 18:23:30, creis wrote:
> nit: This can all fit on one line.

Done.

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
File content/test/data/cross_site_document_request_target.html (right):

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request_target.html:2: <!-- This page
tests that the renderer process does not crash even the response
On 2013/08/22 18:23:30, creis wrote:
> nit: Just move this text into the body of the page rather than in a comment.

Done.

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request_target.html:17: <!-- image -->
On 2013/08/22 18:23:30, creis wrote:
> nit: No need for the redundant HTML comment if you label it in the body of the
> page.

Done.

https://codereview.chromium.org/22254005/diff/208001/content/test/data/cross_...
content/test/data/cross_site_document_request_target.html:30: should be fixed
later.  -->
On 2013/08/22 18:23:30, creis wrote:
> Can you clarify what "should be fixed" means?

Done.

https://codereview.chromium.org/22254005/diff/208001/content/test/data/site_i...
File content/test/data/site_isolation/comment_js.html (right):

https://codereview.chromium.org/22254005/diff/208001/content/test/data/site_i...
content/test/data/site_isolation/comment_js.html:1: <!-- this is a valid
javascript content -->
On 2013/08/22 18:23:30, creis wrote:
> nit for all of these files: Capitalize sentence, end with a period.
> 
> content -> document
> Maybe mention "despite having an HTML comment."

Done.

https://codereview.chromium.org/22254005/diff/208001/net/tools/testserver/tes...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/22254005/diff/208001/net/tools/testserver/tes...
net/tools/testserver/testserver.py:303: 'txt' : 'text/plain'
On 2013/08/22 18:23:30, creis wrote:
> nit: Alphabetize.

Done.

[darin (slow to review), 2013-08-22 21:50:42]

https://codereview.chromium.org/22254005/diff/161002/content/child/resource_d...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/22254005/diff/161002/content/child/resource_d...
content/child/resource_dispatcher.cc:425: 
perhaps another option here is to call a method on SiteIsolationPolicy, and have
that optionally return a std::string that contains alternate response data? 
then you could keep most of this code with the SiteIsolationPolicy class.

[dsjang, 2013-08-22 22:30:19]

Blocking code is moved to SiteIsolationPolicy from ResourceDispatcher

https://codereview.chromium.org/22254005/diff/161002/content/child/resource_d...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/22254005/diff/161002/content/child/resource_d...
content/child/resource_dispatcher.cc:425: 
On 2013/08/22 21:50:42, darin wrote:
> perhaps another option here is to call a method on SiteIsolationPolicy, and
have
> that optionally return a std::string that contains alternate response data? 
> then you could keep most of this code with the SiteIsolationPolicy class.

Done.

[darin (slow to review), 2013-08-22 22:45:34]

OK, looks better.  Thanks!

Deferring to Charlie on most of this review.  Integration code LGTM.

I think it would be good cleanup for this system if we built the chain of
peers once so you could *easily* chain peers.  But I wouldn't attempt that
as part of this CL ;-)
On Aug 22, 2013 3:30 PM, <dsjang@chromium.org> wrote:

> Blocking code is moved to SiteIsolationPolicy from ResourceDispatcher
>
>
> https://codereview.chromium.**org/22254005/diff/161002/**
>
content/child/resource_**dispatcher.cc<https://codereview.chromium.org/22254005/diff/161002/content/child/resource_dispatcher.cc>
> File content/child/resource_**dispatcher.cc (right):
>
> https://codereview.chromium.**org/22254005/diff/161002/**
>
content/child/resource_**dispatcher.cc#newcode425<https://codereview.chromium.org/22254005/diff/161002/content/child/resource_dispatcher.cc#newcode425>
> content/child/resource_**dispatcher.cc:425:
> On 2013/08/22 21:50:42, darin wrote:
>
>> perhaps another option here is to call a method on
>>
> SiteIsolationPolicy, and have
>
>> that optionally return a std::string that contains alternate response
>>
> data?
>
>> then you could keep most of this code with the SiteIsolationPolicy
>>
> class.
>
> Done.
>
>
https://codereview.chromium.**org/22254005/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Charlie Reis, 2013-08-22 23:05:22]

LGTM with the changes below.

https://codereview.chromium.org/22254005/diff/123003/content/child/site_isola...
File content/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/123003/content/child/site_isola...
content/child/site_isolation_policy.cc:168: //alternative_data->insert(0, " ");
This should be uncommented or removed.  If I had to guess, I'd say removed,
since this only happens after the first data packet?  (If so, maybe it's worth
explaining that alternative_data should be empty in the comment on line 163?)

https://codereview.chromium.org/22254005/diff/123003/content/child/site_isola...
File content/child/site_isolation_policy_browsertest.cc (right):

https://codereview.chromium.org/22254005/diff/123003/content/child/site_isola...
content/child/site_isolation_policy_browsertest.cc:65: // The page will returns
1 from the DOMAutomationController if it succeeds,
nit: return

https://codereview.chromium.org/22254005/diff/123003/content/child/site_isola...
File content/child/site_isolation_policy_unittest.cc (right):

https://codereview.chromium.org/22254005/diff/123003/content/child/site_isola...
content/child/site_isolation_policy_unittest.cc:125: }  // namespace conten
nit: content

https://codereview.chromium.org/22254005/diff/123003/content/public/common/co...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/22254005/diff/123003/content/public/common/co...
content/public/common/content_switches.cc:41: // Block cross-site
document(HTML/XML/JSON) from a renderer process.
Let's elaborate on this a bit, since people use it to understand what a flag
will do:

Block cross-site documents (i.e., HTML/XML/JSON) from being loaded in
subresources when a document is not supposed to read them.  This will later
allow us to block them from the entire renderer process when site isolation is
enabled.

[commit-bot: I haz the power, 2013-08-23 01:30:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dsjang@chromium.org/22254005/226001

[commit-bot: I haz the power, 2013-08-23 01:47:04]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-08-23 01:50:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dsjang@chromium.org/22254005/95002

[commit-bot: I haz the power, 2013-08-23 12:18:13]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-08-23 19:24:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dsjang@chromium.org/22254005/159004

[commit-bot: I haz the power, 2013-08-23 19:51:22]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-23 20:30:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dsjang@chromium.org/22254005/239001

[commit-bot: I haz the power, 2013-08-23 23:55:41]

Change committed as 219383

[awong, 2013-08-26 20:31:40]

Most of this these are style comments.  Please start a new CL for these
comments.

https://codereview.chromium.org/22254005/diff/239001/content/child/site_isola...
File content/child/site_isolation_policy.cc (right):

https://codereview.chromium.org/22254005/diff/239001/content/child/site_isola...
content/child/site_isolation_policy.cc:45: }  // anonymous namespace
nit:

}  // namespace

https://codereview.chromium.org/22254005/diff/239001/content/child/site_isola...
content/child/site_isolation_policy.cc:51: GURL& frame_origin,
non-const refs are not allowed in function signatures by style guide. Please
remove all uses of non-const references.

https://codereview.chromium.org/22254005/diff/239001/content/child/site_isola...
content/child/site_isolation_policy.cc:257: "Not a blockable mime type. This
mime type shouldn't reach here.";
Also output the mime_type.
NOTREACHED() << "Not a blockable mime type: " << resp_data.canonical_mime_type;

You don't need the "shouldn't reach here" because the assert itself already says
thsi.

https://codereview.chromium.org/22254005/diff/239001/content/child/site_isola...
content/child/site_isolation_policy.cc:261: const CommandLine& command_line =
*CommandLine::ForCurrentProcess();
Might as well keep this a pointer so it doesn't look like a local.

https://codereview.chromium.org/22254005/diff/239001/content/child/site_isola...
content/child/site_isolation_policy.cc:277: #undef
SITE_ISOLATION_POLICY_COUNT_NOTBLOCK
How bad does this look if you expand out the macros?

Using macros often has long term consequences.  Aside from being hard to read,
it breaks code analysis tools, refactoring tools, and often makes debugging/
crash parts harder because it messes with the line numbers.

https://codereview.chromium.org/22254005/diff/239001/content/child/site_isola...
content/child/site_isolation_policy.cc:358: std::string access_control_origin) {
Should this be const std::string&?

https://codereview.chromium.org/22254005/diff/239001/content/child/site_isola...
content/child/site_isolation_policy.cc:383: bool
SiteIsolationPolicy::SniffForHTML(const char* data, size_t length) {
Converting all these functions to taking a StringPiece seems like a safer
api...though we should double check that the copy behavior of StringPiece.

https://codereview.chromium.org/22254005/diff/239001/content/child/site_isola...
content/child/site_isolation_policy.cc:393: const char* html_signatures[] =
{"<!DOCTYPE html",  // HTML5 spec
General locally scoped constants should be declared static.

As is, this directs the compiler to, per function invocation, perform one memcpy
copy of the array onto the stack.

Also, constants should be named kHtmlSignatures as per style guide.

Please fix here and elsewhere.

Also, I think this is better formatted as

const char* kHtmlSignatures {
    "<!DOCTYPE html",  // HTML5 spec
    ...
};

The alignment is easier to read.

https://codereview.chromium.org/22254005/diff/239001/content/child/site_isola...
content/child/site_isolation_policy.cc:413: return true;
with a linebreak in the conditional, use braces.

https://codereview.chromium.org/22254005/diff/239001/content/child/site_isola...
content/child/site_isolation_policy.cc:429: return SniffForHTML(data + skipped,
length - skipped);
This recursion is based on data from the user (the network in this case).  How
are you bounding the recursion depth?

https://codereview.chromium.org/22254005/diff/239001/content/child/site_isola...
content/child/site_isolation_policy.cc:543: if (strncmp(data + i, "var ", 4) ==
0)
For safe use of strncmp, your "n" should always be based on the minmal size of
the two buffers.

In particular, if one of your buffers is known to be null-terminated and is
constant, then setting "n" to that size doesn't increase safety at all.

Here, length - i, is the conceptually correct "n" for safety (though that still
needs to be paired with a length > sizeof(kVar) check otherwise data with value
"va" would match).

https://codereview.chromium.org/22254005/diff/239001/content/child/site_isola...
content/child/site_isolation_policy.cc:551:
CR_DEFINE_STATIC_LOCAL(RequestIdToMetaDataMap, metadata_map_, ());
This variable is declared like a static local, but really you're using it as a
global.  Would a LazyInstance declared at the top of this file be more
appropriate?

https://codereview.chromium.org/22254005/diff/239001/content/child/site_isola...
File content/child/site_isolation_policy.h (right):

https://codereview.chromium.org/22254005/diff/239001/content/child/site_isola...
content/child/site_isolation_policy.h:81: struct ResponseMetaData {
Is there a reason for these types to be public?

For that matter...for each of the private functions, unless you need to expose
it for testing, can they made to be file-local to the implementation?