[Reland] Allow headless TabSocket in isolated worlds & remove obsolete logic

Allow headless TabSocket in isolated worlds & remove obsolete logic

This patch only affects C++ Embedders of headless_lib and should have
no effect on normal chrome even with the --headless flag.

Recently we decided to no longer allow arbitrary mojo bindings, instead
relying on the headless::TabSocket API for js<-->C++ communications. 

The public headless API for registering a mojo module has already been
removed and this patch finishes the job by removing the code from
MojoBindingsController which injected js mojo bindings for
BINDINGS_POLICY_HEADLESS.

In this patch we are replacing BINDINGS_POLICY_HEADLESS with
BINDINGS_POLICY_HEADLESS_MAIN_WORLD and
BINDINGS_POLICY_HEADLESS_ISOLATED_WORLD which provide access to the
headless::TabSocket API to either the main world or for isolated worlds
created by the Page.CreateIsolatedWorld DevTools command.

For a security point of view this patch hopefully reduces the attack
surface for the main world because js now only has access to the
headless::TabSocket API rather than arbitrary mojo modules.  Providing
the headless::TabSocket API into isolated worlds is new but hopefully
non-controversial. I note that ContentScripts have access to similar 
APIs for communication with the owning extension. I did investigate
reusing that code but it didn't seem feasible.

