Load pdf library on all platforms.

Load pdf library on all platforms.

Added RenderPDFPageToBitmap signature.

Call ChromeContentUtilityClient::PreSandboxStartup for Zygote. It does not real additional work. Zygote already loads pdf and initialize MediaLibrary for renderer inside content/, pdf is just to hard to access from Utility. ServiceDiscoveryMessageHandler::PreSandboxStartup does nothing for POSIX.

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

[Noam Samuel, 2013-11-13 17:32:08]

lgtm

[Lei Zhang, 2013-11-13 18:12:00]

lgtm

[Vitaly Buka (NO REVIEWS), 2013-11-14 08:23:21]

On 2013/11/13 18:12:00, Lei Zhang wrote:
> lgtm

Please take a look again. Previous patch tries to loads to late for zygote.
This one works on all platforms.

[Lei Zhang, 2013-11-14 21:56:54]

https://codereview.chromium.org/70083006/diff/240001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/70083006/diff/240001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:89: if
(!PathService::Get(chrome::FILE_PDF_PLUGIN, &pdf_module_path) &&
Shouldn't this be:

if (!PathService::Get() || !PathExists()) ?

https://codereview.chromium.org/70083006/diff/240001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:143: // Exported by pdf.dll
nit: "PDF plugin", since this is non-Windows as well.

https://codereview.chromium.org/70083006/diff/240001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:153: RenderPDFPageToBitmapProc
render_pdf_to_bitmap_;
render_pdf_to_bitmap_func_ ?
"render_pdf_to_bitmap_" sounds like it might be a boolean.

[Vitaly Buka (NO REVIEWS), 2013-11-14 22:10:20]

https://codereview.chromium.org/70083006/diff/240001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/70083006/diff/240001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:89: if
(!PathService::Get(chrome::FILE_PDF_PLUGIN, &pdf_module_path) &&
On 2013/11/14 21:56:54, Lei Zhang wrote:
> Shouldn't this be:
> 
> if (!PathService::Get() || !PathExists()) ?

Done.

https://codereview.chromium.org/70083006/diff/240001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:143: // Exported by pdf.dll
On 2013/11/14 21:56:54, Lei Zhang wrote:
> nit: "PDF plugin", since this is non-Windows as well.

Done.

https://codereview.chromium.org/70083006/diff/240001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:153: RenderPDFPageToBitmapProc
render_pdf_to_bitmap_;
On 2013/11/14 21:56:54, Lei Zhang wrote:
> render_pdf_to_bitmap_func_ ?
> "render_pdf_to_bitmap_" sounds like it might be a boolean.

Done.

[Lei Zhang, 2013-11-14 22:45:59]

+security folks in case they have any concerns

[jln (very slow on Chromium), 2013-11-14 23:14:17]

1. Why do you need the PDF library in the utility process on Linux / Chrome OS?

The utility process sandbox is unfortunately barely existing on these platforms.

2. You say "Zygote already loads pdf and initialize MediaLibrary".

Is this done as part of PreloadPepperPlugins() ? If yes, why doesn't it work
already for Zygoted utility process ?

I didn't look in details, but is it just because you need to get a "handle" on
the library ?

Can you please check that the library is not loaded twice ? (look at
/proc/pid/maps for a utility process).

https://codereview.chromium.org/70083006/diff/330001/chrome/app/chrome_main_d...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/70083006/diff/330001/chrome/app/chrome_main_d...
chrome/app/chrome_main_delegate.cc:701:
chrome::ChromeContentUtilityClient::PreSandboxStartup();
Do you mind calling this from content/zygote/zygote_main_linux.cc
PreSandboxInit() /  
 PreloadPepperPlugins() instead ?

This is confusing and should be refactored, but the vast majority of pre-sandbox
library load is done there.

[Vitaly Buka (NO REVIEWS), 2013-11-14 23:53:11]

On 2013/11/14 23:14:17, jln wrote:
> 1. Why do you need the PDF library in the utility process on Linux / Chrome
OS?
> 
> The utility process sandbox is unfortunately barely existing on these
platforms.

We need to call RenderPDFPageToBitmap from pdf module.
If it's unsecure, we can to do that in Renderer.

> 
> 2. You say "Zygote already loads pdf and initialize MediaLibrary".
> 
> Is this done as part of PreloadPepperPlugins() ? If yes, why doesn't it work
> already for Zygoted utility process ?
> 
> I didn't look in details, but is it just because you need to get a "handle" on
> the library ?

Yes. I need module handle and it's inside
content/renderer/pepper/pepper_plugin_registry.h, so it can't be used by
utility.

> 
> Can you please check that the library is not loaded twice ? (look at
> /proc/pid/maps for a utility process).

Looks like it's not.

With my patch:
7fc8aedc8000-7fc8af677000 r-xp 00000000 08:11 4085980                   
src/out/Debug/libpdf.so
7fc8af677000-7fc8af69c000 r--p 008ae000 08:11 4085980                   
src/out/Debug/libpdf.so
7fc8af69c000-7fc8af6c0000 rw-p 008d3000 08:11 4085980                   
src/out/Debug/libpdf.so

Without my patch, unstable channel.
7fbe70123000-7fbe7072b000 r-xp 00000000 08:01 14420124                  
/opt/google/chrome/libpdf.so
7fbe7072b000-7fbe7072c000 ---p 00608000 08:01 14420124                  
/opt/google/chrome/libpdf.so
7fbe7072c000-7fbe7074e000 r--p 00608000 08:01 14420124                  
/opt/google/chrome/libpdf.so
7fbe7074e000-7fbe7076d000 rw-p 0062a000 08:01 14420124                  
/opt/google/chrome/libpdf.so

> 
>
https://codereview.chromium.org/70083006/diff/330001/chrome/app/chrome_main_d...
> File chrome/app/chrome_main_delegate.cc (right):
> 
>
https://codereview.chromium.org/70083006/diff/330001/chrome/app/chrome_main_d...
> chrome/app/chrome_main_delegate.cc:701:
> chrome::ChromeContentUtilityClient::PreSandboxStartup();
> Do you mind calling this from content/zygote/zygote_main_linux.cc
> PreSandboxInit() /  
>  PreloadPepperPlugins() instead ?
> 
> This is confusing and should be refactored, but the vast majority of
pre-sandbox
> library load is done there.

[Vitaly Buka (NO REVIEWS), 2013-11-14 23:53:17]

https://codereview.chromium.org/70083006/diff/330001/chrome/app/chrome_main_d...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/70083006/diff/330001/chrome/app/chrome_main_d...
chrome/app/chrome_main_delegate.cc:701:
chrome::ChromeContentUtilityClient::PreSandboxStartup();
PreSandboxInit() /  
 PreloadPepperPlugins()
are parts of content, I can't call chrome/code from there without re-factoring.

On 2013/11/14 23:14:18, jln wrote:
> Do you mind calling this from content/zygote/zygote_main_linux.cc
> PreSandboxInit() /  
>  PreloadPepperPlugins() instead ?
> 
> This is confusing and should be refactored, but the vast majority of
pre-sandbox
> library load is done there.

[jln (very slow on Chromium), 2013-11-15 00:11:21]

https://codereview.chromium.org/70083006/diff/330001/chrome/app/chrome_main_d...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/70083006/diff/330001/chrome/app/chrome_main_d...
chrome/app/chrome_main_delegate.cc:701:
chrome::ChromeContentUtilityClient::PreSandboxStartup();
On 2013/11/14 23:53:18, Vitaly Buka wrote:
> PreSandboxInit() /  
>  PreloadPepperPlugins()
> are parts of content, I can't call chrome/code from there without
re-factoring.

Ahh yeah, of course, silly me.

I hope that the Zygote trying to reload the same thing will not cause much
computation to be duplicated. It's fairly unlikely that it would if the library
doesn't get loaded twice.

Is there a clean way to exclude the PDF library from PreloadPepperPlugins() with
a comment pointing here?

[Vitaly Buka (NO REVIEWS), 2013-11-15 00:36:20]

On 2013/11/15 00:11:21, jln wrote:
>
https://codereview.chromium.org/70083006/diff/330001/chrome/app/chrome_main_d...
> File chrome/app/chrome_main_delegate.cc (right):
> 
>
https://codereview.chromium.org/70083006/diff/330001/chrome/app/chrome_main_d...
> chrome/app/chrome_main_delegate.cc:701:
> chrome::ChromeContentUtilityClient::PreSandboxStartup();
> On 2013/11/14 23:53:18, Vitaly Buka wrote:
> > PreSandboxInit() /  
> >  PreloadPepperPlugins()
> > are parts of content, I can't call chrome/code from there without
> re-factoring.
> 
> Ahh yeah, of course, silly me.
> 
> I hope that the Zygote trying to reload the same thing will not cause much
> computation to be duplicated. It's fairly unlikely that it would if the
library
> doesn't get loaded twice.
> 
> Is there a clean way to exclude the PDF library from PreloadPepperPlugins()
with
> a comment pointing here?

Actually it sounds like "premature optimization". Do we know that the second
load is slow?

[jln (very slow on Chromium), 2013-11-15 21:22:45]

On 2013/11/15 00:36:20, Vitaly Buka wrote:
> On 2013/11/15 00:11:21, jln wrote:
> >
>
https://codereview.chromium.org/70083006/diff/330001/chrome/app/chrome_main_d...
> > File chrome/app/chrome_main_delegate.cc (right):
> > 
> >
>
https://codereview.chromium.org/70083006/diff/330001/chrome/app/chrome_main_d...
> > chrome/app/chrome_main_delegate.cc:701:
> > chrome::ChromeContentUtilityClient::PreSandboxStartup();
> > On 2013/11/14 23:53:18, Vitaly Buka wrote:
> > > PreSandboxInit() /  
> > >  PreloadPepperPlugins()
> > > are parts of content, I can't call chrome/code from there without
> > re-factoring.
> > 
> > Ahh yeah, of course, silly me.
> > 
> > I hope that the Zygote trying to reload the same thing will not cause much
> > computation to be duplicated. It's fairly unlikely that it would if the
> library
> > doesn't get loaded twice.
> > 
> > Is there a clean way to exclude the PDF library from PreloadPepperPlugins()
> with
> > a comment pointing here?
> 
> Actually it sounds like "premature optimization". Do we know that the second
> load is slow?

Alright, as long as it's confirmed loaded once I'm not too scared. lgtm.

Please make sure to talk to the security team when evaluating doing any PDF
related things in the utility process vs. the renderer. The utility process
needs a better sandbox on Linux.

[jln (very slow on Chromium), 2013-11-15 21:29:49]

On Fri, Nov 15, 2013 at 1:22 PM, <jln@chromium.org> wrote:

> On 2013/11/15 00:36:20, Vitaly Buka wrote:
>
>> On 2013/11/15 00:11:21, jln wrote:
>> >
>>
>
> https://codereview.chromium.org/70083006/diff/330001/
> chrome/app/chrome_main_delegate.cc
>
>> > File chrome/app/chrome_main_delegate.cc (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/70083006/diff/330001/
> chrome/app/chrome_main_delegate.cc#newcode701
>
>> > chrome/app/chrome_main_delegate.cc:701:
>> > chrome::ChromeContentUtilityClient::PreSandboxStartup();
>> > On 2013/11/14 23:53:18, Vitaly Buka wrote:
>> > > PreSandboxInit() /
>> > >  PreloadPepperPlugins()
>> > > are parts of content, I can't call chrome/code from there without
>> > re-factoring.
>> >
>> > Ahh yeah, of course, silly me.
>> >
>> > I hope that the Zygote trying to reload the same thing will not cause
>> much
>> > computation to be duplicated. It's fairly unlikely that it would if the
>> library
>> > doesn't get loaded twice.
>> >
>> > Is there a clean way to exclude the PDF library from
>> PreloadPepperPlugins()
>> with
>> > a comment pointing here?
>>
>
>  Actually it sounds like "premature optimization". Do we know that the
>> second
>> load is slow?
>>
>
> Alright, as long as it's confirmed loaded once I'm not too scared. lgtm.
>
> Please make sure to talk to the security team when evaluating doing any PDF
> related things in the utility process vs. the renderer. The utility process
> needs a better sandbox on Linux.
>
> https://codereview.chromium.org/70083006/
>

Ohh, and of course, this should not start any thread or open any directory
file descriptor. This should now be DCHECK-ed, so nothing to do on your
part.

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

[Lei Zhang, 2013-11-16 00:07:52]

lgtm

[commit-bot: I haz the power, 2013-11-20 06:20:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/70083006/620001

[commit-bot: I haz the power, 2013-11-20 09:07:24]

Change committed as 236212