Manage IsolatedContext with reference counts

Manage IsolatedContext with reference counts

to make it possible to be shared by multiple children.

This patch itself should have no side effect.

BUG=none
TEST=existing tests should pass

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

[tzik, 2012-06-21 14:00:35]

lgtm

http://codereview.chromium.org/10536200/diff/6/webkit/fileapi/isolated_contex...
File webkit/fileapi/isolated_context.cc (right):

http://codereview.chromium.org/10536200/diff/6/webkit/fileapi/isolated_contex...
webkit/fileapi/isolated_context.cc:60: base::AutoLock locker(lock_);
Could you add DCHECK(ref_counts_.find(filesystem_id) != ref_counts_.end());?

[kinuko, 2012-06-22 04:53:30]

http://codereview.chromium.org/10536200/diff/6/webkit/fileapi/isolated_contex...
File webkit/fileapi/isolated_context.cc (right):

http://codereview.chromium.org/10536200/diff/6/webkit/fileapi/isolated_contex...
webkit/fileapi/isolated_context.cc:60: base::AutoLock locker(lock_);
On 2012/06/21 14:00:35, tzik wrote:
> Could you add DCHECK(ref_counts_.find(filesystem_id) != ref_counts_.end());?

Done.  We didn't have similar check in RevokeIsolatedFilesystem so I've updated
unittest code not to call extra RemoveReference.  Can you take one more quick
look?

[kinuko, 2012-06-22 08:16:34]

Re-added RevokeIsolatedFileSystem and fixed DCHECKs. PTAL thanks!

On 2012/06/22 04:53:30, kinuko wrote:
>
http://codereview.chromium.org/10536200/diff/6/webkit/fileapi/isolated_contex...
> File webkit/fileapi/isolated_context.cc (right):
> 
>
http://codereview.chromium.org/10536200/diff/6/webkit/fileapi/isolated_contex...
> webkit/fileapi/isolated_context.cc:60: base::AutoLock locker(lock_);
> On 2012/06/21 14:00:35, tzik wrote:
> > Could you add DCHECK(ref_counts_.find(filesystem_id) != ref_counts_.end());?
> 
> Done.  We didn't have similar check in RevokeIsolatedFilesystem so I've
updated
> unittest code not to call extra RemoveReference.  Can you take one more quick
> look?

[kinuko, 2012-06-25 09:37:23]

Fixed windows test breakage.  Can you take another look?

jam@, can you take a look at render_view_host_unittest.cc change?

On 2012/06/22 08:16:34, kinuko wrote:
> Re-added RevokeIsolatedFileSystem and fixed DCHECKs. PTAL thanks!
> 
> On 2012/06/22 04:53:30, kinuko wrote:
> >
>
http://codereview.chromium.org/10536200/diff/6/webkit/fileapi/isolated_contex...
> > File webkit/fileapi/isolated_context.cc (right):
> > 
> >
>
http://codereview.chromium.org/10536200/diff/6/webkit/fileapi/isolated_contex...
> > webkit/fileapi/isolated_context.cc:60: base::AutoLock locker(lock_);
> > On 2012/06/21 14:00:35, tzik wrote:
> > > Could you add DCHECK(ref_counts_.find(filesystem_id) !=
ref_counts_.end());?
> > 
> > Done.  We didn't have similar check in RevokeIsolatedFilesystem so I've
> updated
> > unittest code not to call extra RemoveReference.  Can you take one more
quick
> > look?

[tzik, 2012-06-25 10:37:54]

lgtm

http://codereview.chromium.org/10536200/diff/31001/webkit/fileapi/isolated_co...
File webkit/fileapi/isolated_context.cc (right):

http://codereview.chromium.org/10536200/diff/31001/webkit/fileapi/isolated_co...
webkit/fileapi/isolated_context.cc:33: DCHECK(!iter->ReferencesParent() &&
iter->IsAbsolute());
As we chatted, platform_app_launcher seems to pass relative path to this.
Could you add some comment or file a bug to it?

[kinuko, 2012-06-25 10:45:06]

On 2012/06/25 10:37:54, tzik wrote:
> lgtm
> 
>
http://codereview.chromium.org/10536200/diff/31001/webkit/fileapi/isolated_co...
> File webkit/fileapi/isolated_context.cc (right):
> 
>
http://codereview.chromium.org/10536200/diff/31001/webkit/fileapi/isolated_co...
> webkit/fileapi/isolated_context.cc:33: DCHECK(!iter->ReferencesParent() &&
> iter->IsAbsolute());
> As we chatted, platform_app_launcher seems to pass relative path to this.
> Could you add some comment or file a bug to it?

(+benwells)

Ben, does platform_app_launcher check if the given path is absolute or not? 
Currently both IsolatedContext and ChildProcessSecurityPolicy assume the given
path is absolute. If it could be given a relative path I think we should fix it
asap.

[jam, 2012-06-25 15:14:41]

lgtm

[benwells, 2012-06-25 18:07:35]

On 2012/06/25 10:45:06, kinuko wrote:
> On 2012/06/25 10:37:54, tzik wrote:
> > lgtm
> > 
> >
>
http://codereview.chromium.org/10536200/diff/31001/webkit/fileapi/isolated_co...
> > File webkit/fileapi/isolated_context.cc (right):
> > 
> >
>
http://codereview.chromium.org/10536200/diff/31001/webkit/fileapi/isolated_co...
> > webkit/fileapi/isolated_context.cc:33: DCHECK(!iter->ReferencesParent() &&
> > iter->IsAbsolute());
> > As we chatted, platform_app_launcher seems to pass relative path to this.
> > Could you add some comment or file a bug to it?
> 
> (+benwells)
> 
> Ben, does platform_app_launcher check if the given path is absolute or not? 
> Currently both IsolatedContext and ChildProcessSecurityPolicy assume the given
> path is absolute. If it could be given a relative path I think we should fix
it
> asap.

crbug.com/134469 logged

[kinuko, 2012-06-26 04:42:20]

On 2012/06/25 18:07:35, benwells wrote:
> On 2012/06/25 10:45:06, kinuko wrote:
> > On 2012/06/25 10:37:54, tzik wrote:
> > > lgtm
> > > 
> > >
> >
>
http://codereview.chromium.org/10536200/diff/31001/webkit/fileapi/isolated_co...
> > > File webkit/fileapi/isolated_context.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10536200/diff/31001/webkit/fileapi/isolated_co...
> > > webkit/fileapi/isolated_context.cc:33: DCHECK(!iter->ReferencesParent() &&
> > > iter->IsAbsolute());
> > > As we chatted, platform_app_launcher seems to pass relative path to this.
> > > Could you add some comment or file a bug to it?
> > 
> > (+benwells)
> > 
> > Ben, does platform_app_launcher check if the given path is absolute or not? 
> > Currently both IsolatedContext and ChildProcessSecurityPolicy assume the
given
> > path is absolute. If it could be given a relative path I think we should fix
> it
> > asap.
> 
> crbug.com/134469 logged

Thx!