PepperFileRefHost: Port to use explicit permission grants in ChildProcessSecurityPolicy.

PepperFileRefHost: Port to use explicit permission grants in ChildProcessSecurityPolicy.

This updates the PepperFileRefHost classes to make use of explicit permission grants instead of base::PlatformFile bitmasks, which are being deprecated.

Depends on: https://codereview.chromium.org/19599006/.

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

[tommycli, 2013-07-18 18:45:13]

I took a shot at the changes needed to Pepper. This review is not urgent.

[vandebo (ex-Chrome), 2013-07-19 17:22:22]

LGTM

[Tom Sepez, 2013-07-19 18:39:28]

drive-by: a few nits.

https://codereview.chromium.org/19770009/diff/9001/content/browser/fileapi/br...
File content/browser/fileapi/browser_file_system_helper.cc (right):

https://codereview.chromium.org/19770009/diff/9001/content/browser/fileapi/br...
content/browser/fileapi/browser_file_system_helper.cc:120: if
(policy->HasPermissionsForFileSystemFile(process_id, url, permissions))
nit: I'd prefer inverting this check, setting error and returning, making the
success path fall through to the end of the function.  Adding additional checks
later on gets easier this way.

https://codereview.chromium.org/19770009/diff/9001/content/browser/fileapi/br...
content/browser/fileapi/browser_file_system_helper.cc:152: if
(!policy->CanReadFile(process_id, *platform_path))
Is this still the case?  Don't the permissions have to or with each other now?
Considering each one of these is a lock get, I'd like to just set it
irrespectively.

https://codereview.chromium.org/19770009/diff/9001/content/browser/fileapi/br...
File content/browser/fileapi/browser_file_system_helper.h (right):

https://codereview.chromium.org/19770009/diff/9001/content/browser/fileapi/br...
content/browser/fileapi/browser_file_system_helper.h:31: // Check whether a file
system URL is valid.
nit: what does it really mean to be valid?

https://codereview.chromium.org/19770009/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_external_file_ref_backend.cc
(right):

https://codereview.chromium.org/19770009/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_external_file_ref_backend.cc:108: 
nit: extra blank line.

https://codereview.chromium.org/19770009/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_file_ref_host.cc (right):

https://codereview.chromium.org/19770009/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_file_ref_host.cc:211: rv =
CanWrite();
Doesn't checking read/write separately have the effect of invoking file url
validtion twice under the covers?

[tommycli, 2013-07-23 21:12:35]

tsepez: Addressed your comments.

kinuko: Need owner review for content/browser/fileapi.

dmichael: Need owner review on content/browser/renderer_host/pepper. Feel free
to re-direct if there's a better reviewer for this.

https://codereview.chromium.org/19770009/diff/9001/content/browser/fileapi/br...
File content/browser/fileapi/browser_file_system_helper.cc (right):

https://codereview.chromium.org/19770009/diff/9001/content/browser/fileapi/br...
content/browser/fileapi/browser_file_system_helper.cc:120: if
(policy->HasPermissionsForFileSystemFile(process_id, url, permissions))
On 2013/07/19 18:39:28, Tom Sepez wrote:
> nit: I'd prefer inverting this check, setting error and returning, making the
> success path fall through to the end of the function.  Adding additional
checks
> later on gets easier this way.

Done.

https://codereview.chromium.org/19770009/diff/9001/content/browser/fileapi/br...
content/browser/fileapi/browser_file_system_helper.cc:152: if
(!policy->CanReadFile(process_id, *platform_path))
On 2013/07/19 18:39:28, Tom Sepez wrote:
> Is this still the case?  Don't the permissions have to or with each other now?
> Considering each one of these is a lock get, I'd like to just set it
> irrespectively.

Done. I see no harm in setting it irrespectively.

https://codereview.chromium.org/19770009/diff/9001/content/browser/fileapi/br...
File content/browser/fileapi/browser_file_system_helper.h (right):

https://codereview.chromium.org/19770009/diff/9001/content/browser/fileapi/br...
content/browser/fileapi/browser_file_system_helper.h:31: // Check whether a file
system URL is valid.
On 2013/07/19 18:39:28, Tom Sepez wrote:
> nit: what does it really mean to be valid?

