[Extensions] Propagate activeTab hosts to extension background pages

[Extensions] Propagate activeTab hosts to extension background pages

Let extensions with active tab use XHR in background pages to any hosts that
they have active tab on.
Also move tab_finder to //extensions.

BUG=452688
Committed: https://crrev.com/c491dbfaad049247a5d2f92ab5ad823725414e70
Cr-Commit-Position: refs/heads/master@{#315147}

[Devlin, 2015-02-02 19:37:45]

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

[Devlin, 2015-02-02 19:37:45]

Ben, take a look when you can.

[not at google - send to devlin, 2015-02-03 19:29:12]

After much staring...

https://codereview.chromium.org/890083002/diff/1/chrome/browser/extensions/ac...
File chrome/browser/extensions/active_tab_permission_granter.cc (right):

https://codereview.chromium.org/890083002/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/active_tab_permission_granter.cc:78:
GetBackgroundHostForExtension(extension->id());
What about if the event page hasn't started yet? For example, the following flow
seems pretty likely:

(1) Event page is not running, but it listens to browserAction.onClicked
(2) User clicks on browser action: grants permission, fires onClicked event
(3) This check returns NULL because there is no background host yet, so the tab
permissions aren't propagated

... and I don't see anything accounting for that. Seems like you may need to
augment ExtensionMsg_Loaded_Params.

This relates to a comment I made in dispatcher.cc.

https://codereview.chromium.org/890083002/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/active_tab/background.js (right):

https://codereview.chromium.org/890083002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/active_tab/background.js:31:
assertTrue(canXhr(url));
canXhr() doesn't take a URL, but it seems like this call does have the right
idea, and it *should* take a URL.

https://codereview.chromium.org/890083002/diff/1/extensions/renderer/dispatch...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/890083002/diff/1/extensions/renderer/dispatch...
extensions/renderer/dispatcher.cc:1076: UpdateOriginPermissions(extension,
old_effective);
I was wondering why this is only happening for the background page. I think it's
because there has always been a bug: it appears as though we're not giving
extensions the permission to XHR to the tab it's running on?

It looks like you can fix that bug, and make this easier for me to understand
generally, all at once.

(1) Do what I suggest below, and make UpdateOriginPermissions dumber.
(2) Don't make that GetEffectiveHostPermissions change to PermissionsData,
instead add a separate method which returns the union of all tab specific
permissions (say, GetAllTabSpecificPermissions for the sake of this comment).
(3) Instead of only running UpdateOriginPermissions on
ExtensionHelper::GetBackgroundPage, just gate the call to
GetAllTabSpecificPermissions to it.

https://codereview.chromium.org/890083002/diff/1/extensions/renderer/dispatch...
extensions/renderer/dispatcher.cc:1117: void
Dispatcher::UpdateOriginPermissions(const Extension* extension,
Even though this method can determine new_patterns from the state of
|extension|, I find this shortcut confusing[*], and prefer the old code. In
fact, how about changing it so that you pass the extension URL in instead of the
extension, to avoid the temptation of this change?

[*] I find it confusing for two reasons:

Firstly, because of the duality of state. We could be in a tab, or we could be
in a background page, and these must be treated differently. The call to
GetEffectiveHostPermissions forces me to try to reason about the difference, and
track down callers. The fact that sometimes this method is gated on there being
a background page, and other times it isn't, is confusing.

Secondly, a method called "Update" taking the *old* state is odd.

[Devlin, 2015-02-04 22:25:53]

Patchset #2 (id:20001) has been deleted

[Devlin, 2015-02-04 22:26:02]

https://codereview.chromium.org/890083002/diff/1/chrome/browser/extensions/ac...
File chrome/browser/extensions/active_tab_permission_granter.cc (right):

https://codereview.chromium.org/890083002/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/active_tab_permission_granter.cc:78:
GetBackgroundHostForExtension(extension->id());
On 2015/02/03 19:29:12, kalman wrote:
> What about if the event page hasn't started yet? For example, the following
flow
> seems pretty likely:
> 
> (1) Event page is not running, but it listens to browserAction.onClicked
> (2) User clicks on browser action: grants permission, fires onClicked event
> (3) This check returns NULL because there is no background host yet, so the
tab
> permissions aren't propagated
> 
> ... and I don't see anything accounting for that. Seems like you may need to
> augment ExtensionMsg_Loaded_Params.
> 
> This relates to a comment I made in dispatcher.cc.

Good point. Done.

https://codereview.chromium.org/890083002/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/active_tab/background.js (right):

https://codereview.chromium.org/890083002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/active_tab/background.js:31:
assertTrue(canXhr(url));
On 2015/02/03 19:29:12, kalman wrote:
> canXhr() doesn't take a URL, but it seems like this call does have the right
> idea, and it *should* take a URL.

It was my circumvention around doing an async tabs.query call.  But done.

https://codereview.chromium.org/890083002/diff/1/extensions/renderer/dispatch...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/890083002/diff/1/extensions/renderer/dispatch...
extensions/renderer/dispatcher.cc:1076: UpdateOriginPermissions(extension,
old_effective);
On 2015/02/03 19:29:12, kalman wrote:
> I was wondering why this is only happening for the background page. I think
it's
> because there has always been a bug: it appears as though we're not giving
> extensions the permission to XHR to the tab it's running on?
> 
> It looks like you can fix that bug, and make this easier for me to understand
> generally, all at once.
> 
> (1) Do what I suggest below, and make UpdateOriginPermissions dumber.
> (2) Don't make that GetEffectiveHostPermissions change to PermissionsData,
> instead add a separate method which returns the union of all tab specific
> permissions (say, GetAllTabSpecificPermissions for the sake of this comment).
> (3) Instead of only running UpdateOriginPermissions on
> ExtensionHelper::GetBackgroundPage, just gate the call to
> GetAllTabSpecificPermissions to it.

As discussed offline, we don't want to update the origin access whitelist for
content scripts, since they share an origin with their current page (and the
whitelist works by saying "request from origin A to origin B granted").  Also
discussed offline, GetEffectiveHostPermissions was probably the right change to
make (though it needed a better comment).

https://codereview.chromium.org/890083002/diff/1/extensions/renderer/dispatch...
extensions/renderer/dispatcher.cc:1117: void
Dispatcher::UpdateOriginPermissions(const Extension* extension,
On 2015/02/03 19:29:12, kalman wrote:
> Even though this method can determine new_patterns from the state of
> |extension|, I find this shortcut confusing[*], and prefer the old code. In
> fact, how about changing it so that you pass the extension URL in instead of
the
> extension, to avoid the temptation of this change?
> 
> [*] I find it confusing for two reasons:
> 
> Firstly, because of the duality of state. We could be in a tab, or we could be
> in a background page, and these must be treated differently. The call to
> GetEffectiveHostPermissions forces me to try to reason about the difference,
and
> track down callers. The fact that sometimes this method is gated on there
being
> a background page, and other times it isn't, is confusing.
> 
> Secondly, a method called "Update" taking the *old* state is odd.

Dumbified.

[not at google - send to devlin, 2015-02-05 00:16:13]

lgtm. I'd feel more comfortable with a little more test coverage.

https://codereview.chromium.org/890083002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/active_tab_permission_granter.cc (right):

https://codereview.chromium.org/890083002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_tab_permission_granter.cc:49: // also sends a
message to the tab process.
So, you could replace the second sentence of this comment as so:

"...given |view_hosts|, and |tab_process|."

It has the same meaning.

However, the behavior is a little different - you send false to |create_message|
if |tab_process| doesn't contain one of |view_hosts|. You should incorporate
that somehow.

https://codereview.chromium.org/890083002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_tab_permission_granter.cc:116:
CreateMessageFunction create_message =
call this |update_message|, and below |clear_message|.

https://codereview.chromium.org/890083002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/890083002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1035: 1,
ExtensionMsg_Loaded_Params(extension, false));
document this (what does false mean / why is it set)

https://codereview.chromium.org/890083002/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/active_tab/background.js (right):

https://codereview.chromium.org/890083002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/active_tab/background.js:13: function
canXhr(url) {
Would be good to test iframes in here somewhere as well.

One way would be to inject an iframe into the tab (would need to be in
web_accessible_resources) then check it can XHR. Not too much extra code :)

https://codereview.chromium.org/890083002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/active_tab/background.js:29:
chrome.tabs.query({active:true}, function(tabs) {
no need for query, browserAction.onClicked takes the Tab

https://codereview.chromium.org/890083002/diff/40001/extensions/browser/exten...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/890083002/diff/40001/extensions/browser/exten...
extensions/browser/extension_web_contents_observer.cc:82: 1,
ExtensionMsg_Loaded_Params(extension, true))));
same deal, document the "true"

https://codereview.chromium.org/890083002/diff/40001/extensions/browser/rende...
File extensions/browser/renderer_startup_helper.cc (right):

https://codereview.chromium.org/890083002/diff/40001/extensions/browser/rende...
extensions/browser/renderer_startup_helper.cc:69:
ExtensionMsg_Loaded_Params(iter->get(), false));
and here

