Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2899973002:
Hide modal permission prompts on Android upon tab navigation/destruction (Closed)
|
Created:
3 years, 7 months ago by Timothy Loh Modified:
3 years, 6 months ago CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, jam, darin-cc_chromium.org, raymes+watch_chromium.org, agrieve+watch_chromium.org, chrome-apps-syd-reviews_chromium.org, gone Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionHide modal permission prompts on Android upon tab navigation/destruction
This patch makes modal permission prompts on Android disappear upon
tab navigation or destruction. We make the PermissionDialogDelegate on
the C++ side a WebContentsObserver to detect these events, and notify
the Java side so the UI can be removed.
BUG=699851
Review-Url: https://codereview.chromium.org/2899973002
Cr-Commit-Position: refs/heads/master@{#475297}
Committed: https://chromium.googlesource.com/chromium/src/+/3fd7dbe308d04ad7132bae51d86aceb33885b4ab
Patch Set 1 #Patch Set 2 : clean up #
Total comments: 20
Patch Set 3 : address comments #
Total comments: 13
Patch Set 4 : address more comments #Patch Set 5 : fix test (permission request from js goes through mojo so need to poll) #Messages
Total messages: 35 (21 generated)
The CQ bit was checked by timloh@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...
timloh@chromium.org changed reviewers: + dominickn@chromium.org
The CQ bit was checked by timloh@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
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The circular deletes are a bit scary. I can't *think* of a potential use-after-free case, but I'll keep thinking about it.... https://codereview.chromium.org/2899973002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java (right): https://codereview.chromium.org/2899973002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:263: public void destroyFromNative(PermissionDialogDelegate delegate) { Nit: I would call this dismissFromNative. "Destroy" is more used for Java calling C++ destructors. https://codereview.chromium.org/2899973002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:264: if (mDialogDelegate == delegate) { Maybe compare the native pointer to be safe. https://codereview.chromium.org/2899973002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:266: AlertDialog dialog = mDialog; Nit: is it problematic to do: mDialog.dismiss(); mDialog = null; https://codereview.chromium.org/2899973002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:270: assert mRequestQueue.contains(delegate); I don't think remove() fails if delegate isn't in the list. Maybe this assert isn't necessary. https://codereview.chromium.org/2899973002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogDelegate.java (right): https://codereview.chromium.org/2899973002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogDelegate.java:110: assert mDialogController == null; I don't think this assert is needed. https://codereview.chromium.org/2899973002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogDelegate.java:118: private void destroyFromNative() { dismissFromNative https://codereview.chromium.org/2899973002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_dialog_delegate.cc (right): https://codereview.chromium.org/2899973002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:167: content::WebContents* web_contents, No need to pass this: use tab->web_contents() https://codereview.chromium.org/2899973002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:169: : WebContentsObserver(web_contents), Nit: content::WebContentsObserver for completeness https://codereview.chromium.org/2899973002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:187: void PermissionDialogDelegate::DestroyJavaDelegate() { Call this DismissDialog() https://codereview.chromium.org/2899973002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:194: if (!navigation_handle->IsInMainFrame() || This means that if we have a third party iframe that is showing a permission request, and it navigates, the permission request remains up. Seems like an edge case though. It may actually be worth adding a test for it.
https://codereview.chromium.org/2899973002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java (right): https://codereview.chromium.org/2899973002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:263: public void destroyFromNative(PermissionDialogDelegate delegate) { On 2017/05/24 03:33:38, dominickn wrote: > Nit: I would call this dismissFromNative. "Destroy" is more used for Java > calling C++ destructors. Done. https://codereview.chromium.org/2899973002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:264: if (mDialogDelegate == delegate) { On 2017/05/24 03:33:38, dominickn wrote: > Maybe compare the native pointer to be safe. Not sure what you want, do you mean that the C++ PDD should pass itself to the dismiss function on the Java PDD? https://codereview.chromium.org/2899973002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:266: AlertDialog dialog = mDialog; On 2017/05/24 03:33:38, dominickn wrote: > Nit: is it problematic to do: > > mDialog.dismiss(); > mDialog = null; Probably not but I think we end up in a weird state because scheduleDisplay() won't end up dequeueing any requests. It shouldn't matter since any queued requests are gonna get removed anyway, but it seems odd. https://codereview.chromium.org/2899973002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:270: assert mRequestQueue.contains(delegate); On 2017/05/24 03:33:38, dominickn wrote: > I don't think remove() fails if delegate isn't in the list. Maybe this assert > isn't necessary. I added it as a sanity check because remove() doesn't fail. https://codereview.chromium.org/2899973002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogDelegate.java (right): https://codereview.chromium.org/2899973002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogDelegate.java:110: assert mDialogController == null; On 2017/05/24 03:33:38, dominickn wrote: > I don't think this assert is needed. OK, removed https://codereview.chromium.org/2899973002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogDelegate.java:118: private void destroyFromNative() { On 2017/05/24 03:33:38, dominickn wrote: > dismissFromNative Done. https://codereview.chromium.org/2899973002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_dialog_delegate.cc (right): https://codereview.chromium.org/2899973002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:167: content::WebContents* web_contents, On 2017/05/24 03:33:38, dominickn wrote: > No need to pass this: use tab->web_contents() Done. https://codereview.chromium.org/2899973002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:169: : WebContentsObserver(web_contents), On 2017/05/24 03:33:38, dominickn wrote: > Nit: content::WebContentsObserver for completeness Done. https://codereview.chromium.org/2899973002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:187: void PermissionDialogDelegate::DestroyJavaDelegate() { On 2017/05/24 03:33:38, dominickn wrote: > Call this DismissDialog() Done. https://codereview.chromium.org/2899973002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:194: if (!navigation_handle->IsInMainFrame() || On 2017/05/24 03:33:38, dominickn wrote: > This means that if we have a third party iframe that is showing a permission > request, and it navigates, the permission request remains up. Seems like an edge > case though. > > It may actually be worth adding a test for it. Yeah, I might add some tests in a separate patch. https://codereview.chromium.org/2899973002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java (right): https://codereview.chromium.org/2899973002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:202: // Null if dismiss initiated by C++, or for some unknown reason (crbug.com/708562). Updated this comment too.
https://codereview.chromium.org/2899973002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java (right): https://codereview.chromium.org/2899973002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:264: if (mDialogDelegate == delegate) { What I meant here was using PermissionDialogDelegate#mNativeDelegatePtr as the equality condition (that's a long that's the value of the pointer to the native-side C++ object)
https://codereview.chromium.org/2899973002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java (right): https://codereview.chromium.org/2899973002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:264: if (mDialogDelegate == delegate) { On 2017/05/24 05:02:53, dominickn wrote: > What I meant here was using PermissionDialogDelegate#mNativeDelegatePtr as the > equality condition (that's a long that's the value of the pointer to the > native-side C++ object) I think referential equality is OK here; implementing .equals() correctly means we'd have to check all of the other members as well and also implement .hashCode(), and if we don't use .equals() the else branch below won't match. dismissFromNative() on the PDD also nulls out the native pointer which I don't think we should assume that happens after it calls dismissFromNative on the PDC.
lgtm, thanks https://codereview.chromium.org/2899973002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java (right): https://codereview.chromium.org/2899973002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:264: if (mDialogDelegate == delegate) { On 2017/05/25 03:11:22, Timothy Loh wrote: > On 2017/05/24 05:02:53, dominickn wrote: > > What I meant here was using PermissionDialogDelegate#mNativeDelegatePtr as the > > equality condition (that's a long that's the value of the pointer to the > > native-side C++ object) > > I think referential equality is OK here; implementing .equals() correctly means > we'd have to check all of the other members as well and also implement > .hashCode(), and if we don't use .equals() the else branch below won't match. > dismissFromNative() on the PDD also nulls out the native pointer which I don't > think we should assume that happens after it calls dismissFromNative on the PDC. Acknowledged.
timloh@chromium.org changed reviewers: + dfalcantara@chromium.org, raymes@chromium.org
dfalcantara@ for chrome/android OWNERS, raymes@ for chrome/browser/permissions OWNERS
timloh@chromium.org changed reviewers: + bauerb@chromium.org - dfalcantara@chromium.org
Whoops, didn't notice dfalcantara@ is OOO. bauerb@ could you review for chrome/android OWNERS? Thanks!
https://codereview.chromium.org/2899973002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java (right): https://codereview.chromium.org/2899973002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:202: // Null if dismiss initiated by C++, or for some unknown reason (crbug.com/708562). On 2017/05/24 04:59:34, Timothy Loh wrote: > Updated this comment too. So, this is sometimes null and we don't know why? That doesn't fill me with confidence :-/ https://codereview.chromium.org/2899973002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/permissions/PermissionTestCaseBase.java (right): https://codereview.chromium.org/2899973002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/permissions/PermissionTestCaseBase.java:142: final String test_url = mTestServer.getURL(url); Nit: The `final` doesn't do much here. Or maybe just inline the variable? https://codereview.chromium.org/2899973002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_dialog_delegate.cc (right): https://codereview.chromium.org/2899973002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:160: j_delegate_ = nullptr; This isn't really necessary if the object is going to be destroyed anyway. https://codereview.chromium.org/2899973002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:199: delete this; I have to say, I don't like these deletions very much either. For me it's not even that much potential UAF, but more that it feels weird for this class to both be destroyed by the Java object[*], but also destroy itself under certain circumstances. If you think about it in terms of ownership, who owns this object? Would it make sense for PermissionDialogController#dismissFromNative to just call PermissionDialogDelegate#destroy? That way you would have a clean ownership story and not need additional code paths to destroy this object. [*] I'm regarding the Destroy() method here as external destruction and the WebContentsObserver methods as self-destruction even though both internally call `delete this`, because a method called Destroy() pretty much implies deletion in its contract; it's just that we have to use this syntax because of JNI.
The CQ bit was checked by timloh@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...
https://codereview.chromium.org/2899973002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java (right): https://codereview.chromium.org/2899973002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/permissions/PermissionDialogController.java:202: // Null if dismiss initiated by C++, or for some unknown reason (crbug.com/708562). On 2017/05/25 09:57:17, Bernhard Bauer wrote: > On 2017/05/24 04:59:34, Timothy Loh wrote: > > Updated this comment too. > > So, this is sometimes null and we don't know why? That doesn't fill me with > confidence :-/ Well that's what the existing comment said; I couldn't see how it would otherwise end up null but I thought it'd be better to preserve the comment. https://codereview.chromium.org/2899973002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/permissions/PermissionTestCaseBase.java (right): https://codereview.chromium.org/2899973002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/permissions/PermissionTestCaseBase.java:142: final String test_url = mTestServer.getURL(url); On 2017/05/25 09:57:17, Bernhard Bauer wrote: > Nit: The `final` doesn't do much here. Or maybe just inline the variable? Done. https://codereview.chromium.org/2899973002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_dialog_delegate.cc (right): https://codereview.chromium.org/2899973002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:160: j_delegate_ = nullptr; On 2017/05/25 09:57:17, Bernhard Bauer wrote: > This isn't really necessary if the object is going to be destroyed anyway. Err yeah, removed. https://codereview.chromium.org/2899973002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:199: delete this; On 2017/05/25 09:57:17, Bernhard Bauer wrote: > I have to say, I don't like these deletions very much either. For me it's not > even that much potential UAF, but more that it feels weird for this class to > both be destroyed by the Java object[*], but also destroy itself under certain > circumstances. If you think about it in terms of ownership, who owns this > object? > > Would it make sense for PermissionDialogController#dismissFromNative to just > call PermissionDialogDelegate#destroy? That way you would have a clean ownership > story and not need additional code paths to destroy this object. > > [*] I'm regarding the Destroy() method here as external destruction and the > WebContentsObserver methods as self-destruction even though both internally call > `delete this`, because a method called Destroy() pretty much implies deletion in > its contract; it's just that we have to use this syntax because of JNI. I considered it but figured it would be nicer this way because we don't have to go C++ -> Java -> C++. I haven't done much C++ <--> Java stuff in the past though, so if you think it's probably better to go about it that way I don't mind. I changed it in the latest patch set.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm https://codereview.chromium.org/2899973002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_dialog_delegate.cc (right): https://codereview.chromium.org/2899973002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_dialog_delegate.cc:199: delete this; On 2017/05/26 05:08:03, Timothy Loh wrote: > On 2017/05/25 09:57:17, Bernhard Bauer wrote: > > I have to say, I don't like these deletions very much either. For me it's not > > even that much potential UAF, but more that it feels weird for this class to > > both be destroyed by the Java object[*], but also destroy itself under certain > > circumstances. If you think about it in terms of ownership, who owns this > > object? > > > > Would it make sense for PermissionDialogController#dismissFromNative to just > > call PermissionDialogDelegate#destroy? That way you would have a clean > ownership > > story and not need additional code paths to destroy this object. > > > > [*] I'm regarding the Destroy() method here as external destruction and the > > WebContentsObserver methods as self-destruction even though both internally > call > > `delete this`, because a method called Destroy() pretty much implies deletion > in > > its contract; it's just that we have to use this syntax because of JNI. > > I considered it but figured it would be nicer this way because we don't have to > go C++ -> Java -> C++. I haven't done much C++ <--> Java stuff in the past > though, so if you think it's probably better to go about it that way I don't > mind. I changed it in the latest patch set. Thanks! I do think this is nicer. If you want to, you could add a comment that DismissDialog() will indirectly end up destroying the object, so the object shouldn't be accessed afterwards.
OWNERS rs lgtm for c/b/permissions
The CQ bit was checked by timloh@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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by timloh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, bauerb@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2899973002/#ps80001 (title: "fix test (permission request from js goes through mojo so need to poll)")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496037187913160,
"parent_rev": "7c9f28d128319e9d3f534440143ab8f1eeb2e3c4", "commit_rev":
"3fd7dbe308d04ad7132bae51d86aceb33885b4ab"}
Message was sent while issue was closed.
Description was changed from ========== Hide modal permission prompts on Android upon tab navigation/destruction This patch makes modal permission prompts on Android disappear upon tab navigation or destruction. We make the PermissionDialogDelegate on the C++ side a WebContentsObserver to detect these events, and notify the Java side so the UI can be removed. BUG=699851 ========== to ========== Hide modal permission prompts on Android upon tab navigation/destruction This patch makes modal permission prompts on Android disappear upon tab navigation or destruction. We make the PermissionDialogDelegate on the C++ side a WebContentsObserver to detect these events, and notify the Java side so the UI can be removed. BUG=699851 Review-Url: https://codereview.chromium.org/2899973002 Cr-Commit-Position: refs/heads/master@{#475297} Committed: https://chromium.googlesource.com/chromium/src/+/3fd7dbe308d04ad7132bae51d86a... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/3fd7dbe308d04ad7132bae51d86a... |