Done.

https://codereview.chromium.org/19770009/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_external_file_ref_backend.cc
(right):

https://codereview.chromium.org/19770009/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_external_file_ref_backend.cc:108: 
On 2013/07/19 18:39:28, Tom Sepez wrote:
> nit: extra blank line.

Done.

https://codereview.chromium.org/19770009/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_file_ref_host.cc (right):

https://codereview.chromium.org/19770009/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_file_ref_host.cc:211: rv =
CanWrite();
On 2013/07/19 18:39:28, Tom Sepez wrote:
> Doesn't checking read/write separately have the effect of invoking file url
> validtion twice under the covers?

Done.

[kinuko, 2013-07-24 09:25:22]

https://codereview.chromium.org/19770009/diff/32001/content/browser/fileapi/b...
File content/browser/fileapi/browser_file_system_helper.cc (right):

https://codereview.chromium.org/19770009/diff/32001/content/browser/fileapi/b...
content/browser/fileapi/browser_file_system_helper.cc:149:
policy->GrantReadFile(process_id, *platform_path);
Has situation changed so that we don't need to check it's readable first?

[tommycli, 2013-07-24 14:38:00]

https://codereview.chromium.org/19770009/diff/32001/content/browser/fileapi/b...
File content/browser/fileapi/browser_file_system_helper.cc (right):

https://codereview.chromium.org/19770009/diff/32001/content/browser/fileapi/b...
content/browser/fileapi/browser_file_system_helper.cc:149:
policy->GrantReadFile(process_id, *platform_path);
On 2013/07/24 09:25:23, kinuko wrote:
> Has situation changed so that we don't need to check it's readable first?

Yes. It used to simply replace the granted permission. Now it does a bitwise OR
and adds the permission.

[kinuko, 2013-07-24 14:47:55]

On 2013/07/24 14:38:00, tommycli wrote:
>
https://codereview.chromium.org/19770009/diff/32001/content/browser/fileapi/b...
> File content/browser/fileapi/browser_file_system_helper.cc (right):
> 
>
https://codereview.chromium.org/19770009/diff/32001/content/browser/fileapi/b...
> content/browser/fileapi/browser_file_system_helper.cc:149:
> policy->GrantReadFile(process_id, *platform_path);
> On 2013/07/24 09:25:23, kinuko wrote:
> > Has situation changed so that we don't need to check it's readable first?
> 
> Yes. It used to simply replace the granted permission. Now it does a bitwise
OR
> and adds the permission.

Doesn't it still override existing permission if, say, the process has
read/write permission for its parent directory?

[tommycli, 2013-07-24 14:52:48]

On 2013/07/24 14:47:55, kinuko wrote:
> On 2013/07/24 14:38:00, tommycli wrote:
> >
>
https://codereview.chromium.org/19770009/diff/32001/content/browser/fileapi/b...
> > File content/browser/fileapi/browser_file_system_helper.cc (right):
> > 
> >
>
https://codereview.chromium.org/19770009/diff/32001/content/browser/fileapi/b...
> > content/browser/fileapi/browser_file_system_helper.cc:149:
> > policy->GrantReadFile(process_id, *platform_path);
> > On 2013/07/24 09:25:23, kinuko wrote:
> > > Has situation changed so that we don't need to check it's readable first?
> > 
> > Yes. It used to simply replace the granted permission. Now it does a bitwise
> OR
> > and adds the permission.
> 
> Doesn't it still override existing permission if, say, the process has
> read/write permission for its parent directory?

I don't think so. It calls down into
ChildProcessSecurityPolicyImpl::SecurityState::GrantPermissionsForFile which
looks like: 

void GrantPermissionsForFile(const base::FilePath& file, int permissions) {
    base::FilePath stripped = file.StripTrailingSeparators();
    file_permissions_[stripped] |= permissions;
    UMA_HISTOGRAM_COUNTS("ChildProcessSecurityPolicy.FilePermissionPathLength",
                         stripped.value().size());
  }

[kinuko, 2013-07-24 15:00:42]