BUG=546953
Review-Url: https://codereview.chromium.org/2873283002
Cr-Original-Commit-Position: refs/heads/master@{#474238}
Committed: https://chromium.googlesource.com/chromium/src/+/07ee7c399295b210c5338dfeb543f2d30f748340
Review-Url: https://codereview.chromium.org/2873283002
Cr-Commit-Position: refs/heads/master@{#474311}
Committed: https://chromium.googlesource.com/chromium/src/+/1a4a3bab489e96546385236195be3d9b449a10e0

[alex clarke (OOO till 29th), 2017-05-12 08:21:47]

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

[commit-bot: I haz the power, 2017-05-12 08:22:10]

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, 2017-05-12 09:23:20]

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

[commit-bot: I haz the power, 2017-05-12 09:23:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alex clarke (OOO till 29th), 2017-05-12 10:06:39]

Description was changed from

==========
Prototype for IsolatedWorld HeadlessTabSocket

BUG=546953
==========

to

==========
This patch only affects C++ Embedders of headless_lib and should have
no effect on normal chrome even with the --headless flag.

Recently we decided to no longer allow arbitrary mojo bindings, instead
relying on the headless::TabSocket API for js<-->C++ communications. 

The public headless API for registering a mojo module has already been
removed and this patch finishes the job by removing the code from
MojoBindingsController which injected js mojo bindings for
BINDINGS_POLICY_HEADLESS.

In this patch we are replacing BINDINGS_POLICY_HEADLESS with
BINDINGS_POLICY_HEADLESS_MAIN_WORLD and
BINDINGS_POLICY_HEADLESS_ISOLATED_WORLD which provide access to the
headless::TabSocket API to either the main world or for isolated worlds
created by the Page.CreateIsolatedWorld DevTools command.

For a security point of view this patch hopefully reduces the attack
surface for the main world because js now only has access to the
headless::TabSocket API rather than arbitrary mojo modules.  Providing
the headless::TabSocket API into isolated worlds is new but hopefully
non-controversial. I note that ContentScripts have access to similar 
APIs for communication with the owning extension. I did investigate
reusing that code but it didn't seem feasible.

BUG=546953
==========

[alex clarke (OOO till 29th), 2017-05-12 10:08:35]

alexclarke@chromium.org changed reviewers:
+ mkwst@chromium.org, skyostil@chromium.org

[alex clarke (OOO till 29th), 2017-05-12 10:08:36]

Sami: PTAL
Mike: Can you please take a look at this from a security POV
and review the gin dependency? Happy to answer any question here or over chat /
vc.

Thanks!

[alex clarke (OOO till 29th), 2017-05-12 10:09:18]

alexclarke@chromium.org changed reviewers:
+ jochen@chromium.org

[alex clarke (OOO till 29th), 2017-05-12 10:09:18]

Jochen can you please review the content changes?

[Sami, 2017-05-12 17:18:02]

https://codereview.chromium.org/2873283002/diff/40001/content/public/common/b...
File content/public/common/bindings_policy.h (right):

https://codereview.chromium.org/2873283002/diff/40001/content/public/common/b...
content/public/common/bindings_policy.h:35: // Bindings that allows the JS
content within a HeadlessWebContents to access
"main world JS content"

https://codereview.chromium.org/2873283002/diff/40001/content/public/common/b...
content/public/common/bindings_policy.h:39: // access from within DevTools
created isolated worlds.
"access only from"

https://codereview.chromium.org/2873283002/diff/40001/content/public/common/b...
content/public/common/bindings_policy.h:40:
BINDINGS_POLICY_HEADLESS_ISOLATED_WORLD = 1 << 5,
Since these are only used and make sense in headless/, should we make these
options on the HeadlessWebContents instead?

https://codereview.chromium.org/2873283002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_web_contents_impl.cc (right):

https://codereview.chromium.org/2873283002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_web_contents_impl.cc:392: create_tab_socket_ =
create_tab_socket;
create_tab_socket_in_main_world_ to be explicit?

https://codereview.chromium.org/2873283002/diff/40001/headless/lib/renderer/h...
File headless/lib/renderer/headless_content_renderer_client.cc (right):

https://codereview.chromium.org/2873283002/diff/40001/headless/lib/renderer/h...
headless/lib/renderer/headless_content_renderer_client.cc:46: class
HeadlessIsolatedWorldTabSocketBindings
s/Isolated// since this is also used for the main world?

https://codereview.chromium.org/2873283002/diff/40001/headless/lib/renderer/h...
headless/lib/renderer/headless_content_renderer_client.cc:123:
ASCIIStringToV8String(isolate, message),
Hmm, is the message really guaranteed to ASCII?

https://codereview.chromium.org/2873283002/diff/40001/headless/public/headles...
File headless/public/headless_web_contents.h (right):

https://codereview.chromium.org/2873283002/diff/40001/headless/public/headles...
headless/public/headless_web_contents.h:119: Builder& CreateTabSocket(bool
create_tab_socket, bool in_isolated_worlds);
Maybe it would be good to be explicit at this level too, e.g., enum
TabSocketType { NONE, MAIN_WORLD, ISOLATED_WORLD }.

[jochen (gone - plz use gerrit), 2017-05-15 05:41:56]

nit. the first line of the cl description needs to repeat the CL's title

https://codereview.chromium.org/2873283002/diff/40001/headless/lib/renderer/h...
File headless/lib/renderer/headless_content_renderer_client.cc (right):

https://codereview.chromium.org/2873283002/diff/40001/headless/lib/renderer/h...
headless/lib/renderer/headless_content_renderer_client.cc:34:
kDevToolsFirstIsolatedWorldId = (1 << 29) + 1,
can you pull those IDs into a separate header, so you can just include it here?

https://codereview.chromium.org/2873283002/diff/40001/headless/lib/renderer/h...
headless/lib/renderer/headless_content_renderer_client.cc:38:
v8::Local<v8::String> ASCIIStringToV8String(v8::Isolate* isolate,
that's gin::Converter<std::string>::ToV8()

https://codereview.chromium.org/2873283002/diff/40001/headless/lib/renderer/h...
headless/lib/renderer/headless_content_renderer_client.cc:153:
context_.Reset(blink::MainThreadIsolate(), context);
this will create a memory leak, as the context now can never die, and so can
this object that lives in the context never die

[alex clarke (OOO till 29th), 2017-05-15 09:50:57]

PTAL

https://codereview.chromium.org/2873283002/diff/40001/content/public/common/b...
File content/public/common/bindings_policy.h (right):

https://codereview.chromium.org/2873283002/diff/40001/content/public/common/b...
content/public/common/bindings_policy.h:35: // Bindings that allows the JS
content within a HeadlessWebContents to access
On 2017/05/12 17:18:02, Sami wrote:
> "main world JS content"

Done.

https://codereview.chromium.org/2873283002/diff/40001/content/public/common/b...
content/public/common/bindings_policy.h:39: // access from within DevTools
created isolated worlds.
On 2017/05/12 17:18:02, Sami wrote:
> "access only from"

Done.

https://codereview.chromium.org/2873283002/diff/40001/content/public/common/b...
content/public/common/bindings_policy.h:40:
BINDINGS_POLICY_HEADLESS_ISOLATED_WORLD = 1 << 5,
On 2017/05/12 17:18:02, Sami wrote:
> Since these are only used and make sense in headless/, should we make these
> options on the HeadlessWebContents instead?

That's a possibility however what if these values change here so something new
overlaps?  Feels safer if it's here.

https://codereview.chromium.org/2873283002/diff/40001/headless/lib/browser/he...
File headless/lib/browser/headless_web_contents_impl.cc (right):

https://codereview.chromium.org/2873283002/diff/40001/headless/lib/browser/he...
headless/lib/browser/headless_web_contents_impl.cc:392: create_tab_socket_ =
create_tab_socket;
On 2017/05/12 17:18:02, Sami wrote:
> create_tab_socket_in_main_world_ to be explicit?

Maybe an enum is better still.

https://codereview.chromium.org/2873283002/diff/40001/headless/lib/renderer/h...
File headless/lib/renderer/headless_content_renderer_client.cc (right):

https://codereview.chromium.org/2873283002/diff/40001/headless/lib/renderer/h...
headless/lib/renderer/headless_content_renderer_client.cc:34:
kDevToolsFirstIsolatedWorldId = (1 << 29) + 1,
On 2017/05/15 05:41:56, jochen wrote:
> can you pull those IDs into a separate header, so you can just include it
here?

It would be nice if we could share the same header from inside blink too.  Not
sure where such a header would live however.  For now I've just added it in the
same directory as this file.

https://codereview.chromium.org/2873283002/diff/40001/headless/lib/renderer/h...
headless/lib/renderer/headless_content_renderer_client.cc:38:
v8::Local<v8::String> ASCIIStringToV8String(v8::Isolate* isolate,
On 2017/05/15 05:41:56, jochen wrote:
> that's gin::Converter<std::string>::ToV8()

Done.

https://codereview.chromium.org/2873283002/diff/40001/headless/lib/renderer/h...
headless/lib/renderer/headless_content_renderer_client.cc:46: class
HeadlessIsolatedWorldTabSocketBindings
On 2017/05/12 17:18:02, Sami wrote:
> s/Isolated// since this is also used for the main world?

Good point, this was originally isolated only but it morphed into this.

https://codereview.chromium.org/2873283002/diff/40001/headless/lib/renderer/h...
headless/lib/renderer/headless_content_renderer_client.cc:123:
ASCIIStringToV8String(isolate, message),
On 2017/05/12 17:18:02, Sami wrote:
> Hmm, is the message really guaranteed to ASCII?

That's the api contract.

https://codereview.chromium.org/2873283002/diff/40001/headless/lib/renderer/h...
headless/lib/renderer/headless_content_renderer_client.cc:153:
context_.Reset(blink::MainThreadIsolate(), context);
On 2017/05/15 05:41:56, jochen wrote:
> this will create a memory leak, as the context now can never die, and so can
> this object that lives in the context never die

I based this on what mojo does (see gin::ContextHolder & MojoMainRunner).  It
seems they release the UniquePersistents in WillReleaseScriptContext and I've
done the same, hopefully that works.

https://codereview.chromium.org/2873283002/diff/40001/headless/public/headles...
File headless/public/headless_web_contents.h (right):

https://codereview.chromium.org/2873283002/diff/40001/headless/public/headles...
headless/public/headless_web_contents.h:119: Builder& CreateTabSocket(bool
create_tab_socket, bool in_isolated_worlds);
On 2017/05/12 17:18:02, Sami wrote:
> Maybe it would be good to be explicit at this level too, e.g., enum
> TabSocketType { NONE, MAIN_WORLD, ISOLATED_WORLD }.

Done.

[alex clarke (OOO till 29th), 2017-05-15 09:51:04]

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

[commit-bot: I haz the power, 2017-05-15 09:51:25]

Dry run: CQ is trying da patch.

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

[alex clarke (OOO till 29th), 2017-05-15 09:53:25]

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

[commit-bot: I haz the power, 2017-05-15 09:53:40]

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, 2017-05-15 10:13:51]

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

[commit-bot: I haz the power, 2017-05-15 10:13:52]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[alex clarke (OOO till 29th), 2017-05-15 10:31:14]

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

[commit-bot: I haz the power, 2017-05-15 10:31:22]

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, 2017-05-15 12:39:45]

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

[commit-bot: I haz the power, 2017-05-15 12:39:46]

Dry run: This issue passed the CQ dry run.

[Sami, 2017-05-15 16:30:25]

https://codereview.chromium.org/2873283002/diff/40001/content/public/common/b...
File content/public/common/bindings_policy.h (right):

https://codereview.chromium.org/2873283002/diff/40001/content/public/common/b...
content/public/common/bindings_policy.h:40:
BINDINGS_POLICY_HEADLESS_ISOLATED_WORLD = 1 << 5,
On 2017/05/15 09:50:57, alex clarke wrote:
> On 2017/05/12 17:18:02, Sami wrote:
> > Since these are only used and make sense in headless/, should we make these
> > options on the HeadlessWebContents instead?
> 
> That's a possibility however what if these values change here so something new
> overlaps?  Feels safer if it's here.

I meant since content/ doesn't look at these flags at all, they shouldn't need
to be defined here (unless I missed something).

If content/ needs the flag, perhaps we should just keep BINDINGS_POLICY_HEADLESS
and have headless control the main/isolated world distinction based on a
different setting?

[alex clarke (OOO till 29th), 2017-05-16 10:21:24]

https://codereview.chromium.org/2873283002/diff/40001/content/public/common/b...
File content/public/common/bindings_policy.h (right):

https://codereview.chromium.org/2873283002/diff/40001/content/public/common/b...
content/public/common/bindings_policy.h:40:
BINDINGS_POLICY_HEADLESS_ISOLATED_WORLD = 1 << 5,
On 2017/05/15 16:30:25, Sami wrote:
> On 2017/05/15 09:50:57, alex clarke wrote:
> > On 2017/05/12 17:18:02, Sami wrote:
> > > Since these are only used and make sense in headless/, should we make
these
> > > options on the HeadlessWebContents instead?
> > 
> > That's a possibility however what if these values change here so something
new
> > overlaps?  Feels safer if it's here.
> 
> I meant since content/ doesn't look at these flags at all, they shouldn't need
> to be defined here (unless I missed something).

The bindings policy is sent from browser to renderer and it seems you're
suggesting we add a new IPC.  I'd prefer not to since this field seems
appropriately named.

[Sami, 2017-05-16 13:49:14]

> The bindings policy is sent from browser to renderer and it seems you're
> suggesting we add a new IPC.  I'd prefer not to since this field seems
> appropriately named.

Right, the field feels like a good match for what we want but routing it through
content seems wrong since it's only really defined at the embedder level. As an
alternative I was thinking we could either extend tab_socket.mojom with a query
for the enabled bindings or maybe add a new headless.mojom for sending config
data to the renderer. That said, I think I can live with this approach too but
just wanted to explain why it felt a little odd.

https://codereview.chromium.org/2873283002/diff/100001/headless/BUILD.gn
File headless/BUILD.gn (right):

https://codereview.chromium.org/2873283002/diff/100001/headless/BUILD.gn#newc...
headless/BUILD.gn:409: "//third_party/WebKit/public:blink",
Did you mean to put this behind enable_basic_priting?

https://codereview.chromium.org/2873283002/diff/100001/headless/lib/browser/h...
File headless/lib/browser/headless_web_contents_impl.h (right):

https://codereview.chromium.org/2873283002/diff/100001/headless/lib/browser/h...
headless/lib/browser/headless_web_contents_impl.h:118: bool
inject_mojo_services_into_isolated_world_;
Not initialized in constructor?

https://codereview.chromium.org/2873283002/diff/100001/headless/lib/renderer/...
File headless/lib/renderer/headless_content_renderer_client.cc (right):

https://codereview.chromium.org/2873283002/diff/100001/headless/lib/renderer/...
headless/lib/renderer/headless_content_renderer_client.cc:102:
base::Unretained(this)));
What's the lifetime of this object? Should we use a weak pointer here?

