Flag and whitelist to allow crxfs api in NaCl

Flag and whitelist to allow crxfs api in NaCl

This is pretty much the same as existing Pepper Socket API.  Since the
original code for socket is in chrome_content_browser_client.h for
crossing chrome-content boundary, it's moved to the new file
pepper_util.cc, so that PepperCrxFileSystemMessageFilter can access
directory inside chrome space.

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

[victorhsieh, 2013-05-21 17:47:48]

This is not urgent, but would be nice if you can look at this when you have
time.  Some of our work is blocked by this.

[yzshen1, 2013-05-22 18:14:23]

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/pepper_util.cc
File chrome/browser/pepper_util.cc (right):

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/pepper_util...
chrome/browser/pepper_util.cc:79: // Need to check this now and not on
construction because otherwise it won't
This comment seems confusing now. Please revise or remove it.

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/pepper_util.h
File chrome/browser/pepper_util.h (right):

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/pepper_util...
chrome/browser/pepper_util.h:5: #ifndef CHROME_BROWSER_PEPPER_UTIL_H_
nit: maybe a more specific name such as pepper_permission_util.h?

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/pepper_util...
chrome/browser/pepper_util.h:20: std::set<std::string> whitelist,
const &, please.

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/pepper_util...
chrome/browser/pepper_util.h:21: const char command_line_switch[]);
It seems better to use const char*.

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/renderer_ho...
File
chrome/browser/renderer_host/pepper/pepper_crx_file_system_message_filter.cc
(right):

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_crx_file_system_message_filter.cc:65:

nit: unnecessary empty line.

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_crx_file_system_message_filter.cc:125:
content::SiteInstance* site_instance = render_view_host->GetSiteInstance();
Why do we need site_instance here?

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_crx_file_system_message_filter.cc:133:
<< " cannot use CrxFs API or destination is not allowed";
What is the meaning of 'destination' here?

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_crx_file_system_message_filter.cc:142:
return PP_ERROR_NOTSUPPORTED;
(1) Is it better to use PP_ERROR_NOACCESS for this and below?

(2) I think this scheme check is also performed by
IsExtensionOrSharedModuleWhitelisted(), maybe we could remove it?

https://codereview.chromium.org/15521002/diff/2001/chrome/common/chrome_switc...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/15521002/diff/2001/chrome/common/chrome_switc...
chrome/common/chrome_switches.cc:52: const char kAllowNaClCrxFsAPI[]            
= "allow-nacl-crxfs-api";
alphabetically, please.

https://codereview.chromium.org/15521002/diff/2001/chrome/common/chrome_switc...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/15521002/diff/2001/chrome/common/chrome_switc...
chrome/common/chrome_switches.h:34: extern const char kAllowNaClCrxFsAPI[];
nit: alphabetically, please.

[victorhsieh, 2013-05-22 19:47:32]

PTAL

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/pepper_util.cc
File chrome/browser/pepper_util.cc (right):

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/pepper_util...
chrome/browser/pepper_util.cc:79: // Need to check this now and not on
construction because otherwise it won't
On 2013/05/22 18:14:23, yzshen1 wrote:
> This comment seems confusing now. Please revise or remove it.

It was moved from constructor of original code.  I think the comment is not
useful now.
https://chromiumcodereview.appspot.com/9646027/diff/8029/chrome/browser/chrom...

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/pepper_util.h
File chrome/browser/pepper_util.h (right):

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/pepper_util...
chrome/browser/pepper_util.h:5: #ifndef CHROME_BROWSER_PEPPER_UTIL_H_
On 2013/05/22 18:14:23, yzshen1 wrote:
> nit: maybe a more specific name such as pepper_permission_util.h?

Sometimes I saw this kind of util file ends up with only a few function in it. 
And sometimes there are several small util files in one directory.  I'm not sure
this is good, so I chose a broad name and thought we could split later if the
file grows bigger.

But it's up to you.  Having a specific filename can be good as the future reader
won't have to open the file to know what it is about.

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/pepper_util...
chrome/browser/pepper_util.h:20: std::set<std::string> whitelist,
On 2013/05/22 18:14:23, yzshen1 wrote:
> const &, please.

Done.

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/pepper_util...
chrome/browser/pepper_util.h:21: const char command_line_switch[]);
On 2013/05/22 18:14:23, yzshen1 wrote:
> It seems better to use const char*.

Done.

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/renderer_ho...
File
chrome/browser/renderer_host/pepper/pepper_crx_file_system_message_filter.cc
(right):

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_crx_file_system_message_filter.cc:65:

