Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2143573002:
Implement AwPermissionManager::RequestPermissions (Closed)
|
Created:
4 years, 5 months ago by Takashi Toyoshima Modified:
4 years, 4 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement AwPermissionManager::RequestPermissions
This is needed to work with new Web MIDI permission requests.
BUG=582328
TEST=git cl try
TEST=run_android_webview_unittests --gtest_filter='AwPermissionManagerTest.*'
TEST=run_system_webview_shell_layout_test_apk with built SystemWebViewGoogle.apk
Committed: https://crrev.com/055d3a2166c34eeef2a15281fdfb0082b7994787
Cr-Commit-Position: refs/heads/master@{#408371}
Patch Set 1 #
Total comments: 9
Patch Set 2 : review #5 #Patch Set 3 : git cl format #
Total comments: 14
Patch Set 4 : review #11 #
Total comments: 2
Patch Set 5 : review #13 #
Total comments: 2
Patch Set 6 : review #15, #16 (check requesting_origin) #Patch Set 7 : adding unit tests (wip) #Patch Set 8 : unit tests and some minor fixes #
Total comments: 4
Patch Set 9 : reviw #20 #
Total comments: 4
Patch Set 10 : review #25 #Patch Set 11 : (rebase - nothing was changed from PS10) #Patch Set 12 : allow multiple requests #
Messages
Total messages: 41 (15 generated)
Description was changed from ========== Implement AwPermissionManager::RequestPermissions This is needed to work with new Web MIDI permission requests. BUG=582328 TEST=git cl try TEST=run_system_webview_shell_layout_test_apk with built SystemWebViewGoogle.apk ========== to ========== Implement AwPermissionManager::RequestPermissions This is needed to work with new Web MIDI permission requests. BUG=582328 TEST=git cl try TEST=run_system_webview_shell_layout_test_apk with built SystemWebViewGoogle.apk ==========
toyoshim@chromium.org changed reviewers: + tobiasjs@chromium.org
This is another version of https://codereview.chromium.org/2125893002/ that does not have dependency to https://codereview.chromium.org/2116763002/. If this code can be submitted first, we do not need workarounds in order to use mojom::blink::PermissionService from Web MIDI, though test expectations will still need to be changed slightly even after this change.
toyoshim@chromium.org changed reviewers: + torne@chromium.org
https://codereview.chromium.org/2143573002/diff/1/android_webview/browser/aw_... File android_webview/browser/aw_permission_manager.cc (right): https://codereview.chromium.org/2143573002/diff/1/android_webview/browser/aw_... android_webview/browser/aw_permission_manager.cc:148: class AwPermissionManager::PendingRequest { I understand the desire to clean up, but this class is internal to this file, and reordering arguments/initializers/members and adding a lot of getters complicates the diff and doesn't appear to add to the readability. Would you please consider trying to minimise the size of this change and do cleanup in a different CL? https://codereview.chromium.org/2143573002/diff/1/android_webview/browser/aw_... android_webview/browser/aw_permission_manager.cc:176: results_[i] = status; Call SetPermissionStatus(i, status) here. https://codereview.chromium.org/2143573002/diff/1/android_webview/browser/aw_... android_webview/browser/aw_permission_manager.cc:332: case PermissionType::BACKGROUND_SYNC: After your change, |callback| will not be called until something calls OnRequestResponse, which might not happen in a timely fashion (or at all). Maybe you could only add pending_request to pending_requests_ at the end of the function, if it's not already complete (and otherwise call the callback). This would also make it easier to make pending_request a unique_ptr. https://codereview.chromium.org/2143573002/diff/1/android_webview/browser/aw_... android_webview/browser/aw_permission_manager.cc:447: pending_requests_.Remove(request_id); Should this call be outside the loop?
Thanks, please take another look. https://codereview.chromium.org/2143573002/diff/1/android_webview/browser/aw_... File android_webview/browser/aw_permission_manager.cc (right): https://codereview.chromium.org/2143573002/diff/1/android_webview/browser/aw_... android_webview/browser/aw_permission_manager.cc:148: class AwPermissionManager::PendingRequest { My intention was to minimize difference between this AwPermissionManager and the original PermissionManager (chrome/browser/permissions/permission_manager.cc). Anyway, this is in android_webview/. As you said, keeping the original style would make this file consistent to other WebView code. Let me revert unnecessary changes. https://codereview.chromium.org/2143573002/diff/1/android_webview/browser/aw_... android_webview/browser/aw_permission_manager.cc:176: results_[i] = status; On 2016/07/12 12:43:23, Tobias Sargeant wrote: > Call SetPermissionStatus(i, status) here. Done. https://codereview.chromium.org/2143573002/diff/1/android_webview/browser/aw_... android_webview/browser/aw_permission_manager.cc:233: for (PendingRequestsMap::Iterator<PendingRequest> it(&pending_requests_); Should I call CancelPermissionRequest too? https://codereview.chromium.org/2143573002/diff/1/android_webview/browser/aw_... android_webview/browser/aw_permission_manager.cc:332: case PermissionType::BACKGROUND_SYNC: Thank you for catching this bug. request_id is needed to call delegate functions, and it could be obtained by pending_requests_.Add(pending_request). So probably, I should check IsComplete() after this for-loop, then kick the callback if it is needed. By the way, chrome's PermissionManager also cares for the case the callback was invoked synchronously and pending_request was removed from the pending_request_at the end of the for-loop. I'd also add the same check just in case. https://codereview.chromium.org/2143573002/diff/1/android_webview/browser/aw_... android_webview/browser/aw_permission_manager.cc:447: pending_requests_.Remove(request_id); Right. Thank you for catching this bug too.
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2143573002/diff/40001/android_webview/browser... File android_webview/browser/aw_permission_manager.cc (right): https://codereview.chromium.org/2143573002/diff/40001/android_webview/browser... android_webview/browser/aw_permission_manager.cc:271: for (size_t i = 0; i < permissions.size(); ++i) { I think there's another issue here: Imagine you have a prior pending request for two permissions, and one has already returned via OnRequestResponse, but the second hasn't. Then you request a single permission equal to the one that has been completed. You would correctly not kick off a new request, but it would never be completed because the OnRequestResponse that completes the first request would not complete the second request. I think maybe what you should do here is subsequently check if i.GetCurrentValue().IsComplete(permission_type) [would have to add this] and if so, directly set the permission status for this request to the result from the old request. https://codereview.chromium.org/2143573002/diff/40001/android_webview/browser... android_webview/browser/aw_permission_manager.cc:354: if (!manager.get()) Previously the callback would be run even if the manager had been gc'ed. I can't tell if this is a problem in practice. https://codereview.chromium.org/2143573002/diff/40001/android_webview/browser... android_webview/browser/aw_permission_manager.cc:395: const GURL& embedding_origin = pending_request->embedding_origin; Is the change to how embedding_origin is calculated significant? I presume that this code could have used pending_request->embedding_origin previously. https://codereview.chromium.org/2143573002/diff/40001/android_webview/browser... android_webview/browser/aw_permission_manager.cc:410: for (auto permission : pending_request->permissions) { Here, you could end up sending a cancel for a request that has completed. I guess you could use the same IsComplete(permission_type) to avoid that if necessary.
https://codereview.chromium.org/2143573002/diff/40001/android_webview/browser... File android_webview/browser/aw_permission_manager.cc (right): https://codereview.chromium.org/2143573002/diff/40001/android_webview/browser... android_webview/browser/aw_permission_manager.cc:271: for (size_t i = 0; i < permissions.size(); ++i) { Oh, thank you again. That's the case I overlooked. https://codereview.chromium.org/2143573002/diff/40001/android_webview/browser... android_webview/browser/aw_permission_manager.cc:276: // TODO(toyoshim): Shall we check to match requesting_origin too? I would like to revisit this TODO now to understand the original code well. The original RequestPermission() also does not check requesting_origin in the equivalent check, but delegate->Request*Permission() looks per-requesting_origin manner. Was this a problem? If I understand correctly, the very right implementation should check requesting_origin here, and if it is different, we should enqueue this request so that we can call delegate->Request*Permission() function after the previous call for the same permission type is resolved. That will make the PendingRequest handling a bit complicated. Another curious truth is that result_cache_ cares both requesting_origin and embedding_origin. But probably this is because ResetPermission() and GetPermissionStatus() interfaces require to aware both origins. https://codereview.chromium.org/2143573002/diff/40001/android_webview/browser... android_webview/browser/aw_permission_manager.cc:354: if (!manager.get()) So, how about changing this check to DCHECK with some comments? https://codereview.chromium.org/2143573002/diff/40001/android_webview/browser... android_webview/browser/aw_permission_manager.cc:395: const GURL& embedding_origin = pending_request->embedding_origin; The original code calculate the embedding_origin from WebContents again. But could it make a different result? I just reuse cached embedding_origin here as we do in other places. The embedding_origin in the pending_request object was calculated through completely same functions when it was added. Probably, we could remove comments above because we do not need any assumption here. We can pickup right information for permission type and origins from the request_id. And, actually, the embedding_origin is used only for |result_cache_| query. In my understanding, the origin pair should be the same with the one we used in RequestPermissions(). So, if recalculation could make a different result, using cached value is correct idea. https://codereview.chromium.org/2143573002/diff/40001/android_webview/browser... android_webview/browser/aw_permission_manager.cc:410: for (auto permission : pending_request->permissions) { Also I notice another issue. We can call the actual delegate Cancel method only when the pending_request is the only one that requests this type of permission. I'd check all pending_request_ for each permission, and will need to introduce additional flag to PendingRequest class to mark the request is cancelled.
https://codereview.chromium.org/2143573002/diff/40001/android_webview/browser... File android_webview/browser/aw_permission_manager.cc (right): https://codereview.chromium.org/2143573002/diff/40001/android_webview/browser... android_webview/browser/aw_permission_manager.cc:276: // TODO(toyoshim): Shall we check to match requesting_origin too? On 2016/07/14 06:42:52, toyoshim wrote: > I would like to revisit this TODO now to understand the original code well. > > The original RequestPermission() also does not check requesting_origin in the > equivalent check, but delegate->Request*Permission() looks per-requesting_origin > manner. Was this a problem? > > If I understand correctly, the very right implementation should check > requesting_origin here, and if it is different, we should enqueue this request > so that we can call delegate->Request*Permission() function after the previous > call for the same permission type is resolved. That will make the PendingRequest > handling a bit complicated. > > Another curious truth is that result_cache_ cares both requesting_origin and > embedding_origin. But probably this is because ResetPermission() and > GetPermissionStatus() interfaces require to aware both origins. My feeling is that it's more consistent to check the requesting_origin. As you point out, it's an argument to the delegate methods, and as such if we avoid it we are assuming that the response for any origin is the same. However, sadly the Java API doesn't pass this information on, so there's nothing that an app can use to distinguish the two responses. Unfortunately the WebView API is just broken in this regard. It's probably not going to break any tests to make that change, but I don't have a strong intuition about how this will affect apps in the wild. torne@ should probably make the call about whether to be correct here and potentially cause more permission requests to the app, or stick with the old behaviour. https://codereview.chromium.org/2143573002/diff/40001/android_webview/browser... android_webview/browser/aw_permission_manager.cc:354: if (!manager.get()) On 2016/07/14 06:42:52, toyoshim wrote: > So, how about changing this check to DCHECK with some comments? Good. https://codereview.chromium.org/2143573002/diff/40001/android_webview/browser... android_webview/browser/aw_permission_manager.cc:395: const GURL& embedding_origin = pending_request->embedding_origin; On 2016/07/14 06:42:51, toyoshim wrote: > The original code calculate the embedding_origin from WebContents again. But > could it make a different result? I just reuse cached embedding_origin here as > we do in other places. The embedding_origin in the pending_request object was > calculated through completely same functions when it was added. > > Probably, we could remove comments above because we do not need any assumption > here. We can pickup right information for permission type and origins from the > request_id. > > And, actually, the embedding_origin is used only for |result_cache_| query. In > my understanding, the origin pair should be the same with the one we used in > RequestPermissions(). So, if recalculation could make a different result, using > cached value is correct idea. Good point about the cache. https://codereview.chromium.org/2143573002/diff/60001/android_webview/browser... File android_webview/browser/aw_permission_manager.cc (right): https://codereview.chromium.org/2143573002/diff/60001/android_webview/browser... android_webview/browser/aw_permission_manager.cc:450: if (it.GetCurrentValue()->HasPermissionType(permission)) { && !it.GetCurrentValue()->IsComplete(permission)
https://codereview.chromium.org/2143573002/diff/60001/android_webview/browser... File android_webview/browser/aw_permission_manager.cc (right): https://codereview.chromium.org/2143573002/diff/60001/android_webview/browser... android_webview/browser/aw_permission_manager.cc:450: if (it.GetCurrentValue()->HasPermissionType(permission)) { Thanks, I misunderstood that check at line 440 was enough.
https://codereview.chromium.org/2143573002/diff/40001/android_webview/browser... File android_webview/browser/aw_permission_manager.cc (right): https://codereview.chromium.org/2143573002/diff/40001/android_webview/browser... android_webview/browser/aw_permission_manager.cc:276: // TODO(toyoshim): Shall we check to match requesting_origin too? On 2016/07/14 09:24:29, Tobias Sargeant wrote: > On 2016/07/14 06:42:52, toyoshim wrote: > > I would like to revisit this TODO now to understand the original code well. > > > > The original RequestPermission() also does not check requesting_origin in the > > equivalent check, but delegate->Request*Permission() looks > per-requesting_origin > > manner. Was this a problem? > > > > If I understand correctly, the very right implementation should check > > requesting_origin here, and if it is different, we should enqueue this request > > so that we can call delegate->Request*Permission() function after the previous > > call for the same permission type is resolved. That will make the > PendingRequest > > handling a bit complicated. > > > > Another curious truth is that result_cache_ cares both requesting_origin and > > embedding_origin. But probably this is because ResetPermission() and > > GetPermissionStatus() interfaces require to aware both origins. > > My feeling is that it's more consistent to check the requesting_origin. As you > point out, it's an argument to the delegate methods, and as such if we avoid it > we are assuming that the response for any origin is the same. However, sadly the > Java API doesn't pass this information on, so there's nothing that an app can > use to distinguish the two responses. Unfortunately the WebView API is just > broken in this regard. > > It's probably not going to break any tests to make that change, but I don't have > a strong intuition about how this will affect apps in the wild. > > torne@ should probably make the call about whether to be correct here and > potentially cause more permission requests to the app, or stick with the old > behaviour. The Java code does pass the requesting origin up, just not the embedding origin. We discussed this in person and agree that we should check the requesting origin, and the previous code was broken.
https://codereview.chromium.org/2143573002/diff/80001/android_webview/browser... File android_webview/browser/aw_permission_manager.cc (right): https://codereview.chromium.org/2143573002/diff/80001/android_webview/browser... android_webview/browser/aw_permission_manager.cc:454: if (it.GetCurrentValue()->HasPermissionType(permission) && checking the requesting origin when making the request means that we probably need to check the requesting origin when cancelling, too.
I update the CL to check requesting_origin. This requires additional logic to request another permission to delegate when the previous one is finished, or cancelled, and probably I should check my code again by myself tomorrow. https://codereview.chromium.org/2143573002/diff/40001/android_webview/browser... File android_webview/browser/aw_permission_manager.cc (right): https://codereview.chromium.org/2143573002/diff/40001/android_webview/browser... android_webview/browser/aw_permission_manager.cc:276: // TODO(toyoshim): Shall we check to match requesting_origin too? OK, let me add requesting_origin check here, and other related places. https://codereview.chromium.org/2143573002/diff/80001/android_webview/browser... File android_webview/browser/aw_permission_manager.cc (right): https://codereview.chromium.org/2143573002/diff/80001/android_webview/browser... android_webview/browser/aw_permission_manager.cc:454: if (it.GetCurrentValue()->HasPermissionType(permission) && On 2016/07/14 10:03:40, Tobias Sargeant wrote: > checking the requesting origin when making the request means that we probably > need to check the requesting origin when cancelling, too. Done.
Update: I factored out RFH touching code to virtual methods so that I can write unit tests. I'm now writing test cases we discussed in the review. Please stay tuned until I upload the new patch set.
Hi, since I finished to write unit tests as PS8, can you take another look? The test source is a little large, but I believe this is useful now and in the future.
Thanks for the comprehensive tests! https://codereview.chromium.org/2143573002/diff/140001/android_webview/browse... File android_webview/browser/aw_permission_manager.cc (right): https://codereview.chromium.org/2143573002/diff/140001/android_webview/browse... android_webview/browser/aw_permission_manager.cc:486: CancelPermissionRequest(request_id); Why is the callback no longer run? https://codereview.chromium.org/2143573002/diff/140001/android_webview/browse... File android_webview/browser/aw_permission_manager_unittest.cc (right): https://codereview.chromium.org/2143573002/diff/140001/android_webview/browse... android_webview/browser/aw_permission_manager_unittest.cc:130: std::list<Response*> response_; Can you make these lists of unique_ptr so that you don't leak objects when you call list::erase?
https://codereview.chromium.org/2143573002/diff/140001/android_webview/browse... File android_webview/browser/aw_permission_manager.cc (right): https://codereview.chromium.org/2143573002/diff/140001/android_webview/browse... android_webview/browser/aw_permission_manager.cc:486: CancelPermissionRequest(request_id); No string reason, but I decided not to call it because of following reasons. - generally speaking, invoking uncontrollable callbacks in shutdown/destructors isn't easy. easy to broken for many reasons and could result in security issues. - when PermissionManager shutdown, we could not expect renders continue to work fine, and invoking callbacks here isn't useless. probably we want to use Mojo's error handler for such cases if something should be handled correctly. - We can return DENIED in such cases, but it isn't ideal, but probably we may introduce ABORT or something, but it looks overkill just for handling such exceptional cases here. https://codereview.chromium.org/2143573002/diff/140001/android_webview/browse... File android_webview/browser/aw_permission_manager_unittest.cc (right): https://codereview.chromium.org/2143573002/diff/140001/android_webview/browse... android_webview/browser/aw_permission_manager_unittest.cc:130: std::list<Response*> response_; On 2016/07/15 13:44:35, Tobias Sargeant wrote: > Can you make these lists of unique_ptr so that you don't leak objects when you > call list::erase? Done.
> fine, and invoking callbacks here isn't useless. probably we want to use Mojo's Sorry, invoking callbacks here is useless, or isn't useful, that's I want to say..
OK, the last patch set passed "git cl try" again. PTAL.
friendly ping (Tobias is back today?)
https://codereview.chromium.org/2143573002/diff/160001/android_webview/browse... File android_webview/browser/aw_permission_manager.cc (right): https://codereview.chromium.org/2143573002/diff/160001/android_webview/browse... android_webview/browser/aw_permission_manager.cc:268: should_delegate_request = false; Should this also be dependent on a requesting_origin match? https://codereview.chromium.org/2143573002/diff/160001/android_webview/browse... android_webview/browser/aw_permission_manager.cc:490: bool AwPermissionManager::RequestPermissionInternal( Couldn't this method just take request and delegate as parameters, rather than looking them up each time?
https://codereview.chromium.org/2143573002/diff/160001/android_webview/browse... File android_webview/browser/aw_permission_manager.cc (right): https://codereview.chromium.org/2143573002/diff/160001/android_webview/browse... android_webview/browser/aw_permission_manager.cc:268: should_delegate_request = false; No. If there is a running request for the same permission type, we should avoid to make a new request for the same type simultaneously. This is because each delegate method for the permissions can accept only one running request IIUC, and the next request should wait until the previous one is resolved. In OnRequestResponse(), MayRequestNextPermission() is called. It checks all pending requests, and if a request for the same type that is just resolved exists, make a new delegate call for it. This case is covered by multiple tests, e.g. ComplicatedRequestScenario2. https://codereview.chromium.org/2143573002/diff/160001/android_webview/browse... android_webview/browser/aw_permission_manager.cc:490: bool AwPermissionManager::RequestPermissionInternal( On 2016/07/25 13:29:53, Tobias Sargeant wrote: > Couldn't this method just take request and delegate as parameters, rather than > looking them up each time? Done.
On 2016/07/26 04:58:30, toyoshim wrote: > This is because each delegate method for the permissions can accept only one > running request IIUC, and the next request should wait until the previous one is > resolved. Itt seems like Request and Cancel delegate methods take a requesting_origin, so at this level there's enough information to distinguish. What makes you think the requests need to be serialized?
Oh, you are right. I just thought each delegate method can not accept multiple request at once, and that was the reason why original code prefer not supporting strict origin checks and why we need pending request management here. But that was my wrong conclusion. OK, let me fix this to support multiple requests. But it also requires modifying test scenarios. I probably can work on that tomorrow.
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patch Set 12 was the one I allowed to request multiple same type permissions to delegate. It's more like Patch Set 5 that didn't support request_origin checks, and diff size gets to be smaller than Patch Set 11. Please take a look again.
Description was changed from ========== Implement AwPermissionManager::RequestPermissions This is needed to work with new Web MIDI permission requests. BUG=582328 TEST=git cl try TEST=run_system_webview_shell_layout_test_apk with built SystemWebViewGoogle.apk ========== to ========== Implement AwPermissionManager::RequestPermissions This is needed to work with new Web MIDI permission requests. BUG=582328 TEST=git cl try TEST=run_android_webview_unittests --gtest_filter='AwPermissionManagerTest.*' TEST=run_system_webview_shell_layout_test_apk with built SystemWebViewGoogle.apk ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, thanks.
The CQ bit was checked by toyoshim@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Implement AwPermissionManager::RequestPermissions This is needed to work with new Web MIDI permission requests. BUG=582328 TEST=git cl try TEST=run_android_webview_unittests --gtest_filter='AwPermissionManagerTest.*' TEST=run_system_webview_shell_layout_test_apk with built SystemWebViewGoogle.apk ========== to ========== Implement AwPermissionManager::RequestPermissions This is needed to work with new Web MIDI permission requests. BUG=582328 TEST=git cl try TEST=run_android_webview_unittests --gtest_filter='AwPermissionManagerTest.*' TEST=run_system_webview_shell_layout_test_apk with built SystemWebViewGoogle.apk ==========
Message was sent while issue was closed.
Committed patchset #12 (id:210001)
Message was sent while issue was closed.
Description was changed from ========== Implement AwPermissionManager::RequestPermissions This is needed to work with new Web MIDI permission requests. BUG=582328 TEST=git cl try TEST=run_android_webview_unittests --gtest_filter='AwPermissionManagerTest.*' TEST=run_system_webview_shell_layout_test_apk with built SystemWebViewGoogle.apk ========== to ========== Implement AwPermissionManager::RequestPermissions This is needed to work with new Web MIDI permission requests. BUG=582328 TEST=git cl try TEST=run_android_webview_unittests --gtest_filter='AwPermissionManagerTest.*' TEST=run_system_webview_shell_layout_test_apk with built SystemWebViewGoogle.apk Committed: https://crrev.com/055d3a2166c34eeef2a15281fdfb0082b7994787 Cr-Commit-Position: refs/heads/master@{#408371} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/055d3a2166c34eeef2a15281fdfb0082b7994787 Cr-Commit-Position: refs/heads/master@{#408371} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