https://codereview.chromium.org/2873283002/diff/100001/headless/public/headle...
File headless/public/headless_web_contents.h (right):

https://codereview.chromium.org/2873283002/diff/100001/headless/public/headle...
headless/public/headless_web_contents.h:116: NONE,           // No tabspcket
binds created (default).
typo: TabSocket

[alex clarke (OOO till 29th), 2017-05-17 09:09:08]

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

[alex clarke (OOO till 29th), 2017-05-17 09:09:32]

PTAL

https://codereview.chromium.org/2873283002/diff/100001/headless/BUILD.gn
File headless/BUILD.gn (right):

https://codereview.chromium.org/2873283002/diff/100001/headless/BUILD.gn#newc...
headless/BUILD.gn:409: "//third_party/WebKit/public:blink",
On 2017/05/16 13:49:14, Sami wrote:
> Did you mean to put this behind enable_basic_priting?

Done.

https://codereview.chromium.org/2873283002/diff/100001/headless/lib/browser/h...
File headless/lib/browser/headless_web_contents_impl.h (right):

https://codereview.chromium.org/2873283002/diff/100001/headless/lib/browser/h...
headless/lib/browser/headless_web_contents_impl.h:118: bool
inject_mojo_services_into_isolated_world_;
On 2017/05/16 13:49:14, Sami wrote:
> Not initialized in constructor?

