Initialize V8 in PDFium from external files (plugin process only)

Initialize V8 in PDFium from external files (plugin process only)

BUG=421063
Committed: https://crrev.com/5c1aafbd7729d2d1fd1bea34e865cd83080ddd8c
Cr-Commit-Position: refs/heads/master@{#304223}

[baixo1, 2014-11-10 18:13:22]

baixo@google.com changed reviewers:
+ baixo@google.com, raymes@chromium.org, rmcilroy@chromium.org

[baixo1, 2014-11-10 18:13:23]

** Please, ignore the other CL **

This CL assumes that the "in-renderer" one has landed already. Could you guys
please take a look? Do you think the included DEPS on gin are ok (in chrome and
ppapi)?

Thanks!

[raymes, 2014-11-11 03:26:39]

Looks good, thank you!

https://codereview.chromium.org/718453003/diff/1/chrome/plugin/chrome_content...
File chrome/plugin/chrome_content_plugin_client.cc (right):

https://codereview.chromium.org/718453003/diff/1/chrome/plugin/chrome_content...
chrome/plugin/chrome_content_plugin_client.cc:27: #ifdef
V8_USE_EXTERNAL_STARTUP_DATA
Should we just bury this #ifdef in gin as well or are there times when 
gin::IsolateHolder::LoadV8Snapshot(); needs to be called regardless of the
#ifdef?

https://codereview.chromium.org/718453003/diff/1/chrome/plugin/chrome_content...
chrome/plugin/chrome_content_plugin_client.cc:28:
gin::IsolateHolder::LoadV8Snapshot();
I suggest getting jschuh@ or cpu@ to review this part of the change, just to
make sure they are ok with adding this in before the sandbox initialization.

https://codereview.chromium.org/718453003/diff/1/ppapi/proxy/DEPS
File ppapi/proxy/DEPS (right):

https://codereview.chromium.org/718453003/diff/1/ppapi/proxy/DEPS#newcode3
ppapi/proxy/DEPS:3: "+gin/public/isolate_holder.h",
I think you can just have +gin

https://codereview.chromium.org/718453003/diff/1/ppapi/proxy/pdf_resource.cc
File ppapi/proxy/pdf_resource.cc (right):

https://codereview.chromium.org/718453003/diff/1/ppapi/proxy/pdf_resource.cc#...
ppapi/proxy/pdf_resource.cc:213: natives_size_out, snapshot_data_out,
snapshot_size_out);
nit: 4 space indent

https://codereview.chromium.org/718453003/diff/1/ppapi/thunk/ppb_pdf_thunk.cc
File ppapi/thunk/ppb_pdf_thunk.cc (right):

https://codereview.chromium.org/718453003/diff/1/ppapi/thunk/ppb_pdf_thunk.cc...
ppapi/thunk/ppb_pdf_thunk.cc:173: natives_size_out, snapshot_data_out,
snapshot_size_out);
nit: 4 space indent

[baixo1, 2014-11-11 10:56:10]

baixo@google.com changed reviewers:
+ jochen@chromium.org, jschuh@chromium.org

[baixo1, 2014-11-11 10:56:11]

jochen@: could you please take a look at the chrome part?

jschuh@: could you please take a look at
chrome/plugin/chrome_content_plugin_client.cc ?

Thanks!

https://codereview.chromium.org/718453003/diff/1/chrome/plugin/chrome_content...
File chrome/plugin/chrome_content_plugin_client.cc (right):

https://codereview.chromium.org/718453003/diff/1/chrome/plugin/chrome_content...
chrome/plugin/chrome_content_plugin_client.cc:27: #ifdef
V8_USE_EXTERNAL_STARTUP_DATA
On 2014/11/11 03:26:39, raymes wrote:
> Should we just bury this #ifdef in gin as well or are there times when 
> gin::IsolateHolder::LoadV8Snapshot(); needs to be called regardless of the
> #ifdef?

+jochen@

LoadV8Snapshot is currently only visible if V8_USE_EXTERNAL_STARTUP_DATA is
defined. When called, it'll try do map the files right away and return a bool
indicating success/failure.

I could expose this function even when V8_USE_EXTERNAL_STARTUP_DATA is *not*
defined as well and make it return "success" in such case (not using external
V8). Are you ok with this, Jochen?