On 2013/07/24 14:52:48, tommycli wrote:
> On 2013/07/24 14:47:55, kinuko wrote:
> > On 2013/07/24 14:38:00, tommycli wrote:
> > >
> >
>
https://codereview.chromium.org/19770009/diff/32001/content/browser/fileapi/b...
> > > File content/browser/fileapi/browser_file_system_helper.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/19770009/diff/32001/content/browser/fileapi/b...
> > > content/browser/fileapi/browser_file_system_helper.cc:149:
> > > policy->GrantReadFile(process_id, *platform_path);
> > > On 2013/07/24 09:25:23, kinuko wrote:
> > > > Has situation changed so that we don't need to check it's readable
first?
> > > 
> > > Yes. It used to simply replace the granted permission. Now it does a
bitwise
> > OR
> > > and adds the permission.
> > 
> > Doesn't it still override existing permission if, say, the process has
> > read/write permission for its parent directory?
> 
> I don't think so. It calls down into
> ChildProcessSecurityPolicyImpl::SecurityState::GrantPermissionsForFile which
> looks like: 
> 
> void GrantPermissionsForFile(const base::FilePath& file, int permissions) {
>     base::FilePath stripped = file.StripTrailingSeparators();
>     file_permissions_[stripped] |= permissions;
>    
UMA_HISTOGRAM_COUNTS("ChildProcessSecurityPolicy.FilePermissionPathLength",
>                          stripped.value().size());
>   }

I'm worrying about checking side.  Afair CPSP checks file permission from leaf
to root, i.e. if it doesn't find permission on the target path it checks its
parent's permission.  So if a process has read/write permission for a folder it
means it has the read/write permission for all its children.  But if we set read
permission on a specific file in the folder the process lose write permission on
it.  Or has there been a change in the checking code too?

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ch...

[tommycli, 2013-07-24 15:09:35]

On 2013/07/24 15:00:42, kinuko wrote:
> On 2013/07/24 14:52:48, tommycli wrote:
> > On 2013/07/24 14:47:55, kinuko wrote:
> > > On 2013/07/24 14:38:00, tommycli wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/19770009/diff/32001/content/browser/fileapi/b...
> > > > File content/browser/fileapi/browser_file_system_helper.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/19770009/diff/32001/content/browser/fileapi/b...
> > > > content/browser/fileapi/browser_file_system_helper.cc:149:
> > > > policy->GrantReadFile(process_id, *platform_path);
> > > > On 2013/07/24 09:25:23, kinuko wrote:
> > > > > Has situation changed so that we don't need to check it's readable
> first?
> > > > 
> > > > Yes. It used to simply replace the granted permission. Now it does a
> bitwise
> > > OR
> > > > and adds the permission.
> > > 
> > > Doesn't it still override existing permission if, say, the process has
> > > read/write permission for its parent directory?
> > 
> > I don't think so. It calls down into
> > ChildProcessSecurityPolicyImpl::SecurityState::GrantPermissionsForFile which
> > looks like: 
> > 
> > void GrantPermissionsForFile(const base::FilePath& file, int permissions) {
> >     base::FilePath stripped = file.StripTrailingSeparators();
> >     file_permissions_[stripped] |= permissions;
> >    
> UMA_HISTOGRAM_COUNTS("ChildProcessSecurityPolicy.FilePermissionPathLength",
> >                          stripped.value().size());
> >   }
> 
> I'm worrying about checking side.  Afair CPSP checks file permission from leaf
> to root, i.e. if it doesn't find permission on the target path it checks its
> parent's permission.  So if a process has read/write permission for a folder
it
> means it has the read/write permission for all its children.  But if we set
read
> permission on a specific file in the folder the process lose write permission
on
> it.  Or has there been a change in the checking code too?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ch...

kinkuo: You are correct. Removing the check does change the behavior when
permission has already been granted to a parent directory. I added the check
back.

tsepez: You can comment again if you still think we don't need the check.

[kinuko, 2013-07-24 15:24:10]

fileapi lgtm

[Tom Sepez, 2013-07-24 16:51:13]

LGTM with the check back in.

[dmichael (off chromium), 2013-07-24 17:27:43]