Done.

https://codereview.chromium.org/2873283002/diff/100001/headless/lib/renderer/...
File headless/lib/renderer/headless_content_renderer_client.cc (right):

https://codereview.chromium.org/2873283002/diff/100001/headless/lib/renderer/...
headless/lib/renderer/headless_content_renderer_client.cc:102:
base::Unretained(this)));
On 2017/05/16 13:49:14, Sami wrote:
> What's the lifetime of this object? Should we use a weak pointer here?

Done.

https://codereview.chromium.org/2873283002/diff/100001/headless/public/headle...
File headless/public/headless_web_contents.h (right):

https://codereview.chromium.org/2873283002/diff/100001/headless/public/headle...
headless/public/headless_web_contents.h:116: NONE,           // No tabspcket
binds created (default).
On 2017/05/16 13:49:14, Sami wrote:
> typo: TabSocket

Done.

[commit-bot: I haz the power, 2017-05-17 09:09:43]

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, 2017-05-17 11:14:21]

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

[commit-bot: I haz the power, 2017-05-17 11:14:22]

Dry run: This issue passed the CQ dry run.

[Sami, 2017-05-17 12:49:42]

lgtm, although I'd appreciate if someone with more gin knowledge makes sure the
bindings look fine.

[alex clarke (OOO till 29th), 2017-05-18 07:28:23]

Description was changed from

==========
This patch only affects C++ Embedders of headless_lib and should have
no effect on normal chrome even with the --headless flag.

Recently we decided to no longer allow arbitrary mojo bindings, instead
relying on the headless::TabSocket API for js<-->C++ communications. 

The public headless API for registering a mojo module has already been
removed and this patch finishes the job by removing the code from
MojoBindingsController which injected js mojo bindings for
BINDINGS_POLICY_HEADLESS.

In this patch we are replacing BINDINGS_POLICY_HEADLESS with
BINDINGS_POLICY_HEADLESS_MAIN_WORLD and
BINDINGS_POLICY_HEADLESS_ISOLATED_WORLD which provide access to the
headless::TabSocket API to either the main world or for isolated worlds
created by the Page.CreateIsolatedWorld DevTools command.

For a security point of view this patch hopefully reduces the attack
surface for the main world because js now only has access to the
headless::TabSocket API rather than arbitrary mojo modules.  Providing
the headless::TabSocket API into isolated worlds is new but hopefully
non-controversial. I note that ContentScripts have access to similar 
APIs for communication with the owning extension. I did investigate
reusing that code but it didn't seem feasible.

BUG=546953
==========

to

==========
Allow headless TabSocket in isolated worlds & remove obsolete logic

This patch only affects C++ Embedders of headless_lib and should have
no effect on normal chrome even with the --headless flag.

Recently we decided to no longer allow arbitrary mojo bindings, instead
relying on the headless::TabSocket API for js<-->C++ communications. 

The public headless API for registering a mojo module has already been
removed and this patch finishes the job by removing the code from
MojoBindingsController which injected js mojo bindings for
BINDINGS_POLICY_HEADLESS.

In this patch we are replacing BINDINGS_POLICY_HEADLESS with
BINDINGS_POLICY_HEADLESS_MAIN_WORLD and
BINDINGS_POLICY_HEADLESS_ISOLATED_WORLD which provide access to the
headless::TabSocket API to either the main world or for isolated worlds
created by the Page.CreateIsolatedWorld DevTools command.

For a security point of view this patch hopefully reduces the attack
surface for the main world because js now only has access to the
headless::TabSocket API rather than arbitrary mojo modules.  Providing
the headless::TabSocket API into isolated worlds is new but hopefully
non-controversial. I note that ContentScripts have access to similar 
APIs for communication with the owning extension. I did investigate
reusing that code but it didn't seem feasible.

BUG=546953
==========

[alex clarke (OOO till 29th), 2017-05-18 07:28:59]

Jochen could you please take another look? Thanks!

[jochen (gone - plz use gerrit), 2017-05-19 13:33:14]

https://codereview.chromium.org/2873283002/diff/120001/headless/lib/renderer/...
File headless/lib/renderer/headless_content_renderer_client.cc (right):

