Extracting and unittesting PrepareDropDataForChildProcess function.

Extracting and unittesting PrepareDropDataForChildProcess function.

This CL extracts PrepareDropDataForChildProcess out of
RenderWidgetHostImpl::GrantFileAccessFromDropData into a standalone
function.  This allows newly added unit tests to provide
|security_policy|, |child_id| and |file_system_context| without having a
dependency on RenderProcessHostImpl and/or RenderWidgetHostImpl.

This CL adds unit tests that PrepareDropDataForChildProcess
works as expected for DropData::filenames and for
DropData::file_system_files.  The latter one is a regression test
that would have prevented http://crbug.com/705295 from happening.

BUG=705295
Review-Url: https://codereview.chromium.org/2830743004
Cr-Commit-Position: refs/heads/master@{#466883}
Committed: https://chromium.googlesource.com/chromium/src/+/bf8f0b785a35a56c879f034013ea6210b9d2ed7c

[Łukasz Anforowicz, 2017-04-20 16:11:55]

Description was changed from

==========
Unit test for GrantFileAccessFromDropData vs FileSystemURLs.

This CL creates a unit test that verifies that GrantFileAccessFromDropData
works as expected for FileSystemURLs.  To make this method easier to
unit test this method is moved to ChildProcessSecurityPolicyImpl class.

BUG=705295
==========

to

==========
Unit tests for GrantFileAccessFromDropData method.

This CL creates unit tests that verify that GrantFileAccessFromDropData
works as expected for DropData::filenames and for
DropData::file_system_files.  The latter one is a regression test
that would have prevented http://crbug.com/705295 from happening.

To make things easier to unit test, the GrantFileAccessFromDropData
method is moved from RenderWidgetHostImpl to
ChildProcessSecurityPolicyImpl class.  This allows the unit tests
to provide |child_id| and |file_system_context| without having
a dependency on RenderProcessHostImpl and/or RenderWidgetHostImpl.

BUG=705295
==========

[Łukasz Anforowicz, 2017-04-20 16:12:05]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 16:12:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-04-20 16:18:25]

Description was changed from

==========
Unit tests for GrantFileAccessFromDropData method.

This CL creates unit tests that verify that GrantFileAccessFromDropData
works as expected for DropData::filenames and for
DropData::file_system_files.  The latter one is a regression test
that would have prevented http://crbug.com/705295 from happening.

To make things easier to unit test, the GrantFileAccessFromDropData
method is moved from RenderWidgetHostImpl to
ChildProcessSecurityPolicyImpl class.  This allows the unit tests
to provide |child_id| and |file_system_context| without having
a dependency on RenderProcessHostImpl and/or RenderWidgetHostImpl.

BUG=705295
==========

to

==========
Unit tests for GrantFileAccessFromDropData method.

This CL creates unit tests that verify that GrantFileAccessFromDropData
works as expected for DropData::filenames and for
DropData::file_system_files.  The latter one is a regression test
that would have prevented http://crbug.com/705295 from happening.

To make things easier to unit test, the GrantFileAccessFromDropData
method is moved from RenderWidgetHostImpl to
ChildProcessSecurityPolicyImpl class.  This allows the unit tests
to provide |child_id| and |file_system_context| without having
a dependency on RenderProcessHostImpl and/or RenderWidgetHostImpl.

BUG=705295
==========

[commit-bot: I haz the power, 2017-04-20 16:49:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 16:49:49]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Łukasz Anforowicz, 2017-04-20 19:28:34]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 19:29:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-04-20 20:36:56]

lukasza@chromium.org changed reviewers:
+ nick@chromium.org

[Łukasz Anforowicz, 2017-04-20 20:36:56]

nick@, can you PTAL?

[commit-bot: I haz the power, 2017-04-20 22:18:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 22:18:01]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ncarter (slow), 2017-04-24 18:54:46]

https://codereview.chromium.org/2830743004/diff/40001/content/browser/child_p...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/2830743004/diff/40001/content/browser/child_p...
content/browser/child_process_security_policy_impl.cc:967: filename.display_name
= base::FilePath::FromUTF8Unsafe(name);
Mutating the |drop_data| seems like an unusual contract for a Grant operation. 

