[Autofill] Revise ownership in the AuthenticatorSelectionDialog. [chromium/src : main]
Timofey Chudakov (Gerrit)
Timofey Chudakov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Timofey Chudakov
| Commit-Queue | +1 |
1 comment
Hey Chris, I wanted to get your feedback on the CL before sending it to other reviewers. We could walk through it on a call.
Related details
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 9 comments![]( "Open in Gerrit")
2. AuthenticatorSelectionDialogViewAndroid::Dismiss calls the Java
dismiss.
3. The dialog is dismissed asynchronously.
4. AuthenticatorSelectionDialogViewAndroid::OnDismissed is called.
5. controller_->OnDialogClosed is called when the controller is already
destroyed.I don't see yet how this crashes: Doesn't [this](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/autofill/authenticator_selection_dialog_view_android.cc;l=43-46;drc=f4718a40657cbf0de22a483cc1389ac95a6685ba) in Step 2 prevent us from enterining [the `if` statement](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/autofill/authenticator_selection_dialog_view_android.cc;l=72-76;drc=d648b1b5371523e04f0955321a24422d928c49e4) that leads to Step 5?
// The dismissal can happen either from C++ or from Java. `Dismiss` isWhat do you mean by "the dismissal" here?
The call of `AuthenticatorSelectionDialogViewAndroid::Dismiss()`? The next sentence sounds like it's saying the opposite.
Or do you mean the union of `Dismiss()` and `OnDismissed()`?
// Don't show the dialog twice. This should not be possible becauseDo you mean "should not be possible" as in "we believe it's impossible" or "we want it to be impossible"? Could you rephrase this?
// `ShowDialog` is always called once from thenitty nit: remove
// Destroy the view immediatelynitty nit: punctuation
#include <memory>nit: not needed, is it?
typedef std::unique_ptr<CardUnmaskAuthenticationSelectionDialog> DialogHolder;nit: use modern syntax: `using DialogHolder = ...;`?
class CardUnmaskAuthenticationSelectionDialogWrapper {nit: could you add documentation?
dialog_view_.reset();nit: `dialog_view_ = nullptr` has the same effect on as `dialog_view_.reset()`, so the `#if` isn't necessary.
Related details
- Timofey Chudakov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Timofey Chudakov (Gerrit)
Timofey Chudakov added 9 comments![]( "Open in Gerrit")
2. AuthenticatorSelectionDialogViewAndroid::Dismiss calls the Java
dismiss.
3. The dialog is dismissed asynchronously.
4. AuthenticatorSelectionDialogViewAndroid::OnDismissed is called.
5. controller_->OnDialogClosed is called when the controller is already
destroyed.I don't see yet how this crashes: Doesn't [this](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/autofill/authenticator_selection_dialog_view_android.cc;l=43-46;drc=f4718a40657cbf0de22a483cc1389ac95a6685ba) in Step 2 prevent us from enterining [the `if` statement](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/autofill/authenticator_selection_dialog_view_android.cc;l=72-76;drc=d648b1b5371523e04f0955321a24422d928c49e4) that leads to Step 5?
You're right, I updated the scenario. The C++ memory management appears to be correct, but I would like to get rid of `delete this` calls to simplify lifecycle reasoning.
// The dismissal can happen either from C++ or from Java. `Dismiss` isWhat do you mean by "the dismissal" here?
The call of `AuthenticatorSelectionDialogViewAndroid::Dismiss()`? The next sentence sounds like it's saying the opposite.
Or do you mean the union of `Dismiss()` and `OnDismissed()`?
I rephrased the comment, PTAL
// Don't show the dialog twice. This should not be possible becauseDo you mean "should not be possible" as in "we believe it's impossible" or "we want it to be impossible"? Could you rephrase this?
Rephrased, PTAL.
// `ShowDialog` is always called once from theTimofey Chudakovnitty nit: remove
Done
// Destroy the view immediatelyTimofey Chudakovnitty nit: punctuation
Done
nit: not needed, is it?
Done
typedef std::unique_ptr<CardUnmaskAuthenticationSelectionDialog> DialogHolder;nit: use modern syntax: `using DialogHolder = ...;`?
Done
class CardUnmaskAuthenticationSelectionDialogWrapper {nit: could you add documentation?
Done
nit: `dialog_view_ = nullptr` has the same effect on as `dialog_view_.reset()`, so the `#if` isn't necessary.
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 5 comments![]( "Open in Gerrit")
JNIEnv* env = base::android::AttachCurrentThread();nitty nit: move inside `if`?
// Don't show the dialog twice. This should not be possible becauseTimofey ChudakovDo you mean "should not be possible" as in "we believe it's impossible" or "we want it to be impossible"? Could you rephrase this?
Rephrased, PTAL.
My confusion came from the "should": It can be understood as "we hope that" or "we enforce that". You mean the former, right? IMO we should avoid "should" because it's often ambiguous.
WDYT about adding `CHECK(controller_, base::NotFatalUntil::M1xx)`?
CardUnmaskAuthenticationSelectionDialog* operator->();
const CardUnmaskAuthenticationSelectionDialog* operator->() const;nit: to avoid surprises, I'd also overload `operator*()` so that `a->b` is the same as `(*a).b`.
class CardUnmaskAuthenticationSelectionDialogWrapper {Doesn't it suffices to use `DialogHolder` directly everywhere?
Both `raw_ptr` and `std::unique_ptr` define `operator*()`, `operator->()`, `operator bool()`, and even a `get()` function. You'd just have to use `x = nullptr` instead of `x.Reset()`, I think.
Related details
- Timofey Chudakov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Timofey Chudakov (Gerrit)
Timofey Chudakov added 3 comments![]( "Open in Gerrit")
// The dismissal can happen either from C++ or from Java. `Dismiss` isTimofey ChudakovWhat do you mean by "the dismissal" here?
The call of `AuthenticatorSelectionDialogViewAndroid::Dismiss()`? The next sentence sounds like it's saying the opposite.
Or do you mean the union of `Dismiss()` and `OnDismissed()`?
I rephrased the comment, PTAL
Done
JNIEnv* env = base::android::AttachCurrentThread();nitty nit: move inside `if`?
Done
// Don't show the dialog twice. This should not be possible becauseTimofey ChudakovDo you mean "should not be possible" as in "we believe it's impossible" or "we want it to be impossible"? Could you rephrase this?
Christoph SchweringRephrased, PTAL.
My confusion came from the "should": It can be understood as "we hope that" or "we enforce that". You mean the former, right? IMO we should avoid "should" because it's often ambiguous.
WDYT about adding `CHECK(controller_, base::NotFatalUntil::M1xx)`?
Done, I assume you mean `CHECK(!java_object_, base::NotFatalUntil::M150);`
Related details
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Timofey Chudakov (Gerrit)
Timofey Chudakov added 2 comments![]( "Open in Gerrit")
CardUnmaskAuthenticationSelectionDialog* operator->();
const CardUnmaskAuthenticationSelectionDialog* operator->() const;nit: to avoid surprises, I'd also overload `operator*()` so that `a->b` is the same as `(*a).b`.
Done
class CardUnmaskAuthenticationSelectionDialogWrapper {Doesn't it suffices to use `DialogHolder` directly everywhere?
Both `raw_ptr` and `std::unique_ptr` define `operator*()`, `operator->()`, `operator bool()`, and even a `get()` function. You'd just have to use `x = nullptr` instead of `x.Reset()`, I think.
Good point.
Related details
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering voted and added 4 comments![]( "Open in Gerrit")
Votes added by Christoph Schwering
| Code-Review | +1 |
4 comments
2. AuthenticatorSelectionDialogViewAndroid::Dismiss calls the Java
dismiss.
3. The dialog is dismissed asynchronously.
4. AuthenticatorSelectionDialogViewAndroid::OnDismissed is called.
5. controller_->OnDialogClosed is called when the controller is already
destroyed.Timofey ChudakovI don't see yet how this crashes: Doesn't [this](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/autofill/authenticator_selection_dialog_view_android.cc;l=43-46;drc=f4718a40657cbf0de22a483cc1389ac95a6685ba) in Step 2 prevent us from enterining [the `if` statement](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/android/autofill/authenticator_selection_dialog_view_android.cc;l=72-76;drc=d648b1b5371523e04f0955321a24422d928c49e4) that leads to Step 5?
You're right, I updated the scenario. The C++ memory management appears to be correct, but I would like to get rid of `delete this` calls to simplify lifecycle reasoning.
Acknowledged
// Don't show the dialog twice. This should not be possible becauseTimofey ChudakovDo you mean "should not be possible" as in "we believe it's impossible" or "we want it to be impossible"? Could you rephrase this?
Christoph SchweringRephrased, PTAL.
Timofey ChudakovMy confusion came from the "should": It can be understood as "we hope that" or "we enforce that". You mean the former, right? IMO we should avoid "should" because it's often ambiguous.
WDYT about adding `CHECK(controller_, base::NotFatalUntil::M1xx)`?
Done, I assume you mean `CHECK(!java_object_, base::NotFatalUntil::M150);`
Oops yes :)
// A wrapper around a pointer to a CardUnmaskAuthenticationSelectionDialog. Thisnitty nit: it's not a wrapper anymore :)
Related details
- Timofey Chudakov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Timofey Chudakov (Gerrit)
Timofey Chudakov added 1 comment![]( "Open in Gerrit")
// A wrapper around a pointer to a CardUnmaskAuthenticationSelectionDialog. Thisnitty nit: it's not a wrapper anymore :)
I meant that either the `raw_ptr` or `std::unique_ptr` is a wrapper the controller uses.
Related details
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Timofey Chudakov (Gerrit)
Timofey Chudakov added 1 comment![]( "Open in Gerrit")
// A wrapper around a pointer to a CardUnmaskAuthenticationSelectionDialog. ThisTimofey Chudakovnitty nit: it's not a wrapper anymore :)
I meant that either the `raw_ptr` or `std::unique_ptr` is a wrapper the controller uses.
Acknowledged
Related details
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Timofey Chudakov (Gerrit)
Timofey Chudakov voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Christoph Schwering
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Autofill] Revise ownership in the AuthenticatorSelectionDialog.
CardUnmaskAuthenticationSelectionDialogControllerImpl used to store the
CardUnmaskAuthenticationSelectionDialog as a raw pointer. This makes
sense on Desktop because the created view is owned by the dialog
manager.
The Android's AuthenticatorSelectionDialogView is not owned by the
system component and was responsible for deleting itself. This is
problematic from the memory management perspective.
The root cause of the memory issue is that the Android dialog can be
dismissed both from C++ and from Java. The C++ dismissal happens when
the server request fails or the WebContents is destroyed. The Java
dismissal happens when the use clicks a dialog button.
The crashing scenario is following:
1. CardUnmaskAuthenticationSelectionDialogControllerImpl destructor is
called.
2. AuthenticatorSelectionDialogViewAndroid::Dismiss calls the Java
dismiss.
3. The dialog is dismissed asynchronously.
4. AuthenticatorSelectionDialogViewAndroid::OnDismissed is called, which
deletes the native view.
5. AuthenticatorSelectionDialog still holds the native pointer. The user
clicks one of the dialog buttons.
6. AuthenticatorSelectionDialogBridge calls the native view in response
to the button click, but the view has already been destroyed.
(1) The AuthenticatorSelectionDialogViewAndroid is now owned by the
unique pointer to simplify the memory management. A special wrapper is
created to store the native view via a raw pointer on Desktop and iOS.
(2) AuthenticatorSelectionDialogViewAndroid is destroyed immediately
by the controller after it's dismissed. This means that Java will never
call the C++ view if the dismissal call was coming from C++.
(3) AuthenticatorSelectionDialogBridge now resets the native pointer
immediately when the dismissal happens. This means that Java side will
never get called by the C++ side after the view is dismissed by the
user.
[1] https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/autofill/payments/card_unmask_authentication_selection_dialog_view.cc;l=282-286;drc=0be01c1048de19d23525ffb5eaae17d9979282fb
- M chrome/browser/ui/android/autofill/authenticator_selection_dialog_view_android.cc
- M chrome/browser/ui/android/autofill/internal/java/src/org/chromium/chrome/browser/ui/autofill/AuthenticatorSelectionDialogBridge.java
- M chrome/browser/ui/android/autofill/internal/java/src/org/chromium/chrome/browser/ui/autofill/AuthenticatorSelectionDialogBridgeTest.java
- M components/autofill/core/browser/ui/payments/card_unmask_authentication_selection_dialog_controller_impl.cc
- M components/autofill/core/browser/ui/payments/card_unmask_authentication_selection_dialog_controller_impl.h
Code-Review: +1 by Christoph Schwering
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Timofey Chudakov (Gerrit)
Timofey Chudakov has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |