Avoid crash in ContentSettingsObserver::GetOriginOrURL with top remote frames.

Avoid crash in ContentSettingsObserver::GetOriginOrURL with top remote frames.

The intent of the fallback to top()->document().url() in
GetOriginOrURL() is to support file path matching for content setting
exceptions in pages loaded from a file: scheme.  This fallback can
cause crashes in OOPIF modes when the top frame is remote, even in
cases that have nothing to do with file: exceptions, such as a
sandboxed main frame (which has a "null" origin) embedding an
OOPIF.

Longer-term, local and remote frames should be treated the same way
for content settings exceptions for the file: scheme; and content
settings will be refactored to be based on origins rather than GURLs
in issue 621724.  In the short term though, avoid the crash by falling
back to document->url() only for local top frames.  This shouldn't
actually affect file exceptions, as --isolate-extensions, which is the
only OOPIF mode currently enabled by default on trunk, won't put
subframes inside file: pages into a separate process.

BUG=628759, 466297
Committed: https://crrev.com/78aed40a1509c6cab064f9684e9653b37daff826
Cr-Commit-Position: refs/heads/master@{#419899}

[alexmos, 2016-09-20 20:23:30]

The CQ bit was checked by alexmos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 20:24:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alexmos, 2016-09-20 20:43:51]

Description was changed from

==========
Avoid crash in ContentSettingsObserver::GetOriginOrURL with top remote frames.

BUG=628759
==========

to

==========
Avoid crash in ContentSettingsObserver::GetOriginOrURL with top remote frames.

The intent of the fallback to top()->document().url() in
GetOriginOrURL() is to support file path matching for content setting
exceptions in pages loaded from a file: scheme.  This fallback can
cause crashes in OOPIF modes when the top frame is remote, even in
cases that have nothing to do with file: exceptions, such as a
sandboxed main frame (which has a "null" origin) embedding an
OOPIF. Longer-term, local and remote frames should be treat the same
way for content settings exceptions involving the file: scheme; and
content settings will be refactored to be based on origins rather than
GURLs in issue 621724.  In the short term though, avoid the crash by
falling back to document->url() only for local top frames.  This
shouldn't actually affect file exceptions, as --isolate-extensions,
which is the only OOPIF mode currently default on trunk, won't put
subframes inside file: pages into a separate process.

BUG=628759
==========

[alexmos, 2016-09-20 20:44:37]

Description was changed from

==========
Avoid crash in ContentSettingsObserver::GetOriginOrURL with top remote frames.

The intent of the fallback to top()->document().url() in
GetOriginOrURL() is to support file path matching for content setting
exceptions in pages loaded from a file: scheme.  This fallback can
cause crashes in OOPIF modes when the top frame is remote, even in
cases that have nothing to do with file: exceptions, such as a
sandboxed main frame (which has a "null" origin) embedding an
OOPIF. Longer-term, local and remote frames should be treat the same
way for content settings exceptions involving the file: scheme; and
content settings will be refactored to be based on origins rather than
GURLs in issue 621724.  In the short term though, avoid the crash by
falling back to document->url() only for local top frames.  This
shouldn't actually affect file exceptions, as --isolate-extensions,
which is the only OOPIF mode currently default on trunk, won't put
subframes inside file: pages into a separate process.

BUG=628759
==========

to

==========
Avoid crash in ContentSettingsObserver::GetOriginOrURL with top remote frames.

The intent of the fallback to top()->document().url() in
GetOriginOrURL() is to support file path matching for content setting
exceptions in pages loaded from a file: scheme.  This fallback can
cause crashes in OOPIF modes when the top frame is remote, even in
cases that have nothing to do with file: exceptions, such as a
sandboxed main frame (which has a "null" origin) embedding an
OOPIF. Longer-term, local and remote frames should be treat the same
way for content settings exceptions involving the file: scheme; and
content settings will be refactored to be based on origins rather than
GURLs in issue 621724.  In the short term though, avoid the crash by
falling back to document->url() only for local top frames.  This
shouldn't actually affect file exceptions, as --isolate-extensions,
which is the only OOPIF mode currently default on trunk, won't put
subframes inside file: pages into a separate process.

BUG=628759, 466297
==========

[alexmos, 2016-09-20 20:51:36]

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

[alexmos, 2016-09-20 20:51:37]

Charlie, what do you think?

Ideally, this would use the origin for both local and remote cases, but that
involves some work: we need to find a way to not have existing file path
exceptions would fail open, add an UMA to measure how often a path exception is
used vs. a scheme-wide file: exception, etc.  While this is ongoing, this CL
would prevent the crash we're seeing.  It shouldn't actually affect file:
content settings since we won't create OOPIFs on file: URLs.  It might mean that
content settings for images/scripts won't be respected on OOPIFs inside sandbox
main frames (since the latter have a "null" origin and would've previously used
the URL), but in the short term this is probably better than crashing on those
pages.

[alexmos, 2016-09-20 20:55:50]

Description was changed from

==========
Avoid crash in ContentSettingsObserver::GetOriginOrURL with top remote frames.

The intent of the fallback to top()->document().url() in
GetOriginOrURL() is to support file path matching for content setting
exceptions in pages loaded from a file: scheme.  This fallback can
cause crashes in OOPIF modes when the top frame is remote, even in
cases that have nothing to do with file: exceptions, such as a
sandboxed main frame (which has a "null" origin) embedding an
OOPIF. Longer-term, local and remote frames should be treat the same
way for content settings exceptions involving the file: scheme; and
content settings will be refactored to be based on origins rather than
GURLs in issue 621724.  In the short term though, avoid the crash by
falling back to document->url() only for local top frames.  This
shouldn't actually affect file exceptions, as --isolate-extensions,
which is the only OOPIF mode currently default on trunk, won't put
subframes inside file: pages into a separate process.

BUG=628759, 466297
==========

to

==========
Avoid crash in ContentSettingsObserver::GetOriginOrURL with top remote frames.

The intent of the fallback to top()->document().url() in
GetOriginOrURL() is to support file path matching for content setting
exceptions in pages loaded from a file: scheme.  This fallback can
cause crashes in OOPIF modes when the top frame is remote, even in
cases that have nothing to do with file: exceptions, such as a
sandboxed main frame (which has a "null" origin) embedding an
OOPIF.

Longer-term, local and remote frames should be treated the same way
for content settings exceptions for the file: scheme; and content
settings will be refactored to be based on origins rather than GURLs
in issue 621724.  In the short term though, avoid the crash by falling
back to document->url() only for local top frames.  This shouldn't
actually affect file exceptions, as --isolate-extensions, which is the
only OOPIF mode currently enabled by default on trunk, won't put
subframes inside file: pages into a separate process.

BUG=628759, 466297
==========

[commit-bot: I haz the power, 2016-09-20 21:25:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 21:26:01]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-09-20 22:25:22]

LGTM.

On 2016/09/20 20:51:37, alexmos wrote:
> Charlie, what do you think?
> 
> Ideally, this would use the origin for both local and remote cases, but that
> involves some work: we need to find a way to not have existing file path
> exceptions would fail open, add an UMA to measure how often a path exception
is
> used vs. a scheme-wide file: exception, etc.  While this is ongoing, this CL
> would prevent the crash we're seeing.

Yes, I think avoiding the crash is a good idea.

> It shouldn't actually affect file:
> content settings since we won't create OOPIFs on file: URLs.

Is it possible for extensions to inject frames into file:// pages?  I wonder if
that's affected (though not crashing is better regardless).

> It might mean that
> content settings for images/scripts won't be respected on OOPIFs inside
sandbox
> main frames (since the latter have a "null" origin and would've previously
used
> the URL), but in the short term this is probably better than crashing on those
> pages.

And even then, only if the setting matched a specific path and not an origin. 
Given that we crash today in that case, I think your change is a better interim
state.

[alexmos, 2016-09-20 22:39:57]

Thanks!