+teravest, who's really familiar with this.

Owners' lgtm for content/browser/renderer_host/pepper, but please wait for
teravest@'s approval.

https://codereview.chromium.org/19770009/diff/40001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_internal_file_ref_backend.cc
(right):

https://codereview.chromium.org/19770009/diff/40001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_internal_file_ref_backend.cc:263:
return PP_ERROR_FAILED;
teravest: Do you know what we would do in the case of a bad FileSystemURL today?
I just want to make sure we're doing the same thing. I'm guessing this doesn't
really happen in practice anyway.

[teravest, 2013-07-24 17:51:20]

https://codereview.chromium.org/19770009/diff/40001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_external_file_ref_backend.cc
(right):

https://codereview.chromium.org/19770009/diff/40001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_external_file_ref_backend.cc:135: if
(!policy->CanReadFile(render_process_id_, path_) ||
Shouldn't this be &&?
It seems to me that the caller should just do CanRead() && CanWrite() instead of
adding this.

https://codereview.chromium.org/19770009/diff/40001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_internal_file_ref_backend.cc
(right):

https://codereview.chromium.org/19770009/diff/40001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_internal_file_ref_backend.cc:263:
return PP_ERROR_FAILED;
On 2013/07/24 17:27:44, dmichael wrote:
> teravest: Do you know what we would do in the case of a bad FileSystemURL
today?
> I just want to make sure we're doing the same thing. I'm guessing this doesn't
> really happen in practice anyway.

I'll look into this; the old code would just pass the bad URL along to the
fileapi layer (as far as I can tell), but I don't see how any operations would
succeed when we don't have a sane url.

[tommycli, 2013-07-24 18:05:29]

https://codereview.chromium.org/19770009/diff/40001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_external_file_ref_backend.cc
(right):

https://codereview.chromium.org/19770009/diff/40001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_external_file_ref_backend.cc:135: if
(!policy->CanReadFile(render_process_id_, path_) ||
On 2013/07/24 17:51:21, teravest wrote:
> Shouldn't this be &&?
> It seems to me that the caller should just do CanRead() && CanWrite() instead
of
> adding this.

If either read or write is not allowed, it should fail.

Originally it was CanRead() && CanWrite(), but in the
PepperInternalFileRefBackend, that would cause the FileSystemURL validation to
be invoked twice, which another reviewer found objectionable. See patchset 4
PepperFileRefHost.

https://codereview.chromium.org/19770009/diff/40001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_internal_file_ref_backend.cc
(right):

https://codereview.chromium.org/19770009/diff/40001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_internal_file_ref_backend.cc:263:
return PP_ERROR_FAILED;
On 2013/07/24 17:51:21, teravest wrote:
> On 2013/07/24 17:27:44, dmichael wrote:
> > teravest: Do you know what we would do in the case of a bad FileSystemURL
> today?
> > I just want to make sure we're doing the same thing. I'm guessing this
doesn't
> > really happen in practice anyway.
> 
> I'll look into this; the old code would just pass the bad URL along to the
> fileapi layer (as far as I can tell), but I don't see how any operations would
> succeed when we don't have a sane url.

CheckFileSystemPermissionsForProcess would return a
base::PLATFORM_FILE_ERROR_INVALID_URL, which
ppapi::PlatformFileErrorToPepperError would convert to PP_ERROR_FAILED. So I
think I duplicated the original behavior for invalid URLs.

[teravest, 2013-07-24 19:16:55]

lgtm

On Wed, Jul 24, 2013 at 12:05 PM,  <tommycli@chromium.org> wrote:
>
>
https://codereview.chromium.org/19770009/diff/40001/content/browser/renderer_...
> File
> content/browser/renderer_host/pepper/pepper_external_file_ref_backend.cc
> (right):
>
>
https://codereview.chromium.org/19770009/diff/40001/content/browser/renderer_...
> content/browser/renderer_host/pepper/pepper_external_file_ref_backend.cc:135:
> if (!policy->CanReadFile(render_process_id_, path_) ||
> On 2013/07/24 17:51:21, teravest wrote:
>>
>> Shouldn't this be &&?
>> It seems to me that the caller should just do CanRead() && CanWrite()
>
> instead of
>>
>> adding this.
>
>
> If either read or write is not allowed, it should fail.
>
> Originally it was CanRead() && CanWrite(), but in the
> PepperInternalFileRefBackend, that would cause the FileSystemURL
> validation to be invoked twice, which another reviewer found
> objectionable. See patchset 4 PepperFileRefHost.

I missed the not operators. Oops.
Thanks for the explanation.
It seems a little weird to provide these convenience functions at all
(everywhere that does permissions checking could just do the URL
validation explicitly)

>
>
>
https://codereview.chromium.org/19770009/diff/40001/content/browser/renderer_...
> File
> content/browser/renderer_host/pepper/pepper_internal_file_ref_backend.cc
> (right):
>
>
https://codereview.chromium.org/19770009/diff/40001/content/browser/renderer_...
> content/browser/renderer_host/pepper/pepper_internal_file_ref_backend.cc:263:
> return PP_ERROR_FAILED;
> On 2013/07/24 17:51:21, teravest wrote:
>>
>> On 2013/07/24 17:27:44, dmichael wrote:
>> > teravest: Do you know what we would do in the case of a bad
>
> FileSystemURL
>>
>> today?
>> > I just want to make sure we're doing the same thing. I'm guessing
>
> this doesn't
>>
>> > really happen in practice anyway.
>
>
>> I'll look into this; the old code would just pass the bad URL along to
>
> the
>>
>> fileapi layer (as far as I can tell), but I don't see how any
>
> operations would
>>
>> succeed when we don't have a sane url.
>
>
> CheckFileSystemPermissionsForProcess would return a
> base::PLATFORM_FILE_ERROR_INVALID_URL, which
> ppapi::PlatformFileErrorToPepperError would convert to PP_ERROR_FAILED.
> So I think I duplicated the original behavior for invalid URLs.
>
> https://codereview.chromium.org/19770009/

[tommycli, 2013-07-24 20:53:08]

thanks.

https://codereview.chromium.org/19770009/diff/40001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_internal_file_ref_backend.cc
(right):

https://codereview.chromium.org/19770009/diff/40001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_internal_file_ref_backend.cc:263:
return PP_ERROR_FAILED;
On 2013/07/24 18:05:30, tommycli wrote:
> On 2013/07/24 17:51:21, teravest wrote:
> > On 2013/07/24 17:27:44, dmichael wrote:
> > > teravest: Do you know what we would do in the case of a bad FileSystemURL
> > today?
> > > I just want to make sure we're doing the same thing. I'm guessing this
> doesn't
> > > really happen in practice anyway.
> > 
> > I'll look into this; the old code would just pass the bad URL along to the
> > fileapi layer (as far as I can tell), but I don't see how any operations
would
> > succeed when we don't have a sane url.
> 
> CheckFileSystemPermissionsForProcess would return a
> base::PLATFORM_FILE_ERROR_INVALID_URL, which
> ppapi::PlatformFileErrorToPepperError would convert to PP_ERROR_FAILED. So I
> think I duplicated the original behavior for invalid URLs.

Yeah it's kind of weird. Probably because You have a PepperFileRefHost that
calls into a backend that implements CanRead* etc. via virtual functions - and
Only PepperInternalFileRefBackend does the URL validation.

Agreed though - not 100% happy with design... more like 70% happy.

[commit-bot: I haz the power, 2013-07-24 21:54:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/19770009/40001

[commit-bot: I haz the power, 2013-07-25 13:22:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/19770009/40001

[commit-bot: I haz the power, 2013-07-25 13:22:34]

Failed to apply patch for content/browser/fileapi/browser_file_system_helper.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/fileapi/browser_file_system_helper.cc
  Hunk #1 FAILED at 92.
  Hunk #2 succeeded at 129 (offset -2 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
content/browser/fileapi/browser_file_system_helper.cc.rej

Patch:       content/browser/fileapi/browser_file_system_helper.cc
Index: content/browser/fileapi/browser_file_system_helper.cc
diff --git a/content/browser/fileapi/browser_file_system_helper.cc
b/content/browser/fileapi/browser_file_system_helper.cc
index
e8e81ce12362aa51871e5bc12a3da4d6705d8200..6502d5044809d5b4090982d9e30196e796ea04c0
100644
--- a/content/browser/fileapi/browser_file_system_helper.cc
+++ b/content/browser/fileapi/browser_file_system_helper.cc
@@ -92,34 +92,34 @@ scoped_refptr<fileapi::FileSystemContext>
CreateFileSystemContext(
   return file_system_context;
 }
 
+bool FileSystemURLIsValid(
+    fileapi::FileSystemContext* context,
+    const fileapi::FileSystemURL& url) {
+  if (!url.is_valid())
+    return false;
+
+  return context->GetFileSystemBackend(url.type()) != NULL;
+}
+
 bool CheckFileSystemPermissionsForProcess(
     fileapi::FileSystemContext* context, int process_id,
     const fileapi::FileSystemURL& url, int permissions,
     base::PlatformFileError* error) {
   DCHECK(error);
-  *error = base::PLATFORM_FILE_OK;
 
-  if (!url.is_valid()) {
+  if (!FileSystemURLIsValid(context, url)) {
     *error = base::PLATFORM_FILE_ERROR_INVALID_URL;
     return false;
   }
 
-  fileapi::FileSystemBackend* mount_point_provider =
-      context->GetFileSystemBackend(url.type());
-  if (!mount_point_provider) {
-    *error = base::PLATFORM_FILE_ERROR_INVALID_URL;
+  if (!ChildProcessSecurityPolicyImpl::GetInstance()->
+          HasPermissionsForFileSystemFile(process_id, url, permissions)) {
+    *error = base::PLATFORM_FILE_ERROR_SECURITY;
     return false;
   }
 
-  base::FilePath file_path;
-  ChildProcessSecurityPolicyImpl* policy =
-      ChildProcessSecurityPolicyImpl::GetInstance();
-
-  if (policy->HasPermissionsForFileSystemFile(process_id, url, permissions))
-    return true;
-
-  *error = base::PLATFORM_FILE_ERROR_SECURITY;
-  return false;
+  *error = base::PLATFORM_FILE_OK;
+  return true;
 }
 
 void SyncGetPlatformPath(fileapi::FileSystemContext* context,
@@ -131,27 +131,24 @@ void SyncGetPlatformPath(fileapi::FileSystemContext*
context,
   DCHECK(platform_path);
   *platform_path = base::FilePath();
   fileapi::FileSystemURL url(context->CrackURL(path));
-  if (!url.is_valid())
+  if (!FileSystemURLIsValid(context, url))
     return;
 
   // Make sure if this file is ok to be read (in the current architecture
   // which means roughly same as the renderer is allowed to get the platform
   // path to the file).
-  base::PlatformFileError error;
-  if (!CheckFileSystemPermissionsForProcess(
-      context, process_id, url, fileapi::kReadFilePermissions, &error))
+  ChildProcessSecurityPolicyImpl* policy =
+      ChildProcessSecurityPolicyImpl::GetInstance();
+  if (!policy->CanReadFileSystemFile(process_id, url))
     return;
 
   context->operation_runner()->SyncGetPlatformPath(url, platform_path);
 
   // The path is to be attached to URLLoader so we grant read permission
-  // for the file. (We first need to check if it can already be read not to
-  // overwrite existing permissions)
-  if (!ChildProcessSecurityPolicyImpl::GetInstance()->CanReadFile(
-          process_id, *platform_path)) {
-    ChildProcessSecurityPolicyImpl::GetInstance()->GrantReadFile(
-        process_id, *platform_path);
-  }
+  // for the file. (We need to check first because a parent directory may
+  // already have the permissions and we don't need to grant it to the file.)
+  if (!policy->CanReadFile(process_id, *platform_path))
+    policy->GrantReadFile(process_id, *platform_path);
 }
 
 }  // namespace content

[commit-bot: I haz the power, 2013-07-25 16:34:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/19770009/64001

[commit-bot: I haz the power, 2013-07-26 01:08:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/19770009/64001

[commit-bot: I haz the power, 2013-07-26 10:29:42]

Change committed as 213823