https://codereview.chromium.org/2873283002/diff/120001/headless/lib/renderer/...
headless/lib/renderer/headless_content_renderer_client.cc:122: context,
context->Global(), arraysize(argv), argv);
this should probably use WebLocalFrame::requestExecuteScriptInIsolatedWorld?

[jochen (gone - plz use gerrit), 2017-05-19 13:33:29]

also, is it possible to write tests for this?

[jochen (gone - plz use gerrit), 2017-05-19 13:34:05]

https://codereview.chromium.org/2873283002/diff/120001/headless/lib/renderer/...
File headless/lib/renderer/devtools_isolated_world_ids.h (right):

https://codereview.chromium.org/2873283002/diff/120001/headless/lib/renderer/...
headless/lib/renderer/devtools_isolated_world_ids.h:14: };
why not put this file in public/web ? it's ok for webkit to include headers from
public/web

[alex clarke (OOO till 29th), 2017-05-19 14:46:06]

On 2017/05/19 13:33:29, jochen wrote:
> also, is it possible to write tests for this?

Yes!  See headless/lib/headless_web_contents_browsertest.cc where I'm adding
GetHeadlessTabSocketButNoTabSocket,  HeadlessMainWorldTabSocketTest,
HeadlessMainWorldTabSocketNotThereTest, HeadlessIsolatedWorldTabSocketTest,
HeadlessIsolatedWorldTabSocketNotThereTest which test various aspects of this.

[alex clarke (OOO till 29th), 2017-05-19 14:46:15]

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

[commit-bot: I haz the power, 2017-05-19 14:46:35]

Dry run: CQ is trying da patch.

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

[alex clarke (OOO till 29th), 2017-05-19 14:53:01]

PTAL

https://codereview.chromium.org/2873283002/diff/120001/headless/lib/renderer/...
File headless/lib/renderer/devtools_isolated_world_ids.h (right):

https://codereview.chromium.org/2873283002/diff/120001/headless/lib/renderer/...
headless/lib/renderer/devtools_isolated_world_ids.h:14: };
On 2017/05/19 13:34:05, jochen wrote:
> why not put this file in public/web ? it's ok for webkit to include headers
from
> public/web

Done.  I wonder if I should have pulled all the enums from DOMWrapperWorld in
too?  I didn't because the comments mentioning  DOMWrapperWorld APIs felt odd to
be there.

https://codereview.chromium.org/2873283002/diff/120001/headless/lib/renderer/...
File headless/lib/renderer/headless_content_renderer_client.cc (right):

https://codereview.chromium.org/2873283002/diff/120001/headless/lib/renderer/...
headless/lib/renderer/headless_content_renderer_client.cc:122: context,
context->Global(), arraysize(argv), argv);
On 2017/05/19 13:33:13, jochen wrote:
> this should probably use WebLocalFrame::requestExecuteScriptInIsolatedWorld?

The TabSocket lets us do this:

TabSocket.onmessage =
  function(message) {
    TabSocket.send('Embedder sent us: ' + message);
};

I can see that WebLocalFrame::requestExecuteScriptInIsolatedWorld lets us run JS
but I'm not clear how I can get it to run the function in
|on_message_callback_|. 

What I'm trying to build here is similar to what content scripts have, and I
notice that ScriptContext::SafeCallFunction uses the
WebLocalFrame::RequestExecuteV8Function API.  Should i be using that here?

[commit-bot: I haz the power, 2017-05-19 17:52:07]

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

[commit-bot: I haz the power, 2017-05-19 17:52:08]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-05-22 12:04:22]

On 2017/05/19 at 14:53:01, alexclarke wrote:
> PTAL
> 
>
https://codereview.chromium.org/2873283002/diff/120001/headless/lib/renderer/...
> File headless/lib/renderer/devtools_isolated_world_ids.h (right):
> 
>
https://codereview.chromium.org/2873283002/diff/120001/headless/lib/renderer/...
> headless/lib/renderer/devtools_isolated_world_ids.h:14: };
> On 2017/05/19 13:34:05, jochen wrote:
> > why not put this file in public/web ? it's ok for webkit to include headers
from
> > public/web
> 
> Done.  I wonder if I should have pulled all the enums from DOMWrapperWorld in
too?  I didn't because the comments mentioning  DOMWrapperWorld APIs felt odd to
be there.
> 
>
https://codereview.chromium.org/2873283002/diff/120001/headless/lib/renderer/...
> File headless/lib/renderer/headless_content_renderer_client.cc (right):
> 
>
https://codereview.chromium.org/2873283002/diff/120001/headless/lib/renderer/...
> headless/lib/renderer/headless_content_renderer_client.cc:122: context,
context->Global(), arraysize(argv), argv);
> On 2017/05/19 13:33:13, jochen wrote:
> > this should probably use WebLocalFrame::requestExecuteScriptInIsolatedWorld?
> 
> The TabSocket lets us do this:
> 
> TabSocket.onmessage =
>   function(message) {
>     TabSocket.send('Embedder sent us: ' + message);
> };
> 
> I can see that WebLocalFrame::requestExecuteScriptInIsolatedWorld lets us run
JS but I'm not clear how I can get it to run the function in
|on_message_callback_|. 
> 
> What I'm trying to build here is similar to what content scripts have, and I
notice that ScriptContext::SafeCallFunction uses the
WebLocalFrame::RequestExecuteV8Function API.  Should i be using that here?

I'd add a version of requestExecuteScriptInIsolatedWorld that takes a
v8::Local<v8::Function> etc..

You both want SafeCallFunciton and this SuspendableExecutor to avoid running
script while script execution is suspended

