Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2829023002: Fix cancelling permission requests on Android when the PermissionRequestManager is enabled (Closed)

Created:
7 months ago by Timothy Loh
Modified:
5 months, 3 weeks ago
Reviewers:
raymes
CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, tfarina, raymes+watch_chromium.org, mac-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : cancel thing #

Patch Set 3 : wip test #

Patch Set 4 : fix up #

Patch Set 5 : comment #

Patch Set 6 : comment #

Total comments: 4

Patch Set 7 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+170 lines, -54 lines) Patch
M chrome/android/BUILD.gn View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M chrome/android/java_sources.gni View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
A chrome/android/javatests/src/org/chromium/chrome/browser/permissions/PermissionFrameTest.java View 1 2 3 1 chunk +65 lines, -0 lines 0 comments Download
M chrome/android/javatests/src/org/chromium/chrome/browser/permissions/PermissionTestCaseBase.java View 1 2 3 4 5 6 6 chunks +50 lines, -16 lines 0 comments Download
M chrome/browser/permissions/permission_prompt_android.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/permissions/permission_prompt_android.cc View 1 2 3 4 5 6 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/permissions/permission_request_manager.cc View 1 2 3 4 5 6 1 chunk +7 lines, -16 lines 0 comments Download
M chrome/browser/ui/cocoa/permission_bubble/permission_bubble_cocoa.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/cocoa/permission_bubble/permission_bubble_cocoa.mm View 1 2 3 4 5 6 2 chunks +7 lines, -8 lines 0 comments Download
M chrome/browser/ui/permission_bubble/mock_permission_prompt.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/permission_bubble/mock_permission_prompt.cc View 1 2 3 4 5 6 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/ui/permission_bubble/permission_prompt.h View 1 2 3 4 5 6 1 chunk +7 lines, -4 lines 0 comments Download
M chrome/browser/ui/views/permission_bubble/permission_prompt_impl.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc View 1 2 3 4 5 6 1 chunk +5 lines, -2 lines 0 comments Download
A content/test/data/android/permission_frame_test.html View 1 2 3 1 chunk +17 lines, -0 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 24 (22 generated)
Timothy Loh
This doesn't feel great but it's the best I came up with (aside from actually ...
6 months, 3 weeks ago (2017-04-27 09:50:09 UTC) #17
raymes
6 months, 2 weeks ago (2017-05-01 04:23:35 UTC) #20
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,

Powered by Google App Engine
This is Rietveld efc10ee0f