If you move this into a static function (as suggested below), I might consider
giving it a name more like "PrepareDropDataForProcess" to make it clearer that
DropData is an in/out parameter.

https://codereview.chromium.org/2830743004/diff/40001/content/browser/child_p...
content/browser/child_process_security_policy_impl.cc:996:
isolated_context->RegisterDraggedFileSystem(files);
I am not convinced that CPSP is the right place for this operation to live. I
think of CPSP as a dumb recorder of state; the actual policy decisions are
supposed to live in the caller of CPSP. But this function has a lot of business
logic specific to drag and drop. Calling into IsolatedContext, in particular,
seems like a layering violation.

What do you think of making this a utility function in
content/browser/fileapi/browser_file_system_helper.h (it could even accept the
IsolatedContext and CPSP as arguments, to make explicit what it's mutating?)?

I also noticed some web_drag_ things in content/browser/web_contents/ but that
doesn't feel as appropriate.

[Łukasz Anforowicz, 2017-04-24 21:24:08]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 21:24:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-04-24 21:30:24]

Thanks Nick - can you take another look please?

https://codereview.chromium.org/2830743004/diff/40001/content/browser/child_p...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/2830743004/diff/40001/content/browser/child_p...
content/browser/child_process_security_policy_impl.cc:967: filename.display_name
= base::FilePath::FromUTF8Unsafe(name);
On 2017/04/24 18:54:46, ncarter wrote:
> Mutating the |drop_data| seems like an unusual contract for a Grant operation.


Good point.  I should have raised this as a concern, rather than quietly hiding
this in the CL.
> 
> If you move this into a static function (as suggested below), I might consider
> giving it a name more like "PrepareDropDataForProcess" to make it clearer that
> DropData is an in/out parameter.

Thanks for the suggestion.  Done (more or less - I went forward with
"PrepareDropDataForChildProcess").

https://codereview.chromium.org/2830743004/diff/40001/content/browser/child_p...
content/browser/child_process_security_policy_impl.cc:996:
isolated_context->RegisterDraggedFileSystem(files);
On 2017/04/24 18:54:46, ncarter wrote:
> I am not convinced that CPSP is the right place for this operation to live. I
> think of CPSP as a dumb recorder of state; the actual policy decisions are
> supposed to live in the caller of CPSP. But this function has a lot of
business
> logic specific to drag and drop. Calling into IsolatedContext, in particular,
> seems like a layering violation.
> 
> What do you think of making this a utility function in
> content/browser/fileapi/browser_file_system_helper.h (it could even accept the
> IsolatedContext and CPSP as arguments, to make explicit what it's mutating?)?
> 
> I also noticed some web_drag_ things in content/browser/web_contents/ but that
> doesn't feel as appropriate.

Thanks for pointing this out - I haven't considered layering (nor was I aware of
where we try to place CPSP).  I like the suggestion to put the helper function
in content/browser/fileapi/browser_file_system_helper.h.  Done. 

PS. TBH it feels like the helper function should be [layer-wise] above *both* 1)
filesystem and 2) cpsp;  OTOH, I wasn't able to find a better place for the
helper and content/browser/fileapi/browser_file_system_helper.h seems okay.  The
only alternative I could think of is to create a new drag_helpers.h compilation
unit (but then putting it next to
content/browser/renderer_host/render_widget_host_impl.cc seems for some reason
icky to me).

https://codereview.chromium.org/2830743004/diff/120001/content/browser/fileap...
File content/browser/fileapi/browser_file_system_helper.h (right):