[alex clarke (OOO till 29th), 2017-05-23 08:16:28]

On 2017/05/22 12:04:22, jochen wrote:
> On 2017/05/19 at 14:53:01, alexclarke wrote:
> > PTAL
> > 
> >
>
https://codereview.chromium.org/2873283002/diff/120001/headless/lib/renderer/...
> > File headless/lib/renderer/devtools_isolated_world_ids.h (right):
> > 
> >
>
https://codereview.chromium.org/2873283002/diff/120001/headless/lib/renderer/...
> > headless/lib/renderer/devtools_isolated_world_ids.h:14: };
> > On 2017/05/19 13:34:05, jochen wrote:
> > > why not put this file in public/web ? it's ok for webkit to include
headers
> from
> > > public/web
> > 
> > Done.  I wonder if I should have pulled all the enums from DOMWrapperWorld
in
> too?  I didn't because the comments mentioning  DOMWrapperWorld APIs felt odd
to
> be there.
> > 
> >
>
https://codereview.chromium.org/2873283002/diff/120001/headless/lib/renderer/...
> > File headless/lib/renderer/headless_content_renderer_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/2873283002/diff/120001/headless/lib/renderer/...
> > headless/lib/renderer/headless_content_renderer_client.cc:122: context,
> context->Global(), arraysize(argv), argv);
> > On 2017/05/19 13:33:13, jochen wrote:
> > > this should probably use
WebLocalFrame::requestExecuteScriptInIsolatedWorld?
> > 
> > The TabSocket lets us do this:
> > 
> > TabSocket.onmessage =
> >   function(message) {
> >     TabSocket.send('Embedder sent us: ' + message);
> > };
> > 
> > I can see that WebLocalFrame::requestExecuteScriptInIsolatedWorld lets us
run
> JS but I'm not clear how I can get it to run the function in
> |on_message_callback_|. 
> > 
> > What I'm trying to build here is similar to what content scripts have, and I
> notice that ScriptContext::SafeCallFunction uses the
> WebLocalFrame::RequestExecuteV8Function API.  Should i be using that here?
> 
> I'd add a version of requestExecuteScriptInIsolatedWorld that takes a
> v8::Local<v8::Function> etc..
> 
> You both want SafeCallFunciton and this SuspendableExecutor to avoid running
> script while script execution is suspended

It seems to work fine with RequestExecuteV8Function for both main world and
isolated script contexts. Do we need a new API point?

[alex clarke (OOO till 29th), 2017-05-23 08:16:39]

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

[commit-bot: I haz the power, 2017-05-23 08:17:03]

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, 2017-05-23 08:19:21]

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

[commit-bot: I haz the power, 2017-05-23 08:19:22]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alex clarke (OOO till 29th), 2017-05-23 08:24:05]

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

[commit-bot: I haz the power, 2017-05-23 08:24:16]

Dry run: CQ is trying da patch.

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

[jochen (gone - plz use gerrit), 2017-05-23 11:10:55]

oh, nice

lgtm with header moved to platform

https://codereview.chromium.org/2873283002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/bindings/DEPS (right):

https://codereview.chromium.org/2873283002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/bindings/DEPS:3: "+public/web"
that won't work. Maybe just move the Ids header to public/platform instead?

[commit-bot: I haz the power, 2017-05-23 13:16:51]

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

[commit-bot: I haz the power, 2017-05-23 13:16:52]

Dry run: This issue passed the CQ dry run.

[Mike West, 2017-05-23 13:35:48]

LGTM. I'm happy with the way this turned out.

[alex clarke (OOO till 29th), 2017-05-24 07:57:49]

Thanks all.

https://codereview.chromium.org/2873283002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/bindings/DEPS (right):

https://codereview.chromium.org/2873283002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/bindings/DEPS:3: "+public/web"
On 2017/05/23 11:10:55, jochen wrote:
> that won't work. Maybe just move the Ids header to public/platform instead?

Done.

[alex clarke (OOO till 29th), 2017-05-24 07:57:56]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2017-05-24 07:57:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org, mkwst@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2873283002/#ps220001
(title: "Moved the header to public/platform as requested")

[commit-bot: I haz the power, 2017-05-24 07:58:35]

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, 2017-05-24 10:07:12]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1495612676170300,
"parent_rev": "69568ed51b77ef4a94d96f99134e09223acf6223", "commit_rev":
"07ee7c399295b210c5338dfeb543f2d30f748340"}

[commit-bot: I haz the power, 2017-05-24 10:07:27]

Description was changed from

==========
Allow headless TabSocket in isolated worlds & remove obsolete logic

This patch only affects C++ Embedders of headless_lib and should have
no effect on normal chrome even with the --headless flag.

Recently we decided to no longer allow arbitrary mojo bindings, instead
relying on the headless::TabSocket API for js<-->C++ communications. 

The public headless API for registering a mojo module has already been
removed and this patch finishes the job by removing the code from
MojoBindingsController which injected js mojo bindings for
BINDINGS_POLICY_HEADLESS.

In this patch we are replacing BINDINGS_POLICY_HEADLESS with
BINDINGS_POLICY_HEADLESS_MAIN_WORLD and
BINDINGS_POLICY_HEADLESS_ISOLATED_WORLD which provide access to the
headless::TabSocket API to either the main world or for isolated worlds
created by the Page.CreateIsolatedWorld DevTools command.

