Fix extension bindings injection for iframes (reland)

Fix extension bindings injection for iframes (reland)

For iframes, we don't want to use the source url for determining the
associated extension because it starts out with an about:blank context
that is scriptable by its parent.

This originally landed in codereview.chromium.org/2151693002/ but
was reverted because of bug 630928 as well as the test failing under
DrMemory (not with memory errors; just not succeeding which likely
indicates some kind of race condition in the test). I've added a fix for
bug 630928 but haven't been able to locally reproduce the test failure
under DrMemory, so I've added some extra logging to the test to hopefully
better understand what might be going wrong.

Memory sheriffs: If the FramesExtensionBindingsApiTest.FramesBeforeNavigation
test fails again without any actual memory errors, please do not revert the
entire CL (since it is an important security fix); instead just disable the
test or add it to a suppression file so I can iterate on a fix.

BUG=573131, 630928
Committed: https://crrev.com/79b64c3e741cc9c6afbb23885945831a45c6baa5
Cr-Commit-Position: refs/heads/master@{#409819}

[asargent_no_longer_on_chrome, 2016-08-03 00:10:39]

Patchset #2 (id:20001) has been deleted

[asargent_no_longer_on_chrome, 2016-08-03 00:11:00]

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

[commit-bot: I haz the power, 2016-08-03 00:11:42]

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

[asargent_no_longer_on_chrome, 2016-08-03 00:15:06]

Description was changed from

==========
Fix extension bindings injection for iframes (reland)

For iframes, we don't want to use the source url for determining the
associated extension because it starts out with an about:blank context
that is scriptable by its parent.

This originally landed in codereview.chromium.org/2151693002/ but
was reverted because of bug 630928 as well as some DrMemory test
failures.

BUG=573131,630928
==========

to

==========
Fix extension bindings injection for iframes (reland)

For iframes, we don't want to use the source url for determining the
associated extension because it starts out with an about:blank context
that is scriptable by its parent.

This originally landed in codereview.chromium.org/2151693002/ but
was reverted because of bug 630928 as well as the test failing under
DrMemory (not with memory errors; just not succeeding which likely 
indicates some kind of race condition in the test). I've added a fix for 
bug 630928 but haven't been able to reproduce the test failure reliably
enough to debug, so I've added some extra logging to the test in cases
where it might fail.

BUG=573131,630928
==========

[asargent_no_longer_on_chrome, 2016-08-03 00:20:00]

Description was changed from

==========
Fix extension bindings injection for iframes (reland)

For iframes, we don't want to use the source url for determining the
associated extension because it starts out with an about:blank context
that is scriptable by its parent.

This originally landed in codereview.chromium.org/2151693002/ but
was reverted because of bug 630928 as well as the test failing under
DrMemory (not with memory errors; just not succeeding which likely 
indicates some kind of race condition in the test). I've added a fix for 
bug 630928 but haven't been able to reproduce the test failure reliably
enough to debug, so I've added some extra logging to the test in cases
where it might fail.

BUG=573131,630928
==========

to

==========
Fix extension bindings injection for iframes (reland)

For iframes, we don't want to use the source url for determining the
associated extension because it starts out with an about:blank context
that is scriptable by its parent.

This originally landed in codereview.chromium.org/2151693002/ but
was reverted because of bug 630928 as well as the test failing under
DrMemory (not with memory errors; just not succeeding which likely 
indicates some kind of race condition in the test). I've added a fix for 
bug 630928 but haven't been able to locally reproduce the test failure
under DrMemory, so I've added some extra logging to the test to hopefully
better understand what might be going wrong.

Memory sheriffs: If the test fails again without any actual memory errors,
please do not revert the entire CL (since it is an important security fix);
instead just disable the test or add it to a suppression file so I can 
iterate on a fix. 

BUG=573131,630928
==========

[asargent_no_longer_on_chrome, 2016-08-03 00:21:06]

Description was changed from

==========
Fix extension bindings injection for iframes (reland)

For iframes, we don't want to use the source url for determining the
associated extension because it starts out with an about:blank context
that is scriptable by its parent.

This originally landed in codereview.chromium.org/2151693002/ but
was reverted because of bug 630928 as well as the test failing under
DrMemory (not with memory errors; just not succeeding which likely 
indicates some kind of race condition in the test). I've added a fix for 
bug 630928 but haven't been able to locally reproduce the test failure
under DrMemory, so I've added some extra logging to the test to hopefully
better understand what might be going wrong.

Memory sheriffs: If the test fails again without any actual memory errors,
please do not revert the entire CL (since it is an important security fix);
instead just disable the test or add it to a suppression file so I can 
iterate on a fix. 

BUG=573131,630928
==========

to

==========
Fix extension bindings injection for iframes (reland)

For iframes, we don't want to use the source url for determining the
associated extension because it starts out with an about:blank context
that is scriptable by its parent.

This originally landed in codereview.chromium.org/2151693002/ but
was reverted because of bug 630928 as well as the test failing under
DrMemory (not with memory errors; just not succeeding which likely 
indicates some kind of race condition in the test). I've added a fix for 
bug 630928 but haven't been able to locally reproduce the test failure
under DrMemory, so I've added some extra logging to the test to hopefully
better understand what might be going wrong.

Memory sheriffs: If the FramesExtensionBindingsApiTest.FramesBeforeNavigation
test fails again without any actual memory errors, please do not revert the
entire CL (since it is an important security fix); instead just disable the
test or add it to a suppression file so I can iterate on a fix. 

BUG=573131,630928
==========

[asargent_no_longer_on_chrome, 2016-08-03 00:22:23]

asargent@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[asargent_no_longer_on_chrome, 2016-08-03 00:22:24]

https://codereview.chromium.org/2208483002/diff/40001/extensions/renderer/scr...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/2208483002/diff/40001/extensions/renderer/scr...
extensions/renderer/script_context.cc:122: : web_frame_->dataSource();
Note that there's a little duplication here with the code in
GetDataSourceURLForFrame - I didn't want to just re-use GetDataSourceURLForFrame
as-is since this code runs on every frame creation and I wanted to avoid the
overhead of taking a blink::WebURL, converting it to a GURL to return from
GetDataSourceURLForFrame, and then immediately converting it back to a WebURL to
do the security origin test. I suppose I could refactor things to have a common
method that returns a blink::WebURL and then use that in both places. Let me
know what you think.

[commit-bot: I haz the power, 2016-08-03 02:22:34]

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

[commit-bot: I haz the power, 2016-08-03 02:22:35]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-08-03 16:16:49]