https://codereview.chromium.org/2830743004/diff/120001/content/browser/fileap...
content/browser/fileapi/browser_file_system_helper.h:50: CONTENT_EXPORT void
PrepareDropDataForChildProcess(
TBH I don't understand why |CONTENT_EXPORT| is needed here.  I am guessing that
things don't link without it, because there is an (unexpected to me) component
build boundary between here and
content/browser/renderer_host/render_widget_host_impl.cc.  FWIW, other
content-internal things in this file use CONTENT_EXPORT, so this is probably
fine.

https://codereview.chromium.org/2830743004/diff/120001/content/browser/fileap...
content/browser/fileapi/browser_file_system_helper.h:52:
ChildProcessSecurityPolicyImpl* security_policy,
This has to be CPSPImpl, because CPSP doesn't expose
GrantRequestSpecificFileURL.

https://codereview.chromium.org/2830743004/diff/120001/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/2830743004/diff/120001/content/test/BUILD.gn#...
content/test/BUILD.gn:933: "//testing/perf",
|git cl presubmit| and |git cl format| made me do it...

[ncarter (slow), 2017-04-24 22:01:51]

lgtm, but you need to rename the CL which refers to the old name

https://codereview.chromium.org/2830743004/diff/40001/content/browser/child_p...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/2830743004/diff/40001/content/browser/child_p...
content/browser/child_process_security_policy_impl.cc:996:
isolated_context->RegisterDraggedFileSystem(files);
On 2017/04/24 21:30:23, Łukasz A. wrote:
> On 2017/04/24 18:54:46, ncarter wrote:
> > I am not convinced that CPSP is the right place for this operation to live.
I
> > think of CPSP as a dumb recorder of state; the actual policy decisions are
> > supposed to live in the caller of CPSP. But this function has a lot of
> business
> > logic specific to drag and drop. Calling into IsolatedContext, in
particular,
> > seems like a layering violation.
> > 
> > What do you think of making this a utility function in
> > content/browser/fileapi/browser_file_system_helper.h (it could even accept
the
> > IsolatedContext and CPSP as arguments, to make explicit what it's
mutating?)?
> > 
> > I also noticed some web_drag_ things in content/browser/web_contents/ but
that
> > doesn't feel as appropriate.
> 
> Thanks for pointing this out - I haven't considered layering (nor was I aware
of
> where we try to place CPSP).  I like the suggestion to put the helper function
> in content/browser/fileapi/browser_file_system_helper.h.  Done. 
> 
> PS. TBH it feels like the helper function should be [layer-wise] above *both*
1)
> filesystem and 2) cpsp;  OTOH, I wasn't able to find a better place for the
> helper and content/browser/fileapi/browser_file_system_helper.h seems okay. 
The
> only alternative I could think of is to create a new drag_helpers.h
compilation
> unit (but then putting it next to
> content/browser/renderer_host/render_widget_host_impl.cc seems for some reason
> icky to me).

I agree with the feeling that it ought to be both layer-wise above both
filesystem and cpsp.  //content/browser/fileapi is kind of there, in the sense
that it's in a layer above //storage.

So, I think fileapi is not such a bad place for this. The CPSP grants are all
file-system related, and the fileapi's scope has some overlap with file:///.

https://codereview.chromium.org/2830743004/diff/120001/content/browser/fileap...
File content/browser/fileapi/browser_file_system_helper.h (right):

https://codereview.chromium.org/2830743004/diff/120001/content/browser/fileap...
content/browser/fileapi/browser_file_system_helper.h:41: 
Maybe this as an intro sentence:

// Make it possible for a |drop_data|'s resources to be read by |child_id|'s
// process -- by granting permissions, rewriting |drop_data|, or both.
//

