Allow RequestOSFileHandle if an app has unlimited storage

Allow RequestOSFileHandle if an app has unlimited storage

When FileIO::Open is called and renderer gets file handle from process,
the IPC also passes the info of quota limit.

BUG=224123, 224753
TEST=browser_tests, trybots

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

[kinuko, 2013-04-04 11:01:41]

https://codereview.chromium.org/13508005/diff/1/content/renderer/pepper/peppe...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/13508005/diff/1/content/renderer/pepper/peppe...
content/renderer/pepper/pepper_file_io_host.cc:541: //quota_limit_type_ =
quota_limit_type;
need to comment this in?

https://codereview.chromium.org/13508005/diff/1/webkit/quota/quota_types.h
File webkit/quota/quota_types.h (right):

https://codereview.chromium.org/13508005/diff/1/webkit/quota/quota_types.h#ne...
webkit/quota/quota_types.h:32: kQuotaLimitTypeUnlimited,
I think I suggested an enum like this, but on the second thought 'Unlimited'
quota may not be enough to consider we can expose fd to plugin, as for arbitrary
plugins we probably still want to cap quota lower than the actual available disk
space (as well as we do so for arbitrary webapps).

We're planning to add yet another flag which gives stronger quota permission to
bypass all quota check (http://crbug.com/220029), and I think we should allow
returning fd only for apps with the privilege. For short-term can we rename this
enum like following:

enum QuotaPolicy {
  kQuotaPolicyUnknown,
  kQuotaPolicyCheckRequired,
  kQuotaPolicyCanBypassCheck,
};

and return 'CanBypassCheck' for IsStorageUnlimited() apps for now but with a
TODO comment? As we chatted offline probably later we'll need revisit this to
make it a bit stricter.

[hamaji, 2013-04-04 11:50:12]

Thanks for the quick review (and local discussion)!

https://codereview.chromium.org/13508005/diff/1/content/renderer/pepper/peppe...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/13508005/diff/1/content/renderer/pepper/peppe...
content/renderer/pepper/pepper_file_io_host.cc:541: //quota_limit_type_ =
quota_limit_type;
On 2013/04/04 11:01:42, kinuko wrote:
> need to comment this in?

Wow :) Done.

https://codereview.chromium.org/13508005/diff/1/webkit/quota/quota_types.h
File webkit/quota/quota_types.h (right):

https://codereview.chromium.org/13508005/diff/1/webkit/quota/quota_types.h#ne...
webkit/quota/quota_types.h:32: kQuotaLimitTypeUnlimited,
On 2013/04/04 11:01:42, kinuko wrote:
> I think I suggested an enum like this, but on the second thought 'Unlimited'
> quota may not be enough to consider we can expose fd to plugin, as for
arbitrary
> plugins we probably still want to cap quota lower than the actual available
disk
> space (as well as we do so for arbitrary webapps).
> 
> We're planning to add yet another flag which gives stronger quota permission
to
> bypass all quota check (http://crbug.com/220029), and I think we should allow
> returning fd only for apps with the privilege. For short-term can we rename
this
> enum like following:
> 
> enum QuotaPolicy {
>   kQuotaPolicyUnknown,
>   kQuotaPolicyCheckRequired,
>   kQuotaPolicyCanBypassCheck,
> };
> 
> and return 'CanBypassCheck' for IsStorageUnlimited() apps for now but with a
> TODO comment? As we chatted offline probably later we'll need revisit this to
> make it a bit stricter.

Done.

[dmichael (off chromium), 2013-04-04 17:25:08]

https://chromiumcodereview.appspot.com/13508005/diff/5002/chrome/test/ppapi/p...
File chrome/test/ppapi/ppapi_test.cc (left):

https://chromiumcodereview.appspot.com/13508005/diff/5002/chrome/test/ppapi/p...
chrome/test/ppapi/ppapi_test.cc:137: "127.0.0.1");
Are you planning to remove that flag in a followup CL?