https://codereview.chromium.org/2208483002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_messages_apitest.cc (right):

https://codereview.chromium.org/2208483002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_messages_apitest.cc:1041: for (int i = 0; i
< browser()->tab_strip_model()->count(); i++) {
This is a shame, because UrlLoadObserver could already know the web contents
that committed the url.  Instead, we should probably just put a web_contents()
accessor on the UrlLoadObserver().  If you don't want to do that in this patch
to keep it targeted, that's fine, but maybe add a TODO?  (feel free to
TODO(devlin))

https://codereview.chromium.org/2208483002/diff/40001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/messaging/externally_connectable/sites/popup_opener.html
(right):

https://codereview.chromium.org/2208483002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/messaging/externally_connectable/sites/popup_opener.html:7:
var url = location.href.replace("popup_opener.html", "chromium.org.html");
nit: prefer single quotes.

https://codereview.chromium.org/2208483002/diff/40001/extensions/renderer/scr...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/2208483002/diff/40001/extensions/renderer/scr...
extensions/renderer/script_context.cc:122: : web_frame_->dataSource();
On 2016/08/03 00:22:24, Antony Sargent wrote:
> Note that there's a little duplication here with the code in
> GetDataSourceURLForFrame - I didn't want to just re-use
GetDataSourceURLForFrame
> as-is since this code runs on every frame creation and I wanted to avoid the
> overhead of taking a blink::WebURL, converting it to a GURL to return from
> GetDataSourceURLForFrame, and then immediately converting it back to a WebURL
to
> do the security origin test. I suppose I could refactor things to have a
common
> method that returns a blink::WebURL and then use that in both places. Let me
> know what you think. 