https://codereview.chromium.org/2830743004/diff/120001/content/browser/fileap...
content/browser/fileapi/browser_file_system_helper.h:50: CONTENT_EXPORT void
PrepareDropDataForChildProcess(
On 2017/04/24 21:30:24, Łukasz A. wrote:
> TBH I don't understand why |CONTENT_EXPORT| is needed here.  I am guessing
that
> things don't link without it, because there is an (unexpected to me) component
> build boundary between here and
> content/browser/renderer_host/render_widget_host_impl.cc.  FWIW, other
> content-internal things in this file use CONTENT_EXPORT, so this is probably
> fine.

In the component build, both render_widget_host_impl.cc and
browser_file_system_helper.cc get compiled into content.dll. If you didn't have
a unittest, you wouldn't need CONTENT_EXPORT.

You need the CONTENT_EXPORT because otherwise your unittest (which calls this
function directly) doesn't know to look in content.dll for this function.

https://codereview.chromium.org/2830743004/diff/120001/content/browser/fileap...
content/browser/fileapi/browser_file_system_helper.h:52:
ChildProcessSecurityPolicyImpl* security_policy,
On 2017/04/24 21:30:23, Łukasz A. wrote:
> This has to be CPSPImpl, because CPSP doesn't expose
> GrantRequestSpecificFileURL.

That's fine.

[Łukasz Anforowicz, 2017-04-24 22:05:16]

Description was changed from

==========
Unit tests for GrantFileAccessFromDropData method.

This CL creates unit tests that verify that GrantFileAccessFromDropData
works as expected for DropData::filenames and for
DropData::file_system_files.  The latter one is a regression test
that would have prevented http://crbug.com/705295 from happening.

To make things easier to unit test, the GrantFileAccessFromDropData
method is moved from RenderWidgetHostImpl to
ChildProcessSecurityPolicyImpl class.  This allows the unit tests
to provide |child_id| and |file_system_context| without having
a dependency on RenderProcessHostImpl and/or RenderWidgetHostImpl.

BUG=705295
==========

to

==========
Extracting and unittesting PrepareDropDataForChildProcess function.

This CL extracts PrepareDropDataForChildProcess out of
RenderWidgetHostImpl::GrantFileAccessFromDropData into a standalone
function.  This allows newly added unit tests to provide
|security_policy|, |child_id| and |file_system_context| without having a
dependency on RenderProcessHostImpl and/or RenderWidgetHostImpl.

This CL adds unit tests that PrepareDropDataForChildProcess
works as expected for DropData::filenames and for
DropData::file_system_files.  The latter one is a regression test
that would have prevented http://crbug.com/705295 from happening.

BUG=705295
==========

[Łukasz Anforowicz, 2017-04-24 22:18:11]

On 2017/04/24 22:01:51, ncarter wrote:
> lgtm, but you need to rename the CL which refers to the old name
> 
Yes - I've just realized this and uploaded a new CL description :-/

Also - I see that trybots say that my new tests are failing on Windows.  I am
still trying to build and investigate the problem locally (for some reason I
can't access LogDog logs for the failures from the trybots).

https://codereview.chromium.org/2830743004/diff/120001/content/browser/fileap...
File content/browser/fileapi/browser_file_system_helper.h (right):

https://codereview.chromium.org/2830743004/diff/120001/content/browser/fileap...
content/browser/fileapi/browser_file_system_helper.h:41: 
On 2017/04/24 22:01:51, ncarter wrote:
> Maybe this as an intro sentence:
> 
> // Make it possible for a |drop_data|'s resources to be read by |child_id|'s
> // process -- by granting permissions, rewriting |drop_data|, or both.
> //

Done.

https://codereview.chromium.org/2830743004/diff/120001/content/browser/fileap...
content/browser/fileapi/browser_file_system_helper.h:50: CONTENT_EXPORT void
PrepareDropDataForChildProcess(
On 2017/04/24 22:01:51, ncarter wrote:
> On 2017/04/24 21:30:24, Łukasz A. wrote:
> > TBH I don't understand why |CONTENT_EXPORT| is needed here.  I am guessing
> that
> > things don't link without it, because there is an (unexpected to me)
component
> > build boundary between here and
> > content/browser/renderer_host/render_widget_host_impl.cc.  FWIW, other
> > content-internal things in this file use CONTENT_EXPORT, so this is probably
> > fine.
> 
> In the component build, both render_widget_host_impl.cc and
> browser_file_system_helper.cc get compiled into content.dll. If you didn't
have
> a unittest, you wouldn't need CONTENT_EXPORT.
> 
> You need the CONTENT_EXPORT because otherwise your unittest (which calls this
> function directly) doesn't know to look in content.dll for this function.

Oh, ok - thanks for the explanation.  I guess I should have looked closer at the
error message from the linker to realize that it is complaining about the unit
tests, not about the product code.

[Łukasz Anforowicz, 2017-04-24 22:51:04]

Description was changed from

==========
Extracting and unittesting PrepareDropDataForChildProcess function.

This CL extracts PrepareDropDataForChildProcess out of
RenderWidgetHostImpl::GrantFileAccessFromDropData into a standalone
function.  This allows newly added unit tests to provide
|security_policy|, |child_id| and |file_system_context| without having a
dependency on RenderProcessHostImpl and/or RenderWidgetHostImpl.

This CL adds unit tests that PrepareDropDataForChildProcess
works as expected for DropData::filenames and for
DropData::file_system_files.  The latter one is a regression test
that would have prevented http://crbug.com/705295 from happening.

BUG=705295
==========

to

==========
Extracting and unittesting PrepareDropDataForChildProcess function.

This CL extracts PrepareDropDataForChildProcess out of
RenderWidgetHostImpl::GrantFileAccessFromDropData into a standalone
function.  This allows newly added unit tests to provide
|security_policy|, |child_id| and |file_system_context| without having a
dependency on RenderProcessHostImpl and/or RenderWidgetHostImpl.

This CL adds unit tests that PrepareDropDataForChildProcess
works as expected for DropData::filenames and for
DropData::file_system_files.  The latter one is a regression test
that would have prevented http://crbug.com/705295 from happening.

BUG=705295
==========

[Łukasz Anforowicz, 2017-04-24 23:54:49]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 23:55:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-04-24 23:57:03]

On 2017/04/24 22:18:11, Łukasz A. wrote:
> Also - I see that trybots say that my new tests are failing on Windows.  I am
> still trying to build and investigate the problem locally (for some reason I
> can't access LogDog logs for the failures from the trybots).

I think this is fixed now - I had to use absolute paths in 2 places.  2 notes
about that:
- AFAICT the requirement to use absolute paths makes sense / is not a bug in the
product code.
- I don't see a way to have an absolute path in the tests, without actually
temporarily creating a ScopedTempDir.

Keeping fingers crossed that all trybots will come back green this time around.

[Łukasz Anforowicz, 2017-04-24 23:59:19]

On 2017/04/24 23:57:03, Łukasz A. wrote:
> - AFAICT the requirement to use absolute paths makes sense / is not a bug in
the
> product code.

Oh, and for the record, the requirement comes from:

1. storage::ExternalMountPoints::ValidateNewMountPoint (validation of |path|).

2. content::ChildProcessSecurityPolicyImpl::SecurityState::HasPermissionsForFile
(validation of |file|).

[commit-bot: I haz the power, 2017-04-25 00:58:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 00:58:14]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2017-04-25 03:15:39]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2017-04-25 03:15:40]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org
Link to the patchset: https://codereview.chromium.org/2830743004/#ps200001
(title: "Readding a summary comment to PrepareDropDataForChildProcess as
suggested in the CR feedback... (it…")

[commit-bot: I haz the power, 2017-04-25 03:16:07]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 05:22:31]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1493090139644150,
"parent_rev": "2a1375ebfbf6f242cfacf55b093066a6386ba227", "commit_rev":
"bf8f0b785a35a56c879f034013ea6210b9d2ed7c"}

[commit-bot: I haz the power, 2017-04-25 05:22:39]

Description was changed from

==========
Extracting and unittesting PrepareDropDataForChildProcess function.

This CL extracts PrepareDropDataForChildProcess out of
RenderWidgetHostImpl::GrantFileAccessFromDropData into a standalone
function.  This allows newly added unit tests to provide
|security_policy|, |child_id| and |file_system_context| without having a
dependency on RenderProcessHostImpl and/or RenderWidgetHostImpl.

This CL adds unit tests that PrepareDropDataForChildProcess
works as expected for DropData::filenames and for
DropData::file_system_files.  The latter one is a regression test
that would have prevented http://crbug.com/705295 from happening.

BUG=705295
==========

to

==========
Extracting and unittesting PrepareDropDataForChildProcess function.

This CL extracts PrepareDropDataForChildProcess out of
RenderWidgetHostImpl::GrantFileAccessFromDropData into a standalone
function.  This allows newly added unit tests to provide
|security_policy|, |child_id| and |file_system_context| without having a
dependency on RenderProcessHostImpl and/or RenderWidgetHostImpl.

This CL adds unit tests that PrepareDropDataForChildProcess
works as expected for DropData::filenames and for
DropData::file_system_files.  The latter one is a regression test
that would have prevented http://crbug.com/705295 from happening.

BUG=705295

Review-Url: https://codereview.chromium.org/2830743004
Cr-Commit-Position: refs/heads/master@{#466883}
Committed:
https://chromium.googlesource.com/chromium/src/+/bf8f0b785a35a56c879f034013ea...
==========

[commit-bot: I haz the power, 2017-04-25 05:22:40]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/bf8f0b785a35a56c879f034013ea...