https://codereview.chromium.org/890083002/diff/40001/extensions/common/extens...
File extensions/common/extension_messages.cc (right):

https://codereview.chromium.org/890083002/diff/40001/extensions/common/extens...
extensions/common/extension_messages.cc:81: for (const auto& pair :
tab_specific_permissions) {
Should only really do this if |include_tab_permissions| is true, much as it
makes no difference, symmetry is nice.

[Devlin, 2015-02-05 19:54:42]

New patchsets have been uploaded after l-g-t-m from kalman@chromium.org

[Devlin, 2015-02-05 19:54:50]

https://codereview.chromium.org/890083002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/active_tab_permission_granter.cc (right):

https://codereview.chromium.org/890083002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_tab_permission_granter.cc:49: // also sends a
message to the tab process.
On 2015/02/05 00:16:13, kalman wrote:
> So, you could replace the second sentence of this comment as so:
> 
> "...given |view_hosts|, and |tab_process|."
> 
> It has the same meaning.
> 
> However, the behavior is a little different - you send false to
|create_message|
> if |tab_process| doesn't contain one of |view_hosts|. You should incorporate
> that somehow.

Done.

https://codereview.chromium.org/890083002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_tab_permission_granter.cc:116:
CreateMessageFunction create_message =
On 2015/02/05 00:16:13, kalman wrote:
> call this |update_message|, and below |clear_message|.

Done.

https://codereview.chromium.org/890083002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/890083002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1035: 1,
ExtensionMsg_Loaded_Params(extension, false));
On 2015/02/05 00:16:13, kalman wrote:
> document this (what does false mean / why is it set)

