Fix cancelling permission requests on Android when the PermissionRequestManager is enabled

Fix cancelling permission requests on Android when the PermissionRequestManager is enabled

This patch fixes cancelling of permission requests (infobars) on Android
when the PermissionRequestManager is enabled. While we do not actually
support cancelling infobars in this code path (we decided that for code
simplicity we the PermissionPromptAndroid shouldn't keep a pointer to
the InfoBar), we still need to be able to gracefully handle the case of
an iframe which has requested a permission navigating (which currently
crashes upon resolving the prompt).

We change CanAcceptRequestUpdate() + Hide() into MaybeCancelRequest(),
and now call Show() again if the cancel fails so that on Android we can
use the dummy PermissionRequests instead of UAFing on the original ones.

BUG=606138, 671052

[Timothy Loh, 2017-04-20 05:24:24]

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

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

Dry run: 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-20 05:56:32]

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

[commit-bot: I haz the power, 2017-04-20 05:56:33]

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

[Timothy Loh, 2017-04-21 03:52:42]

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

[commit-bot: I haz the power, 2017-04-21 03:52:59]

Dry run: 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-21 04:37:35]

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

[commit-bot: I haz the power, 2017-04-21 04:37:36]

Dry run: This issue passed the CQ dry run.

[Timothy Loh, 2017-04-27 03:55:37]

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

[commit-bot: I haz the power, 2017-04-27 03:56:22]

Dry run: CQ is trying da patch.

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

[Timothy Loh, 2017-04-27 05:10:41]

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

[commit-bot: I haz the power, 2017-04-27 05:11:06]

Dry run: CQ is trying da patch.

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

[Timothy Loh, 2017-04-27 05:57:58]

Description was changed from

==========
wip not for review

BUG=
==========

to

==========
Fix cancelling permission requests on Android when the PermissionRequestManager
is enabled

This patch fixes cancelling of permission requests (infobars) on Android
when the PermissionRequestManager is enabled. While we do not actually
support cancelling infobars in this code path (we decided that for code
simplicity we the PermissionPromptAndroid shouldn't keep a pointer to
the InfoBar), we still need to be able to gracefully handle the case of
an iframe which has requested a permission navigating (which currently
crashes upon resolving the prompt).

We change CanAcceptRequestUpdate() + Hide() into MaybeCancelRequest(),
and now call Show() again if the cancel fails so that on Android we can
use the dummy PermissionRequests instead of UAFing on the original ones.

BUG=606138,671052
==========

[Timothy Loh, 2017-04-27 05:57:58]

timloh@chromium.org changed reviewers:
+ raymes@chromium.org

[Timothy Loh, 2017-04-27 08:31:02]

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

[commit-bot: I haz the power, 2017-04-27 08:31:16]

Dry run: CQ is trying da patch.

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

[Timothy Loh, 2017-04-27 09:50:09]

This doesn't feel great but it's the best I came up with (aside from actually
cancelling...)

I think the added call to Show() is what was intended when cancelling was added
(going by the edited comments in permission_prompt.h), and it just didn't get
implemented because it didn't seem to be needed by the existing code-paths.

[commit-bot: I haz the power, 2017-04-27 10:17:04]

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

[commit-bot: I haz the power, 2017-04-27 10:17:04]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-05-01 04:23:35]

https://codereview.chromium.org/2829023002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_prompt_android.cc (right):

https://codereview.chromium.org/2829023002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_prompt_android.cc:46: // We do not support
cancelling on Android to keep the code simple.
Isn't the reason because we think that infobars should not disappear while the
user is viewing them?

https://codereview.chromium.org/2829023002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_request_manager.cc (right):

https://codereview.chromium.org/2829023002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_manager.cc:196: } else if
(view_->MaybeCancelRequest()) {
I think we could document each of these cases better. Firstly, maybe we should
talk about hiding the prompt here rather than cancelling the request, because
this won't actually modify the request? Secondly, the prompt will be hidden if
the user isn't interacting with the prompt. So maybe we should call the function
something like:
HideIfUserNotInteracting ?

https://codereview.chromium.org/2829023002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_manager.cc:202: // Cancel the
existing request and replace it with a dummy.
I think we can clarify this comment a bit.
// If the user is still interacting with the prompt, we don't 
// hide it but we still cancel the permission request and replace
// it with a dummy request that will do nothing when the permission
// prompt is answered.

https://codereview.chromium.org/2829023002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_manager.cc:207:
view_->Show(requests_, accept_states_);
As discussed I think it would be preferable to allow access to the vector of
requests via the delegate, which would remove the need for calling Show() again
and also ensure that we don't have the 2 lists getting out of sync,

[Timothy Loh, 2017-05-09 04:29:11]

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

[commit-bot: I haz the power, 2017-05-09 04:30:26]

Dry run: 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-05-09 05:20:44]

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

[commit-bot: I haz the power, 2017-05-09 05:20:45]

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