Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2049363003:
Adds support for headless chrome embedder mojo services (Closed)
|
Created:
4 years, 6 months ago by alex clarke (OOO till 29th) Modified:
4 years, 4 months ago Reviewers:
jochen (gone - plz use gerrit), Sami, altimin, jam, dcheng, Sam McNally, Mike West, Eric Seckler, Tom Sepez, yzshen1, Charlie Reis CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), Łukasz Anforowicz, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more embedder
provided mojo services and request js bindings.
We add a BINDINGS_POLICY_HEADLESS which instructs
MojoContextState to use the new headless-mojom:// protocol to
fetch the mojo bindings.
Design doc:
https://docs.google.com/document/d/1Fr6_DJH6OK9rG3-ibMvRPTNnHsAXPk0VzxxiuJDSK3M/edit
More context is available from our BlinkOn presentation
slides: https://docs.google.com/presentation/d/1gqK9F4lGAY3TZudAtdcxzMQNEE7PcuQrGu83No3l0lw/edit#slide=id.g14ebf0ab58_0_0
recording: https://youtu.be/zlNgsoPV3ho?t=6m55s
BUG=546953, 623954
Committed: https://crrev.com/a50533eeece4f0e34c4deb6c4539772a0269d63f
Cr-Commit-Position: refs/heads/master@{#408617}
Patch Set 1 #Patch Set 2 : Fix indent #Patch Set 3 : Added a comment #
Total comments: 18
Patch Set 4 : Big refactor to the way mojoms are loaded. Decided to use the layout test protocol handler #Patch Set 5 : Remove a few includes and simplify some code #Patch Set 6 : Added a test with the mojo js inside a <head> tag #Patch Set 7 : Went back to browser pushing bindings due to security concerns with the pull approach. #Patch Set 8 : Added a missing file #Patch Set 9 : Fix a bug with mojo bindings after the onload event fired. #
Total comments: 8
Patch Set 10 : Addressing review comments. #Patch Set 11 : Refactored the JS for clarity #
Total comments: 40
Patch Set 12 : Changes for Sami #Patch Set 13 : Adding mojo OWNERS change plus throw an error if user tries to assign to the mojo services proxy cl… #Patch Set 14 : Seems we need a new OWNERS file #
Total comments: 2
Patch Set 15 : Added a TODO #
Total comments: 4
Patch Set 16 : Remove string litteral :( #
Total comments: 12
Patch Set 17 : Made mojo bindings get reinstalled after navigation. #Patch Set 18 : Fix typo #
Total comments: 4
Patch Set 19 : Fix MojoBindingsReinstalledAfterNavigation to use a browser initiated cross-origin navigation. #
Total comments: 2
Patch Set 20 : Fix typo #
Total comments: 2
Patch Set 21 : Abandon (for now) the concice bindings. Add BINDINGS_POLICY_HEADLESS and headless-mojo:// #
Total comments: 14
Patch Set 22 : Changes for Sami #
Total comments: 6
Patch Set 23 : Comment plus a rename #
Total comments: 1
Patch Set 24 : Rebase #Patch Set 25 : Fix headless_shell compile #Patch Set 26 : Add a setting to try and prevent unintended exposure of mojo to webcontent #Patch Set 27 : Add a browser test to lock in the default behavior of disabling http when mojo bindings added to a … #Patch Set 28 : Remove stray ; #Patch Set 29 : Rebase #Messages
Total messages: 190 (86 generated)
Description was changed from ========== Adds support for headless chrome embedder mojo services You can now request a HeadlessWebContents be created with one or more embedder provided mojo services. This will allow JS binding to embedder C++. BUG=546953 ========== to ========== Adds support for headless chrome embedder mojo services You can now request a HeadlessWebContents be created with one or more embedder provided mojo services. This will allow JS binding to embedder C++. BUG=546953 ==========
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/1
alexclarke@chromium.org changed reviewers: + altimin@chromium.org, eseckler@chromium.org, skyostil@chromium.org
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/20001
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks great -- various comments. https://codereview.chromium.org/2049363003/diff/40001/headless/lib/browser/he... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_web_contents_impl.cc:134: void HeadlessWebContentsImpl::DocumentOnLoadCompletedInMainFrame() { Is there an earlier event we could use? It might be surprising if I create a frame pointing at some JS which excepts to use the mojo bindings, but they won't be there yet. https://codereview.chromium.org/2049363003/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_web_contents_impl.cc:143: render_frame_host->ExecuteJavaScriptForTests(base::UTF8ToUTF16(R"( Was there a non-test interface we could use for this? https://codereview.chromium.org/2049363003/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_web_contents_impl.cc:155: return result; Do you need both resolve and return? https://codereview.chromium.org/2049363003/diff/40001/headless/lib/browser/he... File headless/lib/browser/headless_web_contents_impl.h (right): https://codereview.chromium.org/2049363003/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_web_contents_impl.h:78: std::list<EmbedderMojoService>& embedder_mojo_services); const or turn into a pointer if it needs to be mutable? edit: looks like you're moving this so it could be just a plain value. https://codereview.chromium.org/2049363003/diff/40001/headless/lib/embedder_m... File headless/lib/embedder_mojo_browsertest.cc (right): https://codereview.chromium.org/2049363003/diff/40001/headless/lib/embedder_m... headless/lib/embedder_mojo_browsertest.cc:31: #if defined(OS_MACOSX) Let's leave this out until we actually support mac since we can't really test that its working at the moment. https://codereview.chromium.org/2049363003/diff/40001/headless/lib/embedder_m... headless/lib/embedder_mojo_browsertest.cc:48: class EmbedderMojoTest : public HeadlessBrowserTest, Would it simplify things to subclass HeadlessAsyncDevTooledBrowserTest which sets up devtools? https://codereview.chromium.org/2049363003/diff/40001/headless/lib/embedder_m... headless/lib/embedder_mojo_browsertest.cc:60: void DevToolsTargetReady() override { nit: Could you move these into chronological order to make it a bit easier to see the flow? https://codereview.chromium.org/2049363003/diff/40001/headless/public/headles... File headless/public/headless_web_contents.h (right): https://codereview.chromium.org/2049363003/diff/40001/headless/public/headles... headless/public/headless_web_contents.h:88: // Specify an embedder provided Mojo service to be installed. Please document the arguments. https://codereview.chromium.org/2049363003/diff/40001/headless/public/headles... headless/public/headless_web_contents.h:134: DISALLOW_COPY_AND_ASSIGN(EmbedderMojoService); private:
One more question I had: are the services bound to a specific WebContents on the renderer side? Or a just the top-level frame? In any case there shouldn't be any possibility of leaking the service to a WebContents you didn't mean, right?
PTAL https://codereview.chromium.org/2049363003/diff/40001/headless/lib/browser/he... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_web_contents_impl.cc:134: void HeadlessWebContentsImpl::DocumentOnLoadCompletedInMainFrame() { On 2016/06/10 10:27:38, Sami wrote: > Is there an earlier event we could use? It might be surprising if I create a > frame pointing at some JS which excepts to use the mojo bindings, but they won't > be there yet. I've refactored this to: 1. Install the mojo.define shim at the earliest possible override HeadlessContentRendererClient::RunScriptsAtDocumentStart. 2. Use the layout-test-mojom// protocol handler. I realized that although we want each WebContents to have it's own mojo instance, the generated JS bindings are made at compile time and hence the protocol is fine actually. Besides it simplifies the browser test considerably. https://codereview.chromium.org/2049363003/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_web_contents_impl.cc:143: render_frame_host->ExecuteJavaScriptForTests(base::UTF8ToUTF16(R"( On 2016/06/10 10:27:38, Sami wrote: > Was there a non-test interface we could use for this? Yes. this has been moved. https://codereview.chromium.org/2049363003/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_web_contents_impl.cc:155: return result; On 2016/06/10 10:27:38, Sami wrote: > Do you need both resolve and return? Apparently the resolve isn't needed?! https://codereview.chromium.org/2049363003/diff/40001/headless/lib/browser/he... File headless/lib/browser/headless_web_contents_impl.h (right): https://codereview.chromium.org/2049363003/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_web_contents_impl.h:78: std::list<EmbedderMojoService>& embedder_mojo_services); On 2016/06/10 10:27:38, Sami wrote: > const or turn into a pointer if it needs to be mutable? > > edit: looks like you're moving this so it could be just a plain value. Done. https://codereview.chromium.org/2049363003/diff/40001/headless/lib/embedder_m... File headless/lib/embedder_mojo_browsertest.cc (right): https://codereview.chromium.org/2049363003/diff/40001/headless/lib/embedder_m... headless/lib/embedder_mojo_browsertest.cc:31: #if defined(OS_MACOSX) On 2016/06/10 10:27:39, Sami wrote: > Let's leave this out until we actually support mac since we can't really test > that its working at the moment. Done. https://codereview.chromium.org/2049363003/diff/40001/headless/lib/embedder_m... headless/lib/embedder_mojo_browsertest.cc:48: class EmbedderMojoTest : public HeadlessBrowserTest, On 2016/06/10 10:27:39, Sami wrote: > Would it simplify things to subclass HeadlessAsyncDevTooledBrowserTest which > sets up devtools? This has got a lot simpler and I'm not sure that's worth doing now. WDYT? https://codereview.chromium.org/2049363003/diff/40001/headless/lib/embedder_m... headless/lib/embedder_mojo_browsertest.cc:60: void DevToolsTargetReady() override { On 2016/06/10 10:27:38, Sami wrote: > nit: Could you move these into chronological order to make it a bit easier to > see the flow? Most of these methods have gone. https://codereview.chromium.org/2049363003/diff/40001/headless/public/headles... File headless/public/headless_web_contents.h (right): https://codereview.chromium.org/2049363003/diff/40001/headless/public/headles... headless/public/headless_web_contents.h:88: // Specify an embedder provided Mojo service to be installed. On 2016/06/10 10:27:39, Sami wrote: > Please document the arguments. Done. https://codereview.chromium.org/2049363003/diff/40001/headless/public/headles... headless/public/headless_web_contents.h:134: DISALLOW_COPY_AND_ASSIGN(EmbedderMojoService); On 2016/06/10 10:27:39, Sami wrote: > private: Done.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/60001
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/80001
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sami here's a patchset with the fancy ES6 proxy magic we discussed. It certainly makes the convenience bindings look really nice.
Description was changed from
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more
embedder provided mojo services.
This will allow JS binding to embedder C++.
BUG=546953
==========
to
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more
embedder provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test
style pull,
because we don't want the renderer to be able to load arbitrary files and to
allow better
isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a
promise to implement our convenience bindings. You can do something
like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
BUG=546953
==========
Description was changed from
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more
embedder provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test
style pull,
because we don't want the renderer to be able to load arbitrary files and to
allow better
isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a
promise to implement our convenience bindings. You can do something
like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
BUG=546953
==========
to
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more
embedder provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test style
pull,
because we don't want the renderer to be able to load arbitrary files and to
allow
better isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a
promise to implement our convenience bindings. You can do something
like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
BUG=546953
==========
Description was changed from
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more
embedder provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test style
pull,
because we don't want the renderer to be able to load arbitrary files and to
allow
better isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a
promise to implement our convenience bindings. You can do something
like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
BUG=546953
==========
to
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more embedder
provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test style
pull,
because we don't want the renderer to be able to load arbitrary files and to
allow
better isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a promise to
implement our convenience bindings. This works around a whole
host of otherwise gnarly issues to do with getting data from the browser to the
renderer at the right time.
You can do something
like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
BUG=546953
==========
Description was changed from
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more embedder
provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test style
pull,
because we don't want the renderer to be able to load arbitrary files and to
allow
better isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a promise to
implement our convenience bindings. This works around a whole
host of otherwise gnarly issues to do with getting data from the browser to the
renderer at the right time.
You can do something
like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
BUG=546953
==========
to
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more embedder
provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test style
pull,
because we don't want the renderer to be able to load arbitrary files and to
allow
better isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a promise to
implement our convenience bindings. This works around a whole
host of otherwise gnarly issues to do with getting data from the browser to the
renderer
at the right time. You can now do something like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
BUG=546953
==========
Description was changed from
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more embedder
provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test style
pull,
because we don't want the renderer to be able to load arbitrary files and to
allow
better isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a promise to
implement our convenience bindings. This works around a whole
host of otherwise gnarly issues to do with getting data from the browser to the
renderer
at the right time. You can now do something like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
BUG=546953
==========
to
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more embedder
provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test style
pull,
because we don't want the renderer to be able to load arbitrary files and to
allow
better isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a promise to
implement our convenience bindings. This works around a whole
host of otherwise gnarly issues to do with getting data from the browser to the
renderer at the right time. You can now do something like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
BUG=546953
==========
alexclarke@chromium.org changed reviewers: + yzshen@chromium.org
+yzshen for mojo DEPS
Description was changed from
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more embedder
provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test style
pull,
because we don't want the renderer to be able to load arbitrary files and to
allow
better isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a promise to
implement our convenience bindings. This works around a whole
host of otherwise gnarly issues to do with getting data from the browser to the
renderer at the right time. You can now do something like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
BUG=546953
==========
to
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more embedder
provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test style
pull,
because we don't want the renderer to be able to load arbitrary files and to
allow
better isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a promise to
implement our convenience bindings. This works around a whole
host of otherwise gnarly issues to do with getting data from the browser to the
renderer at the right time. You can now do something like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
Design doc:
https://docs.google.com/document/d/1Fr6_DJH6OK9rG3-ibMvRPTNnHsAXPk0VzxxiuJDSK...
BUG=546953
==========
Description was changed from
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more embedder
provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test style
pull,
because we don't want the renderer to be able to load arbitrary files and to
allow
better isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a promise to
implement our convenience bindings. This works around a whole
host of otherwise gnarly issues to do with getting data from the browser to the
renderer at the right time. You can now do something like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
Design doc:
https://docs.google.com/document/d/1Fr6_DJH6OK9rG3-ibMvRPTNnHsAXPk0VzxxiuJDSK...
BUG=546953
==========
to
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more embedder
provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test style
pull,
because we don't want the renderer to be able to load arbitrary files and to
allow
better isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a promise to
implement our convenience bindings. This works around a whole
host of otherwise gnarly issues to do with getting data from the browser to the
renderer at the right time. You can now do something like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
Design doc:
https://docs.google.com/document/d/1Fr6_DJH6OK9rG3-ibMvRPTNnHsAXPk0VzxxiuJDSK...
BUG=546953
==========
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2049363003/diff/180001/headless/DEPS File headless/DEPS (right): https://codereview.chromium.org/2049363003/diff/180001/headless/DEPS#newcode4 headless/DEPS:4: "+mojo", Is it sufficient to use only +mojo/public?
Had a quick look at the JS parts, looks like quite a fancy way of dealing with mojo services :) https://codereview.chromium.org/2049363003/diff/180001/headless/lib/renderer/... File headless/lib/renderer/headless_content_renderer_client.cc (right): https://codereview.chromium.org/2049363003/diff/180001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:80: window.mojo.resolvePromisesForService = function(serviceNames) { I'd suggest to rename this function to ..ForServices (plural) to signal that it's called once for all services rather than multiple times. https://codereview.chromium.org/2049363003/diff/180001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:92: // A mojom binding may contain bindings for a number of intefaces. typo (interfaces) https://codereview.chromium.org/2049363003/diff/180001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:107: mojoBindings.delete(service.name); I might be missing something, but if you delete the binding here, does that mean that any later use of the service (through e.g. mojo.services.embedder_test.TestEmbedderService.then()) will fail? It seems to me that you're supporting resolving-before-use in the if-branch directly above, how come we don't expect a later use here?
All comments addressed thanks. https://codereview.chromium.org/2049363003/diff/180001/headless/DEPS File headless/DEPS (right): https://codereview.chromium.org/2049363003/diff/180001/headless/DEPS#newcode4 headless/DEPS:4: "+mojo", On 2016/06/14 17:03:44, yzshen1 wrote: > Is it sufficient to use only +mojo/public? mojo/public is fine. Done. https://codereview.chromium.org/2049363003/diff/180001/headless/lib/renderer/... File headless/lib/renderer/headless_content_renderer_client.cc (right): https://codereview.chromium.org/2049363003/diff/180001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:80: window.mojo.resolvePromisesForService = function(serviceNames) { On 2016/06/14 17:50:48, Eric Seckler wrote: > I'd suggest to rename this function to ..ForServices (plural) to signal that > it's called once for all services rather than multiple times. Makes sense, done. https://codereview.chromium.org/2049363003/diff/180001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:92: // A mojom binding may contain bindings for a number of intefaces. On 2016/06/14 17:50:48, Eric Seckler wrote: > typo (interfaces) Done. Note to self install the spell checker on my editor! https://codereview.chromium.org/2049363003/diff/180001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:107: mojoBindings.delete(service.name); On 2016/06/14 17:50:48, Eric Seckler wrote: > I might be missing something, but if you delete the binding here, does that mean > that any later use of the service (through e.g. > mojo.services.embedder_test.TestEmbedderService.then()) will fail? It seems to > me that you're supporting resolving-before-use in the if-branch directly above, > how come we don't expect a later use here? Mmm yes this is a bad idea. What I really want to to (I think) is remove the reject attribute because I don't want the code below to try to reject it.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/200001
On 2016/06/14 19:06:05, commit-bot: I haz the power wrote: > Dry run: CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/2049363003/200001 mojo DEPS LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) ios-device-gn on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
Looks great. I really like how concise the new bindings code is. https://codereview.chromium.org/2049363003/diff/220001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2049363003/diff/220001/headless/BUILD.gn#newc... headless/BUILD.gn:29: "$root_gen_dir/headless/headless_browsertest_resources.pak", Could we avoid packing this in to the release pak file? Maybe another repack step that is only used by headless_browsertests? https://codereview.chromium.org/2049363003/diff/220001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:158: content::RenderFrameHost* render_frame_host = web_contents_->GetMainFrame(); Could you add a short comment saying why we need to do this at onload? (It was because the mojo transport won't be available until document creation, right?) https://codereview.chromium.org/2049363003/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:165: module_names = binding.mojom_name; We should probably use base::GetQuotedJSONString() for safety. Another option would be to construct a base::ListValue and turn that into a string. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:167: module_names = module_names + "', '" + binding.mojom_name; nit: base::JoinString might be a little neater. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:171: // Note this function lets us run JS at anytime. nit: as opposed to some other function? Maybe mention why we can't use the non-test one? https://codereview.chromium.org/2049363003/diff/220001/headless/lib/embedder_... File headless/lib/embedder_mojo_browsertest.cc (right): https://codereview.chromium.org/2049363003/diff/220001/headless/lib/embedder_... headless/lib/embedder_mojo_browsertest.cc:63: .as_string(); Idle thought: normal headless embedders probably wouldn't use resource bundles for anything, so there's no need for us to expose them in the API. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/embedder_... headless/lib/embedder_mojo_browsertest.cc:115: class RejectNonExistantBindingsTest : public EmbedderMojoTest { typo: Nonexistent (couple other instances too) https://codereview.chromium.org/2049363003/diff/220001/headless/lib/embedder_... headless/lib/embedder_mojo_browsertest.cc:147: requestAnimationFrame(function() { rAF can be triggered before the onload. Maybe schedule a rAF from onload instead? https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... File headless/lib/renderer/headless_content_renderer_client.cc (right): https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:31: return result; Did you mention this return could be removed? https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:48: // resolves to a promise to the corresponding mojo inteerface.. typo: interface. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:67: return false; Should we throw an Error here? https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:83: return false; Ditto. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:88: window.mojo.resolvePromisesForServices = function(serviceNames) { Should we call this resolvePromisesForServices_ since it's really an internal function? https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:103: for (var m in serviceMojom) { s/var/let/ Also maybe "for..of" would be more appropriate? https://codereview.chromium.org/2049363003/diff/220001/headless/public/headle... File headless/public/headless_web_contents.h (right): https://codereview.chromium.org/2049363003/diff/220001/headless/public/headle... headless/public/headless_web_contents.h:91: // assumed to be in their default location gen/path_to/filenme.mojom.js This comment about the JS bindings file is no longer accurate, right? https://codereview.chromium.org/2049363003/diff/220001/headless/public/headle... headless/public/headless_web_contents.h:107: Builder& AddJsMojoBindings(const std::string& mojom_name, Is |mojom_file| a file that is read off disk somewhere? It's a little hard to tell. If so, could we make it a base::FilePath to make it obvious? https://codereview.chromium.org/2049363003/diff/220001/headless/public/headle... headless/public/headless_web_contents.h:108: const std::string& js_bindings); Could you document |js_bindings| and where to get it from?
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/240001
PTAL https://codereview.chromium.org/2049363003/diff/220001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2049363003/diff/220001/headless/BUILD.gn#newc... headless/BUILD.gn:29: "$root_gen_dir/headless/headless_browsertest_resources.pak", On 2016/06/20 16:58:16, Sami wrote: > Could we avoid packing this in to the release pak file? Maybe another repack > step that is only used by headless_browsertests? Done. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:158: content::RenderFrameHost* render_frame_host = web_contents_->GetMainFrame(); On 2016/06/20 16:58:16, Sami wrote: > Could you add a short comment saying why we need to do this at onload? (It was > because the mojo transport won't be available until document creation, right?) Done. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:165: module_names = binding.mojom_name; On 2016/06/20 16:58:16, Sami wrote: > We should probably use base::GetQuotedJSONString() for safety. Why would that improve safety? These values come from browser process C++. Another option > would be to construct a base::ListValue and turn that into a string. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:167: module_names = module_names + "', '" + binding.mojom_name; On 2016/06/20 16:58:16, Sami wrote: > nit: base::JoinString might be a little neater. I ended up using json writer. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:171: // Note this function lets us run JS at anytime. On 2016/06/20 16:58:16, Sami wrote: > nit: as opposed to some other function? Maybe mention why we can't use the > non-test one? Done. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/embedder_... File headless/lib/embedder_mojo_browsertest.cc (right): https://codereview.chromium.org/2049363003/diff/220001/headless/lib/embedder_... headless/lib/embedder_mojo_browsertest.cc:63: .as_string(); On 2016/06/20 16:58:16, Sami wrote: > Idle thought: normal headless embedders probably wouldn't use resource bundles > for anything, so there's no need for us to expose them in the API. Right but the API doesn't mention them? :) https://codereview.chromium.org/2049363003/diff/220001/headless/lib/embedder_... headless/lib/embedder_mojo_browsertest.cc:115: class RejectNonExistantBindingsTest : public EmbedderMojoTest { On 2016/06/20 16:58:16, Sami wrote: > typo: Nonexistent (couple other instances too) Done. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/embedder_... headless/lib/embedder_mojo_browsertest.cc:147: requestAnimationFrame(function() { On 2016/06/20 16:58:16, Sami wrote: > rAF can be triggered before the onload. Maybe schedule a rAF from onload > instead? Done. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... File headless/lib/renderer/headless_content_renderer_client.cc (right): https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:31: return result; On 2016/06/20 16:58:16, Sami wrote: > Did you mention this return could be removed? We need the return but the resolve isn't needed. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:48: // resolves to a promise to the corresponding mojo inteerface.. On 2016/06/20 16:58:16, Sami wrote: > typo: interface. Done. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:48: // resolves to a promise to the corresponding mojo inteerface.. On 2016/06/20 16:58:16, Sami wrote: > typo: interface. Done. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:88: window.mojo.resolvePromisesForServices = function(serviceNames) { On 2016/06/20 16:58:16, Sami wrote: > Should we call this resolvePromisesForServices_ since it's really an internal > function? Done. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:103: for (var m in serviceMojom) { On 2016/06/20 16:58:16, Sami wrote: > s/var/let/ > > Also maybe "for..of" would be more appropriate? Done. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:103: for (var m in serviceMojom) { On 2016/06/20 16:58:16, Sami wrote: > s/var/let/ > > Also maybe "for..of" would be more appropriate? Done. https://codereview.chromium.org/2049363003/diff/220001/headless/public/headle... File headless/public/headless_web_contents.h (right): https://codereview.chromium.org/2049363003/diff/220001/headless/public/headle... headless/public/headless_web_contents.h:91: // assumed to be in their default location gen/path_to/filenme.mojom.js On 2016/06/20 16:58:17, Sami wrote: > This comment about the JS bindings file is no longer accurate, right? Done. https://codereview.chromium.org/2049363003/diff/220001/headless/public/headle... headless/public/headless_web_contents.h:107: Builder& AddJsMojoBindings(const std::string& mojom_name, On 2016/06/20 16:58:16, Sami wrote: > Is |mojom_file| a file that is read off disk somewhere? It's a little hard to > tell. If so, could we make it a base::FilePath to make it obvious? No it's not. https://codereview.chromium.org/2049363003/diff/220001/headless/public/headle... headless/public/headless_web_contents.h:108: const std::string& js_bindings); On 2016/06/20 16:58:16, Sami wrote: > Could you document |js_bindings| and where to get it from? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
alexclarke@chromium.org changed reviewers: + mkwst@chromium.org
+mkwst@ the presubmit says we need a security review for .mojom file changes. Could you please take a look?
https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... File headless/lib/renderer/headless_content_renderer_client.cc (right): https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:67: return false; On 2016/06/20 16:58:16, Sami wrote: > Should we throw an Error here? Done. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:83: return false; On 2016/06/20 16:58:16, Sami wrote: > Ditto. Done.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/280001
Thanks, lgtm. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/220001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:165: module_names = binding.mojom_name; On 2016/06/20 23:19:13, alex clarke wrote: > On 2016/06/20 16:58:16, Sami wrote: > > We should probably use base::GetQuotedJSONString() for safety. > Why would that improve safety? These values come from browser process C++. Sure, but they might be reading them from somewhere else and failing to verify. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/embedder_... File headless/lib/embedder_mojo_browsertest.cc (right): https://codereview.chromium.org/2049363003/diff/220001/headless/lib/embedder_... headless/lib/embedder_mojo_browsertest.cc:63: .as_string(); On 2016/06/20 23:19:13, alex clarke wrote: > On 2016/06/20 16:58:16, Sami wrote: > > Idle thought: normal headless embedders probably wouldn't use resource bundles > > for anything, so there's no need for us to expose them in the API. > > Right but the API doesn't mention them? :) Yep, and we should probably keep it that way. https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... File headless/lib/renderer/headless_content_renderer_client.cc (right): https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:103: for (var m in serviceMojom) { On 2016/06/20 23:19:13, alex clarke wrote: > On 2016/06/20 16:58:16, Sami wrote: > > s/var/let/ > > > > Also maybe "for..of" would be more appropriate? > > Done. (Did you mean to leave the "in" here? Sometimes it can give surprising results but maybe that's not an issue here) https://codereview.chromium.org/2049363003/diff/280001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/280001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:167: // ForTests version. TODO to fix this?
https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... File headless/lib/renderer/headless_content_renderer_client.cc (right): https://codereview.chromium.org/2049363003/diff/220001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:103: for (var m in serviceMojom) { On 2016/06/21 10:22:26, Sami wrote: > On 2016/06/20 23:19:13, alex clarke wrote: > > On 2016/06/20 16:58:16, Sami wrote: > > > s/var/let/ > > > > > > Also maybe "for..of" would be more appropriate? > > > > Done. > > (Did you mean to leave the "in" here? Sometimes it can give surprising results > but maybe that's not an issue here) for..of only works for iterables but serviceMojom is an object so we have to use for..in here. https://codereview.chromium.org/2049363003/diff/280001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/280001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:167: // ForTests version. On 2016/06/21 10:22:26, Sami wrote: > TODO to fix this? Done.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/300001
mkwst@chromium.org changed reviewers: + dcheng@chromium.org, tsepez@chromium.org
dcheng@/tsepez@ would be better reviewers for this than I am. The .mojom file looks fine to me, but I'm not familiar enough with the innards yet to give reasonable feedback on the integration.
Description was changed from
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more embedder
provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test style
pull,
because we don't want the renderer to be able to load arbitrary files and to
allow
better isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a promise to
implement our convenience bindings. This works around a whole
host of otherwise gnarly issues to do with getting data from the browser to the
renderer at the right time. You can now do something like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
Design doc:
https://docs.google.com/document/d/1Fr6_DJH6OK9rG3-ibMvRPTNnHsAXPk0VzxxiuJDSK...
BUG=546953
==========
to
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more embedder
provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test style
pull,
because we don't want the renderer to be able to load arbitrary files and to
allow
better isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a promise to
implement our convenience bindings. This works around a whole
host of otherwise gnarly issues to do with getting data from the browser to the
renderer at the right time. You can now do something like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
Design doc:
https://docs.google.com/document/d/1Fr6_DJH6OK9rG3-ibMvRPTNnHsAXPk0VzxxiuJDSK...
More context is available here: https://youtu.be/zlNgsoPV3ho?t=6m55s
BUG=546953
==========
Description was changed from
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more embedder
provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test style
pull,
because we don't want the renderer to be able to load arbitrary files and to
allow
better isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a promise to
implement our convenience bindings. This works around a whole
host of otherwise gnarly issues to do with getting data from the browser to the
renderer at the right time. You can now do something like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
Design doc:
https://docs.google.com/document/d/1Fr6_DJH6OK9rG3-ibMvRPTNnHsAXPk0VzxxiuJDSK...
More context is available here: https://youtu.be/zlNgsoPV3ho?t=6m55s
BUG=546953
==========
to
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more embedder
provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test style
pull,
because we don't want the renderer to be able to load arbitrary files and to
allow
better isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a promise to
implement our convenience bindings. This works around a whole
host of otherwise gnarly issues to do with getting data from the browser to the
renderer at the right time. You can now do something like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
Design doc:
https://docs.google.com/document/d/1Fr6_DJH6OK9rG3-ibMvRPTNnHsAXPk0VzxxiuJDSK...
More context is available from our blinkon presentation here:
https://youtu.be/zlNgsoPV3ho?t=6m55s
BUG=546953
==========
Description was changed from
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more embedder
provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test style
pull,
because we don't want the renderer to be able to load arbitrary files and to
allow
better isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a promise to
implement our convenience bindings. This works around a whole
host of otherwise gnarly issues to do with getting data from the browser to the
renderer at the right time. You can now do something like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
Design doc:
https://docs.google.com/document/d/1Fr6_DJH6OK9rG3-ibMvRPTNnHsAXPk0VzxxiuJDSK...
More context is available from our blinkon presentation here:
https://youtu.be/zlNgsoPV3ho?t=6m55s
BUG=546953
==========
to
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more embedder
provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test style
pull,
because we don't want the renderer to be able to load arbitrary files and to
allow
better isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a promise to
implement our convenience bindings. This works around a whole
host of otherwise gnarly issues to do with getting data from the browser to the
renderer at the right time. You can now do something like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
Design doc:
https://docs.google.com/document/d/1Fr6_DJH6OK9rG3-ibMvRPTNnHsAXPk0VzxxiuJDSK...
More context is available from our BlinkOn presentation
slides:
https://docs.google.com/presentation/d/1gqK9F4lGAY3TZudAtdcxzMQNEE7PcuQrGu83N...
recording : https://youtu.be/zlNgsoPV3ho?t=6m55s
BUG=546953
==========
Description was changed from
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more embedder
provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test style
pull,
because we don't want the renderer to be able to load arbitrary files and to
allow
better isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a promise to
implement our convenience bindings. This works around a whole
host of otherwise gnarly issues to do with getting data from the browser to the
renderer at the right time. You can now do something like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
Design doc:
https://docs.google.com/document/d/1Fr6_DJH6OK9rG3-ibMvRPTNnHsAXPk0VzxxiuJDSK...
More context is available from our BlinkOn presentation
slides:
https://docs.google.com/presentation/d/1gqK9F4lGAY3TZudAtdcxzMQNEE7PcuQrGu83N...
recording : https://youtu.be/zlNgsoPV3ho?t=6m55s
BUG=546953
==========
to
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more embedder
provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test style
pull,
because we don't want the renderer to be able to load arbitrary files and to
allow
better isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a promise to
implement our convenience bindings. This works around a whole
host of otherwise gnarly issues to do with getting data from the browser to the
renderer at the right time. You can now do something like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
Design doc:
https://docs.google.com/document/d/1Fr6_DJH6OK9rG3-ibMvRPTNnHsAXPk0VzxxiuJDSK...
More context is available from our BlinkOn presentation
slides:
https://docs.google.com/presentation/d/1gqK9F4lGAY3TZudAtdcxzMQNEE7PcuQrGu83N...
recording: https://youtu.be/zlNgsoPV3ho?t=6m55s
BUG=546953
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The change itself seems fine. Some high-level questions though: - Have you talked with creis or nasko about installing bindings like this? - Is this a test-only thing? It looks like it at the moment. - Will it stop being a test-only thing at some point? How will we audit what's exposed to the renderer in this way? https://codereview.chromium.org/2049363003/diff/300001/headless/lib/renderer/... File headless/lib/renderer/headless_content_renderer_client.cc (right): https://codereview.chromium.org/2049363003/diff/300001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:18: render_frame->ExecuteJavaScript(base::UTF8ToUTF16(R"( Raw string literals are still banned AFAIK. https://codereview.chromium.org/2049363003/diff/300001/headless/public/headle... File headless/public/headless_web_contents.h (left): https://codereview.chromium.org/2049363003/diff/300001/headless/public/headle... headless/public/headless_web_contents.h:8: #include "base/callback.h" Nit: should still include callback.h
skyostil@chromium.org changed reviewers: + creis@chromium.org
On 2016/06/21 14:52:10, dcheng wrote: > The change itself seems fine. Some high-level questions though: > > - Have you talked with creis or nasko about installing bindings like this? If my memory hasn't failed me (chat history certainly did), we talked about this with +creis@ some weeks ago and agreed this seems like a sensible direction to explore. > - Is this a test-only thing? It looks like it at the moment. This is only meant for users of the headless Chrome C++ library (e.g., folks writing test harnesses like PhantomJS). Normal Chrome will never have these kind of custom services exposed to web content. > - Will it stop being a test-only thing at some point? How will we audit what's > exposed to the renderer in this way? Not in regular Chrome, no. The embedders will define their own services, and auditing will be their responsibility. I think we'll want to publish guidelines about how to write secure services, but I'm not sure there's anything automated we could do.
https://codereview.chromium.org/2049363003/diff/300001/headless/lib/renderer/... File headless/lib/renderer/headless_content_renderer_client.cc (right): https://codereview.chromium.org/2049363003/diff/300001/headless/lib/renderer/... headless/lib/renderer/headless_content_renderer_client.cc:18: render_frame->ExecuteJavaScript(base::UTF8ToUTF16(R"( On 2016/06/21 14:52:10, dcheng wrote: > Raw string literals are still banned AFAIK. Ah so they are :/ I got confused because they are allowed in google internal code. Sadly this code is in the renderer, it would have been nicer to load this from a pak file given the way it looks now but that involves investing something new to get it here in time :( https://codereview.chromium.org/2049363003/diff/300001/headless/public/headle... File headless/public/headless_web_contents.h (left): https://codereview.chromium.org/2049363003/diff/300001/headless/public/headle... headless/public/headless_web_contents.h:8: #include "base/callback.h" On 2016/06/21 14:52:10, dcheng wrote: > Nit: should still include callback.h Done.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
On 2016/06/21 15:52:38, Sami wrote: > On 2016/06/21 14:52:10, dcheng wrote: > > The change itself seems fine. Some high-level questions though: > > > > - Have you talked with creis or nasko about installing bindings like this? > > If my memory hasn't failed me (chat history certainly did), we talked about this > with +creis@ some weeks ago and agreed this seems like a sensible direction to > explore. I'm having trouble remembering the conversation. (I blame jet lag after BlinkOn.) :) Can you remind me? Generally, it's not easy to get RVH bindings correct, and we have to be careful about both security issues and crashes from the security checks we have in place. We had lots of bugs when layout tests added Mojo bindings, for example. One example mentioned below. https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:141: content::BINDINGS_POLICY_MOJO); This seems like a problem. If you do a cross-process navigation in this WebContents, it looks like the new RenderViewHost won't get these bindings. Same if you go back after a cross-process navigation, in which case the NavEntry will remember that you had bindings before (and notice that you don't the second time) and cause a SetBindings crash.
https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:141: content::BINDINGS_POLICY_MOJO); On 2016/06/21 18:23:49, Charlie Reis wrote: > This seems like a problem. If you do a cross-process navigation in this > WebContents, it looks like the new RenderViewHost won't get these bindings. > Same if you go back after a cross-process navigation, in which case the NavEntry > will remember that you had bindings before (and notice that you don't the second > time) and cause a SetBindings crash. Is there a better way of installing them? It's hard to predict everything people will use this for, I hadn't even considered somebody might want to navigate a webcontents after the bindings have been set. Obviously that's possible so we need to fix it :)
creis@chromium.org changed reviewers: + sammc@chromium.org
https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:141: content::BINDINGS_POLICY_MOJO); On 2016/06/21 19:52:44, alex clarke wrote: > On 2016/06/21 18:23:49, Charlie Reis wrote: > > This seems like a problem. If you do a cross-process navigation in this > > WebContents, it looks like the new RenderViewHost won't get these bindings. > > Same if you go back after a cross-process navigation, in which case the > NavEntry > > will remember that you had bindings before (and notice that you don't the > second > > time) and cause a SetBindings crash. > > Is there a better way of installing them? > > It's hard to predict everything people will use this for, I hadn't even > considered somebody might want to navigate a webcontents after the bindings have > been set. Obviously that's possible so we need to fix it :) WebContentsObserver::RenderFrameCreated is a better way to watch for it. There's an example in BlinkTestController, which calls HandleNewRenderFrameHost to allow the bindings. (Side note: that class also listens to RenderFrameHostChanged, but I have a suspicion that's redundant with RenderFrameCreated.) While looking that up, I also remembered that RenderFrameImpl::MaybeEnableMojoBindings() is likely to cause problems for you. It makes the assumption that BIDNINGS_POLICY_MOJO is only used for layout tests and tells MojoBindingsController (which appears to affect MojoContextState). I brought that up as a concern when reviewing https://codereview.chromium.org/1707233003, but apparently it's still implemented that way. It looks like the code assumes that bindings for WebUI and the MOJO bindings (for layout tests) are mutually exclusive, but that's probably no longer true for your case. You might want to check with sammc about what the implications are there.
https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:141: content::BINDINGS_POLICY_MOJO); On 2016/06/21 20:05:48, Charlie Reis wrote: > On 2016/06/21 19:52:44, alex clarke wrote: > > On 2016/06/21 18:23:49, Charlie Reis wrote: > > > This seems like a problem. If you do a cross-process navigation in this > > > WebContents, it looks like the new RenderViewHost won't get these bindings. > > > Same if you go back after a cross-process navigation, in which case the > > NavEntry > > > will remember that you had bindings before (and notice that you don't the > > second > > > time) and cause a SetBindings crash. > > > > Is there a better way of installing them? > > > > It's hard to predict everything people will use this for, I hadn't even > > considered somebody might want to navigate a webcontents after the bindings > have > > been set. Obviously that's possible so we need to fix it :) > > WebContentsObserver::RenderFrameCreated is a better way to watch for it. > There's an example in BlinkTestController, which calls HandleNewRenderFrameHost > to allow the bindings. OK. I'll have a go at using that and write a browser test. > > (Side note: that class also listens to RenderFrameHostChanged, but I have a > suspicion that's redundant with RenderFrameCreated.) > > While looking that up, I also remembered that > RenderFrameImpl::MaybeEnableMojoBindings() is likely to cause problems for you. > It makes the assumption that BIDNINGS_POLICY_MOJO is only used for layout tests > and tells MojoBindingsController (which appears to affect MojoContextState). Yes I had to use the mojo.define => define shim which adds complexity. I > brought that up as a concern when reviewing > https://codereview.chromium.org/1707233003, but apparently it's still > implemented that way. It looks like the code assumes that bindings for WebUI > and the MOJO bindings (for layout tests) are mutually exclusive, but that's > probably no longer true for your case. > > You might want to check with sammc about what the implications are there. I did try implementing this in terms of WebUI bindings but that seems to have implications that seemed scary requiring privileged views. Also doing so exposes the UI mojo modules which isn't something we need and might cause problems (we wouldn't want somebody to rely on them if the API isn't stable).
PTAL https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:141: content::BINDINGS_POLICY_MOJO); On 2016/06/21 20:05:48, Charlie Reis wrote: > On 2016/06/21 19:52:44, alex clarke wrote: > > On 2016/06/21 18:23:49, Charlie Reis wrote: > > > This seems like a problem. If you do a cross-process navigation in this > > > WebContents, it looks like the new RenderViewHost won't get these bindings. > > > Same if you go back after a cross-process navigation, in which case the > > NavEntry > > > will remember that you had bindings before (and notice that you don't the > > second > > > time) and cause a SetBindings crash. > > > > Is there a better way of installing them? > > > > It's hard to predict everything people will use this for, I hadn't even > > considered somebody might want to navigate a webcontents after the bindings > have > > been set. Obviously that's possible so we need to fix it :) > > WebContentsObserver::RenderFrameCreated is a better way to watch for it. > There's an example in BlinkTestController, which calls HandleNewRenderFrameHost > to allow the bindings. > > (Side note: that class also listens to RenderFrameHostChanged, but I have a > suspicion that's redundant with RenderFrameCreated.) > > While looking that up, I also remembered that > RenderFrameImpl::MaybeEnableMojoBindings() is likely to cause problems for you. > It makes the assumption that BIDNINGS_POLICY_MOJO is only used for layout tests > and tells MojoBindingsController (which appears to affect MojoContextState). I > brought that up as a concern when reviewing > https://codereview.chromium.org/1707233003, but apparently it's still > implemented that way. It looks like the code assumes that bindings for WebUI > and the MOJO bindings (for layout tests) are mutually exclusive, but that's > probably no longer true for your case. > > You might want to check with sammc about what the implications are there. Done.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/340001
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/360001
Thanks for the quick change and extra test! I don't think the test is exercising the bug, though, and I'm still concerned about the RenderFrameImpl behavior. See below. https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:141: content::BINDINGS_POLICY_MOJO); On 2016/06/21 20:39:23, alex clarke wrote: > On 2016/06/21 20:05:48, Charlie Reis wrote: > > On 2016/06/21 19:52:44, alex clarke wrote: > > > On 2016/06/21 18:23:49, Charlie Reis wrote: > > > > This seems like a problem. If you do a cross-process navigation in this > > > > WebContents, it looks like the new RenderViewHost won't get these > bindings. > > > > Same if you go back after a cross-process navigation, in which case the > > > NavEntry > > > > will remember that you had bindings before (and notice that you don't the > > > second > > > > time) and cause a SetBindings crash. > > > > > > Is there a better way of installing them? > > > > > > It's hard to predict everything people will use this for, I hadn't even > > > considered somebody might want to navigate a webcontents after the bindings > > have > > > been set. Obviously that's possible so we need to fix it :) > > > > WebContentsObserver::RenderFrameCreated is a better way to watch for it. > > There's an example in BlinkTestController, which calls > HandleNewRenderFrameHost > > to allow the bindings. > OK. I'll have a go at using that and write a browser test. > > > > > (Side note: that class also listens to RenderFrameHostChanged, but I have a > > suspicion that's redundant with RenderFrameCreated.) > > > > While looking that up, I also remembered that > > RenderFrameImpl::MaybeEnableMojoBindings() is likely to cause problems for > you. > > It makes the assumption that BIDNINGS_POLICY_MOJO is only used for layout > tests > > and tells MojoBindingsController (which appears to affect MojoContextState). > Yes I had to use the mojo.define => define shim which adds complexity. > I don't know what this means. Won't you still hit RenderFrameImpl::MaybeEnableMojoBindings via RenderFrameImpl::Initialize, and thus tell MojoContextState that it's for_layout_tests, setting the module_prefix_ to layout-test-mojom://? I don't know a lot about Mojo, but that sounds problematic to me. sammc@, can you confirm? > I > > brought that up as a concern when reviewing > > https://codereview.chromium.org/1707233003, but apparently it's still > > implemented that way. It looks like the code assumes that bindings for WebUI > > and the MOJO bindings (for layout tests) are mutually exclusive, but that's > > probably no longer true for your case. > > > > You might want to check with sammc about what the implications are there. > > I did try implementing this in terms of WebUI bindings but that seems to have > implications that seemed scary requiring privileged views. Also doing so > exposes the UI mojo modules which isn't something we need and might cause > problems (we wouldn't want somebody to rely on them if the API isn't stable). Agreed, WebUI is definitely not a safe way to implement anything visiting real web pages. https://codereview.chromium.org/2049363003/diff/360001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/360001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:130: void HeadlessWebContentsImpl::RenderFrameCreated( Yes, RenderFrameCreated should do what you want for cross-process navigations. (We've also confirmed that the RenderFrameHostChanged use in the other code was unnecessary: https://codereview.chromium.org/2085023003/.) https://codereview.chromium.org/2049363003/diff/360001/headless/test/data/pag... File headless/test/data/page_one.html (right): https://codereview.chromium.org/2049363003/diff/360001/headless/test/data/pag... headless/test/data/page_one.html:7: window.location.pathname = "/page_two.html"; This probably won't exercise the bug, since it's not a cross-process navigation. (Did the test fail without the RenderFrameCreated fix?) Is there a way to make your test do a browser-initiated cross-site navigation instead? That would be cross-process. (There's also SetupCrossSiteRedirector which would let you link to /cross-site/foo.com/page_two.html, but that would be renderer-initiated and thus stay in the same process unless you run with --site-per-process.)
https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:141: content::BINDINGS_POLICY_MOJO); On 2016/06/21 23:08:31, Charlie Reis wrote: > On 2016/06/21 20:39:23, alex clarke wrote: > > On 2016/06/21 20:05:48, Charlie Reis wrote: > > > On 2016/06/21 19:52:44, alex clarke wrote: > > > > On 2016/06/21 18:23:49, Charlie Reis wrote: > > > > > This seems like a problem. If you do a cross-process navigation in this > > > > > WebContents, it looks like the new RenderViewHost won't get these > > bindings. > > > > > Same if you go back after a cross-process navigation, in which case the > > > > NavEntry > > > > > will remember that you had bindings before (and notice that you don't > the > > > > second > > > > > time) and cause a SetBindings crash. > > > > > > > > Is there a better way of installing them? > > > > > > > > It's hard to predict everything people will use this for, I hadn't even > > > > considered somebody might want to navigate a webcontents after the > bindings > > > have > > > > been set. Obviously that's possible so we need to fix it :) > > > > > > WebContentsObserver::RenderFrameCreated is a better way to watch for it. > > > There's an example in BlinkTestController, which calls > > HandleNewRenderFrameHost > > > to allow the bindings. > > OK. I'll have a go at using that and write a browser test. > > > > > > > > (Side note: that class also listens to RenderFrameHostChanged, but I have a > > > suspicion that's redundant with RenderFrameCreated.) > > > > > > While looking that up, I also remembered that > > > RenderFrameImpl::MaybeEnableMojoBindings() is likely to cause problems for > > you. > > > It makes the assumption that BIDNINGS_POLICY_MOJO is only used for layout > > tests > > > and tells MojoBindingsController (which appears to affect MojoContextState). > > > Yes I had to use the mojo.define => define shim which adds complexity. > > > > I don't know what this means. Won't you still hit > RenderFrameImpl::MaybeEnableMojoBindings via RenderFrameImpl::Initialize, and > thus tell MojoContextState that it's for_layout_tests, setting the > module_prefix_ to layout-test-mojom://? > > I don't know a lot about Mojo, but that sounds problematic to me. sammc@, can > you confirm? Yes, BINDINGS_POLICY_MOJO is only suitable for layout tests. Fixing/replacing the current JS mojo bindings has been on our todo list for a while but the current house of cards hasn't caught fire and fallen over yet so it's been fairly low priority so far. > > > > I > > > brought that up as a concern when reviewing > > > https://codereview.chromium.org/1707233003, but apparently it's still > > > implemented that way. It looks like the code assumes that bindings for > WebUI > > > and the MOJO bindings (for layout tests) are mutually exclusive, but that's > > > probably no longer true for your case. > > > > > > You might want to check with sammc about what the implications are there. > > > > I did try implementing this in terms of WebUI bindings but that seems to have > > implications that seemed scary requiring privileged views. Also doing so > > exposes the UI mojo modules which isn't something we need and might cause > > problems (we wouldn't want somebody to rely on them if the API isn't stable). > > Agreed, WebUI is definitely not a safe way to implement anything visiting real > web pages.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:141: content::BINDINGS_POLICY_MOJO); On 2016/06/21 23:08:31, Charlie Reis wrote: > On 2016/06/21 20:39:23, alex clarke wrote: > > On 2016/06/21 20:05:48, Charlie Reis wrote: > > > On 2016/06/21 19:52:44, alex clarke wrote: > > > > On 2016/06/21 18:23:49, Charlie Reis wrote: > > > > > This seems like a problem. If you do a cross-process navigation in this > > > > > WebContents, it looks like the new RenderViewHost won't get these > > bindings. > > > > > Same if you go back after a cross-process navigation, in which case the > > > > NavEntry > > > > > will remember that you had bindings before (and notice that you don't > the > > > > second > > > > > time) and cause a SetBindings crash. > > > > > > > > Is there a better way of installing them? > > > > > > > > It's hard to predict everything people will use this for, I hadn't even > > > > considered somebody might want to navigate a webcontents after the > bindings > > > have > > > > been set. Obviously that's possible so we need to fix it :) > > > > > > WebContentsObserver::RenderFrameCreated is a better way to watch for it. > > > There's an example in BlinkTestController, which calls > > HandleNewRenderFrameHost > > > to allow the bindings. > > OK. I'll have a go at using that and write a browser test. > > > > > > > > (Side note: that class also listens to RenderFrameHostChanged, but I have a > > > suspicion that's redundant with RenderFrameCreated.) > > > > > > While looking that up, I also remembered that > > > RenderFrameImpl::MaybeEnableMojoBindings() is likely to cause problems for > > you. > > > It makes the assumption that BIDNINGS_POLICY_MOJO is only used for layout > > tests > > > and tells MojoBindingsController (which appears to affect MojoContextState). > > > Yes I had to use the mojo.define => define shim which adds complexity. > > > > I don't know what this means. Won't you still hit > RenderFrameImpl::MaybeEnableMojoBindings via RenderFrameImpl::Initialize, and > thus tell MojoContextState that it's for_layout_tests, setting the > module_prefix_ to layout-test-mojom://? Right. That actually isn't a problem for us since we deliberately push the bindings from browser to renderer. I did try the pull method in an earlier patch set but we where worried about letting the renderer request potentially arbitrary files from disc. One consequence of BINDINGS_POLICY_MOJO is the define api is called mojo.define and we need a shim (copied from the layout test helpers) to map define() => mojo.define(). It would be nice if we didn't have to do that. > > I don't know a lot about Mojo, but that sounds problematic to me. sammc@, can > you confirm? > > > > I > > > brought that up as a concern when reviewing > > > https://codereview.chromium.org/1707233003, but apparently it's still > > > implemented that way. It looks like the code assumes that bindings for > WebUI > > > and the MOJO bindings (for layout tests) are mutually exclusive, but that's > > > probably no longer true for your case. > > > > > > You might want to check with sammc about what the implications are there. > > > > I did try implementing this in terms of WebUI bindings but that seems to have > > implications that seemed scary requiring privileged views. Also doing so > > exposes the UI mojo modules which isn't something we need and might cause > > problems (we wouldn't want somebody to rely on them if the API isn't stable). > > Agreed, WebUI is definitely not a safe way to implement anything visiting real > web pages. https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:141: content::BINDINGS_POLICY_MOJO); On 2016/06/22 01:09:11, Sam McNally wrote: > On 2016/06/21 23:08:31, Charlie Reis wrote: > > On 2016/06/21 20:39:23, alex clarke wrote: > > > On 2016/06/21 20:05:48, Charlie Reis wrote: > > > > On 2016/06/21 19:52:44, alex clarke wrote: > > > > > On 2016/06/21 18:23:49, Charlie Reis wrote: > > > > > > This seems like a problem. If you do a cross-process navigation in > this > > > > > > WebContents, it looks like the new RenderViewHost won't get these > > > bindings. > > > > > > Same if you go back after a cross-process navigation, in which case > the > > > > > NavEntry > > > > > > will remember that you had bindings before (and notice that you don't > > the > > > > > second > > > > > > time) and cause a SetBindings crash. > > > > > > > > > > Is there a better way of installing them? > > > > > > > > > > It's hard to predict everything people will use this for, I hadn't even > > > > > considered somebody might want to navigate a webcontents after the > > bindings > > > > have > > > > > been set. Obviously that's possible so we need to fix it :) > > > > > > > > WebContentsObserver::RenderFrameCreated is a better way to watch for it. > > > > There's an example in BlinkTestController, which calls > > > HandleNewRenderFrameHost > > > > to allow the bindings. > > > OK. I'll have a go at using that and write a browser test. > > > > > > > > > > > (Side note: that class also listens to RenderFrameHostChanged, but I have > a > > > > suspicion that's redundant with RenderFrameCreated.) > > > > > > > > While looking that up, I also remembered that > > > > RenderFrameImpl::MaybeEnableMojoBindings() is likely to cause problems for > > > you. > > > > It makes the assumption that BIDNINGS_POLICY_MOJO is only used for layout > > > tests > > > > and tells MojoBindingsController (which appears to affect > MojoContextState). > > > > > Yes I had to use the mojo.define => define shim which adds complexity. > > > > > > > I don't know what this means. Won't you still hit > > RenderFrameImpl::MaybeEnableMojoBindings via RenderFrameImpl::Initialize, and > > thus tell MojoContextState that it's for_layout_tests, setting the > > module_prefix_ to layout-test-mojom://? > > > > I don't know a lot about Mojo, but that sounds problematic to me. sammc@, can > > you confirm? > > Yes, BINDINGS_POLICY_MOJO is only suitable for layout tests. Fixing/replacing > the current JS mojo bindings has been on our todo list for a while but the > current house of cards hasn't caught fire and fallen over yet so it's been > fairly low priority so far. Agreed that BINDINGS_POLICY_MOJO doesn't quite fit the needs of headless. What I (think) we need for headless is: * Browser pushes js bindings (we think that's safer) * The define() interface is added instead of mojo.define() * Don't need a privileged view What I think we should do is land this patch (that will unblock some fairly high priority internal work), and decide how best to support this new use case. Once that's sorted out I'll be happy to change headless to use the new way. I don't mind implementing the new way although I don't fully understand the other use cases. > > > > > > > I > > > > brought that up as a concern when reviewing > > > > https://codereview.chromium.org/1707233003, but apparently it's still > > > > implemented that way. It looks like the code assumes that bindings for > > WebUI > > > > and the MOJO bindings (for layout tests) are mutually exclusive, but > that's > > > > probably no longer true for your case. > > > > > > > > You might want to check with sammc about what the implications are there. > > > > > > I did try implementing this in terms of WebUI bindings but that seems to > have > > > implications that seemed scary requiring privileged views. Also doing so > > > exposes the UI mojo modules which isn't something we need and might cause > > > problems (we wouldn't want somebody to rely on them if the API isn't > stable). > > > > Agreed, WebUI is definitely not a safe way to implement anything visiting real > > web pages. > https://codereview.chromium.org/2049363003/diff/360001/headless/test/data/pag... File headless/test/data/page_one.html (right): https://codereview.chromium.org/2049363003/diff/360001/headless/test/data/pag... headless/test/data/page_one.html:7: window.location.pathname = "/page_two.html"; On 2016/06/21 23:08:32, Charlie Reis wrote: > This probably won't exercise the bug, since it's not a cross-process navigation. > (Did the test fail without the RenderFrameCreated fix?) No it didn't :) I've copied a trick used in one of our other browser tests to fake a different TLD and I instrumented the code and can see that RenderFrameCreated is called twice and the bindings are installed twice. > > Is there a way to make your test do a browser-initiated cross-site navigation > instead? That would be cross-process. > > (There's also SetupCrossSiteRedirector which would let you link to > /cross-site/foo.com/page_two.html, but that would be renderer-initiated and thus > stay in the same process unless you run with --site-per-process.)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/380001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:141: content::BINDINGS_POLICY_MOJO); On 2016/06/22 10:04:00, alex clarke wrote: > On 2016/06/22 01:09:11, Sam McNally wrote: > > On 2016/06/21 23:08:31, Charlie Reis wrote: > > > On 2016/06/21 20:39:23, alex clarke wrote: > > > > On 2016/06/21 20:05:48, Charlie Reis wrote: > > > > > On 2016/06/21 19:52:44, alex clarke wrote: > > > > > > On 2016/06/21 18:23:49, Charlie Reis wrote: > > > > > > > This seems like a problem. If you do a cross-process navigation in > > this > > > > > > > WebContents, it looks like the new RenderViewHost won't get these > > > > bindings. > > > > > > > Same if you go back after a cross-process navigation, in which case > > the > > > > > > NavEntry > > > > > > > will remember that you had bindings before (and notice that you > don't > > > the > > > > > > second > > > > > > > time) and cause a SetBindings crash. > > > > > > > > > > > > Is there a better way of installing them? > > > > > > > > > > > > It's hard to predict everything people will use this for, I hadn't > even > > > > > > considered somebody might want to navigate a webcontents after the > > > bindings > > > > > have > > > > > > been set. Obviously that's possible so we need to fix it :) > > > > > > > > > > WebContentsObserver::RenderFrameCreated is a better way to watch for it. > > > > > > There's an example in BlinkTestController, which calls > > > > HandleNewRenderFrameHost > > > > > to allow the bindings. > > > > OK. I'll have a go at using that and write a browser test. > > > > > > > > > > > > > > (Side note: that class also listens to RenderFrameHostChanged, but I > have > > a > > > > > suspicion that's redundant with RenderFrameCreated.) > > > > > > > > > > While looking that up, I also remembered that > > > > > RenderFrameImpl::MaybeEnableMojoBindings() is likely to cause problems > for > > > > you. > > > > > It makes the assumption that BIDNINGS_POLICY_MOJO is only used for > layout > > > > tests > > > > > and tells MojoBindingsController (which appears to affect > > MojoContextState). > > > > > > > Yes I had to use the mojo.define => define shim which adds complexity. > > > > > > > > > > I don't know what this means. Won't you still hit > > > RenderFrameImpl::MaybeEnableMojoBindings via RenderFrameImpl::Initialize, > and > > > thus tell MojoContextState that it's for_layout_tests, setting the > > > module_prefix_ to layout-test-mojom://? > Right. That actually isn't a problem for us since we deliberately push the > bindings from browser to renderer. I did try the pull method in an earlier > patch set but we where worried about letting the renderer request potentially > arbitrary files from disc. Sorry, I don't know what you're referring to with push vs pull. If RenderFrameImpl::MaybeEnableMojoBindings() thinks you have BINDING_POLICY_MOJO, then you're likely to have problems. Are you saying that RenderProcess::current()->GetEnabledBindings() won't return BINDINGS_POLICY_MOJO in your case, so MaybeEnableMojoBindings() won't create a MojoBindingsController? > > One consequence of BINDINGS_POLICY_MOJO is the define api is called mojo.define > and we need a shim (copied from the layout test helpers) to map define() => > mojo.define(). It would be nice if we didn't have to do that. > I'll have to defer the "define" stuff to others who know more about Mojo. > > > > > > I don't know a lot about Mojo, but that sounds problematic to me. sammc@, > can > > > you confirm? > > > > Yes, BINDINGS_POLICY_MOJO is only suitable for layout tests. Fixing/replacing > > the current JS mojo bindings has been on our todo list for a while but the > > current house of cards hasn't caught fire and fallen over yet so it's been > > fairly low priority so far. > > Agreed that BINDINGS_POLICY_MOJO doesn't quite fit the needs of headless. What > I (think) we need for headless is: > > * Browser pushes js bindings (we think that's safer) > * The define() interface is added instead of mojo.define() > * Don't need a privileged view > > What I think we should do is land this patch (that will unblock some fairly high > priority internal work), and decide how best to support this new use case. Once > that's sorted out I'll be happy to change headless to use the new way. I don't > mind implementing the new way although I don't fully understand the other use > cases. I don't see how this is safe to land yet, but maybe your answer to my MaybeEnableMojoBindings() question above will resolve it. (Happy to defer to sammc@ if he thinks it's safe.) > > > > > > > > > > > I > > > > > brought that up as a concern when reviewing > > > > > https://codereview.chromium.org/1707233003, but apparently it's still > > > > > implemented that way. It looks like the code assumes that bindings for > > > WebUI > > > > > and the MOJO bindings (for layout tests) are mutually exclusive, but > > that's > > > > > probably no longer true for your case. > > > > > > > > > > You might want to check with sammc about what the implications are > there. > > > > > > > > I did try implementing this in terms of WebUI bindings but that seems to > > have > > > > implications that seemed scary requiring privileged views. Also doing so > > > > exposes the UI mojo modules which isn't something we need and might cause > > > > problems (we wouldn't want somebody to rely on them if the API isn't > > stable). > > > > > > Agreed, WebUI is definitely not a safe way to implement anything visiting > real > > > web pages. > > > https://codereview.chromium.org/2049363003/diff/360001/headless/test/data/pag... File headless/test/data/page_one.html (right): https://codereview.chromium.org/2049363003/diff/360001/headless/test/data/pag... headless/test/data/page_one.html:7: window.location.pathname = "/page_two.html"; On 2016/06/22 10:04:00, alex clarke wrote: > On 2016/06/21 23:08:32, Charlie Reis wrote: > > This probably won't exercise the bug, since it's not a cross-process > navigation. > > (Did the test fail without the RenderFrameCreated fix?) > > No it didn't :) > > I've copied a trick used in one of our other browser tests to fake a different > TLD and I instrumented the code and can see that RenderFrameCreated is called > twice and the bindings are installed twice. > > > > > > Is there a way to make your test do a browser-initiated cross-site navigation > > instead? That would be cross-process. > > > > (There's also SetupCrossSiteRedirector which would let you link to > > /cross-site/foo.com/page_two.html, but that would be renderer-initiated and > thus > > stay in the same process unless you run with --site-per-process.) > Great-- that looks better. https://codereview.chromium.org/2049363003/diff/380001/headless/lib/embedder_... File headless/lib/embedder_mojo_browsertest.cc (right): https://codereview.chromium.org/2049363003/diff/380001/headless/lib/embedder_... headless/lib/embedder_mojo_browsertest.cc:286: // We want to make sure bindings work across browser initiaited cross-origin nit: initiated
https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:141: content::BINDINGS_POLICY_MOJO); On 2016/06/22 19:30:59, Charlie Reis wrote: > On 2016/06/22 10:04:00, alex clarke wrote: > > On 2016/06/22 01:09:11, Sam McNally wrote: > > > On 2016/06/21 23:08:31, Charlie Reis wrote: > > > > On 2016/06/21 20:39:23, alex clarke wrote: > > > > > On 2016/06/21 20:05:48, Charlie Reis wrote: > > > > > > On 2016/06/21 19:52:44, alex clarke wrote: > > > > > > > On 2016/06/21 18:23:49, Charlie Reis wrote: > > > > > > > > This seems like a problem. If you do a cross-process navigation > in > > > this > > > > > > > > WebContents, it looks like the new RenderViewHost won't get these > > > > > bindings. > > > > > > > > Same if you go back after a cross-process navigation, in which > case > > > the > > > > > > > NavEntry > > > > > > > > will remember that you had bindings before (and notice that you > > don't > > > > the > > > > > > > second > > > > > > > > time) and cause a SetBindings crash. > > > > > > > > > > > > > > Is there a better way of installing them? > > > > > > > > > > > > > > It's hard to predict everything people will use this for, I hadn't > > even > > > > > > > considered somebody might want to navigate a webcontents after the > > > > bindings > > > > > > have > > > > > > > been set. Obviously that's possible so we need to fix it :) > > > > > > > > > > > > WebContentsObserver::RenderFrameCreated is a better way to watch for > it. > > > > > > > > There's an example in BlinkTestController, which calls > > > > > HandleNewRenderFrameHost > > > > > > to allow the bindings. > > > > > OK. I'll have a go at using that and write a browser test. > > > > > > > > > > > > > > > > > (Side note: that class also listens to RenderFrameHostChanged, but I > > have > > > a > > > > > > suspicion that's redundant with RenderFrameCreated.) > > > > > > > > > > > > While looking that up, I also remembered that > > > > > > RenderFrameImpl::MaybeEnableMojoBindings() is likely to cause problems > > for > > > > > you. > > > > > > It makes the assumption that BIDNINGS_POLICY_MOJO is only used for > > layout > > > > > tests > > > > > > and tells MojoBindingsController (which appears to affect > > > MojoContextState). > > > > > > > > > Yes I had to use the mojo.define => define shim which adds complexity. > > > > > > > > > > > > > I don't know what this means. Won't you still hit > > > > RenderFrameImpl::MaybeEnableMojoBindings via RenderFrameImpl::Initialize, > > and > > > > thus tell MojoContextState that it's for_layout_tests, setting the > > > > module_prefix_ to layout-test-mojom://? > > > Right. That actually isn't a problem for us since we deliberately push the > > bindings from browser to renderer. I did try the pull method in an earlier > > patch set but we where worried about letting the renderer request potentially > > arbitrary files from disc. > > Sorry, I don't know what you're referring to with push vs pull. If > RenderFrameImpl::MaybeEnableMojoBindings() thinks you have BINDING_POLICY_MOJO, > then you're likely to have problems. > > Are you saying that RenderProcess::current()->GetEnabledBindings() won't return > BINDINGS_POLICY_MOJO in your case, so MaybeEnableMojoBindings() won't create a > MojoBindingsController? Happy to set up a VC if that would help. But I'll try and explain here :) There are two classes of js mojo bindings: 1. Special built in ones installed by RenderFrame::EnsureMojoBuiltinsAreAvailable. 2. General .mojom bindings. Normally are fetched on demand by MojoContextState::FetchModule and then the js is evaluated. I've been calling this the 'pull method'. Sami doesn't think #2 is safe in the context of headless, the problem being the renderer is asking to browser to load an arbitrary file. For layout tests this is loading is done by MojomProtocolHandler in layout_test_browser_context.cc, we'd probably need something similar for headless. For headless we know what modules the user wishes to install and can 'push' them to the renderer by using RenderFrameHost::ExecuteJavaScriptForTests to eval the binding js (see HeadlessWebContentsImpl::DocumentOnLoadCompletedInMainFrame below). Thus it's fine if MaybeEnableMojoBindings is called. We don't care if the module_prefix_ is layout-test-mojom:// the reason being since we supply the binding MojoContextState::FetchModule never gets called so the prefix isn't used. One problem with our approach is JS on the page might run before the browser has had a chance to push the bindings to the renderer. Our convenience bindings installed by HeadlessContentRendererClient::RunScriptsAtDocumentStart work around that with promises and ES6 proxy class trickery. HeadScriptEmbedderMojoBindingsTest is there to test that is working. There is one complication we have to work around with BINDING_POLICY_MOJO. The MojoContextState constructor creates a mojo.define gin binding instead of a define binding. This is a problem because the mojom js bindings assume that define() is used so we some js to implement define() in terms of mojo.define(). I'd like not to have to do that :) > > > > > One consequence of BINDINGS_POLICY_MOJO is the define api is called > mojo.define > > and we need a shim (copied from the layout test helpers) to map define() => > > mojo.define(). It would be nice if we didn't have to do that. > > > > I'll have to defer the "define" stuff to others who know more about Mojo. > > > > > > > > > > I don't know a lot about Mojo, but that sounds problematic to me. sammc@, > > can > > > > you confirm? > > > > > > Yes, BINDINGS_POLICY_MOJO is only suitable for layout tests. > Fixing/replacing > > > the current JS mojo bindings has been on our todo list for a while but the > > > current house of cards hasn't caught fire and fallen over yet so it's been > > > fairly low priority so far. > > > > Agreed that BINDINGS_POLICY_MOJO doesn't quite fit the needs of headless. > What > > I (think) we need for headless is: > > > > * Browser pushes js bindings (we think that's safer) > > * The define() interface is added instead of mojo.define() > > * Don't need a privileged view > > > > What I think we should do is land this patch (that will unblock some fairly > high > > priority internal work), and decide how best to support this new use case. > Once > > that's sorted out I'll be happy to change headless to use the new way. I > don't > > mind implementing the new way although I don't fully understand the other use > > cases. > > I don't see how this is safe to land yet, but maybe your answer to my > MaybeEnableMojoBindings() question above will resolve it. (Happy to defer to > sammc@ if he thinks it's safe.) > > > > > > > > > > > > > > > > > > I > > > > > > brought that up as a concern when reviewing > > > > > > https://codereview.chromium.org/1707233003, but apparently it's still > > > > > > implemented that way. It looks like the code assumes that bindings > for > > > > WebUI > > > > > > and the MOJO bindings (for layout tests) are mutually exclusive, but > > > that's > > > > > > probably no longer true for your case. > > > > > > > > > > > > You might want to check with sammc about what the implications are > > there. > > > > > > > > > > I did try implementing this in terms of WebUI bindings but that seems to > > > have > > > > > implications that seemed scary requiring privileged views. Also doing > so > > > > > exposes the UI mojo modules which isn't something we need and might > cause > > > > > problems (we wouldn't want somebody to rely on them if the API isn't > > > stable). > > > > > > > > Agreed, WebUI is definitely not a safe way to implement anything visiting > > real > > > > web pages. > > > > > > https://codereview.chromium.org/2049363003/diff/380001/headless/lib/embedder_... File headless/lib/embedder_mojo_browsertest.cc (right): https://codereview.chromium.org/2049363003/diff/380001/headless/lib/embedder_... headless/lib/embedder_mojo_browsertest.cc:286: // We want to make sure bindings work across browser initiaited cross-origin On 2016/06/22 19:30:59, Charlie Reis wrote: > nit: initiated Done.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049363003/400001
Thanks for trying to explain it to me, and I'm sorry that I'm not grasping it. If sammc@ can approve the Mojo bit, then my only remaining concern is the use of ExecuteJavascriptForTests. https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/320001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:141: content::BINDINGS_POLICY_MOJO); On 2016/06/22 20:54:22, alex clarke wrote: > On 2016/06/22 19:30:59, Charlie Reis wrote: > > On 2016/06/22 10:04:00, alex clarke wrote: > > > On 2016/06/22 01:09:11, Sam McNally wrote: > > > > On 2016/06/21 23:08:31, Charlie Reis wrote: > > > > > On 2016/06/21 20:39:23, alex clarke wrote: > > > > > > On 2016/06/21 20:05:48, Charlie Reis wrote: > > > > > > > On 2016/06/21 19:52:44, alex clarke wrote: > > > > > > > > On 2016/06/21 18:23:49, Charlie Reis wrote: > > > > > > > > > This seems like a problem. If you do a cross-process navigation > > in > > > > this > > > > > > > > > WebContents, it looks like the new RenderViewHost won't get > these > > > > > > bindings. > > > > > > > > > Same if you go back after a cross-process navigation, in which > > case > > > > the > > > > > > > > NavEntry > > > > > > > > > will remember that you had bindings before (and notice that you > > > don't > > > > > the > > > > > > > > second > > > > > > > > > time) and cause a SetBindings crash. > > > > > > > > > > > > > > > > Is there a better way of installing them? > > > > > > > > > > > > > > > > It's hard to predict everything people will use this for, I hadn't > > > even > > > > > > > > considered somebody might want to navigate a webcontents after the > > > > > bindings > > > > > > > have > > > > > > > > been set. Obviously that's possible so we need to fix it :) > > > > > > > > > > > > > > WebContentsObserver::RenderFrameCreated is a better way to watch for > > it. > > > > > > > > > > There's an example in BlinkTestController, which calls > > > > > > HandleNewRenderFrameHost > > > > > > > to allow the bindings. > > > > > > OK. I'll have a go at using that and write a browser test. > > > > > > > > > > > > > > > > > > > > (Side note: that class also listens to RenderFrameHostChanged, but I > > > have > > > > a > > > > > > > suspicion that's redundant with RenderFrameCreated.) > > > > > > > > > > > > > > While looking that up, I also remembered that > > > > > > > RenderFrameImpl::MaybeEnableMojoBindings() is likely to cause > problems > > > for > > > > > > you. > > > > > > > It makes the assumption that BIDNINGS_POLICY_MOJO is only used for > > > layout > > > > > > tests > > > > > > > and tells MojoBindingsController (which appears to affect > > > > MojoContextState). > > > > > > > > > > > Yes I had to use the mojo.define => define shim which adds complexity. > > > > > > > > > > > > > > > > I don't know what this means. Won't you still hit > > > > > RenderFrameImpl::MaybeEnableMojoBindings via > RenderFrameImpl::Initialize, > > > and > > > > > thus tell MojoContextState that it's for_layout_tests, setting the > > > > > module_prefix_ to layout-test-mojom://? > > > > > Right. That actually isn't a problem for us since we deliberately push the > > > bindings from browser to renderer. I did try the pull method in an earlier > > > patch set but we where worried about letting the renderer request > potentially > > > arbitrary files from disc. > > > > Sorry, I don't know what you're referring to with push vs pull. If > > RenderFrameImpl::MaybeEnableMojoBindings() thinks you have > BINDING_POLICY_MOJO, > > then you're likely to have problems. > > > > Are you saying that RenderProcess::current()->GetEnabledBindings() won't > return > > BINDINGS_POLICY_MOJO in your case, so MaybeEnableMojoBindings() won't create a > > MojoBindingsController? > > Happy to set up a VC if that would help. But I'll try and explain here :) Maybe a VC would be useful, or I could defer to someone who knows this better. (I'm sick/WFH today, though.) > > There are two classes of js mojo bindings: > > 1. Special built in ones installed by > RenderFrame::EnsureMojoBuiltinsAreAvailable. > 2. General .mojom bindings. Normally are fetched on demand by > MojoContextState::FetchModule and then the js is evaluated. I've been calling > this the 'pull method'. > > Sami doesn't think #2 is safe in the context of headless, the problem being the > renderer is asking to browser to load an arbitrary file. For layout tests this > is loading is done by MojomProtocolHandler in layout_test_browser_context.cc, > we'd probably need something similar for headless. > > For headless we know what modules the user wishes to install and can 'push' them > to the renderer by using RenderFrameHost::ExecuteJavaScriptForTests to eval the > binding js (see HeadlessWebContentsImpl::DocumentOnLoadCompletedInMainFrame > below). This is a problem in itself. I've left a comment about the use of ExecuteJavaScriptForTests, which is not allowed. > Thus it's fine if MaybeEnableMojoBindings is called. We don't care if > the module_prefix_ is layout-test-mojom:// the reason being since we supply the > binding MojoContextState::FetchModule never gets called so the prefix isn't > used. I clearly don't understand enough of this to approve it as safe. It seems to me like the layout test situation with BINDING_POLICY_MOJO is setting up a bunch of landmines that you might be dodging now but could hit later. > > One problem with our approach is JS on the page might run before the browser has > had a chance to push the bindings to the renderer. Our convenience bindings > installed by HeadlessContentRendererClient::RunScriptsAtDocumentStart work > around that with promises and ES6 proxy class trickery. > HeadScriptEmbedderMojoBindingsTest is there to test that is working. > > There is one complication we have to work around with BINDING_POLICY_MOJO. The > MojoContextState constructor creates a mojo.define gin binding instead of a > define binding. This is a problem because the mojom js bindings assume that > define() is used so we some js to implement define() in terms of mojo.define(). > > I'd like not to have to do that :) > > > > > > > > > One consequence of BINDINGS_POLICY_MOJO is the define api is called > > mojo.define > > > and we need a shim (copied from the layout test helpers) to map define() => > > > mojo.define(). It would be nice if we didn't have to do that. > > > > > > > I'll have to defer the "define" stuff to others who know more about Mojo. > > > > > > > > > > > > > > I don't know a lot about Mojo, but that sounds problematic to me. > sammc@, > > > can > > > > > you confirm? > > > > > > > > Yes, BINDINGS_POLICY_MOJO is only suitable for layout tests. > > Fixing/replacing > > > > the current JS mojo bindings has been on our todo list for a while but the > > > > current house of cards hasn't caught fire and fallen over yet so it's been > > > > fairly low priority so far. > > > > > > Agreed that BINDINGS_POLICY_MOJO doesn't quite fit the needs of headless. > > What > > > I (think) we need for headless is: > > > > > > * Browser pushes js bindings (we think that's safer) > > > * The define() interface is added instead of mojo.define() > > > * Don't need a privileged view > > > > > > What I think we should do is land this patch (that will unblock some fairly > > high > > > priority internal work), and decide how best to support this new use case. > > Once > > > that's sorted out I'll be happy to change headless to use the new way. I > > don't > > > mind implementing the new way although I don't fully understand the other > use > > > cases. > > > > I don't see how this is safe to land yet, but maybe your answer to my > > MaybeEnableMojoBindings() question above will resolve it. (Happy to defer to > > sammc@ if he thinks it's safe.) > > > > > > > > > > > > > > > > > > > > > > > > > I > > > > > > > brought that up as a concern when reviewing > > > > > > > https://codereview.chromium.org/1707233003, but apparently it's > still > > > > > > > implemented that way. It looks like the code assumes that bindings > > for > > > > > WebUI > > > > > > > and the MOJO bindings (for layout tests) are mutually exclusive, but > > > > that's > > > > > > > probably no longer true for your case. > > > > > > > > > > > > > > You might want to check with sammc about what the implications are > > > there. > > > > > > > > > > > > I did try implementing this in terms of WebUI bindings but that seems > to > > > > have > > > > > > implications that seemed scary requiring privileged views. Also > doing > > so > > > > > > exposes the UI mojo modules which isn't something we need and might > > cause > > > > > > problems (we wouldn't want somebody to rely on them if the API isn't > > > > stable). > > > > > > > > > > Agreed, WebUI is definitely not a safe way to implement anything > visiting > > > real > > > > > web pages. > > > > > > > > > > https://codereview.chromium.org/2049363003/diff/400001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/400001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:157: render_frame_host->ExecuteJavaScriptForTests( Calling this from non-test code is illegal for security reasons. See https://crbug.com/507809. Jochen's comment in the description: "for drive-by web content, we always run the risk that the page is setup to intercept those calls. also, it's incredible difficult to ensure that those calls actually do what they're supposed to do."
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2049363003/diff/400001/headless/lib/browser/h... File headless/lib/browser/headless_web_contents_impl.cc (right): https://codereview.chromium.org/2049363003/diff/400001/headless/lib/browser/h... headless/lib/browser/headless_web_contents_impl.cc:157: render_frame_host->ExecuteJavaScriptForTests( On 2016/06/22 23:18:48, Charlie Reis wrote: > Calling this from non-test code is illegal for security reasons. See > https://crbug.com/507809. Jochen's comment in the description: > "for drive-by web content, we always run the risk that the page is setup to > intercept those calls. also, it's incredible difficult to ensure that those > calls actually do what they're supposed to do." I spoke to +jochen@ about this. He suggested we add a new scheme* for headless webcontents with mojo bindings, and we'd teach RFH::ExecuteJavaScript about that and use it here instead of the test method. * E.g. chrome-headless:// I don't know how to do that yet, but in principle it sounds reasonable.
Description was changed from
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more embedder
provided mojo services and request js bindings.
We push the bindings from the browser rather than doing a layout test style
pull,
because we don't want the renderer to be able to load arbitrary files and to
allow
better isolation between requests.
We ended up using a pretty funky two tier ES6 proxy backed with a promise to
implement our convenience bindings. This works around a whole
host of otherwise gnarly issues to do with getting data from the browser to the
renderer at the right time. You can now do something like this:
mojo.service.module_name.InterfaceName.then(function(interface) {
interface.myMethod(...);
});
The regular define(...) style of mojo module resolution works too.
Design doc:
https://docs.google.com/document/d/1Fr6_DJH6OK9rG3-ibMvRPTNnHsAXPk0VzxxiuJDSK...
More context is available from our BlinkOn presentation
slides:
https://docs.google.com/presentation/d/1gqK9F4lGAY3TZudAtdcxzMQNEE7PcuQrGu83N...
recording: https://youtu.be/zlNgsoPV3ho?t=6m55s
BUG=546953
==========
to
==========
Adds support for headless chrome embedder mojo services
You can now request a HeadlessWebContents be created with one or more embedder
provided mojo services and request js bindings.
We add a BINDINGS_POLICY_HEADLESS which instructs
MojoContextState to use the new headless-mojom:// protocol to
fetch the mojo bindings.
Design doc:
https://docs.google.com/document/d/1Fr6_DJH6OK9rG3-ibMvRPTNnHsAXPk0VzxxiuJDSK...
More context is available from our BlinkOn presentation
slides:
https://docs.google.com/presentation/d/1gqK9F4lGAY3TZudAtdcxzMQNEE7PcuQrGu83N...
recording: https://youtu.be/zlNgsoPV3ho?t=6m55s
BUG=546953
==========
PTAL I've tried Jochen's suggestion to implement a custom protocol handler. I think this has simplified the patch somewhat and hopefully addresses the security concerns. We no longer need to call RFH::ExecuteJavaScript and RenderFrameImpl::MaybeEnableMojoBindings explicitly supports the headless use case.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I think adding a new scheme for these types of bindings makes sense. lgtm. https://codereview.chromium.org/2049363003/diff/420001/content/public/common/... File content/public/common/bindings_policy.h (right): https://codereview.chromium.org/2049363003/diff/420001/content/public/common/... content/public/common/bindings_policy.h:36: // HeadlessWebConents. typo: HeadlessWebContents https://codereview.chromium.org/2049363003/diff/420001/headless/BUILD.gn File headless/BUILD.gn (left): https://codereview.chromium.org/2049363003/diff/420001/headless/BUILD.gn#oldc... headless/BUILD.gn:28: "$root_gen_dir/headless/headless_lib_resources.pak", Did you mean to drop this? https://codereview.chromium.org/2049363003/diff/420001/headless/lib/browser/h... File headless/lib/browser/headless_browser_context_impl.cc (right): https://codereview.chromium.org/2049363003/diff/420001/headless/lib/browser/h... headless/lib/browser/headless_browser_context_impl.cc:229: protocol_handlers_[kHeadlessMojomProtocol] = Maybe DCHECK that we're not clobbering the users's handler here. https://codereview.chromium.org/2049363003/diff/420001/headless/lib/embedder_... File headless/lib/embedder_mojo_browsertest.cc (right): https://codereview.chromium.org/2049363003/diff/420001/headless/lib/embedder_... headless/lib/embedder_mojo_browsertest.cc:32: // A test fixture which attaches a devtools client before starting the test. and registers mojo interfaces? https://codereview.chromium.org/2049363003/diff/420001/headless/public/headle... File headless/public/headless_browser_context.h (right): https://codereview.chromium.org/2049363003/diff/420001/headless/public/headle... headless/public/headless_browser_context.h:51: Builder& AddJsMojoBindings(const std::string& mojom_name, Let's have a TODO to add this to the default context too. https://codereview.chromium.org/2049363003/diff/420001/headless/public/util/k... File headless/public/util/kv_map_protocol_handler.h (right): https://codereview.chromium.org/2049363003/diff/420001/headless/public/util/k... headless/public/util/kv_map_protocol_handler.h:5: #ifndef HEADLESS_TEST_TEST_PROTOCOL_HANDLER_H_ Please update to match class name & location. https://codereview.chromium.org/2049363003/diff/420001/headless/public/util/k... headless/public/util/kv_map_protocol_handler.h:14: class KVMapProtocolHandler : public net::URLRequestJobFactory::ProtocolHandler { bikeshed: How about InMemoryProtocolHandler?
alexclarke@chromium.org changed reviewers: + jochen@chromium.org
All done. Jochen would you mind taking a look at the content/ change? Thanks! https://codereview.chromium.org/2049363003/diff/420001/content/public/common/... File content/public/common/bindings_policy.h (right): https://codereview.chromium.org/2049363003/diff/420001/content/public/common/... content/public/common/bindings_policy.h:36: // HeadlessWebConents. On 2016/06/27 11:10:41, Sami wrote: > typo: HeadlessWebContents Done. https://codereview.chromium.org/2049363003/diff/420001/headless/BUILD.gn File headless/BUILD.gn (left): https://codereview.chromium.org/2049363003/diff/420001/headless/BUILD.gn#oldc... headless/BUILD.gn:28: "$root_gen_dir/headless/headless_lib_resources.pak", On 2016/06/27 11:10:41, Sami wrote: > Did you mean to drop this? Oops no. https://codereview.chromium.org/2049363003/diff/420001/headless/lib/browser/h... File headless/lib/browser/headless_browser_context_impl.cc (right): https://codereview.chromium.org/2049363003/diff/420001/headless/lib/browser/h... headless/lib/browser/headless_browser_context_impl.cc:229: protocol_handlers_[kHeadlessMojomProtocol] = On 2016/06/27 11:10:42, Sami wrote: > Maybe DCHECK that we're not clobbering the users's handler here. Done. https://codereview.chromium.org/2049363003/diff/420001/headless/lib/embedder_... File headless/lib/embedder_mojo_browsertest.cc (right): https://codereview.chromium.org/2049363003/diff/420001/headless/lib/embedder_... headless/lib/embedder_mojo_browsertest.cc:32: // A test fixture which attaches a devtools client before starting the test. On 2016/06/27 11:10:42, Sami wrote: > and registers mojo interfaces? Done. https://codereview.chromium.org/2049363003/diff/420001/headless/public/headle... File headless/public/headless_browser_context.h (right): https://codereview.chromium.org/2049363003/diff/420001/headless/public/headle... headless/public/headless_browser_context.h:51: Builder& AddJsMojoBindings(const std::string& mojom_name, On 2016/06/27 11:10:42, Sami wrote: > Let's have a TODO to add this to the default context too. Done. https://codereview.chromium.org/2049363003/diff/420001/headless/public/util/k... File headless/public/util/kv_map_protocol_handler.h (right): https://codereview.chromium.org/2049363003/diff/420001/headless/public/util/k... headless/public/util/kv_map_protocol_handler.h:5: #ifndef HEADLESS_TEST_TEST_PROTOCOL_HANDLER_H_ On 2016/06/27 11:10:42, Sami wrote: > Please update to match class name & location. Done. https://codereview.chromium.org/2049363003/diff/420001/headless/public/util/k... headless/public/util/kv_map_protocol_handler.h:14: class KVMapProtocolHandler : public net::URLRequestJobFactory::ProtocolHandler { On 2016/06/27 11:10:42, Sami wrote: > bikeshed: How about InMemoryProtocolHandler? Done.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2049363003/diff/440001/content/renderer/mojo_... File content/renderer/mojo_context_state.cc (right): https://codereview.chromium.org/2049363003/diff/440001/content/renderer/mojo_... content/renderer/mojo_context_state.cc:102: return "headless-mojom://"; this is all a bit of a layering violation, but admittedly already existed before
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
alexclarke@chromium.org changed reviewers: + jam@chromium.org
Thanks Jochen. +jam John I think I need your stamp for the services/shell/public deps change. Could you take a look please?
mojom lgtm
mojom lgtm
mojom lgtm
lgtm
On 2016/06/24 17:31:33, alex clarke wrote: > PTAL I've tried Jochen's suggestion to implement a custom protocol handler. I > think this has simplified the patch somewhat and hopefully addresses the > security concerns. > > We no longer need to call RFH::ExecuteJavaScript and > RenderFrameImpl::MaybeEnableMojoBindings explicitly supports > the headless use case. Thanks, this looks much better to me! I'm almost ready to approve, once we resolve the DCHECK in render_frame_impl.cc below. Also, one followup question on ExecuteJavaScript. On 2016/06/23 15:14:04, alex clarke wrote: > https://codereview.chromium.org/2049363003/diff/400001/headless/lib/browser/h... > File headless/lib/browser/headless_web_contents_impl.cc (right): > > https://codereview.chromium.org/2049363003/diff/400001/headless/lib/browser/h... > headless/lib/browser/headless_web_contents_impl.cc:157: > render_frame_host->ExecuteJavaScriptForTests( > On 2016/06/22 23:18:48, Charlie Reis wrote: > > Calling this from non-test code is illegal for security reasons. See > > https://crbug.com/507809. Jochen's comment in the description: > > "for drive-by web content, we always run the risk that the page is setup to > > intercept those calls. also, it's incredible difficult to ensure that those > > calls actually do what they're supposed to do." > > I spoke to +jochen@ about this. He suggested we add a new scheme* for headless > webcontents with mojo bindings, and we'd teach RFH::ExecuteJavaScript about that > and use it here instead of the test method. > > * E.g. chrome-headless:// > > I don't know how to do that yet, but in principle it sounds reasonable. Did this turn out to be unnecessary? I didn't understand it (since I thought Headless loaded normal web pages and not pages under a special scheme), and I don't see a new scheme or changes to ExecuteJavaScript in the new patch. I'm glad we don't need those-- what did you end up using instead of ExecuteJavaScript? https://codereview.chromium.org/2049363003/diff/440001/content/renderer/mojo_... File content/renderer/mojo_bindings_controller.h (right): https://codereview.chromium.org/2049363003/diff/440001/content/renderer/mojo_... content/renderer/mojo_bindings_controller.h:23: enum class MojoBindingsType { FOR_UI_BINDINGS, FOR_LAYOUT_TESTS, FOR_HEADLESS }; nit: s/FOR_UI_BINDINGS/FOR_WEB_UI/ https://codereview.chromium.org/2049363003/diff/440001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2049363003/diff/440001/content/renderer/rende... content/renderer/render_frame_impl.cc:5909: ((enabled_bindings & kAllBindingsTypes) - 1), Am I misreading this? It looks like it's checking x & (x-1) == 0, and I'm not sure how that equates to checking that enabled_bindings is only one of the possible values (and not multiple). (If that's what it's doing, please add a comment how it works.)
On 2016/06/27 17:30:11, Charlie Reis wrote: > On 2016/06/24 17:31:33, alex clarke wrote: > > PTAL I've tried Jochen's suggestion to implement a custom protocol handler. I > > think this has simplified the patch somewhat and hopefully addresses the > > security concerns. > > > > We no longer need to call RFH::ExecuteJavaScript and > > RenderFrameImpl::MaybeEnableMojoBindings explicitly supports > > the headless use case. > > Thanks, this looks much better to me! I'm almost ready to approve, once we > resolve the DCHECK in render_frame_impl.cc below. Also, one followup question > on ExecuteJavaScript. > > > On 2016/06/23 15:14:04, alex clarke wrote: > > > https://codereview.chromium.org/2049363003/diff/400001/headless/lib/browser/h... > > File headless/lib/browser/headless_web_contents_impl.cc (right): > > > > > https://codereview.chromium.org/2049363003/diff/400001/headless/lib/browser/h... > > headless/lib/browser/headless_web_contents_impl.cc:157: > > render_frame_host->ExecuteJavaScriptForTests( > > On 2016/06/22 23:18:48, Charlie Reis wrote: > > > Calling this from non-test code is illegal for security reasons. See > > > https://crbug.com/507809. Jochen's comment in the description: > > > "for drive-by web content, we always run the risk that the page is setup to > > > intercept those calls. also, it's incredible difficult to ensure that those > > > calls actually do what they're supposed to do." > > > > I spoke to +jochen@ about this. He suggested we add a new scheme* for > headless > > webcontents with mojo bindings, and we'd teach RFH::ExecuteJavaScript about > that > > and use it here instead of the test method. > > > > * E.g. chrome-headless:// > > > > I don't know how to do that yet, but in principle it sounds reasonable. > > Did this turn out to be unnecessary? I didn't understand it (since I thought > Headless loaded normal web pages and not pages under a special scheme), and I > don't see a new scheme or changes to ExecuteJavaScript in the new patch. I'm > glad we don't need those-- what did you end up using instead of > ExecuteJavaScript? You are correct that headless loads normal pages. For our user's convenience convenience we wanted to add a more concise way to get hold of a mojo interface. Doing so required us calling RFH::ExecuteJavaScriptForTests. We hadn't appreciated the security concerns with that function, and initially we thought about running the control pages (ones with mojo bindings) under a special scheme which was allowed to use RFH::ExecuteJavaScript. Then it occurred to us that if we drop the fancy binding (and just use the default mojo way of getting a module in js) we didn't need to call RFH::ExecuteJavaScript at all. NB The bindings are still getting run, however that's done renderer side here: https://cs.chromium.org/chromium/src/content/renderer/mojo_main_runner.cc?sq=... At one point Sami was worried about the renderer loading files on demand. We solved that with the InMemoryProtocolHandler which only returns results we've explicitly whitelisted. > > https://codereview.chromium.org/2049363003/diff/440001/content/renderer/mojo_... > File content/renderer/mojo_bindings_controller.h (right): > > https://codereview.chromium.org/2049363003/diff/440001/content/renderer/mojo_... > content/renderer/mojo_bindings_controller.h:23: enum class MojoBindingsType { > FOR_UI_BINDINGS, FOR_LAYOUT_TESTS, FOR_HEADLESS }; > nit: s/FOR_UI_BINDINGS/FOR_WEB_UI/ > > https://codereview.chromium.org/2049363003/diff/440001/content/renderer/rende... > File content/renderer/render_frame_impl.cc (right): > > https://codereview.chromium.org/2049363003/diff/440001/content/renderer/rende... > content/renderer/render_frame_impl.cc:5909: ((enabled_bindings & > kAllBindingsTypes) - 1), > Am I misreading this? It looks like it's checking x & (x-1) == 0, and I'm not > sure how that equates to checking that enabled_bindings is only one of the > possible values (and not multiple). (If that's what it's doing, please add a > comment how it works.)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/06/27 21:26:44, alex clarke wrote: > On 2016/06/27 17:30:11, Charlie Reis wrote: > > On 2016/06/24 17:31:33, alex clarke wrote: > > > PTAL I've tried Jochen's suggestion to implement a custom protocol handler. > I > > > think this has simplified the patch somewhat and hopefully addresses the > > > security concerns. > > > > > > We no longer need to call RFH::ExecuteJavaScript and > > > RenderFrameImpl::MaybeEnableMojoBindings explicitly supports > > > the headless use case. > > > > Thanks, this looks much better to me! I'm almost ready to approve, once we > > resolve the DCHECK in render_frame_impl.cc below. Also, one followup question > > on ExecuteJavaScript. > > > > > > On 2016/06/23 15:14:04, alex clarke wrote: > > > > > > https://codereview.chromium.org/2049363003/diff/400001/headless/lib/browser/h... > > > File headless/lib/browser/headless_web_contents_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/2049363003/diff/400001/headless/lib/browser/h... > > > headless/lib/browser/headless_web_contents_impl.cc:157: > > > render_frame_host->ExecuteJavaScriptForTests( > > > On 2016/06/22 23:18:48, Charlie Reis wrote: > > > > Calling this from non-test code is illegal for security reasons. See > > > > https://crbug.com/507809. Jochen's comment in the description: > > > > "for drive-by web content, we always run the risk that the page is setup > to > > > > intercept those calls. also, it's incredible difficult to ensure that > those > > > > calls actually do what they're supposed to do." > > > > > > I spoke to +jochen@ about this. He suggested we add a new scheme* for > > headless > > > webcontents with mojo bindings, and we'd teach RFH::ExecuteJavaScript about > > that > > > and use it here instead of the test method. > > > > > > * E.g. chrome-headless:// > > > > > > I don't know how to do that yet, but in principle it sounds reasonable. > > > > Did this turn out to be unnecessary? I didn't understand it (since I thought > > Headless loaded normal web pages and not pages under a special scheme), and I > > don't see a new scheme or changes to ExecuteJavaScript in the new patch. I'm > > glad we don't need those-- what did you end up using instead of > > ExecuteJavaScript? > > You are correct that headless loads normal pages. For our user's convenience > convenience we wanted to add a more concise way to get hold of a mojo interface. > > Doing so required us calling RFH::ExecuteJavaScriptForTests. We hadn't > appreciated the security concerns with that function, and initially we thought > about running > the control pages (ones with mojo bindings) under a special scheme which was > allowed to use RFH::ExecuteJavaScript. Then it occurred to us that if we drop > the fancy > binding (and just use the default mojo way of getting a module in js) we didn't > need to call RFH::ExecuteJavaScript at all. > > NB The bindings are still getting run, however that's done renderer side here: > https://cs.chromium.org/chromium/src/content/renderer/mojo_main_runner.cc?sq=... > > At one point Sami was worried about the renderer loading files on demand. We > solved that with the InMemoryProtocolHandler which only returns results we've > explicitly whitelisted. > > > > > > https://codereview.chromium.org/2049363003/diff/440001/content/renderer/mojo_... > > File content/renderer/mojo_bindings_controller.h (right): > > > > > https://codereview.chromium.org/2049363003/diff/440001/content/renderer/mojo_... > > content/renderer/mojo_bindings_controller.h:23: enum class MojoBindingsType { > > FOR_UI_BINDINGS, FOR_LAYOUT_TESTS, FOR_HEADLESS }; > > nit: s/FOR_UI_BINDINGS/FOR_WEB_UI/ > > > > > https://codereview.chromium.org/2049363003/diff/440001/content/renderer/rende... > > File content/renderer/render_frame_impl.cc (right): > > > > > https://codereview.chromium.org/2049363003/diff/440001/content/renderer/rende... > > content/renderer/render_frame_impl.cc:5909: ((enabled_bindings & > > kAllBindingsTypes) - 1), > > Am I misreading this? It looks like it's checking x & (x-1) == 0, and I'm not > > sure how that equates to checking that enabled_bindings is only one of the > > possible values (and not multiple). (If that's what it's doing, please add a > > comment how it works.)
The code review tool seems to have got confused and isn't sending my comments. I've updated manually. On 2016/06/27 21:28:40, alex clarke wrote: > On 2016/06/27 21:26:44, alex clarke wrote: > > On 2016/06/27 17:30:11, Charlie Reis wrote: > > > On 2016/06/24 17:31:33, alex clarke wrote: > > > > PTAL I've tried Jochen's suggestion to implement a custom protocol > handler. > > I > > > > think this has simplified the patch somewhat and hopefully addresses the > > > > security concerns. > > > > > > > > We no longer need to call RFH::ExecuteJavaScript and > > > > RenderFrameImpl::MaybeEnableMojoBindings explicitly supports > > > > the headless use case. > > > > > > Thanks, this looks much better to me! I'm almost ready to approve, once we > > > resolve the DCHECK in render_frame_impl.cc below. Also, one followup > question > > > on ExecuteJavaScript. > > > > > > > > > On 2016/06/23 15:14:04, alex clarke wrote: > > > > > > > > > > https://codereview.chromium.org/2049363003/diff/400001/headless/lib/browser/h... > > > > File headless/lib/browser/headless_web_contents_impl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2049363003/diff/400001/headless/lib/browser/h... > > > > headless/lib/browser/headless_web_contents_impl.cc:157: > > > > render_frame_host->ExecuteJavaScriptForTests( > > > > On 2016/06/22 23:18:48, Charlie Reis wrote: > > > > > Calling this from non-test code is illegal for security reasons. See > > > > > https://crbug.com/507809. Jochen's comment in the description: > > > > > "for drive-by web content, we always run the risk that the page is setup > > to > > > > > intercept those calls. also, it's incredible difficult to ensure that > > those > > > > > calls actually do what they're supposed to do." > > > > > > > > I spoke to +jochen@ about this. He suggested we add a new scheme* for > > > headless > > > > webcontents with mojo bindings, and we'd teach RFH::ExecuteJavaScript > about > > > that > > > > and use it here instead of the test method. > > > > > > > > * E.g. chrome-headless:// > > > > > > > > I don't know how to do that yet, but in principle it sounds reasonable. > > > > > > Did this turn out to be unnecessary? I didn't understand it (since I > thought > > > Headless loaded normal web pages and not pages under a special scheme), and > I > > > don't see a new scheme or changes to ExecuteJavaScript in the new patch. > I'm > > > glad we don't need those-- what did you end up using instead of > > > ExecuteJavaScript? > > > > You are correct that headless loads normal pages. For our user's convenience > > convenience we wanted to add a more concise way to get hold of a mojo > interface. > > > > Doing so required us calling RFH::ExecuteJavaScriptForTests. We hadn't > > appreciated the security concerns with that function, and initially we thought > > about running > > the control pages (ones with mojo bindings) under a special scheme which was > > allowed to use RFH::ExecuteJavaScript. Then it occurred to us that if we drop > > the fancy > > binding (and just use the default mojo way of getting a module in js) we > didn't > > need to call RFH::ExecuteJavaScript at all. > > > > NB The bindings are still getting run, however that's done renderer side here: > > > https://cs.chromium.org/chromium/src/content/renderer/mojo_main_runner.cc?sq=... > > > > At one point Sami was worried about the renderer loading files on demand. We > > solved that with the InMemoryProtocolHandler which only returns results we've > > explicitly whitelisted. > > > > > > > > > > > https://codereview.chromium.org/2049363003/diff/440001/content/renderer/mojo_... > > > File content/renderer/mojo_bindings_controller.h (right): > > > > > > > > > https://codereview.chromium.org/2049363003/diff/440001/content/renderer/mojo_... > > > content/renderer/mojo_bindings_controller.h:23: enum class MojoBindingsType > { > > > FOR_UI_BINDINGS, FOR_LAYOUT_TESTS, FOR_HEADLESS }; > > > nit: s/FOR_UI_BINDINGS/FOR_WEB_UI/ Done. > > > > > > > > > https://codereview.chromium.org/2049363003/diff/440001/content/renderer/rende... > > > File content/renderer/render_frame_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/2049363003/diff/440001/content/renderer/rende... > > > content/renderer/render_frame_impl.cc:5909: ((enabled_bindings & > > > kAllBindingsTypes) - 1), > > > Am I misreading this? It looks like it's checking x & (x-1) == 0, and I'm > not > > > sure how that equates to checking that enabled_bindings is only one of the > > > possible values (and not multiple). (If that's what it's doing, please add > a > > > comment how it works.) If only one of those flags is set then enabled_bindings & kAllBindingsTypes must be a power of two. x & (x -1) clears the least significant bit, so if x is a power of two then x & (x -1) will be zero. e.g. in binary let x = 000001000 x - 1 = 000000111 x & (x -1) = 00000000 let x = 00001010 x - 1 = 00001001 x & (x - 1) = 00001000 I've added a comment. PS a great resource for bit-twiddling tricks: https://graphics.stanford.edu/~seander/bithacks.html#DetermineIfPowerOf2
> > On 2016/06/23 15:14:04, alex clarke wrote: > > > > > > https://codereview.chromium.org/2049363003/diff/400001/headless/lib/browser/h... > > > File headless/lib/browser/headless_web_contents_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/2049363003/diff/400001/headless/lib/browser/h... > > > headless/lib/browser/headless_web_contents_impl.cc:157: > > > render_frame_host->ExecuteJavaScriptForTests( > > > On 2016/06/22 23:18:48, Charlie Reis wrote: > > > > Calling this from non-test code is illegal for security reasons. See > > > > https://crbug.com/507809. Jochen's comment in the description: > > > > "for drive-by web content, we always run the risk that the page is setup > to > > > > intercept those calls. also, it's incredible difficult to ensure that > those > > > > calls actually do what they're supposed to do." > > > > > > I spoke to +jochen@ about this. He suggested we add a new scheme* for > > headless > > > webcontents with mojo bindings, and we'd teach RFH::ExecuteJavaScript about > > that > > > and use it here instead of the test method. > > > > > > * E.g. chrome-headless:// > > > > > > I don't know how to do that yet, but in principle it sounds reasonable. > > > > Did this turn out to be unnecessary? I didn't understand it (since I thought > > Headless loaded normal web pages and not pages under a special scheme), and I > > don't see a new scheme or changes to ExecuteJavaScript in the new patch. I'm > > glad we don't need those-- what did you end up using instead of > > ExecuteJavaScript? > > You are correct that headless loads normal pages. For our user's convenience > convenience we wanted to add a more concise way to get hold of a mojo interface. > > Doing so required us calling RFH::ExecuteJavaScriptForTests. We hadn't > appreciated the security concerns with that function, and initially we thought > about running > the control pages (ones with mojo bindings) under a special scheme which was > allowed to use RFH::ExecuteJavaScript. Then it occurred to us that if we drop > the fancy > binding (and just use the default mojo way of getting a module in js) we didn't > need to call RFH::ExecuteJavaScript at all. > > NB The bindings are still getting run, however that's done renderer side here: > https://cs.chromium.org/chromium/src/content/renderer/mojo_main_runner.cc?sq=... > > At one point Sami was worried about the renderer loading files on demand. We > solved that with the InMemoryProtocolHandler which only returns results we've > explicitly whitelisted. > To clarify, are these Mojo bindings adding functions that could be called from normal web pages when in Headless mode? If so, we need a security review of the functions being added. We must ensure that sandbox escapes aren't possible, beyond the loading files case Sami caught. (Otherwise we're just side-stepping the security restriction of RFH::ExecuteJavaScript while still facing the same concerns.) https://codereview.chromium.org/2049363003/diff/460001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2049363003/diff/460001/content/renderer/rende... content/renderer/render_frame_impl.cc:5911: // NOTE x & (x - 1) == 0 is true iff x is zero or a power of two. Thanks for adding the comment-- it was too clever for others to understand before. :)
On 2016/06/27 21:53:34, Charlie Reis wrote: > > > On 2016/06/23 15:14:04, alex clarke wrote: > > > > > > > > > > https://codereview.chromium.org/2049363003/diff/400001/headless/lib/browser/h... > > > > File headless/lib/browser/headless_web_contents_impl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2049363003/diff/400001/headless/lib/browser/h... > > > > headless/lib/browser/headless_web_contents_impl.cc:157: > > > > render_frame_host->ExecuteJavaScriptForTests( > > > > On 2016/06/22 23:18:48, Charlie Reis wrote: > > > > > Calling this from non-test code is illegal for security reasons. See > > > > > https://crbug.com/507809. Jochen's comment in the description: > > > > > "for drive-by web content, we always run the risk that the page is setup > > to > > > > > intercept those calls. also, it's incredible difficult to ensure that > > those > > > > > calls actually do what they're supposed to do." > > > > > > > > I spoke to +jochen@ about this. He suggested we add a new scheme* for > > > headless > > > > webcontents with mojo bindings, and we'd teach RFH::ExecuteJavaScript > about > > > that > > > > and use it here instead of the test method. > > > > > > > > * E.g. chrome-headless:// > > > > > > > > I don't know how to do that yet, but in principle it sounds reasonable. > > > > > > Did this turn out to be unnecessary? I didn't understand it (since I > thought > > > Headless loaded normal web pages and not pages under a special scheme), and > I > > > don't see a new scheme or changes to ExecuteJavaScript in the new patch. > I'm > > > glad we don't need those-- what did you end up using instead of > > > ExecuteJavaScript? > > > > You are correct that headless loads normal pages. For our user's convenience > > convenience we wanted to add a more concise way to get hold of a mojo > interface. > > > > Doing so required us calling RFH::ExecuteJavaScriptForTests. We hadn't > > appreciated the security concerns with that function, and initially we thought > > about running > > the control pages (ones with mojo bindings) under a special scheme which was > > allowed to use RFH::ExecuteJavaScript. Then it occurred to us that if we drop > > the fancy > > binding (and just use the default mojo way of getting a module in js) we > didn't > > need to call RFH::ExecuteJavaScript at all. > > > > NB The bindings are still getting run, however that's done renderer side here: > > > https://cs.chromium.org/chromium/src/content/renderer/mojo_main_runner.cc?sq=... > > > > At one point Sami was worried about the renderer loading files on demand. We > > solved that with the InMemoryProtocolHandler which only returns results we've > > explicitly whitelisted. > > > > To clarify, are these Mojo bindings adding functions that could be called from > normal web pages when in Headless mode? If so, we need a security review of the > functions being added. We must ensure that sandbox escapes aren't possible, > beyond the loading files case Sami caught. (Otherwise we're just side-stepping > the security restriction of RFH::ExecuteJavaScript while still facing the same > concerns.) Nevermind. Daniel tried to explain to me that these functions will be injected by testing frameworks, so hopefully it's not like arbitrary web content will be attacking them. (Yes?) If this is documented somewhere and has been through a security review, then I won't hold things up here anymore. LGTM.
On 2016/06/27 21:53:34, Charlie Reis wrote: > > > On 2016/06/23 15:14:04, alex clarke wrote: > > > > > > > > > > https://codereview.chromium.org/2049363003/diff/400001/headless/lib/browser/h... > > > > File headless/lib/browser/headless_web_contents_impl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2049363003/diff/400001/headless/lib/browser/h... > > > > headless/lib/browser/headless_web_contents_impl.cc:157: > > > > render_frame_host->ExecuteJavaScriptForTests( > > > > On 2016/06/22 23:18:48, Charlie Reis wrote: > > > > > Calling this from non-test code is illegal for security reasons. See > > > > > https://crbug.com/507809. Jochen's comment in the description: > > > > > "for drive-by web content, we always run the risk that the page is setup > > to > > > > > intercept those calls. also, it's incredible difficult to ensure that > > those > > > > > calls actually do what they're supposed to do." > > > > > > > > I spoke to +jochen@ about this. He suggested we add a new scheme* for > > > headless > > > > webcontents with mojo bindings, and we'd teach RFH::ExecuteJavaScript > about > > > that > > > > and use it here instead of the test method. > > > > > > > > * E.g. chrome-headless:// > > > > > > > > I don't know how to do that yet, but in principle it sounds reasonable. > > > > > > Did this turn out to be unnecessary? I didn't understand it (since I > thought > > > Headless loaded normal web pages and not pages under a special scheme), and > I > > > don't see a new scheme or changes to ExecuteJavaScript in the new patch. > I'm > > > glad we don't need those-- what did you end up using instead of > > > ExecuteJavaScript? > > > > You are correct that headless loads normal pages. For our user's convenience > > convenience we wanted to add a more concise way to get hold of a mojo > interface. > > > > Doing so required us calling RFH::ExecuteJavaScriptForTests. We hadn't > > appreciated the security concerns with that function, and initially we thought > > about running > > the control pages (ones with mojo bindings) under a special scheme which was > > allowed to use RFH::ExecuteJavaScript. Then it occurred to us that if we drop > > the fancy > > binding (and just use the default mojo way of getting a module in js) we > didn't > > need to call RFH::ExecuteJavaScript at all. > > > > NB The bindings are still getting run, however that's done renderer side here: > > > https://cs.chromium.org/chromium/src/content/renderer/mojo_main_runner.cc?sq=... > > > > At one point Sami was worried about the renderer loading files on demand. We > > solved that with the InMemoryProtocolHandler which only returns results we've > > explicitly whitelisted. > > > > To clarify, are these Mojo bindings adding functions that could be called from > normal web pages when in Headless mode? That's not the intended usage although we can't control what embeders do. We are adding two flavors of Headless chrome 1. chrome.exe with --headless (this doesn't quite exist yet), controlled via DevTools over a WebSocket. In this flavor headless Mojo bindings will not be available. 2. headless_lib which is for C++ embedders controlled via C++ DevTools bindings. In this flavor the embedder uses our API to create WebContents and Mojo bindings will only be installed on a page if they explicitly ask. Note the mojo module is something they write not us. We don't expect Mojo bindings to be used on normal web pages. Rather they're intended to be used in test harnesses such as PhantomJS. If so, we need a security review of the > functions being added. We must ensure that sandbox escapes aren't possible, > beyond the loading files case Sami caught. (Otherwise we're just side-stepping > the security restriction of RFH::ExecuteJavaScript while still facing the same > concerns.) > > https://codereview.chromium.org/2049363003/diff/460001/content/renderer/rende... > File content/renderer/render_frame_impl.cc (right): > > https://codereview.chromium.org/2049363003/diff/460001/content/renderer/rende... > content/renderer/render_frame_impl.cc:5911: // NOTE x & (x - 1) == 0 is true iff > x is zero or a power of two. > Thanks for adding the comment-- it was too clever for others to understand > before. :)
On 2016/06/27 22:16:01, alex clarke wrote: > > To clarify, are these Mojo bindings adding functions that could be called from > > normal web pages when in Headless mode? > > That's not the intended usage although we can't control what embeders do. > > We are adding two flavors of Headless chrome > > 1. chrome.exe with --headless (this doesn't quite exist yet), controlled via > DevTools over a WebSocket. In this flavor headless Mojo bindings will not be > available. > 2. headless_lib which is for C++ embedders controlled via C++ DevTools bindings. > In this flavor the embedder uses our API to create WebContents and Mojo > bindings will only be installed on a page if they explicitly ask. Note the mojo > module is something they write not us. > > We don't expect Mojo bindings to be used on normal web pages. Rather they're > intended to be used in test harnesses such as PhantomJS. Thanks, SGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Adds support for headless chrome embedder mojo services You can now request a HeadlessWebContents be created with one or more embedder provided mojo services and request js bindings. We add a BINDINGS_POLICY_HEADLESS which instructs MojoContextState to use the new headless-mojom:// protocol to fetch the mojo bindings. Design doc: https://docs.google.com/document/d/1Fr6_DJH6OK9rG3-ibMvRPTNnHsAXPk0VzxxiuJDSK... More context is available from our BlinkOn presentation slides: https://docs.google.com/presentation/d/1gqK9F4lGAY3TZudAtdcxzMQNEE7PcuQrGu83N... recording: https://youtu.be/zlNgsoPV3ho?t=6m55s BUG=546953 ========== to ========== Adds support for headless chrome embedder mojo services You can now request a HeadlessWebContents be created with one or more embedder provided mojo services and request js bindings. We add a BINDINGS_POLICY_HEADLESS which instructs MojoContextState to use the new headless-mojom:// protocol to fetch the mojo bindings. Design doc: https://docs.google.com/document/d/1Fr6_DJH6OK9rG3-ibMvRPTNnHsAXPk0VzxxiuJDSK... More context is available from our BlinkOn presentation slides: https://docs.google.com/presentation/d/1gqK9F4lGAY3TZudAtdcxzMQNEE7PcuQrGu83N... recording: https://youtu.be/zlNgsoPV3ho?t=6m55s BUG=546953, 623954 ==========
Thanks for the review everyone! I'm going to wait until we get the green light on http://crbug.com/623954 before submitting this.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Changes to disable http/https by default lgtm.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
We got the green light on the launch bug, I'll submit this now. Thank you for your input this patch ended up all the better for it. https://codereview.chromium.org/2049363003/diff/440001/content/renderer/mojo_... File content/renderer/mojo_bindings_controller.h (right): https://codereview.chromium.org/2049363003/diff/440001/content/renderer/mojo_... content/renderer/mojo_bindings_controller.h:23: enum class MojoBindingsType { FOR_UI_BINDINGS, FOR_LAYOUT_TESTS, FOR_HEADLESS }; On 2016/06/27 17:30:11, Charlie Reis wrote: > nit: s/FOR_UI_BINDINGS/FOR_WEB_UI/ Done. https://codereview.chromium.org/2049363003/diff/440001/content/renderer/mojo_... File content/renderer/mojo_context_state.cc (right): https://codereview.chromium.org/2049363003/diff/440001/content/renderer/mojo_... content/renderer/mojo_context_state.cc:102: return "headless-mojom://"; On 2016/06/27 12:31:57, jochen wrote: > this is all a bit of a layering violation, but admittedly already existed before Acknowledged. https://codereview.chromium.org/2049363003/diff/440001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2049363003/diff/440001/content/renderer/rende... content/renderer/render_frame_impl.cc:5909: ((enabled_bindings & kAllBindingsTypes) - 1), On 2016/06/27 17:30:11, Charlie Reis wrote: > Am I misreading this? No :) > It looks like it's checking x & (x-1) == 0, and I'm not > sure how that equates to checking that enabled_bindings is only one of the > possible values (and not multiple). (If that's what it's doing, please add a > comment how it works.) If only one of those flags is set then enabled_bindings & kAllBindingsTypes must be a power of two. x & (x -1) clears the least significant bit, so if x is a power of two then x & (x -1) will be zero. e.g. in binary let x = 000001000 x - 1 = 000000111 x & (x -1) = 00000000 let x = 00001010 x - 1 = 00001001 x & (x - 1) = 00001000 I've added a comment. PS a great resource for bit-twiddling tricks: https://graphics.stanford.edu/~seander/bithacks.html#DetermineIfPowerOf2
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org, dcheng@chromium.org, jochen@chromium.org, jam@chromium.org, tsepez@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2049363003/#ps560001 (title: "Remove stray ;")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, skyostil@chromium.org, jam@chromium.org, dcheng@chromium.org, tsepez@chromium.org, yzshen@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2049363003/#ps580001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Adds support for headless chrome embedder mojo services You can now request a HeadlessWebContents be created with one or more embedder provided mojo services and request js bindings. We add a BINDINGS_POLICY_HEADLESS which instructs MojoContextState to use the new headless-mojom:// protocol to fetch the mojo bindings. Design doc: https://docs.google.com/document/d/1Fr6_DJH6OK9rG3-ibMvRPTNnHsAXPk0VzxxiuJDSK... More context is available from our BlinkOn presentation slides: https://docs.google.com/presentation/d/1gqK9F4lGAY3TZudAtdcxzMQNEE7PcuQrGu83N... recording: https://youtu.be/zlNgsoPV3ho?t=6m55s BUG=546953, 623954 ========== to ========== Adds support for headless chrome embedder mojo services You can now request a HeadlessWebContents be created with one or more embedder provided mojo services and request js bindings. We add a BINDINGS_POLICY_HEADLESS which instructs MojoContextState to use the new headless-mojom:// protocol to fetch the mojo bindings. Design doc: https://docs.google.com/document/d/1Fr6_DJH6OK9rG3-ibMvRPTNnHsAXPk0VzxxiuJDSK... More context is available from our BlinkOn presentation slides: https://docs.google.com/presentation/d/1gqK9F4lGAY3TZudAtdcxzMQNEE7PcuQrGu83N... recording: https://youtu.be/zlNgsoPV3ho?t=6m55s BUG=546953, 623954 ==========
Message was sent while issue was closed.
Committed patchset #29 (id:580001)
Message was sent while issue was closed.
Description was changed from ========== Adds support for headless chrome embedder mojo services You can now request a HeadlessWebContents be created with one or more embedder provided mojo services and request js bindings. We add a BINDINGS_POLICY_HEADLESS which instructs MojoContextState to use the new headless-mojom:// protocol to fetch the mojo bindings. Design doc: https://docs.google.com/document/d/1Fr6_DJH6OK9rG3-ibMvRPTNnHsAXPk0VzxxiuJDSK... More context is available from our BlinkOn presentation slides: https://docs.google.com/presentation/d/1gqK9F4lGAY3TZudAtdcxzMQNEE7PcuQrGu83N... recording: https://youtu.be/zlNgsoPV3ho?t=6m55s BUG=546953, 623954 ========== to ========== Adds support for headless chrome embedder mojo services You can now request a HeadlessWebContents be created with one or more embedder provided mojo services and request js bindings. We add a BINDINGS_POLICY_HEADLESS which instructs MojoContextState to use the new headless-mojom:// protocol to fetch the mojo bindings. Design doc: https://docs.google.com/document/d/1Fr6_DJH6OK9rG3-ibMvRPTNnHsAXPk0VzxxiuJDSK... More context is available from our BlinkOn presentation slides: https://docs.google.com/presentation/d/1gqK9F4lGAY3TZudAtdcxzMQNEE7PcuQrGu83N... recording: https://youtu.be/zlNgsoPV3ho?t=6m55s BUG=546953, 623954 Committed: https://crrev.com/a50533eeece4f0e34c4deb6c4539772a0269d63f Cr-Commit-Position: refs/heads/master@{#408617} ==========
Message was sent while issue was closed.
Patchset 29 (id:??) landed as https://crrev.com/a50533eeece4f0e34c4deb6c4539772a0269d63f Cr-Commit-Position: refs/heads/master@{#408617} |