On 2013/05/22 18:14:23, yzshen1 wrote:
> nit: unnecessary empty line.

Done.

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_crx_file_system_message_filter.cc:125:
content::SiteInstance* site_instance = render_view_host->GetSiteInstance();
On 2013/05/22 18:14:23, yzshen1 wrote:
> Why do we need site_instance here?

Done.

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_crx_file_system_message_filter.cc:133:
<< " cannot use CrxFs API or destination is not allowed";
On 2013/05/22 18:14:23, yzshen1 wrote:
> What is the meaning of 'destination' here?

Revised.  I thought it's about origin when copying the message from
pepper_socket_utils.cc, but it's not.

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_crx_file_system_message_filter.cc:142:
return PP_ERROR_NOTSUPPORTED;
On 2013/05/22 18:14:23, yzshen1 wrote:
> (1) Is it better to use PP_ERROR_NOACCESS for this and below?
> 
> (2) I think this scheme check is also performed by
> IsExtensionOrSharedModuleWhitelisted(), maybe we could remove it?

Removed.

https://codereview.chromium.org/15521002/diff/2001/chrome/common/chrome_switc...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/15521002/diff/2001/chrome/common/chrome_switc...
chrome/common/chrome_switches.cc:52: const char kAllowNaClCrxFsAPI[]            
= "allow-nacl-crxfs-api";
On 2013/05/22 18:14:23, yzshen1 wrote:
> alphabetically, please.

Done.

https://codereview.chromium.org/15521002/diff/2001/chrome/common/chrome_switc...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/15521002/diff/2001/chrome/common/chrome_switc...
chrome/common/chrome_switches.h:34: extern const char kAllowNaClCrxFsAPI[];
On 2013/05/22 18:14:23, yzshen1 wrote:
> nit: alphabetically, please.

Done.

[yzshen1, 2013-05-22 20:12:47]

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/pepper_util.h
File chrome/browser/pepper_util.h (right):

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/pepper_util...
chrome/browser/pepper_util.h:5: #ifndef CHROME_BROWSER_PEPPER_UTIL_H_
I think it is better to have a more specific name for now.
It is easier to understand what the file is about.


On 2013/05/22 19:47:33, victorhsieh wrote:
> On 2013/05/22 18:14:23, yzshen1 wrote:
> > nit: maybe a more specific name such as pepper_permission_util.h?
> 
> Sometimes I saw this kind of util file ends up with only a few function in it.

> And sometimes there are several small util files in one directory.  I'm not
sure
> this is good, so I chose a broad name and thought we could split later if the
> file grows bigger.
> 
> But it's up to you.  Having a specific filename can be good as the future
reader
> won't have to open the file to know what it is about.

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/renderer_ho...
File
chrome/browser/renderer_host/pepper/pepper_crx_file_system_message_filter.cc
(right):

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_crx_file_system_message_filter.cc:125:
content::SiteInstance* site_instance = render_view_host->GetSiteInstance();
Now we can remove render_view_id_ entirely.

On 2013/05/22 19:47:33, victorhsieh wrote:
> On 2013/05/22 18:14:23, yzshen1 wrote:
> > Why do we need site_instance here?
> 
> Done.

[victorhsieh, 2013-05-22 20:52:22]

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/pepper_util.h
File chrome/browser/pepper_util.h (right):

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/pepper_util...
chrome/browser/pepper_util.h:5: #ifndef CHROME_BROWSER_PEPPER_UTIL_H_
On 2013/05/22 20:12:47, yzshen1 wrote:
> I think it is better to have a more specific name for now.
> It is easier to understand what the file is about.
> 
> 
> On 2013/05/22 19:47:33, victorhsieh wrote:
> > On 2013/05/22 18:14:23, yzshen1 wrote:
> > > nit: maybe a more specific name such as pepper_permission_util.h?
> > 
> > Sometimes I saw this kind of util file ends up with only a few function in
it.
> 
> > And sometimes there are several small util files in one directory.  I'm not
> sure
> > this is good, so I chose a broad name and thought we could split later if
the
> > file grows bigger.
> > 
> > But it's up to you.  Having a specific filename can be good as the future
> reader
> > won't have to open the file to know what it is about.
> 

Done.

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/renderer_ho...
File
chrome/browser/renderer_host/pepper/pepper_crx_file_system_message_filter.cc
(right):