For a security point of view this patch hopefully reduces the attack
surface for the main world because js now only has access to the
headless::TabSocket API rather than arbitrary mojo modules.  Providing
the headless::TabSocket API into isolated worlds is new but hopefully
non-controversial. I note that ContentScripts have access to similar 
APIs for communication with the owning extension. I did investigate
reusing that code but it didn't seem feasible.

BUG=546953
==========

to

==========
Allow headless TabSocket in isolated worlds & remove obsolete logic

This patch only affects C++ Embedders of headless_lib and should have
no effect on normal chrome even with the --headless flag.

Recently we decided to no longer allow arbitrary mojo bindings, instead
relying on the headless::TabSocket API for js<-->C++ communications. 

The public headless API for registering a mojo module has already been
removed and this patch finishes the job by removing the code from
MojoBindingsController which injected js mojo bindings for
BINDINGS_POLICY_HEADLESS.

In this patch we are replacing BINDINGS_POLICY_HEADLESS with
BINDINGS_POLICY_HEADLESS_MAIN_WORLD and
BINDINGS_POLICY_HEADLESS_ISOLATED_WORLD which provide access to the
headless::TabSocket API to either the main world or for isolated worlds
created by the Page.CreateIsolatedWorld DevTools command.

For a security point of view this patch hopefully reduces the attack
surface for the main world because js now only has access to the
headless::TabSocket API rather than arbitrary mojo modules.  Providing
the headless::TabSocket API into isolated worlds is new but hopefully
non-controversial. I note that ContentScripts have access to similar 
APIs for communication with the owning extension. I did investigate
reusing that code but it didn't seem feasible.

BUG=546953

Review-Url: https://codereview.chromium.org/2873283002
Cr-Commit-Position: refs/heads/master@{#474238}
Committed:
https://chromium.googlesource.com/chromium/src/+/07ee7c399295b210c5338dfeb543...
==========

[commit-bot: I haz the power, 2017-05-24 10:07:28]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/07ee7c399295b210c5338dfeb543...

[vitaliii, 2017-05-24 10:44:04]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/2902953003/ by vitaliii@chromium.org.

The reason for reverting is: This CL breaks compile step. Message: 
FAILED: obj/headless/headless_browsertests/headless_web_contents_browsertest.obj

ninja -t msvc -e environment.x86 -- C:\b\c\goma_client/gomacc.exe
"C:\b\depot_tools\win_toolchain\vs_files\d3cb0e37bdd120ad0ac4650b674b09e81be45616\VC\bin\amd64_x86/cl.exe"
/nologo /showIncludes /FC
@obj/headless/headless_browsertests/headless_web_contents_browsertest.obj.rsp /c
../../headless/lib/headless_web_contents_browsertest.cc
/Foobj/headless/headless_browsertests/headless_web_contents_browsertest.obj
/Fd"obj/headless/headless_browsertests_cc.pdb"
c:\b\c\b\win_archive\src\headless\lib\headless_web_contents_browsertest.cc(385)
: error C2220: warning treated as error - no 'object' file generated
c:\b\c\b\win_archive\src\headless\lib\headless_web_contents_browsertest.cc(533)
: error C2220: warning treated as error - no 'object' file generated
c:\b\c\b\win_archive\src\headless\lib\headless_web_contents_browsertest.cc(533)
: warning C4702: unreachable code
c:\b\c\b\win_archive\src\headless\lib\headless_web_contents_browsertest.cc(385)
: warning C4702: unreachable code.

[alex clarke (OOO till 29th), 2017-05-24 10:54:00]

Description was changed from

==========
Allow headless TabSocket in isolated worlds & remove obsolete logic

This patch only affects C++ Embedders of headless_lib and should have
no effect on normal chrome even with the --headless flag.

Recently we decided to no longer allow arbitrary mojo bindings, instead
relying on the headless::TabSocket API for js<-->C++ communications. 

The public headless API for registering a mojo module has already been
removed and this patch finishes the job by removing the code from
MojoBindingsController which injected js mojo bindings for
BINDINGS_POLICY_HEADLESS.

In this patch we are replacing BINDINGS_POLICY_HEADLESS with
BINDINGS_POLICY_HEADLESS_MAIN_WORLD and
BINDINGS_POLICY_HEADLESS_ISOLATED_WORLD which provide access to the
headless::TabSocket API to either the main world or for isolated worlds
created by the Page.CreateIsolatedWorld DevTools command.

For a security point of view this patch hopefully reduces the attack
surface for the main world because js now only has access to the
headless::TabSocket API rather than arbitrary mojo modules.  Providing
the headless::TabSocket API into isolated worlds is new but hopefully
non-controversial. I note that ContentScripts have access to similar 
APIs for communication with the owning extension. I did investigate
reusing that code but it didn't seem feasible.

BUG=546953

Review-Url: https://codereview.chromium.org/2873283002
Cr-Commit-Position: refs/heads/master@{#474238}
Committed:
https://chromium.googlesource.com/chromium/src/+/07ee7c399295b210c5338dfeb543...
==========

to

==========
Allow headless TabSocket in isolated worlds & remove obsolete logic

This patch only affects C++ Embedders of headless_lib and should have
no effect on normal chrome even with the --headless flag.

Recently we decided to no longer allow arbitrary mojo bindings, instead
relying on the headless::TabSocket API for js<-->C++ communications. 

The public headless API for registering a mojo module has already been
removed and this patch finishes the job by removing the code from
MojoBindingsController which injected js mojo bindings for
BINDINGS_POLICY_HEADLESS.

In this patch we are replacing BINDINGS_POLICY_HEADLESS with
BINDINGS_POLICY_HEADLESS_MAIN_WORLD and
BINDINGS_POLICY_HEADLESS_ISOLATED_WORLD which provide access to the
headless::TabSocket API to either the main world or for isolated worlds
created by the Page.CreateIsolatedWorld DevTools command.

For a security point of view this patch hopefully reduces the attack
surface for the main world because js now only has access to the
headless::TabSocket API rather than arbitrary mojo modules.  Providing
the headless::TabSocket API into isolated worlds is new but hopefully
non-controversial. I note that ContentScripts have access to similar 
APIs for communication with the owning extension. I did investigate
reusing that code but it didn't seem feasible.

BUG=546953

Review-Url: https://codereview.chromium.org/2873283002
Cr-Commit-Position: refs/heads/master@{#474238}
Committed:
https://chromium.googlesource.com/chromium/src/+/07ee7c399295b210c5338dfeb543...
==========

[alex clarke (OOO till 29th), 2017-05-24 10:54:23]

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

[commit-bot: I haz the power, 2017-05-24 10:54:31]

Dry run: CQ is trying da patch.

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

[alex clarke (OOO till 29th), 2017-05-24 10:58:58]

The CQ bit was unchecked by alexclarke@chromium.org

[findit-for-me, 2017-05-24 11:14:08]

Findit (https://goo.gl/kROfz5) confirmed this CL at revision 474238 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[alex clarke (OOO till 29th), 2017-05-24 12:18:51]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2017-05-24 12:18:52]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
jochen@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2873283002/#ps240001
(title: "Fix MSVC nonsense")

[commit-bot: I haz the power, 2017-05-24 12:19:19]

CQ is trying da patch.

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

[alex clarke (OOO till 29th), 2017-05-24 14:14:36]

The CQ bit was unchecked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2017-05-24 14:18:58]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2017-05-24 14:18:58]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
jochen@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2873283002/#ps260001
(title: "Fix GN Issue")

[commit-bot: I haz the power, 2017-05-24 14:19:21]

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, 2017-05-24 16:07:49]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1495635538561410,
"parent_rev": "843aa52b01713a6605988ab3ee104b43d9087cd7", "commit_rev":
"1a4a3bab489e96546385236195be3d9b449a10e0"}

[commit-bot: I haz the power, 2017-05-24 16:08:05]

Description was changed from

==========
Allow headless TabSocket in isolated worlds & remove obsolete logic

This patch only affects C++ Embedders of headless_lib and should have
no effect on normal chrome even with the --headless flag.

Recently we decided to no longer allow arbitrary mojo bindings, instead
relying on the headless::TabSocket API for js<-->C++ communications. 

The public headless API for registering a mojo module has already been
removed and this patch finishes the job by removing the code from
MojoBindingsController which injected js mojo bindings for
BINDINGS_POLICY_HEADLESS.

In this patch we are replacing BINDINGS_POLICY_HEADLESS with
BINDINGS_POLICY_HEADLESS_MAIN_WORLD and
BINDINGS_POLICY_HEADLESS_ISOLATED_WORLD which provide access to the
headless::TabSocket API to either the main world or for isolated worlds
created by the Page.CreateIsolatedWorld DevTools command.

For a security point of view this patch hopefully reduces the attack
surface for the main world because js now only has access to the
headless::TabSocket API rather than arbitrary mojo modules.  Providing
the headless::TabSocket API into isolated worlds is new but hopefully
non-controversial. I note that ContentScripts have access to similar 
APIs for communication with the owning extension. I did investigate
reusing that code but it didn't seem feasible.

BUG=546953

Review-Url: https://codereview.chromium.org/2873283002
Cr-Commit-Position: refs/heads/master@{#474238}
Committed:
https://chromium.googlesource.com/chromium/src/+/07ee7c399295b210c5338dfeb543...
==========

to

==========
Allow headless TabSocket in isolated worlds & remove obsolete logic

This patch only affects C++ Embedders of headless_lib and should have
no effect on normal chrome even with the --headless flag.

Recently we decided to no longer allow arbitrary mojo bindings, instead
relying on the headless::TabSocket API for js<-->C++ communications. 

The public headless API for registering a mojo module has already been
removed and this patch finishes the job by removing the code from
MojoBindingsController which injected js mojo bindings for
BINDINGS_POLICY_HEADLESS.

In this patch we are replacing BINDINGS_POLICY_HEADLESS with
BINDINGS_POLICY_HEADLESS_MAIN_WORLD and
BINDINGS_POLICY_HEADLESS_ISOLATED_WORLD which provide access to the
headless::TabSocket API to either the main world or for isolated worlds
created by the Page.CreateIsolatedWorld DevTools command.

For a security point of view this patch hopefully reduces the attack
surface for the main world because js now only has access to the
headless::TabSocket API rather than arbitrary mojo modules.  Providing
the headless::TabSocket API into isolated worlds is new but hopefully
non-controversial. I note that ContentScripts have access to similar 
APIs for communication with the owning extension. I did investigate
reusing that code but it didn't seem feasible.

BUG=546953

Review-Url: https://codereview.chromium.org/2873283002
Cr-Original-Commit-Position: refs/heads/master@{#474238}
Committed:
https://chromium.googlesource.com/chromium/src/+/07ee7c399295b210c5338dfeb543...
Review-Url: https://codereview.chromium.org/2873283002
Cr-Commit-Position: refs/heads/master@{#474311}
Committed:
https://chromium.googlesource.com/chromium/src/+/1a4a3bab489e96546385236195be...
==========

[commit-bot: I haz the power, 2017-05-24 16:08:06]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/1a4a3bab489e96546385236195be...