This also looks very similar to the new logic added in ScriptContextSet.  In
both cases we:
Get the document url
if that's empty, check the data source url
if the data source url is non-empty and the frame can access it, use the data
source url

This makes me think we should probably have a method at least to do that -
something like GetAccessCheckedFrameURL() (strawman name) that we can use in
both places.  There may also be some uses of GetDataSourceURL that should be
converted to that, but we can do that separately.

https://codereview.chromium.org/2208483002/diff/40001/extensions/renderer/scr...
extensions/renderer/script_context.cc:127: if (url_.is_empty())
This can be an else, right?  Because we'll only set url_ above if weburl is
empty.

[asargent_no_longer_on_chrome, 2016-08-04 05:42:53]

Patchset #3 (id:60001) has been deleted

[asargent_no_longer_on_chrome, 2016-08-04 05:47:18]

PTAL

https://codereview.chromium.org/2208483002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_messages_apitest.cc (right):

https://codereview.chromium.org/2208483002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_messages_apitest.cc:1041: for (int i = 0; i
< browser()->tab_strip_model()->count(); i++) {
On 2016/08/03 16:16:49, Devlin wrote:
> This is a shame, because UrlLoadObserver could already know the web contents
> that committed the url.  Instead, we should probably just put a web_contents()
> accessor on the UrlLoadObserver().  If you don't want to do that in this patch
> to keep it targeted, that's fine, but maybe add a TODO?  (feel free to
> TODO(devlin))

Left a TODO, to try and get this CL in faster.

https://codereview.chromium.org/2208483002/diff/40001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/messaging/externally_connectable/sites/popup_opener.html
(right):

https://codereview.chromium.org/2208483002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/messaging/externally_connectable/sites/popup_opener.html:7:
var url = location.href.replace("popup_opener.html", "chromium.org.html");
On 2016/08/03 16:16:49, Devlin wrote:
> nit: prefer single quotes.

Oops, sorry, old habits die hard. Fixed.

https://codereview.chromium.org/2208483002/diff/40001/extensions/renderer/scr...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/2208483002/diff/40001/extensions/renderer/scr...
extensions/renderer/script_context.cc:122: : web_frame_->dataSource();
On 2016/08/03 16:16:49, Devlin wrote:
> On 2016/08/03 00:22:24, Antony Sargent wrote:
> > Note that there's a little duplication here with the code in
> > GetDataSourceURLForFrame - I didn't want to just re-use
> GetDataSourceURLForFrame
> > as-is since this code runs on every frame creation and I wanted to avoid the
> > overhead of taking a blink::WebURL, converting it to a GURL to return from
> > GetDataSourceURLForFrame, and then immediately converting it back to a
WebURL
> to
> > do the security origin test. I suppose I could refactor things to have a
> common
> > method that returns a blink::WebURL and then use that in both places. Let me
> > know what you think. 
> 
> This also looks very similar to the new logic added in ScriptContextSet.  In
> both cases we:
> Get the document url
> if that's empty, check the data source url
> if the data source url is non-empty and the frame can access it, use the data
> source url
> 
> This makes me think we should probably have a method at least to do that -
> something like GetAccessCheckedFrameURL() (strawman name) that we can use in
> both places.  There may also be some uses of GetDataSourceURL that should be
> converted to that, but we can do that separately.

Done.

https://codereview.chromium.org/2208483002/diff/40001/extensions/renderer/scr...
extensions/renderer/script_context.cc:127: if (url_.is_empty())
On 2016/08/03 16:16:49, Devlin wrote:
> This can be an else, right?  Because we'll only set url_ above if weburl is
> empty.

Good point - I guess it doesn't help to assign to url_ from an empty weburl.
But, obsoleted by switching to a method.

[asargent_no_longer_on_chrome, 2016-08-04 05:49:52]

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

[commit-bot: I haz the power, 2016-08-04 05:50:16]

Dry run: 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-08-04 07:06:20]

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

[commit-bot: I haz the power, 2016-08-04 07:06:22]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-08-04 16:26:14]

lgtm

[asargent_no_longer_on_chrome, 2016-08-04 17:11:55]

