Make ContentSettingsObserver security checks work with OOPIF.

Make ContentSettingsObserver security checks work with OOPIF.

ContentSettingsObserver has several security checks that use the current frame's and the top frame's WebSecurityOrigin.  For example, these checks are used when accessing localStorage, indexedDB, openDatabase, and webkitRequestFileSystem.  With --site-per-process, when the top frame is remote, these checks crashed the renderer because they tried to get the SecurityOrigin of the top frame's Document, which doesn't exist.

This CL fixes these checks to access SecurityOrigins on frames, where the remote case is handled properly as part of origin replication (https://crbug.com/426512).  It also adds an OOPIF browser test to ensure that the remote case works.

BUG=426512
Committed: https://crrev.com/75648bb31c04beca9c3a2507343ab2bf4a5bbae0
Cr-Commit-Position: refs/heads/master@{#310366}

[alexmos, 2014-12-11 18:37:07]

alexmos@chromium.org changed reviewers:
+ creis@chromium.org

[alexmos, 2014-12-11 18:45:18]

Charlie, could you please take a look?  I'm converting a few security checks in
ContentSettingsObserver to use origin replication and adding a new OOPIF-only
browser test to check that the localStorage and similar APIs no longer crash us.
 As part of this, I moved NavigateIframeToURL into a public file.  Nick
suggested considering whether to make the test work both with and without OOPIF,
but this particular test seems specific to --site-per-process, similar to
site_per_process_browsertest, so I ended up not going there.

https://codereview.chromium.org/789273006/diff/20001/chrome/browser/chrome_si...
File chrome/browser/chrome_site_per_process_browsertest.cc (right):

https://codereview.chromium.org/789273006/diff/20001/chrome/browser/chrome_si...
chrome/browser/chrome_site_per_process_browsertest.cc:27: void FindFrame(const
GURL& url,
I borrowed this from extension_webui_apitest.cc.  Is it worth bringing it out to
a public place to avoid the duplication?  Or maybe there's a better way to get
the subframe's RFH altogether?

[Charlie Reis, 2014-12-12 18:02:45]

Great!  I'm glad to see the replication start to fix issues.  One question about
the TODO and a few nits.

https://codereview.chromium.org/789273006/diff/20001/chrome/browser/chrome_si...
File chrome/browser/chrome_site_per_process_browsertest.cc (right):

https://codereview.chromium.org/789273006/diff/20001/chrome/browser/chrome_si...
chrome/browser/chrome_site_per_process_browsertest.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
Copyright 2014

https://codereview.chromium.org/789273006/diff/20001/chrome/browser/chrome_si...
chrome/browser/chrome_site_per_process_browsertest.cc:27: void FindFrame(const
GURL& url,
On 2014/12/11 18:45:18, alexmos wrote:
> I borrowed this from extension_webui_apitest.cc.  Is it worth bringing it out
to
> a public place to avoid the duplication?  Or maybe there's a better way to get
> the subframe's RFH altogether?

It does seem like we'd benefit from a public utility function for finding a
subframe RFH.

Using a URL to find it seems unfortunate, though, since that's not unique.  It
would be nicer to look up the frame by ID if we had the ID on the FrameTreeNode,
but we don't need to add that for this CL.  I suppose this URL version can help
in cases where the frame doesn't have an ID either.  Let's just make this public
and document the limitation of requiring the desired frame to have a unique URL.

https://codereview.chromium.org/789273006/diff/20001/chrome/browser/chrome_si...
chrome/browser/chrome_site_per_process_browsertest.cc:89: bool
is_object_created;
nit: Let's initialize this to false.

https://codereview.chromium.org/789273006/diff/20001/chrome/browser/chrome_si...
chrome/browser/chrome_site_per_process_browsertest.cc:94:
EXPECT_TRUE(is_object_created);
Let's set it back to false after this line.

https://codereview.chromium.org/789273006/diff/20001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/789273006/diff/20001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:119: // The the |top_origin| is
unique ("null") e.g., for file:// URLs. Use the
nit: Might as well fix "The the"

https://codereview.chromium.org/789273006/diff/20001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:123: // URL is not currently
replicated.
This makes me sad.  I'd like to argue against replicating the full URL if at all
possible.

Can we take a closer look at how this is used to see if they really need the URL
for unique origins?  Maybe there's a sane default behavior for unique origins
that doesn't require the URL.

https://codereview.chromium.org/789273006/diff/20001/content/public/test/brow...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/789273006/diff/20001/content/public/test/brow...
content/public/test/browser_test_utils.cc:305: // for commit.
Also mention http://crbug.com/436250.

https://codereview.chromium.org/789273006/diff/20001/content/public/test/brow...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/789273006/diff/20001/content/public/test/brow...
content/public/test/browser_test_utils.h:91: // Navigate a frame with ID
|iframe_id| to |url|, blocking until the navigation
Let's move this to the top of the file.

Also, let's clarify that this triggers a renderer-initiated navigation from
script code in the main frame, since that's relevant for how the tests behave.

https://codereview.chromium.org/789273006/diff/20001/content/public/test/brow...
content/public/test/browser_test_utils.h:95: std::string iframe_id);
Let's swap the last two arguments.  (Worth the cleanup if this is going to be a
more public utility function.)

[alexmos, 2014-12-13 00:58:13]

https://codereview.chromium.org/789273006/diff/20001/chrome/browser/chrome_si...
File chrome/browser/chrome_site_per_process_browsertest.cc (right):

https://codereview.chromium.org/789273006/diff/20001/chrome/browser/chrome_si...
chrome/browser/chrome_site_per_process_browsertest.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2014/12/12 18:02:45, Charlie Reis wrote:
> Copyright 2014

Done.

https://codereview.chromium.org/789273006/diff/20001/chrome/browser/chrome_si...
chrome/browser/chrome_site_per_process_browsertest.cc:27: void FindFrame(const
GURL& url,
On 2014/12/12 18:02:45, Charlie Reis wrote:
> On 2014/12/11 18:45:18, alexmos wrote:
> > I borrowed this from extension_webui_apitest.cc.  Is it worth bringing it
out
> to
> > a public place to avoid the duplication?  Or maybe there's a better way to
get
> > the subframe's RFH altogether?
> 
> It does seem like we'd benefit from a public utility function for finding a
> subframe RFH.
> 
> Using a URL to find it seems unfortunate, though, since that's not unique.  It
> would be nicer to look up the frame by ID if we had the ID on the
FrameTreeNode,
> but we don't need to add that for this CL.  I suppose this URL version can
help
> in cases where the frame doesn't have an ID either.  Let's just make this
public
> and document the limitation of requiring the desired frame to have a unique
URL.

Actually, I somehow missed that there's already a public function for this in
browser_test_utils.h: FrameMatchingPredicate(), which can take look for RFHs by
URL (or other predicates).  I changed my test to use that instead and removed
the extra FindFrame.

https://codereview.chromium.org/789273006/diff/20001/chrome/browser/chrome_si...
chrome/browser/chrome_site_per_process_browsertest.cc:89: bool
is_object_created;
On 2014/12/12 18:02:45, Charlie Reis wrote:
> nit: Let's initialize this to false.

Done.

https://codereview.chromium.org/789273006/diff/20001/chrome/browser/chrome_si...
chrome/browser/chrome_site_per_process_browsertest.cc:94:
EXPECT_TRUE(is_object_created);
On 2014/12/12 18:02:45, Charlie Reis wrote:
> Let's set it back to false after this line.

Done.

https://codereview.chromium.org/789273006/diff/20001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/789273006/diff/20001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:119: // The the |top_origin| is
unique ("null") e.g., for file:// URLs. Use the
On 2014/12/12 18:02:45, Charlie Reis wrote:
> nit: Might as well fix "The the"

Done.

https://codereview.chromium.org/789273006/diff/20001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:123: // URL is not currently
replicated.
On 2014/12/12 18:02:45, Charlie Reis wrote:
> This makes me sad.  I'd like to argue against replicating the full URL if at
all
> possible.
> 
> Can we take a closer look at how this is used to see if they really need the
URL
> for unique origins?  Maybe there's a sane default behavior for unique origins
> that doesn't require the URL.

Hmm.  Similarly to what lazyboy@ found in
https://codereview.chromium.org/791763003/, when the top frame is a file:// URL,
it looks like it's possible for these rules to match patterns based on path in
the URL, though there's a TODO in
https://code.google.com/p/chromium/codesearch#chromium/src/components/content...
to stop supporting path-based matching in ContentSettingsPattern::Matches.  

There's a separate issue in that currently origin replication can't tell apart
file:// origins (with enforceFilePathSeparation set to true, which is the
default for chrome) and unique origins, since they're both serialized as "null".
 I will need to fix that at some point, and then just knowing that the top frame
came from file:// may be enough here, provided the TODO to remove path matching
gets done.

I also dug around and saw this related comment thread, which seems to suggest
this was intended for file:// URLs:
https://codereview.chromium.org/7831075/diff/84016/chrome/renderer/content_se....
It would still be good to know which content settings rules could actually apply
when the top origin is "null", and whether we'd break anything if we don't
support that.  Anyone we could ask?

https://codereview.chromium.org/789273006/diff/20001/content/public/test/brow...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/789273006/diff/20001/content/public/test/brow...
content/public/test/browser_test_utils.cc:305: // for commit.
On 2014/12/12 18:02:45, Charlie Reis wrote:
> Also mention http://crbug.com/436250.

Done.

https://codereview.chromium.org/789273006/diff/20001/content/public/test/brow...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/789273006/diff/20001/content/public/test/brow...
content/public/test/browser_test_utils.h:91: // Navigate a frame with ID
|iframe_id| to |url|, blocking until the navigation
On 2014/12/12 18:02:45, Charlie Reis wrote:
> Let's move this to the top of the file.
> 
> Also, let's clarify that this triggers a renderer-initiated navigation from
> script code in the main frame, since that's relevant for how the tests behave.

Done.

https://codereview.chromium.org/789273006/diff/20001/content/public/test/brow...
content/public/test/browser_test_utils.h:95: std::string iframe_id);
On 2014/12/12 18:02:45, Charlie Reis wrote:
> Let's swap the last two arguments.  (Worth the cleanup if this is going to be
a
> more public utility function.)

