Postpone setting up file handler's file permissions if handler is running lazy background page.

Postpone setting up file handler's file permissions if handler is running lazy background page.

We have to wait until handler's extension host loads before we can setup file access permissions
with ChilsProcessSecurityPolicy.
We can't get the extensions host process id before that.

BUG=chromium-os:29475
TEST=*FileBrowser*

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

[tbarzic, 2012-04-13 02:58:30]

http://codereview.chromium.org/10067021/diff/6001/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/external_filesystem_apitest.cc (right):

http://codereview.chromium.org/10067021/diff/6001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/external_filesystem_apitest.cc:122: void
SetUpCommandLine(CommandLine* command_line) {
I messed up indent here..

http://codereview.chromium.org/10067021/diff/6001/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/filesystem_handler_lazy_background/tab.html
(right):

http://codereview.chromium.org/10067021/diff/6001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/filesystem_handler_lazy_background/tab.html:2:
* Copyright (c) 2011 The Chromium Authors. All rights reserved.  Use of this
wrong year..

http://codereview.chromium.org/10067021/diff/6001/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/filesystem_handler_lazy_background/tab.js
(right):

http://codereview.chromium.org/10067021/diff/6001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/filesystem_handler_lazy_background/tab.js:45:
console.log(reader.result);
remove this log

[dgozman, 2012-04-13 12:41:04]

I suspect that filesystem_handler_lazy_background is identical to
filesystem_handler (except for manifest change)?

http://codereview.chromium.org/10067021/diff/6001/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/10067021/diff/6001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.cc:661:
DCHECK(registrar_.IsEmpty());
Don't we need to unregister observers?

http://codereview.chromium.org/10067021/diff/6001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.cc:689: if
(unloaded->extension->id() == extension_id_) {
Why don't we check profile here (as above)?

http://codereview.chromium.org/10067021/diff/6001/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_handler_util.h (right):

http://codereview.chromium.org/10067021/diff/6001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.h:118: // Registers file
permissions from |handler_host_permissions| with
Did you mean private |handler_host_permissions_| ?

[tbarzic, 2012-04-13 16:07:58]

Test is not identical to filesystem_handler one (I made some modifications to
make it work with lazy background page), but it basically does the same thing

http://codereview.chromium.org/10067021/diff/6001/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/10067021/diff/6001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.cc:661:
DCHECK(registrar_.IsEmpty());
On 2012/04/13 12:41:04, dgozman wrote:
> Don't we need to unregister observers?

Not really. They will get unregistered when the object goes away.
And this object should be used to execute only one task, so this method should
be called at most once (so at this point registrar_ should always be empty)

http://codereview.chromium.org/10067021/diff/6001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.cc:689: if
(unloaded->extension->id() == extension_id_) {
On 2012/04/13 12:41:04, dgozman wrote:
> Why don't we check profile here (as above)?

NOTIFICATION_EXTENSION_UNLOADED has registered only for notifications from
|proflie_|, so we know we have the right profile (line 665)

http://codereview.chromium.org/10067021/diff/6001/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_handler_util.h (right):

http://codereview.chromium.org/10067021/diff/6001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.h:118: // Registers file
permissions from |handler_host_permissions| with
On 2012/04/13 12:41:04, dgozman wrote:
> Did you mean private |handler_host_permissions_| ?

yes, I did

[dgozman, 2012-04-13 17:49:36]

This looks good to me, but wait for Vlad or Matt to review (I'm not expert
here).

[Matt Perry, 2012-04-13 18:45:16]

http://codereview.chromium.org/10067021/diff/13001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/10067021/diff/13001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:579: class
ExecuteTasksFileBrowserFunction::Executor: public FileTaskExecutor {
space before last :

http://codereview.chromium.org/10067021/diff/13001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/10067021/diff/13001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_handler_util.cc:512:
RegisterNotificationObservers();
Now that I think about it, this bit is dangerous. If the extension is not
actually listening to the event you are about to dispatch, we will never load
the background page, and this object will wait forever and leak.

I know I advised you against using the LazyBackgroundTaskQueue directly, but I
think it's preferable in this case. For one, it will take a reference to the
Executor, so that you don't need to keep a member var in
ExecuteTasksFileBrowserFunction. Second, it will handle failure cases for you*,
so you don't have to worry about the extension unloading or the page crashing.

Sorry to change my mind on this after the fact...

* Right now you register a callback with LazyBackgroundTaskQueue, and it will
call the callback only if the transient page successfully loads. The callback is
deleted if anything fails. If you can handle the failure case in the Executor
destructor, that should work fine. Otherwise, I can change the LBTQ to invoke a
callback on failure as well, so you can do cleanup.

http://codereview.chromium.org/10067021/diff/13001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_handler_util.h (right):

http://codereview.chromium.org/10067021/diff/13001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_handler_util.h:117: const std::string&
action_id);
add blank line here

[Matt Perry, 2012-04-13 20:29:07]

http://codereview.chromium.org/10067021/diff/13001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/10067021/diff/13001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_handler_util.cc:512:
RegisterNotificationObservers();
On 2012/04/13 18:45:17, Matt Perry wrote:
> Now that I think about it, this bit is dangerous. If the extension is not
> actually listening to the event you are about to dispatch, we will never load
> the background page, and this object will wait forever and leak.
> 
> I know I advised you against using the LazyBackgroundTaskQueue directly, but I
> think it's preferable in this case. For one, it will take a reference to the
> Executor, so that you don't need to keep a member var in
> ExecuteTasksFileBrowserFunction. Second, it will handle failure cases for
you*,
> so you don't have to worry about the extension unloading or the page crashing.
> 
> Sorry to change my mind on this after the fact...
> 
> * Right now you register a callback with LazyBackgroundTaskQueue, and it will
> call the callback only if the transient page successfully loads. The callback
is
> deleted if anything fails. If you can handle the failure case in the Executor
> destructor, that should work fine. Otherwise, I can change the LBTQ to invoke
a
> callback on failure as well, so you can do cleanup.

FYI, I have http://codereview.chromium.org/10082019/ in case you need to do
cleanup when the transient page fails to load.

[tonibarzic, 2012-04-13 20:34:11]

I should be able to do cleanup without it (but it would probably make things bit
cleaner).. 

On 2012/04/13 20:29:07, Matt Perry wrote:
>
http://codereview.chromium.org/10067021/diff/13001/chrome/browser/chromeos/ex...
> File chrome/browser/chromeos/extensions/file_handler_util.cc (right):
> 
>
http://codereview.chromium.org/10067021/diff/13001/chrome/browser/chromeos/ex...
> chrome/browser/chromeos/extensions/file_handler_util.cc:512:
> RegisterNotificationObservers();
> On 2012/04/13 18:45:17, Matt Perry wrote:
> > Now that I think about it, this bit is dangerous. If the extension is not
> > actually listening to the event you are about to dispatch, we will never
load
> > the background page, and this object will wait forever and leak.
> > 
> > I know I advised you against using the LazyBackgroundTaskQueue directly, but
I
> > think it's preferable in this case. For one, it will take a reference to the
> > Executor, so that you don't need to keep a member var in
> > ExecuteTasksFileBrowserFunction. Second, it will handle failure cases for
> you*,
> > so you don't have to worry about the extension unloading or the page
crashing.
> > 
> > Sorry to change my mind on this after the fact...
> > 
> > * Right now you register a callback with LazyBackgroundTaskQueue, and it
will
> > call the callback only if the transient page successfully loads. The
callback
> is
> > deleted if anything fails. If you can handle the failure case in the
Executor
> > destructor, that should work fine. Otherwise, I can change the LBTQ to
invoke
> a
> > callback on failure as well, so you can do cleanup.
> 
> FYI, I have http://codereview.chromium.org/10082019/ in case you need to do
> cleanup when the transient page fails to load.

[tbarzic, 2012-04-13 21:22:27]

Matt PTAL,
this should work both with and without your patch :)

[Matt Perry, 2012-04-13 21:25:09]

Cool, much cleaner. Thanks for bearing with me.

LGTM

[tbarzic, 2012-04-13 22:50:04]

Matt, sorry for bugging you, but can you take another look at changes in
external_filesystem_apitest (I stole a bit from lazy_background_page_apitest :D)


On 2012/04/13 21:25:09, Matt Perry wrote:
> Cool, much cleaner. Thanks for bearing with me.
> 
> LGTM

[Matt Perry, 2012-04-14 00:07:08]

lgtm

[commit-bot: I haz the power, 2012-04-16 06:15:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/10067021/11003

[commit-bot: I haz the power, 2012-04-16 09:36:43]

Try job failure for 10067021-11003 (retry) (retry) on win_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...