On 2016/09/20 22:25:22, Charlie Reis (slow) wrote:
> LGTM.
> 
> On 2016/09/20 20:51:37, alexmos wrote:
> > Charlie, what do you think?
> > 
> > Ideally, this would use the origin for both local and remote cases, but that
> > involves some work: we need to find a way to not have existing file path
> > exceptions would fail open, add an UMA to measure how often a path exception
> is
> > used vs. a scheme-wide file: exception, etc.  While this is ongoing, this CL
> > would prevent the crash we're seeing.
> 
> Yes, I think avoiding the crash is a good idea.
> 
> > It shouldn't actually affect file:
> > content settings since we won't create OOPIFs on file: URLs.
> 
> Is it possible for extensions to inject frames into file:// pages?  I wonder
if
> that's affected (though not crashing is better regardless).

Ah, good point.  Not sure if anything else would prevent extensions from
injecting frames into file: pages, but I was able to embed a extension iframe
into a file: page from DevTools, and it the subframe was put into a separate
process.  Yes, in that case, we'd not apply file-path-based exceptions either.

> 
> > It might mean that
> > content settings for images/scripts won't be respected on OOPIFs inside
> sandbox
> > main frames (since the latter have a "null" origin and would've previously
> used
> > the URL), but in the short term this is probably better than crashing on
those
> > pages.
> 
> And even then, only if the setting matched a specific path and not an origin. 
> Given that we crash today in that case, I think your change is a better
interim
> state.

[alexmos, 2016-09-20 22:43:03]

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

[alexmos, 2016-09-20 22:43:04]

thestig@: can you please review for OWNERS?

[Lei Zhang, 2016-09-20 23:39:05]

lgtm

[alexmos, 2016-09-20 23:45:14]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2016-09-20 23:46:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 23:52:23]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-20 23:55:01]

Description was changed from

==========
Avoid crash in ContentSettingsObserver::GetOriginOrURL with top remote frames.

The intent of the fallback to top()->document().url() in
GetOriginOrURL() is to support file path matching for content setting
exceptions in pages loaded from a file: scheme.  This fallback can
cause crashes in OOPIF modes when the top frame is remote, even in
cases that have nothing to do with file: exceptions, such as a
sandboxed main frame (which has a "null" origin) embedding an
OOPIF.

Longer-term, local and remote frames should be treated the same way
for content settings exceptions for the file: scheme; and content
settings will be refactored to be based on origins rather than GURLs
in issue 621724.  In the short term though, avoid the crash by falling
back to document->url() only for local top frames.  This shouldn't
actually affect file exceptions, as --isolate-extensions, which is the
only OOPIF mode currently enabled by default on trunk, won't put
subframes inside file: pages into a separate process.

BUG=628759, 466297
==========

to

==========
Avoid crash in ContentSettingsObserver::GetOriginOrURL with top remote frames.

The intent of the fallback to top()->document().url() in
GetOriginOrURL() is to support file path matching for content setting
exceptions in pages loaded from a file: scheme.  This fallback can
cause crashes in OOPIF modes when the top frame is remote, even in
cases that have nothing to do with file: exceptions, such as a
sandboxed main frame (which has a "null" origin) embedding an
OOPIF.

Longer-term, local and remote frames should be treated the same way
for content settings exceptions for the file: scheme; and content
settings will be refactored to be based on origins rather than GURLs
in issue 621724.  In the short term though, avoid the crash by falling
back to document->url() only for local top frames.  This shouldn't
actually affect file exceptions, as --isolate-extensions, which is the
only OOPIF mode currently enabled by default on trunk, won't put
subframes inside file: pages into a separate process.

BUG=628759, 466297

Committed: https://crrev.com/78aed40a1509c6cab064f9684e9653b37daff826
Cr-Commit-Position: refs/heads/master@{#419899}
==========

[commit-bot: I haz the power, 2016-09-20 23:55:01]

Patchset 1 (id:??) landed as
https://crrev.com/78aed40a1509c6cab064f9684e9653b37daff826
Cr-Commit-Position: refs/heads/master@{#419899}