Done.

[Charlie Reis, 2014-12-15 20:25:36]

LGTM, and let's follow up with content settings folks for the top-level URL
fallback case.

https://codereview.chromium.org/789273006/diff/20001/chrome/browser/chrome_si...
File chrome/browser/chrome_site_per_process_browsertest.cc (right):

https://codereview.chromium.org/789273006/diff/20001/chrome/browser/chrome_si...
chrome/browser/chrome_site_per_process_browsertest.cc:27: void FindFrame(const
GURL& url,
On 2014/12/13 00:58:12, alexmos wrote:
> On 2014/12/12 18:02:45, Charlie Reis wrote:
> > On 2014/12/11 18:45:18, alexmos wrote:
> > > I borrowed this from extension_webui_apitest.cc.  Is it worth bringing it
> out
> > to
> > > a public place to avoid the duplication?  Or maybe there's a better way to
> get
> > > the subframe's RFH altogether?
> > 
> > It does seem like we'd benefit from a public utility function for finding a
> > subframe RFH.
> > 
> > Using a URL to find it seems unfortunate, though, since that's not unique. 
It
> > would be nicer to look up the frame by ID if we had the ID on the
> FrameTreeNode,
> > but we don't need to add that for this CL.  I suppose this URL version can
> help
> > in cases where the frame doesn't have an ID either.  Let's just make this
> public
> > and document the limitation of requiring the desired frame to have a unique
> URL.
> 
> Actually, I somehow missed that there's already a public function for this in
> browser_test_utils.h: FrameMatchingPredicate(), which can take look for RFHs
by
> URL (or other predicates).  I changed my test to use that instead and removed
> the extra FindFrame.

Even better.

I didn't see any change to extension_webui_apitest.cc here.  Do you want to
remove FindFrame there as well in a separate CL?  (Might as well point people in
the right direction if we can.)

https://codereview.chromium.org/789273006/diff/20001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/789273006/diff/20001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:123: // URL is not currently
replicated.
On 2014/12/13 00:58:12, alexmos wrote:
> On 2014/12/12 18:02:45, Charlie Reis wrote:
> > This makes me sad.  I'd like to argue against replicating the full URL if at
> all
> > possible.
> > 
> > Can we take a closer look at how this is used to see if they really need the
> URL
> > for unique origins?  Maybe there's a sane default behavior for unique
origins
> > that doesn't require the URL.
> 
> Hmm.  Similarly to what lazyboy@ found in
> https://codereview.chromium.org/791763003/, when the top frame is a file://
URL,
> it looks like it's possible for these rules to match patterns based on path in
> the URL, though there's a TODO in
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/content...
> to stop supporting path-based matching in ContentSettingsPattern::Matches.  
> 
> There's a separate issue in that currently origin replication can't tell apart
> file:// origins (with enforceFilePathSeparation set to true, which is the
> default for chrome) and unique origins, since they're both serialized as
"null".
>  I will need to fix that at some point, and then just knowing that the top
frame
> came from file:// may be enough here, provided the TODO to remove path
matching
> gets done.
> 
> I also dug around and saw this related comment thread, which seems to suggest
> this was intended for file:// URLs:
>
https://codereview.chromium.org/7831075/diff/84016/chrome/renderer/content_se....
> It would still be good to know which content settings rules could actually
apply
> when the top origin is "null", and whether we'd break anything if we don't
> support that.  Anyone we could ask?