Done.

https://codereview.chromium.org/890083002/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/active_tab/background.js (right):

https://codereview.chromium.org/890083002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/active_tab/background.js:13: function
canXhr(url) {
On 2015/02/05 00:16:13, kalman wrote:
> Would be good to test iframes in here somewhere as well.
> 
> One way would be to inject an iframe into the tab (would need to be in
> web_accessible_resources) then check it can XHR. Not too much extra code :)

Done.

https://codereview.chromium.org/890083002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/active_tab/background.js:29:
chrome.tabs.query({active:true}, function(tabs) {
On 2015/02/05 00:16:13, kalman wrote:
> no need for query, browserAction.onClicked takes the Tab

Done.

https://codereview.chromium.org/890083002/diff/40001/extensions/browser/exten...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/890083002/diff/40001/extensions/browser/exten...
extensions/browser/extension_web_contents_observer.cc:82: 1,
ExtensionMsg_Loaded_Params(extension, true))));
On 2015/02/05 00:16:13, kalman wrote:
> same deal, document the "true"

Done.

https://codereview.chromium.org/890083002/diff/40001/extensions/browser/rende...
File extensions/browser/renderer_startup_helper.cc (right):

https://codereview.chromium.org/890083002/diff/40001/extensions/browser/rende...
extensions/browser/renderer_startup_helper.cc:69:
ExtensionMsg_Loaded_Params(iter->get(), false));
On 2015/02/05 00:16:13, kalman wrote:
> and here

Done.

