Prompt user for file access permission before download starts

Prompt user for file access permission before download starts

Please bring the discussion offline.

Background:
On some systems we need to ask for user permission before download is able to write to storage.
This change adds an extra step to check file access at the begining of file path generation.
If user is granted file access, download will proceed. Otherwise, it will be canceled.

BUG=501606
Committed: https://crrev.com/1fa54a4e6bac3ecb49a61ee9594fe73fc23edd0d
Cr-Commit-Position: refs/heads/master@{#337686}

[qinmin, 2015-07-01 23:03:08]

Patchset #1 (id:1) has been deleted

[qinmin, 2015-07-01 23:04:37]

qinmin@chromium.org changed reviewers:
+ rdsmith@chromium.org, tedchoc@chromium.org

[qinmin, 2015-07-01 23:04:38]

PTAL.

@tedchoc for content/browser/android/
@rdsmith for chrome/browser/download/

[Randy Smith (Not in Mondays), 2015-07-02 01:51:20]

rdsmith@chromium.org changed reviewers:
+ asanka@chromium.org

[Randy Smith (Not in Mondays), 2015-07-02 01:51:21]

If Asanka is around and able to review this, he'd be a better person than I
(since he wrote DownloadTargetDeterminer).  Otherwise, I'll do it when I get
back from holiday.

[qinmin, 2015-07-06 17:19:38]

@asanka, would you please take a look at this with high priorities? thanks

[Ted C, 2015-07-06 20:03:39]

overall seems good...question about lifetimes but assuming the download
controller is at the process level, then I think we are fine here (but if it's
at the activity level then it could be an issue).

https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:78: // Download controller
for file access prompt.
these two lines shouldn't be indented.

https://codereview.chromium.org/1217223006/diff/20001/content/browser/android...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1217223006/diff/20001/content/browser/android...
content/browser/android/download_controller_android_impl.cc:301:
base::Unretained(this), render_process_id, render_view_id,
what is the lifetime of DownloadControllerAndroidImpl?  Just making sure there
isn't a situation where it could be destroyed while the file access is being
requested?

https://codereview.chromium.org/1217223006/diff/20001/content/browser/android...
File content/browser/android/download_controller_android_impl.h (right):

https://codereview.chromium.org/1217223006/diff/20001/content/browser/android...
content/browser/android/download_controller_android_impl.h:92: WebContents*
download_item,
this and the following line are indented 2 too much

[qinmin, 2015-07-06 20:36:02]

https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:78: // Download controller
for file access prompt.
On 2015/07/06 20:03:39, Ted C wrote:
> these two lines shouldn't be indented.

Done.

https://codereview.chromium.org/1217223006/diff/20001/content/browser/android...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1217223006/diff/20001/content/browser/android...
content/browser/android/download_controller_android_impl.cc:301:
base::Unretained(this), render_process_id, render_view_id,
On 2015/07/06 20:03:39, Ted C wrote:
> what is the lifetime of DownloadControllerAndroidImpl?  Just making sure there
> isn't a situation where it could be destroyed while the file access is being
> requested?

DownloadControllerAndroidImpl's lifetime is at the process level, so we are safe
here.

https://codereview.chromium.org/1217223006/diff/20001/content/browser/android...
File content/browser/android/download_controller_android_impl.h (right):

https://codereview.chromium.org/1217223006/diff/20001/content/browser/android...
content/browser/android/download_controller_android_impl.h:92: WebContents*
download_item,
On 2015/07/06 20:03:39, Ted C wrote:
> this and the following line are indented 2 too much

Done.

[Ted C, 2015-07-06 20:39:00]

lgtm

[asanka, 2015-07-06 21:09:41]

I've left some low level comments, but this permission test doesn't belong in
the download target determiner. The DTD's responsibility is to find a suitable
place to store the downloaded file. Not to check for application level
permissions that don't depend on the target of an individual download. The DTD
is also invoked *after* the tab/origin specific permission for downloads have
been checked.

The invocation of hasFileAccess()/requestFileAccess() should happen in
DownloadResourceThrottle, before invoking DownloadRequestLimiter.

https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:79:
content::DownloadControllerAndroid* g_download_controller_;
I know this'll end up being nullptr, but please initialize it explicitly.

https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:191:
DCHECK(local_path_.empty());
Why are you checking these (here and below)?

In general you should only DCHECK dependencies for this state.

https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:955: void
DownloadTargetDeterminer::SetDownloadControllerAndroidForTestings(
Rather than setting a global override here just for DTD, what about having an
override for DownloadControllerAndroid? It seems fragile that a global is
overridden this way just for a single consumer since other consumers or other
code paths might use the real object. Also any tests performed at a higher level
that'll depend on mocking this dependency will need to reach into each consuming
class that might be invoked during testing.

https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
File chrome/browser/download/download_target_determiner_unittest.cc (right):

https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_unittest.cc:335:
&download_controller_);
You are setting a global pointer to a temporary variable and leaving it as is.
You should reset it to nullptr at TearDown(). unit_tests can invoke multiple
tests during a single invocation.

https://codereview.chromium.org/1217223006/diff/20001/content/browser/android...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1217223006/diff/20001/content/browser/android...
content/browser/android/download_controller_android_impl.cc:113: bool
DownloadControllerAndroidImpl::RequireUserPermissionPrompt(
Why not just have an AcquireFileAccessPermission() or some such that does the
equivalent of checking hasFileAccess() and calling requestFileAccess() if
necessary? It seems none of the call sites care about checking whether a
permission prompt is required without intending to prompt the user.

[qinmin, 2015-07-06 21:41:29]

On 2015/07/06 21:09:41, asanka wrote:
> I've left some low level comments, but this permission test doesn't belong in
> the download target determiner. The DTD's responsibility is to find a suitable
> place to store the downloaded file. Not to check for application level
> permissions that don't depend on the target of an individual download. The DTD
> is also invoked *after* the tab/origin specific permission for downloads have
> been checked.
> 
> The invocation of hasFileAccess()/requestFileAccess() should happen in
> DownloadResourceThrottle, before invoking DownloadRequestLimiter.
> 
>
https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
> File chrome/browser/download/download_target_determiner.cc (right):
> 
>
https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
> chrome/browser/download/download_target_determiner.cc:79:
> content::DownloadControllerAndroid* g_download_controller_;
> I know this'll end up being nullptr, but please initialize it explicitly.
> 
>
https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
> chrome/browser/download/download_target_determiner.cc:191:
> DCHECK(local_path_.empty());
> Why are you checking these (here and below)?
> 
> In general you should only DCHECK dependencies for this state.
> 
>
https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
> chrome/browser/download/download_target_determiner.cc:955: void
> DownloadTargetDeterminer::SetDownloadControllerAndroidForTestings(
> Rather than setting a global override here just for DTD, what about having an
> override for DownloadControllerAndroid? It seems fragile that a global is
> overridden this way just for a single consumer since other consumers or other
> code paths might use the real object. Also any tests performed at a higher
level
> that'll depend on mocking this dependency will need to reach into each
consuming
> class that might be invoked during testing.
> 
>
https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
> File chrome/browser/download/download_target_determiner_unittest.cc (right):
> 
>
https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
> chrome/browser/download/download_target_determiner_unittest.cc:335:
> &download_controller_);
> You are setting a global pointer to a temporary variable and leaving it as is.
> You should reset it to nullptr at TearDown(). unit_tests can invoke multiple
> tests during a single invocation.
> 
>
https://codereview.chromium.org/1217223006/diff/20001/content/browser/android...
> File content/browser/android/download_controller_android_impl.cc (right):
> 
>
https://codereview.chromium.org/1217223006/diff/20001/content/browser/android...
> content/browser/android/download_controller_android_impl.cc:113: bool
> DownloadControllerAndroidImpl::RequireUserPermissionPrompt(
> Why not just have an AcquireFileAccessPermission() or some such that does the
> equivalent of checking hasFileAccess() and calling requestFileAccess() if
> necessary? It seems none of the call sites care about checking whether a
> permission prompt is required without intending to prompt the user.

Hi, Asanka, are you ok leaving the changes in DTD in M44 and move it out to
DownloadRequestLimiter in M45? We have some urgent deadline to catch.

[qinmin, 2015-07-07 00:54:22]

https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:79:
content::DownloadControllerAndroid* g_download_controller_;
On 2015/07/06 21:09:41, asanka wrote:
> I know this'll end up being nullptr, but please initialize it explicitly.

Done. make this a member variable and initialize it in the initializer list.

https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:191:
DCHECK(local_path_.empty());
Ok, removed.

On 2015/07/06 21:09:41, asanka wrote:
> Why are you checking these (here and below)?
> 
> In general you should only DCHECK dependencies for this state.

https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:955: void
DownloadTargetDeterminer::SetDownloadControllerAndroidForTestings(
On 2015/07/06 21:09:41, asanka wrote:
> Rather than setting a global override here just for DTD, what about having an
> override for DownloadControllerAndroid? It seems fragile that a global is
> overridden this way just for a single consumer since other consumers or other
> code paths might use the real object. Also any tests performed at a higher
level
> that'll depend on mocking this dependency will need to reach into each
consuming
> class that might be invoked during testing.

Done. DownloadControllerAndroid::Get() will now returns a mock class by calling
UseMockControllerForTesting() in setup

https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
File chrome/browser/download/download_target_determiner_unittest.cc (right):

https://codereview.chromium.org/1217223006/diff/20001/chrome/browser/download...
chrome/browser/download/download_target_determiner_unittest.cc:335:
&download_controller_);
On 2015/07/06 21:09:41, asanka wrote:
> You are setting a global pointer to a temporary variable and leaving it as is.
> You should reset it to nullptr at TearDown(). unit_tests can invoke multiple
> tests during a single invocation.

Done.

https://codereview.chromium.org/1217223006/diff/20001/content/browser/android...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1217223006/diff/20001/content/browser/android...
content/browser/android/download_controller_android_impl.cc:113: bool
DownloadControllerAndroidImpl::RequireUserPermissionPrompt(
On 2015/07/06 21:09:41, asanka wrote:
> Why not just have an AcquireFileAccessPermission() or some such that does the
> equivalent of checking hasFileAccess() and calling requestFileAccess() if
> necessary? It seems none of the call sites care about checking whether a
> permission prompt is required without intending to prompt the user.

Done.

[qinmin, 2015-07-07 00:59:29]

Patchset #3 (id:60001) has been deleted

[qinmin, 2015-07-07 00:59:45]

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

[qinmin, 2015-07-07 00:59:46]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/1217223006/#ps80001
(title: "addressing asanka's comments")

[commit-bot: I haz the power, 2015-07-07 00:59:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1217223006/80001

[commit-bot: I haz the power, 2015-07-07 02:57:30]

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

[commit-bot: I haz the power, 2015-07-07 02:57:31]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[asanka, 2015-07-07 05:03:34]

https://codereview.chromium.org/1217223006/diff/80001/chrome/browser/download...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/1217223006/diff/80001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:110:
download_controller_(content::DownloadControllerAndroid::Get()),
A member variable is unnecessary for this.

https://codereview.chromium.org/1217223006/diff/80001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:195: #endif
#else
  return CONTINUE;
#endif

https://codereview.chromium.org/1217223006/diff/80001/content/browser/android...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1217223006/diff/80001/content/browser/android...
content/browser/android/download_controller_android_impl.cc:59:
cb.release()->Run(granted);
cb leaks. Don't release().

https://codereview.chromium.org/1217223006/diff/80001/content/browser/android...
content/browser/android/download_controller_android_impl.cc:76: if
(DownloadControllerAndroid::use_mock_controller())
Alternatively, allow setting the DownloadControllerAndroid*. That way test code
can inject a mock that the test code controls.

Let's not compile mocks into production code.

https://codereview.chromium.org/1217223006/diff/80001/content/browser/android...
content/browser/android/download_controller_android_impl.cc:323: LOG(ERROR) <<
"Download failed on URL:" << info.url.spec();
Did you intend to remove these log entries?

[asanka, 2015-07-07 05:05:11]

Does this degrade gracefully when the platform doesn't require a runtime
permission for file access?

[qinmin, 2015-07-07 05:12:44]

On 2015/07/07 05:05:11, asanka wrote:
> Does this degrade gracefully when the platform doesn't require a runtime
> permission for file access?

yes, if no permission is required, this should work since haspermission should
always return true.

[qinmin, 2015-07-07 06:14:05]

https://codereview.chromium.org/1217223006/diff/80001/chrome/browser/download...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/1217223006/diff/80001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:110:
download_controller_(content::DownloadControllerAndroid::Get()),
On 2015/07/07 05:03:33, asanka wrote:
> A member variable is unnecessary for this.

Done.

https://codereview.chromium.org/1217223006/diff/80001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:195: #endif
On 2015/07/07 05:03:33, asanka wrote:
> #else
>   return CONTINUE;
> #endif

Done.

https://codereview.chromium.org/1217223006/diff/80001/content/browser/android...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1217223006/diff/80001/content/browser/android...
content/browser/android/download_controller_android_impl.cc:59:
cb.release()->Run(granted);
On 2015/07/07 05:03:33, asanka wrote:
> cb leaks. Don't release().

Done. Thanks for catching this.

https://codereview.chromium.org/1217223006/diff/80001/content/browser/android...
content/browser/android/download_controller_android_impl.cc:76: if
(DownloadControllerAndroid::use_mock_controller())
On 2015/07/07 05:03:33, asanka wrote:
> Alternatively, allow setting the DownloadControllerAndroid*. That way test
code
> can inject a mock that the test code controls.
> 
> Let's not compile mocks into production code.

Done.

https://codereview.chromium.org/1217223006/diff/80001/content/browser/android...
content/browser/android/download_controller_android_impl.cc:323: LOG(ERROR) <<
"Download failed on URL:" << info.url.spec();
On 2015/07/07 05:03:33, asanka wrote:
> Did you intend to remove these log entries?

This could happen if tab was closed immediately after download initiates. But
that doesn't feel like a bug. Removed the error log.

[qinmin, 2015-07-07 16:16:11]

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

[qinmin, 2015-07-07 16:16:12]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/1217223006/#ps120001
(title: "FixDownloadManagerDelegateTest")

[commit-bot: I haz the power, 2015-07-07 16:16:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1217223006/120001

[Randy Smith (Not in Mondays), 2015-07-07 16:18:12]

I'd prefer to do this right (M45), not quickly with technical debt buildup
(M44)--as I understand it, the whole point of the 6 week release cadence is that
the cost of missing a release isn't that large.  Can we just put this into
DRL/DRT in M45?

[Randy Smith (Not in Mondays), 2015-07-07 16:23:35]

On 2015/07/07 16:18:12, rdsmith wrote:
> I'd prefer to do this right (M45), not quickly with technical debt buildup
> (M44)--as I understand it, the whole point of the 6 week release cadence is
that
> the cost of missing a release isn't that large.  Can we just put this into
> DRL/DRT in M45?

Discussed offline--I'm withdrawing my objection, but would really like to see
this done right as soon as we can.

[commit-bot: I haz the power, 2015-07-07 17:18:00]

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

[commit-bot: I haz the power, 2015-07-07 17:18:01]

Dry run: This issue passed the CQ dry run.

[qinmin, 2015-07-07 17:33:54]

On 2015/07/07 17:18:01, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

ping, @asanka, is this CL good to go? I am working on moving the logic to
DownloadResourceThrottle, and will upload the change once it is finished.

[asanka, 2015-07-07 17:39:24]

LGTM modulo nits. Optionally nits could be resolved during the next cleanup CL.

https://codereview.chromium.org/1217223006/diff/120001/chrome/browser/downloa...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/1217223006/diff/120001/chrome/browser/downloa...
chrome/browser/download/download_target_determiner.cc:197: void
DownloadTargetDeterminer::PromptUserForPermissionDone(bool granted) {
Nit: This is dead code on non OS_ANDROID

https://codereview.chromium.org/1217223006/diff/120001/content/browser/androi...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1217223006/diff/120001/content/browser/androi...
content/browser/android/download_controller_android_impl.cc:75: if
(!DownloadControllerAndroid::download_controller_) {
Nit: Given that this code no longer has the atomicity guarantee that it used to
have, it's worth binding it to a thread with a DCHECK_CURRENTLY_ON(...);

https://codereview.chromium.org/1217223006/diff/120001/content/browser/androi...
content/browser/android/download_controller_android_impl.cc:157: if
(view.is_null()) {
Nit: This check should be performed in AcquireFileAccessPermission() above.

Once that's moved, the rest of the function just consists of the
requestFileAccess() call, which might as well also be folded into AFAP().

[qinmin, 2015-07-07 20:52:50]

https://codereview.chromium.org/1217223006/diff/120001/chrome/browser/downloa...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/1217223006/diff/120001/chrome/browser/downloa...
chrome/browser/download/download_target_determiner.cc:197: void
DownloadTargetDeterminer::PromptUserForPermissionDone(bool granted) {
On 2015/07/07 17:39:24, asanka wrote:
> Nit: This is dead code on non OS_ANDROID

Done.

https://codereview.chromium.org/1217223006/diff/120001/content/browser/androi...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1217223006/diff/120001/content/browser/androi...
content/browser/android/download_controller_android_impl.cc:75: if
(!DownloadControllerAndroid::download_controller_) {
On 2015/07/07 17:39:24, asanka wrote:
> Nit: Given that this code no longer has the atomicity guarantee that it used
to
> have, it's worth binding it to a thread with a DCHECK_CURRENTLY_ON(...);

CreateGETDownload was called on IO thread. Moving it to UI thread involves quite
some refactoring. Added a lock here to protect the download_controller_ instead.

https://codereview.chromium.org/1217223006/diff/120001/content/browser/androi...
content/browser/android/download_controller_android_impl.cc:157: if
(view.is_null()) {
On 2015/07/07 17:39:24, asanka wrote:
> Nit: This check should be performed in AcquireFileAccessPermission() above.
> 
> Once that's moved, the rest of the function just consists of the
> requestFileAccess() call, which might as well also be folded into AFAP().

Done.

[qinmin, 2015-07-07 20:52:57]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2015-07-07 20:52:58]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/1217223006/#ps140001
(title: "nits")

[commit-bot: I haz the power, 2015-07-07 20:54:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1217223006/140001

[commit-bot: I haz the power, 2015-07-07 22:06:14]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-07-07 22:08:17]

Patchset 6 (id:??) landed as
https://crrev.com/1fa54a4e6bac3ecb49a61ee9594fe73fc23edd0d
Cr-Commit-Position: refs/heads/master@{#337686}