From the TODOs and discussion, it looks like bauerb@, jochen@, and markusheintz@
are good candidates.

https://codereview.chromium.org/789273006/diff/60001/chrome/browser/chrome_si...
File chrome/browser/chrome_site_per_process_browsertest.cc (right):

https://codereview.chromium.org/789273006/diff/60001/chrome/browser/chrome_si...
chrome/browser/chrome_site_per_process_browsertest.cc:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved. Use of this
minor nit: We don't use the (c) anymore either.

https://codereview.chromium.org/789273006/diff/60001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/789273006/diff/60001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:123: // URL is not currently
replicated.
nit: Let's remove "currently" to avoid implying that we plan to replicate it.

[alexmos, 2014-12-15 21:22:19]

Thanks for the review!

> I didn't see any change to extension_webui_apitest.cc here.  Do you want to
> remove FindFrame there as well in a separate CL?  (Might as well point people
in
> the right direction if we can.)

Yes, I'll do that in a separate patch.

> From the TODOs and discussion, it looks like bauerb@, jochen@, and
markusheintz@
> are good candidates.

OK, will start a separate thread about this.

https://codereview.chromium.org/789273006/diff/60001/chrome/browser/chrome_si...
File chrome/browser/chrome_site_per_process_browsertest.cc (right):