https://codereview.chromium.org/890083002/diff/40001/extensions/common/extens...
File extensions/common/extension_messages.cc (right):

https://codereview.chromium.org/890083002/diff/40001/extensions/common/extens...
extensions/common/extension_messages.cc:81: for (const auto& pair :
tab_specific_permissions) {
On 2015/02/05 00:16:13, kalman wrote:
> Should only really do this if |include_tab_permissions| is true, much as it
> makes no difference, symmetry is nice.

But then we have to cache include_tab_permissions, and that also means sending
it across IPC.  Is the symmetry worth it?  Or can we just say that the fact that
tab_specific_permissions is/isn't empty is an indication that tab permissions
weren't/were included?

[not at google - send to devlin, 2015-02-05 22:40:23]

lgtm

https://codereview.chromium.org/890083002/diff/40001/extensions/common/extens...
File extensions/common/extension_messages.cc (right):

https://codereview.chromium.org/890083002/diff/40001/extensions/common/extens...
extensions/common/extension_messages.cc:81: for (const auto& pair :
tab_specific_permissions) {
On 2015/02/05 19:54:50, Devlin wrote:
> On 2015/02/05 00:16:13, kalman wrote:
> > Should only really do this if |include_tab_permissions| is true, much as it
> > makes no difference, symmetry is nice.
> 
> But then we have to cache include_tab_permissions, and that also means sending
> it across IPC.  Is the symmetry worth it?  Or can we just say that the fact
that
> tab_specific_permissions is/isn't empty is an indication that tab permissions
> weren't/were included?

Oh, you're not saving it... good point. Yes just say that.

https://codereview.chromium.org/890083002/diff/60001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/active_tab/background.js (right):

https://codereview.chromium.org/890083002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/active_tab/background.js:21: success =
false;
Okie could you also assert that e.name == 'NetworkError', just in case it's some
other kind of exception that was thrown.

https://codereview.chromium.org/890083002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/active_tab/background.js:39: var
injectIframe =
CSP SchmeeSP

https://codereview.chromium.org/890083002/diff/60001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/active_tab/iframe.js (right):

https://codereview.chromium.org/890083002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/active_tab/iframe.js:1: function
canXhr(url) {
you might get a copyright header presubmit fail if you don't add one

https://codereview.chromium.org/890083002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/active_tab/iframe.js:9: console.error(e);
Do you really want to be logging this?

(also same comment re asserting this is a NetworkError)

[Devlin, 2015-02-06 18:19:03]

New patchsets have been uploaded after l-g-t-m from kalman@chromium.org

[Devlin, 2015-02-06 18:59:53]

https://codereview.chromium.org/890083002/diff/60001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/active_tab/background.js (right):

https://codereview.chromium.org/890083002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/active_tab/background.js:21: success =
false;
On 2015/02/05 22:40:22, kalman wrote:
> Okie could you also assert that e.name == 'NetworkError', just in case it's
some
> other kind of exception that was thrown.

Good idea. Done.

https://codereview.chromium.org/890083002/diff/60001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/active_tab/iframe.js (right):

https://codereview.chromium.org/890083002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/active_tab/iframe.js:1: function
canXhr(url) {
On 2015/02/05 22:40:23, kalman wrote:
> you might get a copyright header presubmit fail if you don't add one

Done.

https://codereview.chromium.org/890083002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/active_tab/iframe.js:9: console.error(e);
On 2015/02/05 22:40:23, kalman wrote:
> Do you really want to be logging this?
> 
> (also same comment re asserting this is a NetworkError)

Nope!

[Devlin, 2015-02-06 19:00:50]

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

[Devlin, 2015-02-06 19:00:51]

+Justin for extension messages.

[jschuh, 2015-02-06 23:40:36]

ipc security lgtm (notes: browser -> extension metadata)

[Devlin, 2015-02-07 00:26:44]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2015-02-07 00:27:44]

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

[commit-bot: I haz the power, 2015-02-07 00:32:11]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-02-07 00:33:14]

Patchset 4 (id:??) landed as
https://crrev.com/c491dbfaad049247a5d2f92ab5ad823725414e70
Cr-Commit-Position: refs/heads/master@{#315147}