https://codereview.chromium.org/718453003/diff/1/chrome/plugin/chrome_content...
chrome/plugin/chrome_content_plugin_client.cc:28:
gin::IsolateHolder::LoadV8Snapshot();
On 2014/11/11 03:26:39, raymes wrote:
> I suggest getting jschuh@ or cpu@ to review this part of the change, just to
> make sure they are ok with adding this in before the sandbox initialization.

+jschuh@ : could you take a look at this, please?

https://codereview.chromium.org/718453003/diff/1/ppapi/proxy/DEPS
File ppapi/proxy/DEPS (right):

https://codereview.chromium.org/718453003/diff/1/ppapi/proxy/DEPS#newcode3
ppapi/proxy/DEPS:3: "+gin/public/isolate_holder.h",
On 2014/11/11 03:26:39, raymes wrote:
> I think you can just have +gin

Done.

https://codereview.chromium.org/718453003/diff/1/ppapi/proxy/pdf_resource.cc
File ppapi/proxy/pdf_resource.cc (right):

https://codereview.chromium.org/718453003/diff/1/ppapi/proxy/pdf_resource.cc#...
ppapi/proxy/pdf_resource.cc:213: natives_size_out, snapshot_data_out,
snapshot_size_out);
On 2014/11/11 03:26:39, raymes wrote:
> nit: 4 space indent

Done.

https://codereview.chromium.org/718453003/diff/1/ppapi/thunk/ppb_pdf_thunk.cc
File ppapi/thunk/ppb_pdf_thunk.cc (right):

https://codereview.chromium.org/718453003/diff/1/ppapi/thunk/ppb_pdf_thunk.cc...
ppapi/thunk/ppb_pdf_thunk.cc:173: natives_size_out, snapshot_data_out,
snapshot_size_out);
On 2014/11/11 03:26:39, raymes wrote:
> nit: 4 space indent

Done.

[raymes, 2014-11-11 23:02:22]

Can we add a test inside ppapi/tests/test_pdf.cc. It would just call the
function and do some sanity check on the output.

https://codereview.chromium.org/718453003/diff/1/chrome/plugin/chrome_content...
File chrome/plugin/chrome_content_plugin_client.cc (right):

https://codereview.chromium.org/718453003/diff/1/chrome/plugin/chrome_content...
chrome/plugin/chrome_content_plugin_client.cc:27: #ifdef
V8_USE_EXTERNAL_STARTUP_DATA
Feel free to do this in a followup CL if it requires more complicated changes :)

[jochen (gone - plz use gerrit), 2014-11-12 15:02:49]

https://codereview.chromium.org/718453003/diff/20001/chrome/plugin/DEPS
File chrome/plugin/DEPS (right):

https://codereview.chromium.org/718453003/diff/20001/chrome/plugin/DEPS#newcode3
chrome/plugin/DEPS:3: "+gin/public/isolate_holder.h",
if you add new DEPS, you'll also need to update the gyp/gn files

[baixo1, 2014-11-12 18:32:26]

jochen@: Done.
raymes@: I added the test you requested and I also had to add the gyp/gn
dependencies on gin, since I added new DEPS. Could you please take a look at
this?

Thanks!

https://codereview.chromium.org/718453003/diff/20001/chrome/plugin/DEPS
File chrome/plugin/DEPS (right):

https://codereview.chromium.org/718453003/diff/20001/chrome/plugin/DEPS#newcode3
chrome/plugin/DEPS:3: "+gin/public/isolate_holder.h",
On 2014/11/12 15:02:49, jochen (slow) wrote:
> if you add new DEPS, you'll also need to update the gyp/gn files

Done.

[baixo1, 2014-11-12 18:38:19]

jschuh@: could you please take a look at
chrome/plugin/chrome_content_plugin_client.cc ?

Thanks!

[raymes, 2014-11-12 22:47:51]

lgtm!

[jschuh, 2014-11-13 00:13:26]

https://codereview.chromium.org/718453003/diff/1/chrome/plugin/chrome_content...
File chrome/plugin/chrome_content_plugin_client.cc (right):

https://codereview.chromium.org/718453003/diff/1/chrome/plugin/chrome_content...
chrome/plugin/chrome_content_plugin_client.cc:28:
gin::IsolateHolder::LoadV8Snapshot();
On 2014/11/11 10:56:11, baixo1 wrote:
> On 2014/11/11 03:26:39, raymes wrote:
> > I suggest getting jschuh@ or cpu@ to review this part of the change, just to
> > make sure they are ok with adding this in before the sandbox initialization.
> 
> +jschuh@ : could you take a look at this, please?
> 