https://codereview.chromium.org/789273006/diff/60001/chrome/browser/chrome_si...
chrome/browser/chrome_site_per_process_browsertest.cc:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved. Use of this
On 2014/12/15 20:25:35, Charlie Reis wrote:
> minor nit: We don't use the (c) anymore either.

Done.

https://codereview.chromium.org/789273006/diff/60001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/789273006/diff/60001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:123: // URL is not currently
replicated.
On 2014/12/15 20:25:35, Charlie Reis wrote:
> nit: Let's remove "currently" to avoid implying that we plan to replicate it.

Done.

[alexmos, 2014-12-15 22:33:12]

alexmos@chromium.org changed reviewers:
+ jochen@chromium.org

[alexmos, 2014-12-15 22:33:12]

Jochen, could you please review this for chrome/ owners approval?

Also, do you have any thoughts on the TODO I added to
content_settings_observer.cc on whether knowing the top frame's URL is indeed
necessary for rule matching?

[markusheintz_, 2014-12-16 13:04:42]

markusheintz@chromium.org changed reviewers:
+ markusheintz@chromium.org

[markusheintz_, 2014-12-16 13:04:43]

https://codereview.chromium.org/789273006/diff/80001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/789273006/diff/80001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:118: WebString top_origin =
frame->top()->securityOrigin().toString();
What security origin are you using for file URLs?

GURL returns null as origin for file URL (unless this was changed).

[alexmos, 2014-12-16 20:13:26]

https://codereview.chromium.org/789273006/diff/80001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/789273006/diff/80001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:118: WebString top_origin =
frame->top()->securityOrigin().toString();
On 2014/12/16 13:04:43, markusheintz_ wrote:
> What security origin are you using for file URLs?
> 
> GURL returns null as origin for file URL (unless this was changed).

Correct, file URLs will result in a "null" origin returned from
WebSecurityOrigin::toString().  We haven't changed that behavior for local or
remote frames; both will still return "null" in this case, and hence the need
for the TODO below.  

Based on your comments in the other thread, we will want to replace
frame->top()->document().url() with something that works in --site-per-process. 
If there's no other way, we can replicate the top frame's file path when the top
frame is at a file URL and use it here.  I'd like to leave this for another CL
though, and keep this one for fixing all uses of frame origins.

[alexmos, 2014-12-18 19:48:20]

Jochen - ping for chrome/ owners approval.

The current patch doesn't change ContentSettingsObserver behavior without
--site-per-process, and it improves things with --site-per-process.  We'd like
to land this and figure out the right thing to do for file:// URLs in a separate
CL.

[markusheintz_, 2014-12-19 18:32:51]