https://codereview.chromium.org/15521002/diff/2001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_crx_file_system_message_filter.cc:125:
content::SiteInstance* site_instance = render_view_host->GetSiteInstance();
On 2013/05/22 20:12:47, yzshen1 wrote:
> Now we can remove render_view_id_ entirely.
> 
> On 2013/05/22 19:47:33, victorhsieh wrote:
> > On 2013/05/22 18:14:23, yzshen1 wrote:
> > > Why do we need site_instance here?
> > 
> > Done.
> 

Done.

[yzshen1, 2013-05-23 16:31:47]

lgtm

[sky, 2013-05-23 21:14:07]

LGTM

https://codereview.chromium.org/15521002/diff/39001/chrome/browser/pepper_per...
File chrome/browser/pepper_permission_util.cc (right):

https://codereview.chromium.org/15521002/diff/39001/chrome/browser/pepper_per...
chrome/browser/pepper_permission_util.cc:31: DCHECK(id_hash.length() ==
base::kSHA1Length);
DCHECK_EQ

https://codereview.chromium.org/15521002/diff/39001/chrome/browser/pepper_per...
chrome/browser/pepper_permission_util.cc:49: std::string host = url.host();
nit: const (since you seem to be using const in this file, such as lines 30 and
66).

https://codereview.chromium.org/15521002/diff/39001/chrome/browser/pepper_per...
chrome/browser/pepper_permission_util.cc:68:
std::vector<extensions::SharedModuleInfo::ImportInfo>::const_iterator it;
nit: move into for loopl

https://codereview.chromium.org/15521002/diff/39001/chrome/browser/pepper_per...
chrome/browser/pepper_permission_util.cc:81: std::string allowed_list =
nit: const

https://codereview.chromium.org/15521002/diff/39001/chrome/browser/pepper_per...
chrome/browser/pepper_permission_util.cc:88: } else if (!allowed_list.empty()) {
nit: no else (see style guide).

[victorhsieh, 2013-05-23 21:39:45]

Thanks!

https://codereview.chromium.org/15521002/diff/39001/chrome/browser/pepper_per...
File chrome/browser/pepper_permission_util.cc (right):

https://codereview.chromium.org/15521002/diff/39001/chrome/browser/pepper_per...
chrome/browser/pepper_permission_util.cc:31: DCHECK(id_hash.length() ==
base::kSHA1Length);
On 2013/05/23 21:14:07, sky wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/15521002/diff/39001/chrome/browser/pepper_per...
chrome/browser/pepper_permission_util.cc:49: std::string host = url.host();
On 2013/05/23 21:14:07, sky wrote:
> nit: const (since you seem to be using const in this file, such as lines 30
and
> 66).

Done.

https://codereview.chromium.org/15521002/diff/39001/chrome/browser/pepper_per...
chrome/browser/pepper_permission_util.cc:68:
std::vector<extensions::SharedModuleInfo::ImportInfo>::const_iterator it;
On 2013/05/23 21:14:07, sky wrote:
> nit: move into for loopl

Too long.  I wish I can use auto :/

https://codereview.chromium.org/15521002/diff/39001/chrome/browser/pepper_per...
chrome/browser/pepper_permission_util.cc:81: std::string allowed_list =
On 2013/05/23 21:14:07, sky wrote:
> nit: const

Done.

https://codereview.chromium.org/15521002/diff/39001/chrome/browser/pepper_per...
chrome/browser/pepper_permission_util.cc:88: } else if (!allowed_list.empty()) {
On 2013/05/23 21:14:07, sky wrote:
> nit: no else (see style guide).

Done.

[commit-bot: I haz the power, 2013-05-23 21:41:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/15521002/45001

[commit-bot: I haz the power, 2013-05-23 22:30:29]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[victorhsieh, 2013-05-23 22:55:59]

@sky, I think it makes sense to add #ifdef here, but would like to confirm with
you if this is right thing to do.

So I got some link error on android.  It looks link "pepper_*" is excluded when
enable_plugins==0.  I'm adding some "#if defined(ENABLE_PLUGINS)" in
chrome_content_browser_client.cc.  I guess the code could have unknown behavior
before.

[commit-bot: I haz the power, 2013-05-29 16:52:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/15521002/61001

[commit-bot: I haz the power, 2013-05-29 17:24:59]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-05-29 17:25:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/15521002/61001

[commit-bot: I haz the power, 2013-05-29 17:53:53]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-05-29 19:12:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/15521002/61001

[commit-bot: I haz the power, 2013-05-29 20:29:41]

Change committed as 202951