The CQ bit was checked by asargent@chromium.org

[commit-bot: I haz the power, 2016-08-04 17:12:24]

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-08-04 17:17:31]

Description was changed from

==========
Fix extension bindings injection for iframes (reland)

For iframes, we don't want to use the source url for determining the
associated extension because it starts out with an about:blank context
that is scriptable by its parent.

This originally landed in codereview.chromium.org/2151693002/ but
was reverted because of bug 630928 as well as the test failing under
DrMemory (not with memory errors; just not succeeding which likely 
indicates some kind of race condition in the test). I've added a fix for 
bug 630928 but haven't been able to locally reproduce the test failure
under DrMemory, so I've added some extra logging to the test to hopefully
better understand what might be going wrong.

Memory sheriffs: If the FramesExtensionBindingsApiTest.FramesBeforeNavigation
test fails again without any actual memory errors, please do not revert the
entire CL (since it is an important security fix); instead just disable the
test or add it to a suppression file so I can iterate on a fix. 

BUG=573131,630928
==========

to

==========
Fix extension bindings injection for iframes (reland)

For iframes, we don't want to use the source url for determining the
associated extension because it starts out with an about:blank context
that is scriptable by its parent.

This originally landed in codereview.chromium.org/2151693002/ but
was reverted because of bug 630928 as well as the test failing under
DrMemory (not with memory errors; just not succeeding which likely 
indicates some kind of race condition in the test). I've added a fix for 
bug 630928 but haven't been able to locally reproduce the test failure
under DrMemory, so I've added some extra logging to the test to hopefully
better understand what might be going wrong.

Memory sheriffs: If the FramesExtensionBindingsApiTest.FramesBeforeNavigation
test fails again without any actual memory errors, please do not revert the
entire CL (since it is an important security fix); instead just disable the
test or add it to a suppression file so I can iterate on a fix. 

BUG=573131,630928
==========

[commit-bot: I haz the power, 2016-08-04 17:17:32]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-08-04 17:20:32]

Description was changed from

==========
Fix extension bindings injection for iframes (reland)

For iframes, we don't want to use the source url for determining the
associated extension because it starts out with an about:blank context
that is scriptable by its parent.

This originally landed in codereview.chromium.org/2151693002/ but
was reverted because of bug 630928 as well as the test failing under
DrMemory (not with memory errors; just not succeeding which likely 
indicates some kind of race condition in the test). I've added a fix for 
bug 630928 but haven't been able to locally reproduce the test failure
under DrMemory, so I've added some extra logging to the test to hopefully
better understand what might be going wrong.

Memory sheriffs: If the FramesExtensionBindingsApiTest.FramesBeforeNavigation
test fails again without any actual memory errors, please do not revert the
entire CL (since it is an important security fix); instead just disable the
test or add it to a suppression file so I can iterate on a fix. 

BUG=573131,630928
==========

to

==========
Fix extension bindings injection for iframes (reland)

For iframes, we don't want to use the source url for determining the
associated extension because it starts out with an about:blank context
that is scriptable by its parent.

This originally landed in codereview.chromium.org/2151693002/ but
was reverted because of bug 630928 as well as the test failing under
DrMemory (not with memory errors; just not succeeding which likely
indicates some kind of race condition in the test). I've added a fix for
bug 630928 but haven't been able to locally reproduce the test failure
under DrMemory, so I've added some extra logging to the test to hopefully
better understand what might be going wrong.

Memory sheriffs: If the FramesExtensionBindingsApiTest.FramesBeforeNavigation
test fails again without any actual memory errors, please do not revert the
entire CL (since it is an important security fix); instead just disable the
test or add it to a suppression file so I can iterate on a fix.

BUG=573131,630928

Committed: https://crrev.com/79b64c3e741cc9c6afbb23885945831a45c6baa5
Cr-Commit-Position: refs/heads/master@{#409819}
==========

[commit-bot: I haz the power, 2016-08-04 17:20:33]

Patchset 3 (id:??) landed as
https://crrev.com/79b64c3e741cc9c6afbb23885945831a45c6baa5
Cr-Commit-Position: refs/heads/master@{#409819}