On 2014/12/16 20:13:26, alexmos wrote:
>
https://codereview.chromium.org/789273006/diff/80001/chrome/renderer/content_...
> File chrome/renderer/content_settings_observer.cc (right):
> 
>
https://codereview.chromium.org/789273006/diff/80001/chrome/renderer/content_...
> chrome/renderer/content_settings_observer.cc:118: WebString top_origin =
> frame->top()->securityOrigin().toString();
> On 2014/12/16 13:04:43, markusheintz_ wrote:
> > What security origin are you using for file URLs?
> > 
> > GURL returns null as origin for file URL (unless this was changed).
> 
> Correct, file URLs will result in a "null" origin returned from
> WebSecurityOrigin::toString().  We haven't changed that behavior for local or
> remote frames; both will still return "null" in this case, and hence the need
> for the TODO below.  
> 
> Based on your comments in the other thread, we will want to replace
> frame->top()->document().url() with something that works in
--site-per-process. 
> If there's no other way, we can replicate the top frame's file path when the
top
> frame is at a file URL and use it here.  I'd like to leave this for another CL
> though, and keep this one for fixing all uses of frame origins.

SGTM

[alexmos, 2015-01-06 23:33:50]

On 2014/12/19 18:32:51, markusheintz_ (ooo until Jan 9 wrote:
> On 2014/12/16 20:13:26, alexmos wrote:
> >
>
https://codereview.chromium.org/789273006/diff/80001/chrome/renderer/content_...
> > File chrome/renderer/content_settings_observer.cc (right):
> > 
> >
>
https://codereview.chromium.org/789273006/diff/80001/chrome/renderer/content_...
> > chrome/renderer/content_settings_observer.cc:118: WebString top_origin =
> > frame->top()->securityOrigin().toString();
> > On 2014/12/16 13:04:43, markusheintz_ wrote:
> > > What security origin are you using for file URLs?
> > > 
> > > GURL returns null as origin for file URL (unless this was changed).
> > 
> > Correct, file URLs will result in a "null" origin returned from
> > WebSecurityOrigin::toString().  We haven't changed that behavior for local
or
> > remote frames; both will still return "null" in this case, and hence the
need
> > for the TODO below.  
> > 
> > Based on your comments in the other thread, we will want to replace
> > frame->top()->document().url() with something that works in
> --site-per-process. 
> > If there's no other way, we can replicate the top frame's file path when the
> top
> > frame is at a file URL and use it here.  I'd like to leave this for another
CL
> > though, and keep this one for fixing all uses of frame origins.
> 
> SGTM

Jochen - another friendly ping for chrome/ owners approval.

[Charlie Reis, 2015-01-07 00:13:26]

On 2015/01/06 23:33:50, alexmos wrote:
> Jochen - another friendly ping for chrome/ owners approval.

Note: I think Jochen is OOO at the moment.  Is there another owner you can check
with?

[alexmos, 2015-01-07 00:24:28]

alexmos@chromium.org changed reviewers:
+ sky@chromium.org

[alexmos, 2015-01-07 00:24:29]

sky@, could you please review chrome/, since jochen@ is OOO?

[jochen (gone - plz use gerrit), 2015-01-07 13:42:46]

lgtm

https://codereview.chromium.org/789273006/diff/80001/chrome/browser/chrome_si...
File chrome/browser/chrome_site_per_process_browsertest.cc (right):

https://codereview.chromium.org/789273006/diff/80001/chrome/browser/chrome_si...
chrome/browser/chrome_site_per_process_browsertest.cc:27:
ChromeSitePerProcessTest() {}
nit. please add a virtual dtor

[sky, 2015-01-07 17:08:05]

sky@chromium.org changed reviewers:
- sky@chromium.org

[sky, 2015-01-07 17:08:15]

-sky as Jochen is back

[alexmos, 2015-01-07 20:43:12]

Thanks!

https://codereview.chromium.org/789273006/diff/80001/chrome/browser/chrome_si...
File chrome/browser/chrome_site_per_process_browsertest.cc (right):

https://codereview.chromium.org/789273006/diff/80001/chrome/browser/chrome_si...
chrome/browser/chrome_site_per_process_browsertest.cc:27:
ChromeSitePerProcessTest() {}
On 2015/01/07 13:42:46, jochen (slow) wrote:
> nit. please add a virtual dtor

Done.

[alexmos, 2015-01-07 20:50:28]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2015-01-07 20:51:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/789273006/120001

[commit-bot: I haz the power, 2015-01-07 21:06:56]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-01-07 21:07:49]

Patchset 7 (id:??) landed as
https://crrev.com/75648bb31c04beca9c3a2507343ab2bf4a5bbae0
Cr-Commit-Position: refs/heads/master@{#310366}