When would this get enabled? I.e. is the V8_USE_EXTERNAL_STARTUP_DATA flag
enabled in stable builds of Chrome that we ship, or is this used in some other
context?

[raymes, 2014-11-13 00:26:28]

I think this will be enabled in stable builds of chrome. FWIU we already do
this for the renderer process, this just adds it to the plugin process so
the initialization data can be used by out of process plugins. baixo@ can
probably give more details.

On 11:13, Thu, Nov 13, 2014 null <jschuh@chromium.org> wrote:

>
> https://codereview.chromium.org/718453003/diff/1/chrome/
> plugin/chrome_content_plugin_client.cc
> File chrome/plugin/chrome_content_plugin_client.cc (right):
>
> https://codereview.chromium.org/718453003/diff/1/chrome/
> plugin/chrome_content_plugin_client.cc#newcode28
> chrome/plugin/chrome_content_plugin_client.cc:28
>
<https://codereview.chromium.org/718453003/diff/1/chrome/plugin/chrome_content...>
> :
> gin::IsolateHolder::LoadV8Snapshot();
> On 2014/11/11 10:56:11, baixo1 wrote:
> > On 2014/11/11 03:26:39, raymes wrote:
> > > I suggest getting jschuh@ or cpu@ to review this part of the change,
> just to
> > > make sure they are ok with adding this in before the sandbox
> initialization.
>
> > +jschuh@ : could you take a look at this, please?
>
>
> When would this get enabled? I.e. is the V8_USE_EXTERNAL_STARTUP_DATA
> flag enabled in stable builds of Chrome that we ship, or is this used in
> some other context?
>
> https://codereview.chromium.org/718453003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[baixo1, 2014-11-13 10:59:52]

On 2014/11/13 00:26:28, raymes wrote:
> I think this will be enabled in stable builds of chrome. FWIU we already do
> this for the renderer process, this just adds it to the plugin process so
> the initialization data can be used by out of process plugins. baixo@ can
> probably give more details.
> 
> On 11:13, Thu, Nov 13, 2014 null <mailto:jschuh@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/718453003/diff/1/chrome/
> > plugin/chrome_content_plugin_client.cc
> > File chrome/plugin/chrome_content_plugin_client.cc (right):
> >
> > https://codereview.chromium.org/718453003/diff/1/chrome/
> > plugin/chrome_content_plugin_client.cc#newcode28
> > chrome/plugin/chrome_content_plugin_client.cc:28
> >
>
<https://codereview.chromium.org/718453003/diff/1/chrome/plugin/chrome_content...>
> > :
> > gin::IsolateHolder::LoadV8Snapshot();
> > On 2014/11/11 10:56:11, baixo1 wrote:
> > > On 2014/11/11 03:26:39, raymes wrote:
> > > > I suggest getting jschuh@ or cpu@ to review this part of the change,
> > just to
> > > > make sure they are ok with adding this in before the sandbox
> > initialization.
> >
> > > +jschuh@ : could you take a look at this, please?
> >
> >
> > When would this get enabled? I.e. is the V8_USE_EXTERNAL_STARTUP_DATA
> > flag enabled in stable builds of Chrome that we ship, or is this used in
> > some other context?
> >
> > https://codereview.chromium.org/718453003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Raymes is correct: this will be enabled in stable builds of Chrome.
V8 is currently statically linked into Chrome's .so, but we are
moving to loading V8's initial snapshot from external files instead.
PDFium needs V8 and it can't count on the renderer process to initialize V8
when PDfium is running on a plugin process.
Therefore, we must read these external files before the plugin process
sandbox is sealed, so the plugin can properly initialize V8 when it needs to.

[jschuh, 2014-11-13 16:24:37]

lgtm

[jochen (gone - plz use gerrit), 2014-11-13 19:43:01]

lgtm

[baixo1, 2014-11-13 19:44:41]

The CQ bit was checked by baixo@google.com

[commit-bot: I haz the power, 2014-11-13 19:46:24]

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

[commit-bot: I haz the power, 2014-11-13 19:54:20]

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

[commit-bot: I haz the power, 2014-11-13 19:54:22]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[baixo1, 2014-11-14 15:40:12]

The CQ bit was checked by baixo@google.com

[commit-bot: I haz the power, 2014-11-14 15:40:44]

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

[commit-bot: I haz the power, 2014-11-14 16:34:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-14 16:35:29]

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