https://chromiumcodereview.appspot.com/13508005/diff/5002/content/browser/fil...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://chromiumcodereview.appspot.com/13508005/diff/5002/content/browser/fil...
content/browser/fileapi/fileapi_message_filter.cc:689: base::ProcessHandle
peer_handle) {
nit: Why did you move these? If they still fit, it seems like local style might
be to keep them aligned after the paren (see DidWrite below)

https://chromiumcodereview.appspot.com/13508005/diff/5002/content/browser/fil...
content/browser/fileapi/fileapi_message_filter.cc:700: if (quota_manager_proxy)
{
It looks like if there is no quota_manager_proxy, you'll succeed but provide
kQuotaPolicyUnknown. Is that what you want? I don't know this area, but I would
have guessed you'd send DidFail instead.

[yzshen1, 2013-04-04 18:22:57]

LGTM

Thanks!

[hamaji, 2013-04-04 22:27:22]

https://codereview.chromium.org/13508005/diff/5002/chrome/test/ppapi/ppapi_te...
File chrome/test/ppapi/ppapi_test.cc (left):

https://codereview.chromium.org/13508005/diff/5002/chrome/test/ppapi/ppapi_te...
chrome/test/ppapi/ppapi_test.cc:137: "127.0.0.1");
Yes, I think I'll remove that flag by the end of April. Please let me keep that
flag until we confirm quota check is sufficient for our use. I wrote this in a
comment of content/renderer/pepper/pepper_file_io_host.cc and filed a bug:
http://crbug.com/226386

On 2013/04/04 17:25:09, dmichael wrote:
> Are you planning to remove that flag in a followup CL?

https://codereview.chromium.org/13508005/diff/5002/content/browser/fileapi/fi...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/13508005/diff/5002/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.cc:689: base::ProcessHandle
peer_handle) {
Oops, I modified here because I was adding one more argument in the early
version of this patch. Fixed.

On 2013/04/04 17:25:09, dmichael wrote:
> nit: Why did you move these? If they still fit, it seems like local style
might
> be to keep them aligned after the paren (see DidWrite below)

https://codereview.chromium.org/13508005/diff/5002/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.cc:700: if (quota_manager_proxy)
{
webkit/fileapi/local_file_system_operation.cc seems to allow
!quota_manager_proxy case. Its comment says:

// If we don't have the quota manager or the requested filesystem type
// does not support quota, we should be able to let it go.                    

Kinuko-san, could you check if this code is OK?

On 2013/04/04 17:25:09, dmichael wrote:
> It looks like if there is no quota_manager_proxy, you'll succeed but provide
> kQuotaPolicyUnknown. Is that what you want? I don't know this area, but I
would
> have guessed you'd send DidFail instead.

[kinuko, 2013-04-05 02:37:11]

https://codereview.chromium.org/13508005/diff/5002/content/browser/fileapi/fi...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/13508005/diff/5002/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.cc:700: if (quota_manager_proxy)
{
On 2013/04/04 22:27:22, hamaji wrote:
> webkit/fileapi/local_file_system_operation.cc seems to allow
> !quota_manager_proxy case. Its comment says:
> 
> // If we don't have the quota manager or the requested filesystem type
> // does not support quota, we should be able to let it go.                    
> 
> Kinuko-san, could you check if this code is OK?
> 
> On 2013/04/04 17:25:09, dmichael wrote:
> > It looks like if there is no quota_manager_proxy, you'll succeed but provide
> > kQuotaPolicyUnknown. Is that what you want? I don't know this area, but I
> would
> > have guessed you'd send DidFail instead.

Actually in the content/ layer I think we must always have non-null
quota_manager_proxy, so I guess we can just have DCHECK(!quota_manager_proxy)
here.   (Trybots will tell if I'm wrong...)

[hamaji, 2013-04-05 03:33:18]

https://codereview.chromium.org/13508005/diff/5002/content/browser/fileapi/fi...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/13508005/diff/5002/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.cc:700: if (quota_manager_proxy)
{
I see, thanks! I added DCHECK and removed this if.

On 2013/04/05 02:37:11, kinuko wrote:
> On 2013/04/04 22:27:22, hamaji wrote:
> > webkit/fileapi/local_file_system_operation.cc seems to allow
> > !quota_manager_proxy case. Its comment says:
> > 
> > // If we don't have the quota manager or the requested filesystem type
> > // does not support quota, we should be able to let it go.                  
 
> > 
> > Kinuko-san, could you check if this code is OK?
> > 
> > On 2013/04/04 17:25:09, dmichael wrote:
> > > It looks like if there is no quota_manager_proxy, you'll succeed but
provide
> > > kQuotaPolicyUnknown. Is that what you want? I don't know this area, but I
> > would
> > > have guessed you'd send DidFail instead.
> 
> Actually in the content/ layer I think we must always have non-null
> quota_manager_proxy, so I guess we can just have DCHECK(!quota_manager_proxy)
> here.   (Trybots will tell if I'm wrong...)

[dmichael (off chromium), 2013-04-05 15:39:28]

https://codereview.chromium.org/13508005/diff/18001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/13508005/diff/18001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:700:
DCHECK(quota_manager_proxy->quota_manager());
If quota_manager_proxy is ever actually NULL, you'll dereference a NULL pointer
on 702 in a release build. It might be better to use CHECK().

Otherwise, you might just want the if, but in the else use NOTREACHED and maybe
send DidFail.

[hamaji, 2013-04-05 17:00:49]

https://codereview.chromium.org/13508005/diff/18001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/13508005/diff/18001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:700:
DCHECK(quota_manager_proxy->quota_manager());
Done, by CHECK

On 2013/04/05 15:39:28, dmichael wrote:
> If quota_manager_proxy is ever actually NULL, you'll dereference a NULL
pointer
> on 702 in a release build. It might be better to use CHECK().
> 
> Otherwise, you might just want the if, but in the else use NOTREACHED and
maybe
> send DidFail.

[dmichael (off chromium), 2013-04-05 17:04:06]

lgtm

[kinuko, 2013-04-06 04:37:44]

The permission issue may still change per ongoing discussion, but lgtm in case
you need this now.

[hamaji, 2013-04-10 02:46:30]

Per email discussion, I've reverted most changes in patch set 2 (except a bug
fix).

Chris: ping?

[cevans, 2013-04-10 03:13:44]

[+aedla]

@aedla: you can take this one in your timezone? :D I'm just off home.


On Tue, Apr 9, 2013 at 7:46 PM, <hamaji@chromium.org> wrote:

> Per email discussion, I've reverted most changes in patch set 2 (except a
> bug
> fix).
>
> Chris: ping?
>
>
https://codereview.chromium.**org/13508005/<https://codereview.chromium.org/1...
>

[kinuko, 2013-04-10 03:35:18]

https://codereview.chromium.org/13508005/diff/32001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/13508005/diff/32001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:703: url.origin(),
FileSystemTypeToQuotaStorageType(url.type()))) {
From the discussion, probably we should also check if it's installed app?
(hosted app can also get unlimited quota)  You can check it by calling
QuotaManager::IsInstalledApp(url.origin())

[hamaji, 2013-04-10 14:49:17]

https://codereview.chromium.org/13508005/diff/32001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/13508005/diff/32001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:703: url.origin(),
FileSystemTypeToQuotaStorageType(url.type()))) {
On 2013/04/10 03:35:19, kinuko wrote:
> From the discussion, probably we should also check if it's installed app?
> (hosted app can also get unlimited quota)  You can check it by calling
> QuotaManager::IsInstalledApp(url.origin())

Hmm, I'm not sure, but if we add this check, we end up needing the whiltelist?
We have the plugin mode as well as the extension mode and I think the plugin
mode is considered as a hosted app which requires unlimitedStorage. If I'm
correct, we might want to do something like

if (IsStorageUnlimited && (IsInstalledApp || HasSwitch(kUnlimitedStorage)))

This might work for us.

Anyway, I feel IsStorageUnlimited&&IsInstalledApp is a bit confusing for web
developers. Do you think we need IsInstalledApp forever? Or, will it stay until
we have some extra protections?

[aedla, 2013-04-10 14:49:50]

LGTM on IPC change

[dmichael (off chromium), 2013-04-10 16:02:42]

https://codereview.chromium.org/13508005/diff/32001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/13508005/diff/32001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:703: url.origin(),
FileSystemTypeToQuotaStorageType(url.type()))) {
I'm not sure I understand. IsInstalledApp(path) looks like it would be true for
(legacy) Packaged apps, Platform apps, and Hosted apps:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
(it looks to me like this is what is ultimately called to determine whether the
URL goes in to "installed_apps_":
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...)

That sounds like a good check to me, though it might be redundant (i.e., can any
URL have unlimited storage without having been installed? I think probably not).
Kinuko, it sounded like you were thinking we _don't_ want to support this for
hosted apps? Why not?

[kinuko, 2013-04-11 02:32:45]

https://codereview.chromium.org/13508005/diff/32001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/13508005/diff/32001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:703: url.origin(),
FileSystemTypeToQuotaStorageType(url.type()))) {
On 2013/04/10 16:02:42, dmichael wrote:
> I'm not sure I understand. IsInstalledApp(path) looks like it would be true
for
> (legacy) Packaged apps, Platform apps, and Hosted apps:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
> (it looks to me like this is what is ultimately called to determine whether
the
> URL goes in to "installed_apps_":
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...)
> 
> That sounds like a good check to me, though it might be redundant (i.e., can
any
> URL have unlimited storage without having been installed? I think probably
not).
> Kinuko, it sounded like you were thinking we _don't_ want to support this for
> hosted apps? Why not?

Chatted /w hamaji. Currently the only unlimited&&!installed case I could think
of is specifying command line flag to force unlimitedStorage, and looks like
we'd want to return FD for that case too (for development).  Sorry for having
brought further confusion, I think it'd be enough for checking
IsStorageUnlimited() only.

[hamaji, 2013-04-12 00:28:34]

cevans: Thanks for adding aedla. Now he LGTMed? Could you give me the owner
approval?

[Chris Evans, 2013-04-12 00:29:19]

On 2013/04/12 00:28:34, hamaji wrote:
> cevans: Thanks for adding aedla. Now he LGTMed? Could you give me the owner
> approval?

LGTM

[commit-bot: I haz the power, 2013-04-12 11:12:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hamaji@chromium.org/13508005/32001

[commit-bot: I haz the power, 2013-04-12 13:54:20]

Change committed as 193914