Run any pending injections in ActiveScriptController if permission is granted

Run any pending injections in ActiveScriptController if permission is granted

If ActiveTab permission is granted to an extension through a means other than
ActiveScriptController, have ActiveScriptController run any pending tasks for
that extension.

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

[Devlin, 2014-05-22 16:42:27]

Nice and short, for a change.

[not at google - send to devlin, 2014-05-22 20:44:49]

lgtm

https://codereview.chromium.org/296483011/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller.cc (right):

https://codereview.chromium.org/296483011/diff/20001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:172: DCHECK(extension);
you already DCHECK the extension at all call sites. given it's actually only
this method that cares about it, I'd just have it here (if at all, realy) and
not in those other 2 methods.

https://codereview.chromium.org/296483011/diff/20001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:175: if (iter
==pending_requests_.end())
s/ ==/ == /

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

https://codereview.chromium.org/296483011/diff/20001/chrome/browser/extension...
chrome/browser/extensions/active_tab_permission_granter.cc:81: if
(navigation_entry) {
I don't see why this should be gated on a navigation entry (.. conceptually, at
least).

https://codereview.chromium.org/296483011/diff/20001/chrome/browser/extension...
chrome/browser/extensions/active_tab_permission_granter.cc:88: // an observer
class.
yes indeed :)

which is to say, good comment, don't need to now.

https://codereview.chromium.org/296483011/diff/20001/chrome/browser/extension...
chrome/browser/extensions/active_tab_permission_granter.cc:89:
ActiveScriptController::GetForWebContents(web_contents())
also add a note to do this after the IPC has been sent, so that the renderer
knows about the correct permission state before anything caused by the
ActiveScriptController can happen.

[Devlin, 2014-05-22 22:12:36]

https://codereview.chromium.org/296483011/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller.cc (right):

https://codereview.chromium.org/296483011/diff/20001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:172: DCHECK(extension);
On 2014/05/22 20:44:49, kalman wrote:
> you already DCHECK the extension at all call sites. given it's actually only
> this method that cares about it, I'd just have it here (if at all, realy) and
> not in those other 2 methods.

Fair enough - maybe I was a bit DCHECK() happy. :)

https://codereview.chromium.org/296483011/diff/20001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:175: if (iter
==pending_requests_.end())
On 2014/05/22 20:44:49, kalman wrote:
> s/ ==/ == /

Done.

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

https://codereview.chromium.org/296483011/diff/20001/chrome/browser/extension...
chrome/browser/extensions/active_tab_permission_granter.cc:81: if
(navigation_entry) {
On 2014/05/22 20:44:49, kalman wrote:
> I don't see why this should be gated on a navigation entry (.. conceptually,
at
> least).

Conceptually, if there's no navigation entry, we don't really know what page
we'd even assign the tab-specific permissions to, so it doesn't make sense to
grant them, or run a script on that page.  This should only happen if, e.g., no
page has fully loaded.  So if a page hasn't loaded, but _will_ load
sensitive-website.com, we don't want to prematurely grant access.  Better to
ignore the request.

Non-conceptually, if we don't have a navigation controller, we can't confirm
page id, and it doesn't matter.  Or the renderer won't have the updated
permissions, and we can't execute anyway.  Or, if we move out both the send and
the ASC stuff, we crash on line 83.  So I figured it best in here. :)  (At
least, until we refactor stuff...)

https://codereview.chromium.org/296483011/diff/20001/chrome/browser/extension...
chrome/browser/extensions/active_tab_permission_granter.cc:89:
ActiveScriptController::GetForWebContents(web_contents())
On 2014/05/22 20:44:49, kalman wrote:
> also add a note to do this after the IPC has been sent, so that the renderer
> knows about the correct permission state before anything caused by the
> ActiveScriptController can happen.

Done.

[Devlin, 2014-05-22 22:12:39]

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

[commit-bot: I haz the power, 2014-05-22 22:13:10]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/296483011/...

[not at google - send to devlin, 2014-05-22 22:21:47]

lgtm

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

https://codereview.chromium.org/296483011/diff/20001/chrome/browser/extension...
chrome/browser/extensions/active_tab_permission_granter.cc:81: if
(navigation_entry) {
On 2014/05/22 22:12:36, Devlin wrote:
> On 2014/05/22 20:44:49, kalman wrote:
> > I don't see why this should be gated on a navigation entry (.. conceptually,
> at
> > least).
> 
> Conceptually, if there's no navigation entry, we don't really know what page
> we'd even assign the tab-specific permissions to, so it doesn't make sense to
> grant them, or run a script on that page.  This should only happen if, e.g.,
no
> page has fully loaded.  So if a page hasn't loaded, but _will_ load
> http://sensitive-website.com, we don't want to prematurely grant access. 
Better to
> ignore the request.
> 
> Non-conceptually, if we don't have a navigation controller, we can't confirm
> page id, and it doesn't matter.  Or the renderer won't have the updated
> permissions, and we can't execute anyway.  Or, if we move out both the send
and
> the ASC stuff, we crash on line 83.  So I figured it best in here. :)  (At
> least, until we refactor stuff...)

Yeah so some of this is part of a larger question: should we be checking against
the last committed URL or the visible URL. last committed is typically more
correct for host permission checks, but for granting active tab the visible URL
is what the user is seeing, so that's what they're granting access to; it seems
more correct.

... and this stuff sits in the middle, so it's hard to think about.

in any case, it's the *tab* that's being assigned the permissions, not the page.
I'd expect that ASC will pull out the page ID and check it (... which is does,
but that's not the concern of this class)

the point being: the effect is identical. I just don't think that conceptually
this class should assume anything about ASC. we still update the browser-side
state (line 76) even though there wasn't a navigation entry.

[commit-bot: I haz the power, 2014-05-23 06:50:36]

Change committed as 272413