Refactoring: de-couple Extensions from "script injection System" [render side]:3

Refactoring: de-couple Extensions from "script injection System" [render side]:3

- Introduce WebUIInjectionHost class and plumb InjectionHost interface
  to replace Extension in script injection system in the render.
- Store InjectionHost in ScriptInjection to avoid constructing InjectionHost
  objects everywhere.

BUG=459234
Committed: https://crrev.com/9b8416668b08fc2261773c4e713a95175e029649
Cr-Commit-Position: refs/heads/master@{#319067}

[Xi Han, 2015-02-17 17:14:57]

Patchset #1 (id:1) has been deleted

[Xi Han, 2015-02-17 17:15:19]

Patchset #1 (id:20001) has been deleted

[Xi Han, 2015-02-17 17:19:48]

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

[Xi Han, 2015-02-17 17:19:49]

Hi Devlin, please review all changes. Thanks a lot!

[Devlin, 2015-02-17 18:56:53]

https://codereview.chromium.org/934763003/diff/40001/extensions/renderer/scri...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/934763003/diff/40001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:180: if (injection_host_->IsGone())
This won't really work, because you never update the refptr when the extension
is removed.

https://codereview.chromium.org/934763003/diff/40001/extensions/renderer/scri...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/934763003/diff/40001/extensions/renderer/scri...
extensions/renderer/script_injection_manager.cc:396:
(*iter)->GetInjectionHost(),
This is a bit silly - the ScriptInjection owns the injection host; why does it
need to be passed in?

https://codereview.chromium.org/934763003/diff/40001/extensions/renderer/user...
File extensions/renderer/user_script_set.cc (right):

https://codereview.chromium.org/934763003/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set.cc:197: const InjectionHost* injection_host,
We should probably just create the injection host in this function (since we
pass in the script), rather than passing it around everywhere.

https://codereview.chromium.org/934763003/diff/40001/extensions/renderer/web_...
File extensions/renderer/web_ui_injection_host.cc (right):

https://codereview.chromium.org/934763003/diff/40001/extensions/renderer/web_...
extensions/renderer/web_ui_injection_host.cc:33: return
extensions::PermissionsData::AccessType::ACCESS_ALLOWED;
comment why this is okay.

https://codereview.chromium.org/934763003/diff/40001/extensions/renderer/web_...
File extensions/renderer/web_ui_injection_host.h (right):

https://codereview.chromium.org/934763003/diff/40001/extensions/renderer/web_...
extensions/renderer/web_ui_injection_host.h:16: std::string
GetContentSecurityPolicy() const override;
make these private.

[Xi Han, 2015-02-18 21:04:34]

Patchset #2 (id:60001) has been deleted

[Xi Han, 2015-02-18 21:11:46]

Hi Devlin: code are added to let the Dispatch notify the ScriptInjectionManageer
to update its pending injections list. PTAL, thanks!

https://codereview.chromium.org/934763003/diff/40001/extensions/renderer/scri...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/934763003/diff/40001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:180: if (injection_host_->IsGone())
On 2015/02/17 18:56:52, Devlin wrote:
> This won't really work, because you never update the refptr when the extension
> is removed.

As discussed offline, the Dispatcher will notify the ScriptInjectionManager to
update its pending injection list when any extension is unloaded.

https://codereview.chromium.org/934763003/diff/40001/extensions/renderer/scri...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/934763003/diff/40001/extensions/renderer/scri...
extensions/renderer/script_injection_manager.cc:396:
(*iter)->GetInjectionHost(),
On 2015/02/17 18:56:52, Devlin wrote:
> This is a bit silly - the ScriptInjection owns the injection host; why does it
> need to be passed in?

Removed, and also updated some other functions that take InjectionHost* as input
parameters.

https://codereview.chromium.org/934763003/diff/40001/extensions/renderer/user...
File extensions/renderer/user_script_set.cc (right):

https://codereview.chromium.org/934763003/diff/40001/extensions/renderer/user...
extensions/renderer/user_script_set.cc:197: const InjectionHost* injection_host,
On 2015/02/17 18:56:52, Devlin wrote:
> We should probably just create the injection host in this function (since we
> pass in the script), rather than passing it around everywhere.

Done.

https://codereview.chromium.org/934763003/diff/40001/extensions/renderer/web_...
File extensions/renderer/web_ui_injection_host.cc (right):

https://codereview.chromium.org/934763003/diff/40001/extensions/renderer/web_...
extensions/renderer/web_ui_injection_host.cc:33: return
extensions::PermissionsData::AccessType::ACCESS_ALLOWED;
On 2015/02/17 18:56:52, Devlin wrote:
> comment why this is okay.

Added.

https://codereview.chromium.org/934763003/diff/40001/extensions/renderer/web_...
File extensions/renderer/web_ui_injection_host.h (right):

https://codereview.chromium.org/934763003/diff/40001/extensions/renderer/web_...
extensions/renderer/web_ui_injection_host.h:16: std::string
GetContentSecurityPolicy() const override;
On 2015/02/17 18:56:52, Devlin wrote:
> make these private.

Done.

[Devlin, 2015-02-20 22:39:13]

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/exte...
File extensions/renderer/extension_injection_host.h (right):

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/exte...
extensions/renderer/extension_injection_host.h:22: static scoped_ptr<const
ExtensionInjectionHost> Create(
document (in particular, that this can return a null scoped ptr)

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/exte...
extensions/renderer/extension_injection_host.h:37: const Extension* extension_;
I think it's fine for this to keep a refptr, but whatever's easier. :)

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/scri...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:193: DCHECK(injection_host_.get());
scoped_ptr has a bool operator, so this can be written DCHECK(injection_host_);
(also goes for ifs, etc)

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/scri...
File extensions/renderer/script_injection.h (right):

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/scri...
extensions/renderer/script_injection.h:57: void
set_injection_host(InjectionHost* host) {
two things:
- if this would actually reset the host to a new one, we should probably pass a
scoped_ptr, because it enforces the ownership scheme.
- it doesn't look like we ever use this for anything other than passing nullptr,
so why not just call it "ResetInjectionHost" or "OnHostRemoved" (and move it to
the .cc, please, to avoid the extra import).

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/scri...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/scri...
extensions/renderer/script_injection_manager.cc:245: void
ScriptInjectionManager::OnExtensionUnloaded(
Unfortunately, for the time being, if a script blocks, we run a nested message
loop. This basically means it can be unsafe to erase injections like this. 
Instead, we should check if |injecting_scripts_| is true, and, if so, add the id
to |invalidated_while_injecting_|.

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/scri...
extensions/renderer/script_injection_manager.cc:435: if
(!extension_injection_host.get())
remove the get

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/scri...
extensions/renderer/script_injection_manager.cc:487: (*iter)->host_id().type()
== HostID::EXTENSIONS)
Is there any time when the script injection could have the request id, but not
be an extension?  If not, can we DCHECK this?

[Xi Han, 2015-02-23 16:12:13]

Patchset #3 (id:100001) has been deleted

[Xi Han, 2015-02-23 16:14:59]

PTAL, thanks!

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/exte...
File extensions/renderer/extension_injection_host.h (right):

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/exte...
extensions/renderer/extension_injection_host.h:22: static scoped_ptr<const
ExtensionInjectionHost> Create(
On 2015/02/20 22:39:13, Devlin wrote:
> document (in particular, that this can return a null scoped ptr)

Done.

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/exte...
extensions/renderer/extension_injection_host.h:37: const Extension* extension_;
On 2015/02/20 22:39:13, Devlin wrote:
> I think it's fine for this to keep a refptr, but whatever's easier. :)

Acknowledged.

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/scri...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/scri...
extensions/renderer/script_injection.cc:193: DCHECK(injection_host_.get());
On 2015/02/20 22:39:13, Devlin wrote:
> scoped_ptr has a bool operator, so this can be written
DCHECK(injection_host_);
> (also goes for ifs, etc)

Done.

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/scri...
File extensions/renderer/script_injection.h (right):

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/scri...
extensions/renderer/script_injection.h:57: void
set_injection_host(InjectionHost* host) {
On 2015/02/20 22:39:13, Devlin wrote:
> two things:
> - if this would actually reset the host to a new one, we should probably pass
a
> scoped_ptr, because it enforces the ownership scheme.
> - it doesn't look like we ever use this for anything other than passing
nullptr,
> so why not just call it "ResetInjectionHost" or "OnHostRemoved" (and move it
to
> the .cc, please, to avoid the extra import).


You are right, it makes more sense to have "OnHostRemoved" and just reset the
pointer. Updated.

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/scri...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/scri...
extensions/renderer/script_injection_manager.cc:245: void
ScriptInjectionManager::OnExtensionUnloaded(
On 2015/02/20 22:39:13, Devlin wrote:
> Unfortunately, for the time being, if a script blocks, we run a nested message
> loop. This basically means it can be unsafe to erase injections like this. 
> Instead, we should check if |injecting_scripts_| is true, and, if so, add the
id
> to |invalidated_while_injecting_|.


Thanks for pointing out this. Add a check for |injecting_scripts_| and insert
the |extension_id| to |invalidated_while_injecting_|.

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/scri...
extensions/renderer/script_injection_manager.cc:435: if
(!extension_injection_host.get())
On 2015/02/20 22:39:13, Devlin wrote:
> remove the get

Done.

https://codereview.chromium.org/934763003/diff/80001/extensions/renderer/scri...
extensions/renderer/script_injection_manager.cc:487: (*iter)->host_id().type()
== HostID::EXTENSIONS)
On 2015/02/20 22:39:13, Devlin wrote:
> Is there any time when the script injection could have the request id, but not
> be an extension?  If not, can we DCHECK this?

DCHECK makes more sense, updated.

[Devlin, 2015-02-23 17:27:39]

lgtm

https://codereview.chromium.org/934763003/diff/120001/extensions/renderer/scr...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/934763003/diff/120001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:247: for(auto iter =
pending_injections_.begin();
need a space after 'for'

https://codereview.chromium.org/934763003/diff/120001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:253: else
bracketing should be 
if (...) {
  ...
  ...
} else {
  ...
}

https://codereview.chromium.org/934763003/diff/120001/extensions/renderer/use...
File extensions/renderer/user_script_set.cc (right):

https://codereview.chromium.org/934763003/diff/120001/extensions/renderer/use...
extensions/renderer/user_script_set.cc:187: const Extension* extension  =
extensions_->GetByID(host_id.id());
nit: Why not use the create method?
injection_host = ExtensionInjectionHost::Create(extensions_, host_id.id());
if (!injection_host)
  return injection.Pass();

https://codereview.chromium.org/934763003/diff/120001/extensions/renderer/use...
extensions/renderer/user_script_set.cc:191: } else if (host_id.type() ==
HostID::WEBUI) {
Since there are only two types, this should be an else with a DCHECK_EQ.

https://codereview.chromium.org/934763003/diff/120001/extensions/renderer/use...
File extensions/renderer/user_script_set.h (right):

https://codereview.chromium.org/934763003/diff/120001/extensions/renderer/use...
extensions/renderer/user_script_set.h:20: class InjectionHost;
needed?

https://codereview.chromium.org/934763003/diff/120001/extensions/renderer/web...
File extensions/renderer/web_ui_injection_host.cc (right):

https://codereview.chromium.org/934763003/diff/120001/extensions/renderer/web...
extensions/renderer/web_ui_injection_host.cc:34: return
extensions::PermissionsData::AccessType::ACCESS_ALLOWED;
In theory, shouldn't the document_url or top_frame_url match |url_|?  If so, can
we CHECK that?

[Xi Han, 2015-02-24 16:35:40]

Patchset #4 (id:140001) has been deleted

[Xi Han, 2015-02-24 16:39:50]

Hi Devlin: from my previous patch ([render side]:2) I realize the
webview-specific logic isn't implemented in the
InjectionHost::CanExecuteOnFrame(). Do you think it makes more sense to change
the return value of WebUIInjectionHost::CanExecuteOnFrame() from ACCESS_ALLOWED
to ACCESS_DENIED?

https://codereview.chromium.org/934763003/diff/120001/extensions/renderer/scr...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/934763003/diff/120001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:247: for(auto iter =
pending_injections_.begin();
On 2015/02/23 17:27:38, Devlin wrote:
> need a space after 'for'

Done.

https://codereview.chromium.org/934763003/diff/120001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:253: else
On 2015/02/23 17:27:39, Devlin wrote:
> bracketing should be 
> if (...) {
>   ...
>   ...
> } else {
>   ...
> }

Done.

https://codereview.chromium.org/934763003/diff/120001/extensions/renderer/use...
File extensions/renderer/user_script_set.cc (right):

https://codereview.chromium.org/934763003/diff/120001/extensions/renderer/use...
extensions/renderer/user_script_set.cc:187: const Extension* extension  =
extensions_->GetByID(host_id.id());
On 2015/02/23 17:27:39, Devlin wrote:
> nit: Why not use the create method?
> injection_host = ExtensionInjectionHost::Create(extensions_, host_id.id());
> if (!injection_host)
>   return injection.Pass();

Done.

https://codereview.chromium.org/934763003/diff/120001/extensions/renderer/use...
extensions/renderer/user_script_set.cc:191: } else if (host_id.type() ==
HostID::WEBUI) {
On 2015/02/23 17:27:39, Devlin wrote:
> Since there are only two types, this should be an else with a DCHECK_EQ.

Done.

https://codereview.chromium.org/934763003/diff/120001/extensions/renderer/use...
File extensions/renderer/user_script_set.h (right):

https://codereview.chromium.org/934763003/diff/120001/extensions/renderer/use...
extensions/renderer/user_script_set.h:20: class InjectionHost;
On 2015/02/23 17:27:39, Devlin wrote:
> needed?

Removed.

https://codereview.chromium.org/934763003/diff/120001/extensions/renderer/web...
File extensions/renderer/web_ui_injection_host.cc (right):

https://codereview.chromium.org/934763003/diff/120001/extensions/renderer/web...
extensions/renderer/web_ui_injection_host.cc:34: return
extensions::PermissionsData::AccessType::ACCESS_ALLOWED;
On 2015/02/23 17:27:39, Devlin wrote:
> In theory, shouldn't the document_url or top_frame_url match |url_|?  If so,
can
> we CHECK that?

Hmmm, they don't match. The |url_| is the url of the host (embedder), while
|document_url| or |top_frame_url| is the url of the <webview>.

[Xi Han, 2015-03-02 23:04:40]

The CQ bit was checked by hanxi@chromium.org

[Xi Han, 2015-03-02 23:04:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/934763003/#ps180001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-03-02 23:05:12]

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

[Devlin, 2015-03-02 23:14:57]

Looks like you missed a comment (bracketing).

https://codereview.chromium.org/934763003/diff/180001/extensions/renderer/scr...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/934763003/diff/180001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:253: else {
Bracketing.

https://codereview.chromium.org/934763003/diff/180001/extensions/renderer/use...
File extensions/renderer/user_script_set_manager.cc (right):

https://codereview.chromium.org/934763003/diff/180001/extensions/renderer/use...
extensions/renderer/user_script_set_manager.cc:11: #include
"extensions/renderer/injection_host.h"
Needed?

[Devlin, 2015-03-02 23:33:01]

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

[Xi Han, 2015-03-03 00:03:31]

https://codereview.chromium.org/934763003/diff/180001/extensions/renderer/scr...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/934763003/diff/180001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:253: else {
On 2015/03/02 23:14:57, Devlin wrote:
> Bracketing.

Miss the indent part. Done.

https://codereview.chromium.org/934763003/diff/180001/extensions/renderer/use...
File extensions/renderer/user_script_set_manager.cc (right):

https://codereview.chromium.org/934763003/diff/180001/extensions/renderer/use...
extensions/renderer/user_script_set_manager.cc:11: #include
"extensions/renderer/injection_host.h"
On 2015/03/02 23:14:57, Devlin wrote:
> Needed?

Done.

[Xi Han, 2015-03-03 00:03:39]

The CQ bit was checked by hanxi@chromium.org

[Xi Han, 2015-03-03 00:03:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/934763003/#ps200001
(title: " ")

[commit-bot: I haz the power, 2015-03-03 00:04:37]

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

[Xi Han, 2015-03-03 00:53:05]

The CQ bit was unchecked by hanxi@chromium.org

[Xi Han, 2015-03-04 13:42:06]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2015-03-04 13:43:02]

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

[commit-bot: I haz the power, 2015-03-04 14:36:49]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2015-03-04 14:37:24]

Patchset 6 (id:??) landed as
https://crrev.com/9b8416668b08fc2261773c4e713a95175e029649
Cr-Commit-Position: refs/heads/master@{#319067}